fix: Validate requestRedirect with backendrefs #2161

slayer321 · 2023-07-02T10:31:59Z

What type of PR is this?

/kind bug
/kind test
What this PR does / why we need it:
Validate and Prevent Redirect Filters and BackendRefs in same HTTPRoute Rules.
Which issue(s) this PR fixes:

Fixes #2148

Does this PR introduce a user-facing change?:

Webhook validation now ensures that BackendRefs can not be specified in the same HTTPRoute rule as a Redirect filter

k8s-ci-robot · 2023-07-02T10:32:07Z

Welcome @slayer321!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-07-02T10:32:08Z

Hi @slayer321. 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.

gyohuangxin · 2023-07-03T02:35:28Z

/ok-to-test

gyohuangxin · 2023-07-03T05:44:46Z

@slayer321 The job failed due to a error:

./hack/invalid-examples/v1beta1/httproute/invalid-request-redirect-with-backendref.yaml
  16:19     error    no new line character at the end of file  (new-line-at-end-of-file)
[0;31mTest FAILED: hack/../hack/verify-yamllint.sh 
make: *** [Makefile:114: verify] Error 1

gyohuangxin

LGTM, thanks!

arkodg · 2023-07-04T01:54:56Z

apis/v1alpha2/validation/httproute_test.go

@@ -868,7 +868,7 @@ func TestValidateHTTPRouteTypeMatchesField(t *testing.T) {
 				StatusCode: new(int),
 			},
 		},
-		errCount: 0,
+		errCount: 1,


why has this changed ?

The same reason that I mentioned for v1beta1 changes.

@slayer321 do you happen to have a link handy to that comment or can you just copy it here? Not finding the context easily and also have the same question.

I was taking about this comment #2161 (comment)

arkodg · 2023-07-04T01:55:05Z

apis/v1beta1/validation/httproute_test.go

@@ -1049,7 +1075,7 @@ func TestValidateHTTPRouteTypeMatchesField(t *testing.T) {
 				StatusCode: new(int),
 			},
 		},
-		errCount: 0,
+		errCount: 1,


why has this changed ?

So, as in this issue we need to validate that BackendRefs is not used when Redirect Filters are used in httpRoute.
In this example as we can see that we are using Redirect Filters and then in the for loops we have BackendRefs as well.

After I added a check of it inside validation/httproute.go. The test started failing so I made the errCount as 1 .

ah thanks for the clarification, didnt go through at the entire file earlier 🙈
would be good to add a positive test case, maybe as a new standalone function that ensures thats when the Redirect filter exists and no backendRefs are set, validation is successful

maybe I didn't get it. so for new we already have a standalone function which checks if Redirect filter exists then no backendRefs should be set.

was referring to a standalone alone test TestValidateRequestRedirectFiltersWithNoBackendRef
anyways this is a good to have and is non blocking

make sense. I have added standalone test as you mentioned for both the cases where backendref is added and not added.

arkodg · 2023-07-04T01:55:22Z

apis/v1beta1/validation/httproute.go

+	var errs field.ErrorList
+	for _, filter := range rule.Filters {
+		if filter.RequestRedirect != nil && len(rule.BackendRefs) > 0 {
+			errs = append(errs, field.Invalid(path.Child("filters"), filter, "RequestRedirect filter is not allowed with backendRefs"))


is filter, needed ?

If you are asking if filter needed inside the field.Invalid function. So yeah I think we can change to gatewayv1b1.HTTPRouteFilterRequestRedirect .. as it will be more specific.

errs = append(errs, field.Invalid(path.Child("filters"), gatewayv1b1.HTTPRouteFilterRequestRedirect, "RequestRedirect filter is not allowed with backendRefs"))

thanks, yeah +1 to this change

arkodg

LGTM thanks !

slayer321 · 2023-07-06T15:02:01Z

Hi, as api-verify CI is failing so I tried running the golangci-lint run command .. but it is just showing Killed

sachinmaurya@sachin:~/go/src/github.com/gateway-api (test-request-route-w-backend-ref)$ golangci-lint run 
WARN [runner] The linter 'structcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter.  Replaced by unused. 
WARN [runner] The linter 'deadcode' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter.  Replaced by unused. 
WARN [runner] The linter 'varcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter.  Replaced by unused. 
Killed

What can be the reason for that 🤔

arkodg · 2023-07-06T17:17:10Z

golangci-lint run

@slayer321 can you rebase to main and try ./hack/verify-golint.sh ?

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>

slayer321 · 2023-07-07T14:30:58Z

I tried it but still got below error

sachinmaurya@sachin:~/go/src/github.com/gateway-api (test-request-route-w-backend-ref)$ ./hack/verify-golint.sh
level=warning msg="[runner] Can't run linter goanalysis_metalinter: musttag: running `go list all`: exit status 1"
level=error msg="Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: musttag: running `go list all`: exit status 1\n\n"

After looking for it , I found that there where many similar errors but nothing same as this one. most of them says that we need to run go build.
has anyone else faced the similar error while working on this project or is it some issue with my local setup.

gyohuangxin · 2023-07-08T08:07:41Z

@slayer321 I don't have idea about it. Even if you can't run the ./hack/verify-golint.sh in your environment, you can find the details from the CI job logs: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_gateway-api/2161/pull-gateway-api-verify/1676963399549849600

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>

robscott · 2023-07-17T20:36:44Z

/release-note "Webhook validation now ensures that BackendRefs can not be specified in the same HTTPRoute rule as a Redirect filter"

robscott · 2023-07-17T20:37:39Z

/release-note-edit Webhook validation now ensures that BackendRefs can not be specified in the same HTTPRoute rule as a Redirect filter

robscott · 2023-07-17T20:38:31Z

Thanks @slayer321!

/lgtm
/approve

k8s-ci-robot · 2023-07-17T20:38:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, gyohuangxin, robscott, slayer321

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:

~~OWNERS~~ [robscott]

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 added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/test labels Jul 2, 2023

k8s-ci-robot requested review from bowei and shaneutt July 2, 2023 10:32

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

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 2, 2023

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 Jul 3, 2023

gyohuangxin approved these changes Jul 4, 2023

View reviewed changes

arkodg reviewed Jul 4, 2023

View reviewed changes

arkodg approved these changes Jul 5, 2023

View reviewed changes

slayer321 added 4 commits July 7, 2023 19:32

fix: Validate requestRedirect with backendrefs

080e26a

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>

handle error with alpha version

53fffc7

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>

new line character at the end of file

4c0530f

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>

standalone test for requestRedirect without backendrefs

aa0a7a8

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>

fix the linter error

caa509f

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>

slayer321 force-pushed the test-request-route-w-backend-ref branch from 30cec5f to caa509f Compare July 9, 2023 12:02

slayer321 mentioned this pull request Jul 10, 2023

Failed verifying golint while executing make #2071

Closed

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 17, 2023

k8s-ci-robot assigned robscott Jul 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2023

k8s-ci-robot merged commit d708851 into kubernetes-sigs:main Jul 17, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Validate requestRedirect with backendrefs #2161

fix: Validate requestRedirect with backendrefs #2161

slayer321 commented Jul 2, 2023 •

edited by robscott

Loading

k8s-ci-robot commented Jul 2, 2023

k8s-ci-robot commented Jul 2, 2023

gyohuangxin commented Jul 3, 2023

gyohuangxin commented Jul 3, 2023 •

edited

Loading

gyohuangxin left a comment

arkodg Jul 4, 2023

slayer321 Jul 4, 2023

robscott Jul 10, 2023

slayer321 Jul 11, 2023

arkodg Jul 4, 2023

slayer321 Jul 4, 2023

arkodg Jul 4, 2023

slayer321 Jul 5, 2023

arkodg Jul 5, 2023

slayer321 Jul 6, 2023

arkodg Jul 4, 2023

slayer321 Jul 4, 2023

arkodg Jul 4, 2023

arkodg left a comment

slayer321 commented Jul 6, 2023

arkodg commented Jul 6, 2023

slayer321 commented Jul 7, 2023

gyohuangxin commented Jul 8, 2023

robscott commented Jul 17, 2023

robscott commented Jul 17, 2023

robscott commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

fix: Validate requestRedirect with backendrefs #2161

fix: Validate requestRedirect with backendrefs #2161

Conversation

slayer321 commented Jul 2, 2023 • edited by robscott Loading

k8s-ci-robot commented Jul 2, 2023

k8s-ci-robot commented Jul 2, 2023

gyohuangxin commented Jul 3, 2023

gyohuangxin commented Jul 3, 2023 • edited Loading

gyohuangxin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

slayer321 commented Jul 6, 2023

arkodg commented Jul 6, 2023

slayer321 commented Jul 7, 2023

gyohuangxin commented Jul 8, 2023

robscott commented Jul 17, 2023

robscott commented Jul 17, 2023

robscott commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

slayer321 commented Jul 2, 2023 •

edited by robscott

Loading

gyohuangxin commented Jul 3, 2023 •

edited

Loading