-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Support URLRewrite in HTTPRoutes #4418
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.
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
base: main
Are you sure you want to change the base?
Support URLRewrite in HTTPRoutes #4418
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zac-nixon 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 |
| match := *httpMatch.Path.Value | ||
|
|
||
| /* | ||
| If we're being asked to replace a prefix with "", we still need to keep one '/' to form a valid path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on explaining this in details.
We should also add good documentation with these samples in our docs for this feature once we implement it with ELB gaps.
| }, | ||
| Filters: []gwv1.HTTPRouteFilter{ | ||
| { | ||
| URLRewrite: &gwv1.HTTPURLRewriteFilter{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred if the spec added this validations instead of us doing this for our implementation. Any reason that spec is not doing this on their side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out route validator is not needed -
# httproutes.gateway.networking.k8s.io "http-app-1" was not valid:
# * spec.rules[0]: Invalid value: "object": When using URLRewrite filter with path.replacePrefixMatch, exactly one PathPrefix match must be specified
|
Can you also add E2E tests for this? |
| case HTTPRouteKind: | ||
| return buildHTTPRuleTransforms(gwRule.CommonRulePrecedence.Rule.GetRawRouteRule().(*gwv1.HTTPRouteRule), gwRule.HTTPMatch) | ||
| default: | ||
| return []elbv2model.Transform{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for any other cases, do we want to add a v(1) logging or warning saying only httpRoute is supported for transform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. This is already documented within the gateway api
| return transforms | ||
| } | ||
|
|
||
| func generateHostHeaderRewriteTransform(hostname gwv1.PreciseHostname) elbv2model.Transform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe rename it to generateHttpHostHeaderRewriteTransform, same for generateURLRewritePathTransform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any explanation here? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it feels like this function is only for HTTPRoute, but it is a nit change
| } | ||
|
|
||
| func generatePrefixReplacementRegex(httpMatch *gwv1.HTTPRouteMatch, replacement string) (string, string) { | ||
| match := *httpMatch.Path.Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did not see where we are checking nil for httpMatch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's done in the routeValidator
| input path = '/foo/', prefixRegex = '(^/foo(/)?)', replacement value = '/cat/$2' results in (again) '/cat//' | ||
| Without the capture group, we would have one '/' too few. | ||
| input path = '/foo/bar', prefixRegex = '(^/foo(/)?)', replacement value = '/cat$2' results in '/catbar' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for all explanation. can you verify if *httpMatch.Path.Value(match) can be something like /foo/ or replacement can be with Trailing Slash? those cases will lead to double slash right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup it can, I actually have an explicit test case for this, name = prefix path rewrite with explicit '/' on suffix. To answer your question, no we won't add a double slash, because we check if the replacement ends with a /, in that case we don't append the optional capture group.
pkg/gateway/routeutils/loader.go
Outdated
|
|
||
| // 2. Remove routes that aren't granted attachment by the listener. | ||
| // 3. Validate route configuration, filter out bad configurations. | ||
| validatedRoutes, validationErrors := l.routeValidator.filterToValidRoutes(gw, foo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you attach a screenshot of this status update in route to show a failed case with correct failed reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out route validator is not needed -
# httproutes.gateway.networking.k8s.io "http-app-1" was not valid:
# * spec.rules[0]: Invalid value: "object": When using URLRewrite filter with path.replacePrefixMatch, exactly one PathPrefix match must be specified
|
E2E test result - |
| }, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we verify the transform are applied in the response by sending the http request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do this for other listener rules, I don't see a reason to do it here.
4439c4a to
3a3e396
Compare
3a3e396 to
fb3074e
Compare
Description
Supports path and host header rewrites using URLRewrite found in HTTPRouteRule. https://gateway-api.sigs.k8s.io/guides/http-redirect-rewrite/#rewrites
I put lots of descriptions in the code. The TL;DR is that we can fully support both path and header based rewrites using the newly launched ALB URLRewrite feature. One piece that still needs figuring out is that ALB has more powerful rewrite capabilities than listed here. Originally, I was going to use the ListenerRuleConfiguration object to specify it. However, to keep this change manageable, I have only implemented the Gateway API spec.
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯