fix: SecurityPolicy, BTP and EEP silently dropped/missing status for routes with ListenerSet parentRef#9410
Draft
EmileGoto wants to merge 12 commits into
Draft
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
EmileGoto
changed the title
When an HTTPRoute uses parentRefs pointing to a ListenerSet, both processSecurityPolicyForRoute and processBackendTrafficPolicyForRoute skipped ListenerSet-kind parents entirely, leaving parentRefCtxs and ancestorRefs empty. With mergeType: StrategicMerge the merge loop iterates parentRefCtxs, so an empty slice silently drops the policy with no status entry. Fix: add an else-if branch for isRefToListenerSet(p) that: - fetches the RouteParentContext (which already holds the underlying Gateway listeners, populated during route attachment) - appends it to parentRefCtxs so the merge loop executes - populates gatewayRouteMap using the backing Gateway NN (derived from listener.gateway), since gatewayPolicyMap is keyed by Gateway NN - adds one ancestor ref per backing Gateway for error status reporting Fixes envoyproxy#9409 Signed-off-by: Emile <emile.boutin@goto.com>
EmileGoto
force-pushed
the
fix/listenerset-strategicmerge-policy-9409
branch
from
ce712f8 to
8ec1954
Compare
…tenerSet route Add two golden file test pairs that cover the case where an HTTPRoute has a parentRef pointing to a ListenerSet (not a Gateway directly) and is subject to a StrategicMerge policy: - securitypolicy-with-merge-listenerset-route: gateway-level BasicAuth merged with a route-level CORS SecurityPolicy - backendtrafficpolicy-strategic-merge-listenerset-route: gateway-level timeout merged with a route-level timeout override and bufferLimit Both mirror their existing non-ListenerSet counterparts and fail without the accompanying fix. Signed-off-by: Emile <emile.boutin@goto.com>
Signed-off-by: Emile <emile.boutin@goto.com>
EmileGoto
changed the title
Same ListenerSet-kind parentRef was skipped in processEnvoyExtensionPolicyForRoute, causing all status conditions (Accepted, Overridden, error reporting) to be silently lost for routes attached via a ListenerSet. Add the same isRefToListenerSet branch to populate gatewayRouteMap and ancestorRefs from the backing Gateway listeners. Add testdata coverage: envoyextensionpolicy-listenerset-route. Signed-off-by: Emile <emile.boutin@goto.com>
Move GetGateway() call before parentRefCtxs append in all three policy functions (SecurityPolicy, BackendTrafficPolicy, EnvoyExtensionPolicy). Skip the ListenerSet parentRef early if the backing Gateway context is nil, avoiding the inner conditional for the ancestorRef append. Signed-off-by: Emile <emile.boutin@goto.com>
![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: 34c327deb1
ℹ️ 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".
EmileGoto
force-pushed
the
fix/listenerset-strategicmerge-policy-9409
branch
from
b7f6a94 to
09e54a7
Compare
EmileGoto
changed the title
…nerSet parentRef When a policy (BackendTrafficPolicy, SecurityPolicy, EnvoyExtensionPolicy) targets an HTTPRoute that uses a ListenerSet parentRef, the policy status ancestors were silently dropped because the translation path had no handling for ListenerSet parentRefs. - Add else-if branch for isRefToListenerSet in BTP, SP and EEP route processing - Return backing GatewayContext from GetReferencedListeners for status use - Add GetBackingGateway() on RouteParentContext for policy ancestor reporting - Store backingGateway on RouteParentContext when no listeners match/are allowed - Report Accepted=False/TargetNotFound with Gateway ancestor when no listeners in the ListenerSet match the parentRef sectionName Fixes: envoyproxy#9409 Signed-off-by: Emile <emile.boutin@goto.com>
…testdata to targetRefs Update existing test fixture to use the current targetRefs field instead of the deprecated targetRef singular field. Signed-off-by: Emile <emile.boutin@goto.com>
EmileGoto
force-pushed
the
fix/listenerset-strategicmerge-policy-9409
branch
from
b0f886d to
7a405dc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 255ed87ea4
ℹ️ 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 parentRefCtx := targetedRoute.GetRouteParentContext(p); parentRefCtx != nil { | ||
| parentRefCtxs = append(parentRefCtxs, parentRefCtx) | ||
| } | ||
| } else if isRefToListenerSet(p) { |
There was a problem hiding this comment.
For SecurityPolicy routes whose parentRef is a ListenerSet, this new branch is never reached because the immediately preceding else if *p.Kind == resource.KindListenerSet matches every ListenerSet parentRef first. In the no-matching-listener case, the earlier branch appends a ListenerSet ancestor and a parent context with no listeners, so the merge loop is skipped and the final accepted-status helper marks the policy accepted instead of using the new Gateway TargetNotFound status added here; the new securitypolicy-with-merge-listenerset-route-no-matching-listener expected status therefore is not produced by this code path.
Useful? React with 👍 / 👎.
Signed-off-by: Emile <emile.boutin@goto.com>
EmileGoto
force-pushed
the
fix/listenerset-strategicmerge-policy-9409
branch
from
a7f4300 to
40d2960
Compare
Signed-off-by: Emile <emile.boutin@goto.com>
EmileGoto
force-pushed
the
fix/listenerset-strategicmerge-policy-9409
branch
from
dd7dbf3 to
6f48cc8
Compare
…dant test The *p.Kind == resource.KindListenerSet block in processSecurityPolicyForRoute now uses the same early-continue guard pattern as processBackendTrafficPolicyForRoute and processEnvoyExtensionPolicyForRoute: - check parentRefCtx == nil → continue - check GetGateway() == nil → TargetNotFound + continue - append to parentRefCtxs The securitypolicy-with-merge-listenerset-route test fixtures are removed: PR envoyproxy#9270 already covers StrategicMerge with ListenerSet-attached routes via the securitypolicy-listenerset-merge test (7 policy scenarios). Signed-off-by: Emile <emile.boutin@goto.com>
…P, EEP
The nil-parentRefCtx case (wrong GatewayClass, silent skip) is
structurally distinct from the nil-gateway case (our GatewayClass but
no listeners matched, must report TargetNotFound). Use the nested
if parentRefCtx := ...; parentRefCtx != nil {
if parentRefCtx.GetGateway() == nil { ... } else { ... }
}
pattern in all three policy translators so the silent-skip branch falls
off naturally without an explicit continue.
Signed-off-by: Emile <emile.boutin@goto.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Bug fix.
What this PR does / why we need it
When an HTTPRoute used parentRefs that pointed to a ListenerSet, processSecurityPolicyForRoute, processBackendTrafficPolicyForRoute, and processEnvoyExtensionPolicyForRoute skipped those parents while iterating over parentRefs. They only handled cases where Kind == nil or Kind == Gateway, so parentRefCtxs and ancestorRefs remained empty.
For policies using mergeType: StrategicMerge, this caused the merge loop to iterate over an empty parentRefCtxs slice, silently dropping the policy without setting any status on the route.
For EnvoyExtensionPolicy, which does not use StrategicMerge, the policy was still applied, but all related status conditions were lost, including Accepted, error reporting, and overridden cross-notification.
This PR adds an else if isRefToListenerSet(p) branch to all three functions that:
fetches the RouteParentContext for the ListenerSet parentRef,
appends it to parentRefCtxs so the merge logic can run,
populates gatewayRouteMap using the backing Gateway NamespacedName, since gatewayPolicyMap is keyed by Gateway rather than ListenerSet,
adds one ancestor ref per backing Gateway for status and error reporting.
Which issue(s) this PR fixes
Fixes #9409
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass.release-notes/current/<section>/<pr-number>-<slug>.md. N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.