fix: add webhook validation of redirect usage #1788

Xunzhuo · 2023-03-08T08:19:10Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Webhook validation should prevent a HTTPRoute that specifies a redirect filter from specifying a backendRef

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2023-03-08T08:19:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Xunzhuo
Once this PR has been reviewed and has the lgtm label, please assign bowei for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Xunzhuo · 2023-03-08T08:19:30Z

/kind bug

Signed-off-by: Xunzhuo <mixdeers@gmail.com>

robscott · 2023-03-09T00:36:36Z

Hey @Xunzhuo, thanks for working on this! We really need more information in the PR description to help reviewers understand what the goals are. For example, #1753 has good detail for each section.

As far as the content of the PR, I think we need to have some broader discussion about this before merging. This PR would effectively prevent a case where redirect loop as proposed by @rainest in #1185 is possible. As I see it, we essentially have two options:

Say that redirect loop protection is likely not viable and instead require that both a redirect and backendRef cannot be specified in the same route rule (that's this PR).
Update the spec to define how redirect loop protection would work, enabling a single route rule to express something like "redirect to HTTPS, if already there, forward to backend foo".

My gut tells me that 2 would be pretty challenging and that 1 is the most viable solution at this point, but we need to be very clear that this is a change to the API Spec and include a release note + API Spec update if that's the direction we go.

Since this would be a significant change, I want to hold off until we can discuss this at a community meeting + get some feedback on the high level direction of this PR first.

/hold
/cc @youngnick @rainest @sunjayBhatia @howardjohn

k8s-ci-robot · 2023-03-09T00:36:41Z

@robscott: GitHub didn't allow me to request PR reviews from the following users: rainest.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Hey @Xunzhuo, thanks for working on this! We really need more information in the PR description to help reviewers understand what the goals are. For example, #1753 has good detail for each section.

As far as the content of the PR, I think we need to have some broader discussion about this before merging. This PR would effectively prevent a case where redirect loop as proposed by @rainest in #1185 is possible. As I see it, we essentially have two options:

Say that redirect loop protection is likely not viable and instead require that both a redirect and backendRef cannot be specified in the same route rule (that's this PR).

Update the spec to define how redirect loop protection would work, enabling a single route rule to express something like "redirect to HTTPS, if already there, forward to backend foo".

My gut tells me that 2 would be pretty challenging and that 1 is the most viable solution at this point, but we need to be very clear that this is a change to the API Spec and include a release note + API Spec update if that's the direction we go.

Since this would be a significant change, I want to hold off until we can discuss this at a community meeting + get some feedback on the high level direction of this PR first.

/hold
/cc @youngnick @rainest @sunjayBhatia @howardjohn

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.

Xunzhuo · 2023-03-09T02:23:20Z

Yes @robscott, I am also a bit concerned about this change, since this is a user-facing change. We definitely need more discussion before pushing this forward.

Take this as a draft PR, changes can make after the final approach is out.

sunjayBhatia

I may have missed some discussion here from a community meeting, but in Contour we currently ignore backendrefs if you've set a redirect filter so option 1 Rob describes seems fine

we could also specify that infinite redirect protection is recommended but implementation-specific support (or extended if we can write up a nice conformance test)

robscott

Thanks for the work on this @Xunzhuo!

robscott · 2023-03-23T23:41:35Z

apis/v1alpha2/validation/httproute_test.go

+		backendRefs: []gatewayv1a2.HTTPBackendRef{{
+			BackendRef: gatewayv1a2.BackendRef{
+				BackendObjectReference: gatewayv1a2.BackendObjectReference{
+					Name: gatewayv1a2.ObjectName("test"),
+					Port: ptrTo(gatewayv1b1.PortNumber(8080)),
+				},
+			},
+		}},


It looks like this is repeated a lot, can you move the standard set of backendRefs to a var so there's not quite so much repetition here?

Actually it looks like this isn't modified at all for any of the test cases, maybe just add this to the route populated in t.Run below?

robscott · 2023-03-23T23:45:20Z

apis/v1beta1/validation/httproute.go

+		for bi, backendRef := range rule.BackendRefs {
+			for fi, filter := range backendRef.Filters {
+				if filter.RequestRedirect != nil {
+					errs = append(errs, field.Invalid(path.Index(ri).Child("backendRefs").Index(bi).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify backendRef when using redirect filter"))


Suggested change

errs = append(errs, field.Invalid(path.Index(ri).Child("backendRefs").Index(bi).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify backendRef when using redirect filter"))

errs = append(errs, field.Invalid(path.Index(ri).Child("backendRefs").Index(bi).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify RequestRedirect filter within a backendRef"))

robscott · 2023-03-23T23:45:33Z

apis/v1beta1/validation/httproute.go

+		for fi, filter := range rule.Filters {
+			if filter.RequestRedirect != nil {
+				if len(rule.BackendRefs) != 0 {
+					errs = append(errs, field.Invalid(path.Index(ri).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify backendRefs when using redirect filter"))


Suggested change

errs = append(errs, field.Invalid(path.Index(ri).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify backendRefs when using redirect filter"))

errs = append(errs, field.Invalid(path.Index(ri).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify backendRefs when using RequestRedirect filter"))

robscott · 2023-03-23T23:46:35Z

apis/v1beta1/validation/httproute.go


-	return true
+	return *matches[0].Path.Type == gatewayv1b1.PathMatchPathPrefix


This feels not very safe and also not relevant to the rest of the PR, maybe revert this change?

robscott · 2023-03-23T23:47:25Z

apis/v1beta1/validation/httproute_test.go

+			BackendRef: gatewayv1b1.BackendRef{
+				BackendObjectReference: gatewayv1b1.BackendObjectReference{
+					Name: gatewayv1b1.ObjectName("test"),
+					Port: ptrTo(gatewayv1b1.PortNumber(8080)),
+				},
+			},


This also looks like it's repeated a lot, can we use a var for this?

robscott · 2023-06-26T18:30:21Z

Hey @Xunzhuo are you still interested in working on this? If so, please reopen the PR. Closing for now because it seems to have gone stale.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 8, 2023

k8s-ci-robot requested a review from bowei March 8, 2023 08:19

k8s-ci-robot requested a review from robscott March 8, 2023 08:19

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 8, 2023

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 8, 2023

Xunzhuo force-pushed the add-redirect-webhook branch from 425e494 to 04384fe Compare March 8, 2023 09:20

fix: add webhook validation of redirect usage

58ff989

Signed-off-by: Xunzhuo <mixdeers@gmail.com>

Xunzhuo force-pushed the add-redirect-webhook branch from 04384fe to 58ff989 Compare March 8, 2023 09:22

shaneutt added this to the v0.6.2 milestone Mar 8, 2023

shaneutt self-requested a review March 8, 2023 19:11

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2023

k8s-ci-robot requested review from youngnick, sunjayBhatia and howardjohn March 9, 2023 00:36

shaneutt marked this pull request as draft March 10, 2023 15:23

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2023

shaneutt modified the milestones: v0.6.2, v0.7.0 Mar 14, 2023

sunjayBhatia reviewed Mar 22, 2023

View reviewed changes

robscott reviewed Mar 23, 2023

View reviewed changes

shaneutt modified the milestones: v0.7.0, v0.8.0 Apr 6, 2023

robscott closed this Jun 26, 2023

robscott removed this from the v0.8.0 milestone Jun 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add webhook validation of redirect usage #1788

fix: add webhook validation of redirect usage #1788

Xunzhuo commented Mar 8, 2023 •

edited

k8s-ci-robot commented Mar 8, 2023

Xunzhuo commented Mar 8, 2023

robscott commented Mar 9, 2023

k8s-ci-robot commented Mar 9, 2023

Xunzhuo commented Mar 9, 2023

sunjayBhatia left a comment

robscott left a comment

robscott Mar 23, 2023

robscott Mar 23, 2023

robscott Mar 23, 2023

robscott Mar 23, 2023

robscott Mar 23, 2023

robscott Mar 23, 2023

robscott commented Jun 26, 2023

	errs = append(errs, field.Invalid(path.Index(ri).Child("backendRefs").Index(bi).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify backendRef when using redirect filter"))
	errs = append(errs, field.Invalid(path.Index(ri).Child("backendRefs").Index(bi).Child("filters").Index(fi), filter.RequestRedirect, "cannot specify RequestRedirect filter within a backendRef"))


		return true
		return *matches[0].Path.Type == gatewayv1b1.PathMatchPathPrefix

fix: add webhook validation of redirect usage #1788

fix: add webhook validation of redirect usage #1788

Conversation

Xunzhuo commented Mar 8, 2023 • edited

k8s-ci-robot commented Mar 8, 2023

Xunzhuo commented Mar 8, 2023

robscott commented Mar 9, 2023

k8s-ci-robot commented Mar 9, 2023

Xunzhuo commented Mar 9, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

robscott Mar 23, 2023

Choose a reason for hiding this comment

robscott Mar 23, 2023

Choose a reason for hiding this comment

robscott Mar 23, 2023

Choose a reason for hiding this comment

robscott Mar 23, 2023

Choose a reason for hiding this comment

robscott Mar 23, 2023

Choose a reason for hiding this comment

robscott Mar 23, 2023

Choose a reason for hiding this comment

robscott commented Jun 26, 2023

Xunzhuo commented Mar 8, 2023 •

edited