Repository navigation
Conversation
… per backend A backendRef's filters are translated onto its weighted cluster entry, where Envoy only supports a literal host rewrite. A path rewrite, or a hostname rewrite of type Header, Backend or PathRegex set through an HTTPRouteFilter, was accepted, reported ResolvedRefs=True, and then silently dropped during xDS translation. Reject these in processDestinationFilters, where extension filters are already resolved, with the InvalidBackendFilters reason used for other unsupported backendRef filters, so the route status says why. Fixes envoyproxy#7099 Signed-off-by: Lakhan Yelgate <39281685+lyelgate@users.noreply.github.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c3c7af161
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {{% alert title="Note" color="primary" %}} | ||
| A URL rewrite in the `filters` of a `backendRef` only supports rewriting the hostname to a fixed value. Path | ||
| rewrites, and hostname rewrites of type `Header`, `Backend` or `PathRegex`, are only supported in the filters of a | ||
| route rule; when used on a `backendRef`, that backendRef is rejected with an `InvalidBackendFilters` condition. |
There was a problem hiding this comment.
For a standard Gateway API URLRewrite.path, validateBackendRefFilters returns RouteReasonUnsupportedRefValue (internal/gatewayapi/validate.go:114-120), so the status is ResolvedRefs=False with reason UnsupportedRefValue; InvalidBackendFilters is only the reason used by the new post-resolution HTTPRouteFilter check (as shown in the new golden output at lines 76-80), and in neither case is it the condition type. This blanket statement can therefore make operators look for a condition/reason that is never emitted for the standard path form; describe ResolvedRefs=False and distinguish the two reasons, or narrow the statement to extension rewrites.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. InvalidBackendFilters is the reason, not the condition, and the Gateway API path rewrite goes through the earlier validateBackendRefFilters check with UnsupportedRefValue. Updated the note and the release note to describe ResolvedRefs: False with both reasons in 2c65d54.
…ckendRef rewrites InvalidBackendFilters is the reason, not the condition, and only applies to rewrites set through an HTTPRouteFilter. A Gateway API URLRewrite path rewrite on a backendRef is rejected earlier with reason UnsupportedRefValue. Signed-off-by: Lakhan Yelgate <39281685+lyelgate@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c65d54d90
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if httpFiltersContext.GRPCJSONTranscoder != nil { | ||
| return nil, nil, errors.New("grpcJSONTranscoder is not supported in the filters of a backendRef") | ||
| } | ||
| if err := validateBackendRefURLRewrite(httpFiltersContext.URLRewrite); err != nil { |
There was a problem hiding this comment.
When a backendRef has weight 0, or its Service currently has no ready endpoints, processDestination returns before processDestinationFilters is called, so this new check never runs. The same Header/Backend/PathRegex/regex-path configuration then reports ResolvedRefs=True (only BackendsAvailable=False in the no-endpoint case), and once endpoints appear it unexpectedly flips to InvalidBackendFilters and begins routing that backend's share to 500 responses. Resolve and validate these extension rewrites before the weight and endpoint early exits so configuration validity does not depend on runtime backend health.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that the check only runs once a backendRef has weight and resolvable endpoints. That ordering isn't specific to this check: every backendRef filter error, including the existing grpcJSONTranscoder check and unresolvable HTTPRouteFilter references, is only reported after the weight and endpoint early returns in processDestination. Moving filter validation ahead of those would change status for all of them, so I'd rather keep this PR scoped and handle the ordering in a follow-up if maintainers want it.
What this PR does / why we need it:
A URL rewrite in the
filtersof an HTTPRoutebackendRefis translated onto that backend'sWeightedCluster.ClusterWeightentry (applyWeightedClusterFiltersininternal/xds/translator/route.go). Envoy only supportshost_rewrite_literalthere, so a literal hostname is the only rewrite that can be applied per backend.The other rewrites were accepted and then dropped.
buildDestinationFilterscopiesURLRewriteintoDestinationFilters, the route reportsResolvedRefs: True, and the xDS translator ignores everything exceptHost.Name. Users see a route that looks healthy but doesn't rewrite anything, which is what #7099 reports fortype: Header.URLRewritewith a literalhostnameURLRewritewith apathvalidateBackendRefFiltersHTTPRouteFilterhostnametype: HeaderHTTPRouteFilterhostnametype: BackendHTTPRouteFilterhostnametype: PathRegexHTTPRouteFilterpathtype: ReplaceRegexMatchvalidateBackendRefFiltersalready rejects a Gateway API path rewrite on a backendRef, but it only looks at the filter in the route spec, so it can't see what anExtensionRefto anHTTPRouteFilterresolves to. This PR addsvalidateBackendRefURLRewrite, called fromprocessDestinationFiltersonce extension filters are resolved, next to the existinggrpcJSONTranscodercheck. An unsupported rewrite surfaces the same way:ResolvedRefs: Falsewith reasonInvalidBackendFilters, and a message that names it:This follows @zirain's suggestion on the issue to report the limitation through status. Routes don't have a non-fatal
Warningcondition yet (#9630 is discussing one), so this reuses the reason the translator already uses for unsupported backendRef filters instead of adding a new one.Behavior change worth a look: a backendRef that carries one of the four rejected rewrites currently receives traffic with the rewrite ignored. With this change it is treated like any other
InvalidBackendFiltersbackendRef, so its share of traffic gets a 500. I think a visible failure is better than a configuration that reports success and does something else, and it matches how a Gateway API path rewrite on a backendRef is already handled. If you would rather keep those backends serving and only report the problem, I'm happy to switch to a non-fatal condition once #9630 settles the shape.Out of scope:
appendXForwardedHostisn't honored per backend either (ClusterWeighthas no equivalent field). Rejecting it would break the supported literal-hostname case, since it defaults to true, so it is left as is.Changes:
internal/gatewayapi/route.go: addvalidateBackendRefURLRewriteand call it fromprocessDestinationFilters.internal/gatewayapi/testdata/httproutefilter-with-urlrewrite-on-backendref.{in,out}.yaml: one HTTPRoute per rejected rewrite (Header, Backend and PathRegex hostname, ReplaceRegexMatch path), plus a literal-hostname route as a control that still resolves and keepsurlRewrite.host.namein the IR.site/content/en/latest/tasks/traffic/http-urlrewrite.md: note which rewrites are supported on a backendRef.release-notes/current/bug_fixes/7099-backendref-urlrewrite-validation.md.Testing:
mainwithout the change and passes with it.go test ./internal/...passes.go test ./internal/gatewayapi ./internal/xds/translator --override-testdata=trueleaves no diff in existing golden files.golangci-lint(repo config, CI build tags) on./internal/gatewayapi/...,codespell,yamllintandtools/boilerplate/verify-boilerplate.share clean.validateBackendRefURLRewritehas 100% coverage.Which issue(s) this PR fixes:
Fixes #7099
PR Checklist
git commit -s). See DCO: Sign your work./api.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.) Locally I ran the parts this change touches (listed under Testing); the full targets will run in CI.release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.release-notes/current/breaking_changes/.