added conformance checks for port, scheme and path (extended + experimental features) #1611

LiorLieberman · 2022-12-18T22:47:27Z

What type of PR is this?
/kind feature
/area conformance

What this PR does / why we need it:
Add tests for extended redirect features (port and scheme) and path experimental feature

Which issue(s) this PR fixes:

Issue #1103

Does this PR introduce a user-facing change?:
NONE

k8s-ci-robot · 2022-12-18T22:47:35Z

Hi @LiorLieberman. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

LiorLieberman · 2022-12-18T22:51:46Z

@robscott

conformance/tests/httproute-redirect-port copy.yaml

LiorLieberman · 2022-12-19T09:41:11Z

conformance/utils/http/http.go

+		if expected.RedirectRequest.Host == "" {
+			expected.RedirectRequest.Host = cRes.RedirectRequest.Host
+		}
+
+		if expected.RedirectRequest.Port == "" {
+			expected.RedirectRequest.Port = "80"
+		}
+
+		if expected.RedirectRequest.Scheme == "" {
+			expected.RedirectRequest.Scheme = "http"
+		}


what do you think about these lines?
I am not 100% sure it is best to take these assumptions

The default for scheme and port made me pause slightly. Unless I'm misunderstanding things we're setting defaults here, conventionally you would (almost?) never redirect to http so I'm just wondering if we should flip those 🤔

The thing is that I have the checks down there. For example the line 312 - I am checking for scheme.
so when you would run for example a redirect test for Port when expected.RedirectRequest.Scheme is an empty string (because the implementations should not bother to set Scheme in their expected struct unless they want to check it) then it will fail on the scheme check. Hence I added it.
My understanding is the by default it is http.

For port it is interesting, I will probably need to change it. Now I have tested Path redirect (changes are not in this PR) with envoyGateway other than istio because istio does not yet support it and I noticed that the response is an empty port (not 80 like istio), so "80" is not good default for every impl I guess.

If we decide to set the default to "80" we need to also fill cRes.RedirectRequest.Port to "80" but not sure thats desired.

The other option I see is setting it to what we get from the expected field if it is an empty string but that also feels a bit iffy - something like this:

if expected.RedirectRequest.Port == "" { expected.RedirectRequest.Port = cRes.RedirectRequest.Port } if expected.RedirectRequest.Scheme == "" { expected.RedirectRequest.Scheme = cRes.RedirectRequest.Scheme }

I do have the same thing now for expected.RedirectRequest.Path which I think to add something similar:

if expected.RedirectRequest.Path == "" { expected.RedirectRequest.Path = cRes.RedirectRequest.Path }

@youngnick and @robscott curious as to your thoughts here.

My preference is to avoid defaults here, specify what we care about in the test, and only check for that. In this case that would mean lots of repetitive config, but I think it would more clearly communicate what we actually expect from an implementation.

I have made some refactoring - PTAL

robscott · 2022-12-19T17:32:27Z

/ok-to-test

LiorLieberman · 2022-12-19T21:11:55Z

/assign @shaneutt

shaneutt

Looking good!

I do have a few things I would like to see addressed before we merge, but they are minor.

conformance/tests/httproute-redirect-host-and-status.go

conformance/tests/httproute-redirect-port.go

conformance/tests/httproute-redirect-scheme.go

conformance/utils/http/http.go

shaneutt · 2022-12-20T14:32:36Z

conformance/utils/http/http.go

+		if expected.RedirectRequest.Host == "" {
+			expected.RedirectRequest.Host = cRes.RedirectRequest.Host
+		}
+
+		if expected.RedirectRequest.Port == "" {
+			expected.RedirectRequest.Port = "80"
+		}
+
+		if expected.RedirectRequest.Scheme == "" {
+			expected.RedirectRequest.Scheme = "http"
+		}


The default for scheme and port made me pause slightly. Unless I'm misunderstanding things we're setting defaults here, conventionally you would (almost?) never redirect to http so I'm just wondering if we should flip those 🤔

LiorLieberman · 2022-12-20T15:49:40Z

@shaneutt Thanks very much for the feedback, I did miss some important things.
PTAL
(will rebase once we address all the comments)

shaneutt

Approving so as to no longer block as most of the issues I brought up were resolved, but would still like a review from @youngnick or @robscott for the final lgtm.

k8s-ci-robot · 2022-12-20T20:35:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiorLieberman, shaneutt

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:

~~conformance/OWNERS~~ [shaneutt]

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

robscott

Thanks @LiorLieberman! A few nits but otherwise LGTM.

conformance/tests/httproute-redirect-port.go

conformance/tests/httproute-redirect-port.yaml

conformance/tests/httproute-redirect-scheme.go

conformance/tests/httproute-redirect-scheme.yaml

LiorLieberman · 2022-12-23T17:06:11Z

@shaneutt @robscott

I have made a small refactor

Removed 302 default as per Rob's preference.
added tests for path redirect:
** tested both with envoygateway and istio (after I have contributed the path redirect support to them([gw-api] support experimental path redirect istio/istio#42538)
taking defaults from the original request if they arent set.

PTAL

robscott

Thanks @LiorLieberman, a few tiny nits but otherwise LGTM.

conformance/utils/http/http.go

conformance/utils/suite/suite.go

conformance/tests/httproute-redirect-port.yaml

robscott · 2022-12-29T19:57:56Z

Thanks @LiorLieberman!

/lgtm

…mental features) (#1611) * add conformance checks for port and scheme (extended features) * fix file name * fix file name * address feedback from comments * fix indent * add experimental path redirect * now added path redirect * address feedback from comments * fix typo

add conformance checks for port and scheme (extended features)

85403f6

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 18, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 18, 2022

k8s-ci-robot requested review from keithmattix and youngnick December 18, 2022 22:47

LiorLieberman marked this pull request as ready for review December 18, 2022 22:50

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 18, 2022

k8s-ci-robot requested a review from howardjohn December 18, 2022 22:50

gyohuangxin reviewed Dec 19, 2022

View reviewed changes

conformance/tests/httproute-redirect-port copy.yaml Outdated Show resolved Hide resolved

LiorLieberman commented Dec 19, 2022

View reviewed changes

LiorLieberman added 2 commits December 19, 2022 10:06

fix file name

cacbf74

fix file name

7a5a403

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 19, 2022

k8s-ci-robot assigned shaneutt Dec 19, 2022

shaneutt self-requested a review December 19, 2022 22:38

shaneutt requested changes Dec 20, 2022

View reviewed changes

address feedback from comments

bd74761

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2022

Merge branch 'main' into extended-redirect-conformance

a6f6319

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2022

fix indent

4798852

LiorLieberman removed request for howardjohn and keithmattix December 20, 2022 15:48

LiorLieberman requested review from shaneutt and removed request for youngnick December 20, 2022 15:48

shaneutt requested review from youngnick and robscott December 20, 2022 19:17

shaneutt approved these changes Dec 20, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2022

robscott reviewed Dec 21, 2022

View reviewed changes

LiorLieberman added 2 commits December 23, 2022 17:00

add experimental path redirect

3861cf3

now added path redirect

b303c30

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 23, 2022

LiorLieberman changed the title ~~added conformance checks for port and scheme (extended features)~~ added conformance checks for port, scheme and path (extended + experimental features) Dec 23, 2022

robscott reviewed Dec 29, 2022

View reviewed changes

conformance/utils/http/http.go Show resolved Hide resolved

conformance/utils/suite/suite.go Outdated Show resolved Hide resolved

conformance/tests/httproute-redirect-port.yaml Outdated Show resolved Hide resolved

LiorLieberman added 2 commits December 29, 2022 07:12

address feedback from comments

4bbb2d3

fix typo

1c31536

robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 29, 2022

k8s-ci-robot assigned robscott Dec 29, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 29, 2022

k8s-ci-robot merged commit d037303 into kubernetes-sigs:main Dec 29, 2022

LiorLieberman mentioned this pull request Jan 9, 2023

REQUEST: New membership for LiorLieberman kubernetes/org#3924

Closed

9 tasks

shaneutt added this to the v0.6.1 milestone Feb 7, 2023

arkodg mentioned this pull request Feb 9, 2023

Incorrect default port value check in HTTPRouteRedirectPath test #1721

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added conformance checks for port, scheme and path (extended + experimental features) #1611

added conformance checks for port, scheme and path (extended + experimental features) #1611

LiorLieberman commented Dec 18, 2022 •

edited

Loading

k8s-ci-robot commented Dec 18, 2022

LiorLieberman commented Dec 18, 2022

LiorLieberman Dec 19, 2022

shaneutt Dec 20, 2022

LiorLieberman Dec 20, 2022 •

edited

Loading

LiorLieberman Dec 20, 2022 •

edited

Loading

shaneutt Dec 20, 2022

robscott Dec 21, 2022

LiorLieberman Dec 23, 2022

robscott commented Dec 19, 2022

LiorLieberman commented Dec 19, 2022

shaneutt left a comment

shaneutt Dec 20, 2022

LiorLieberman commented Dec 20, 2022 •

edited

Loading

shaneutt left a comment

k8s-ci-robot commented Dec 20, 2022

robscott left a comment

LiorLieberman commented Dec 23, 2022

robscott left a comment

robscott commented Dec 29, 2022

added conformance checks for port, scheme and path (extended + experimental features) #1611

added conformance checks for port, scheme and path (extended + experimental features) #1611

Conversation

LiorLieberman commented Dec 18, 2022 • edited Loading

k8s-ci-robot commented Dec 18, 2022

LiorLieberman commented Dec 18, 2022

LiorLieberman Dec 19, 2022

Choose a reason for hiding this comment

shaneutt Dec 20, 2022

Choose a reason for hiding this comment

LiorLieberman Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

LiorLieberman Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

shaneutt Dec 20, 2022

Choose a reason for hiding this comment

robscott Dec 21, 2022

Choose a reason for hiding this comment

LiorLieberman Dec 23, 2022

Choose a reason for hiding this comment

robscott commented Dec 19, 2022

LiorLieberman commented Dec 19, 2022

shaneutt left a comment

Choose a reason for hiding this comment

shaneutt Dec 20, 2022

Choose a reason for hiding this comment

LiorLieberman commented Dec 20, 2022 • edited Loading

shaneutt left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 20, 2022

robscott left a comment

Choose a reason for hiding this comment

LiorLieberman commented Dec 23, 2022

robscott left a comment

Choose a reason for hiding this comment

robscott commented Dec 29, 2022

LiorLieberman commented Dec 18, 2022 •

edited

Loading

LiorLieberman Dec 20, 2022 •

edited

Loading

LiorLieberman Dec 20, 2022 •

edited

Loading

LiorLieberman commented Dec 20, 2022 •

edited

Loading