Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add conformance for multiple mirror filters #2359

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Add conformance for multiple mirror filters #2359

merged 4 commits into from
Sep 11, 2023

Conversation

levikobi
Copy link
Member

What type of PR is this?

/kind test
/area conformance

What this PR does / why we need it:
Adds a conformance test for multiple mirror filters within the same rule, tested using Contour's implementation.

Which issue(s) this PR fixes:

Fixes #2210

Does this PR introduce a user-facing change?:

None

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2023
@shaneutt shaneutt added this to the v1.0.0 milestone Aug 28, 2023
@shaneutt shaneutt added the release-blocker MUST be completed to complete the milestone label Aug 28, 2023
LiorLieberman
LiorLieberman reviewed Aug 28, 2023
View reviewed changes
Copy link
Member

@LiorLieberman LiorLieberman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a great PR, thanks @levikobi !

conformance/utils/http/mirror.go Outdated Show resolved Hide resolved
conformance/utils/suite/features.go Outdated Show resolved Hide resolved
conformance/tests/httproute-request-multiple-mirrors.go Outdated Show resolved Hide resolved
conformance/utils/http/mirror.go Outdated Show resolved Hide resolved
conformance/utils/http/mirror.go Outdated Show resolved Hide resolved
conformance/tests/httproute-request-multiple-mirrors.yaml Outdated Show resolved Hide resolved
conformance/tests/httproute-request-multiple-mirrors.go Outdated Show resolved Hide resolved
conformance/tests/httproute-request-multiple-mirrors.go Outdated Show resolved Hide resolved
conformance/tests/httproute-request-multiple-mirrors.go
Comment on lines +59 to +70
Backend: "infra-backend-v1",
MirroredTo: []http.BackendRef{
{
Name: "infra-backend-v2",
Namespace: ns,
},
{
Name: "infra-backend-v3",
Namespace: "another-namespace",
},
},
Namespace: ns,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a requirement for this PR, but I am curious if changing Backend to the new BackendRef struct you created is something we want and how much effort it will take (since it is widely used in a lot of tests).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if there is a demand for it I can address it in a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good if you can do a quick check and see if its something we would want and if so, open an issue for it (it can be a good-first-issue imo)

@LiorLieberman
Copy link
Member

LiorLieberman commented Aug 28, 2023 •
edited
Loading

gateway-api/conformance/utils/suite/features.go

Lines 126 to 136 in 4f8e572

var HTTPRouteExtendedFeatures = sets.New(
SupportHTTPRouteQueryParamMatching,
SupportHTTPRouteMethodMatching,
SupportHTTPResponseHeaderModification,
SupportHTTPRoutePortRedirect,
SupportHTTPRouteSchemeRedirect,
SupportHTTPRoutePathRedirect,
SupportHTTPRouteHostRewrite,
SupportHTTPRoutePathRewrite,
SupportHTTPRouteRequestMirror,
)

Also you probably want to add the new supported feature here

levikobi and others added 2 commits August 29, 2023 07:28
@levikobi @LiorLieberman
Apply suggestions from code review
ac6c497 
Co-authored-by: Lior Lieberman <liorlib7+riskified@gmail.com>
@levikobi
Apply all suggestions from code review
dcefa0f
@levikobi
Copy link
Member Author

Thanks for the review @LiorLieberman!

@levikobi levikobi requested a review from LiorLieberman August 29, 2023 04:39
LiorLieberman
LiorLieberman reviewed Aug 29, 2023
View reviewed changes
Copy link
Member

@LiorLieberman LiorLieberman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @levikobi !
/lgtm

conformance/utils/http/mirror.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2023
@LiorLieberman
Copy link
Member

/assign @shaneutt

@levikobi @LiorLieberman
Update conformance/utils/http/mirror.go
937dcb3 
Co-authored-by: Lior Lieberman <liorlib7+riskified@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023
@LiorLieberman
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023
@robscott
Copy link
Member

robscott commented Sep 5, 2023

/cc @sunjayBhatia @mlavacca @arkodg

@robscott
Copy link
Member

robscott commented Sep 5, 2023

This LGTM, but would like one of the conformance approvers I listed above to sign off on it as well.

@robscott
Copy link
Member

@sunjayBhatia @mlavacca @arkodg can one of you take a look at this? Would love to get this into v0.8.1 (this week) if possible.

arkodg
arkodg approved these changes Sep 11, 2023
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, levikobi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2023
@robscott
Copy link
Member

/cherry-pick release-0.8

@k8s-infra-cherrypick-robot
Copy link
Contributor

@robscott: once the present PR merges, I will cherry-pick it on top of release-0.8 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot merged commit c80d8a3 into kubernetes-sigs:main Sep 11, 2023
@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Sep 11, 2023
Merged
@k8s-infra-cherrypick-robot
Copy link
Contributor

@robscott: new pull request created: #2387

In response to this:

/cherry-pick release-0.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiorLieberman LiorLieberman LiorLieberman left review comments

@arkodg arkodg arkodg approved these changes

@Xunzhuo Xunzhuo Awaiting requested review from Xunzhuo

@mlavacca mlavacca Awaiting requested review from mlavacca

@sunjayBhatia sunjayBhatia Awaiting requested review from sunjayBhatia

Assignees

@shaneutt shaneutt

@LiorLieberman LiorLieberman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-blocker MUST be completed to complete the milestone release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
Gateway API: The Road to GA
Status: Done
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.

Conformance test for multiple mirror filters within the same rule
7 participants
@levikobi @LiorLieberman @robscott @k8s-ci-robot @k8s-infra-cherrypick-robot @arkodg @shaneutt