Conformance tests for HTTPRouteFilterURLRewrite on BackendRef #3869
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chris-kaiser-7 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @chris-kaiser-7. 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 Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
chris-kaiser-7
changed the title
chris-kaiser-7
changed the title
|
I tested this with envoy implementation. Envoy had a validation check preventing this. I removed the validation check but the test still failed. I guess envoy doesn't support this. I am going to try on other implementations tho. |
| Features: []features.FeatureName{ | ||
| features.SupportGateway, | ||
| features.SupportHTTPRoute, | ||
| }, |
There was a problem hiding this comment.
This will definitely need an Extended feature at least, since I suspect that implementability will vary wildly.
There was a problem hiding this comment.
We definitely need it, as the feature is extended
There was a problem hiding this comment.
Ok thanks for finding this. I updated this and a few others that had incorrect features.
Unfortunately, I think it will be hard to find an implementation that supports this today. Recommend asking in #sig-network-gateway-api Slack channel. |
There was a problem hiding this comment.
Thanks for the PR, @chris-kaiser-7! I've left a few comments inline
| features.SupportGateway, | ||
| features.SupportHTTPRoute, | ||
| }, | ||
| Manifests: []string{"tests/httproute-rewrite-host-backend-weights.yaml"}, |
There was a problem hiding this comment.
We do need to set the test as provisional with Provisional: true, as this is a new addition to the test suite
There was a problem hiding this comment.
Thanks for pointing this out. I added Provisional to all the rewrite tests. Am I correct to assume all new tests need Provisional set to true and at some release in the future it is removed?
| // Assert request succeeds before checking traffic | ||
| http.MakeRequestAndExpectEventuallyConsistentResponse(t, s.RoundTripper, s.TimeoutConfig, gwAddr, expected) | ||
|
|
||
| for range 100 { |
There was a problem hiding this comment.
why 100? can we extract this number into a const?
There was a problem hiding this comment.
I was using httproute-request-header-modifier-backend-weights.go as a reference for how to do weights, but I will go ahead and update all these to use a const instead of magic numbers.
| Features: []features.FeatureName{ | ||
| features.SupportGateway, | ||
| features.SupportHTTPRoute, | ||
| }, |
There was a problem hiding this comment.
We definitely need it, as the feature is extended
| - backendRefs: | ||
| - name: infra-backend-v1 | ||
| port: 8080 | ||
| weight: 50 | ||
| filters: | ||
| - type: URLRewrite | ||
| urlRewrite: | ||
| hostname: infra-backend-v1.two | ||
| - name: infra-backend-v2 | ||
| port: 8080 | ||
| weight: 50 | ||
| filters: | ||
| - type: URLRewrite | ||
| urlRewrite: | ||
| hostname: infra-backend-v2.two |
There was a problem hiding this comment.
These backendrefs do not look properly indented
…d magic numbers for roundtripper with const
|
Should I add a new features such as SupportHTTPRouteHostRewriteBackend for this? or is SupportHTTPRouteHostRewrite sufficient. |
What type of PR is this?
/kind test
/area conformance-test
What this PR does / why we need it:
Adds conformance tests for HTTPRouteFilterURLRewrite on the BackendRef for Host, Host+Weights, Host+Headers, Path, Path+Weights, Path+Headers. There was no coverage for HTTPRouteFilterURLRewrite on BackendRef.
Which issue(s) this PR fixes:
Fixes #2937
Does this PR introduce a user-facing change?: