Repository navigation
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9026 +/- ##
==========================================
+ Coverage 75.11% 75.15% +0.04%
==========================================
Files 252 252
Lines 40882 41008 +126
==========================================
+ Hits 30707 30820 +113
- Misses 8084 8092 +8
- Partials 2091 2096 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
1 similar comment
|
/retest |
|
@codex review |
![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: 1d48a9bfb7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@jukie looks like the GHA for the OSV scanner is erroring on Go version |
|
/retest |
|
Can you also read through GEP-1713 and make sure conflicts/override behavior is being handled correctly? For example we may need some additional handling in listener.go or validate.go to filter out An example that may be useful to walk through is two CTPs targeting the same resulting listerner(s). CTP A targeting a ListernerSet, and CTP B targeting a Gateway. For each scenario also consider sectionName handling. |
I reviewed the conflict/override behavior for CTPs targeting ListenerSets vs Gateways. The existing tests already cover:
Added two missing sectionName variants in commit 5f69223:
Is there a specific scenario you had in mind that isn't covered by the above? |
|
/retest |
|
/retest |
Thanks for confirming the logic, what you have above looks right to me. I'll give this another deep look but overall looks good once the last review comments are addressed. |
53c7d4d to
de1f690
Compare
|
/retest |
de1f690 to
eab7bb2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ad431064a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if policyTargetsLS { | ||
| if claimedSections[gatewayKey] == nil { | ||
| claimedSections[gatewayKey] = sets.New[string]() | ||
| } | ||
| claimedSections[gatewayKey].Insert(fmt.Sprintf("%s%s", lsPrefix(ls), string(l.Name))) |
There was a problem hiding this comment.
When a ListenerSet-wide policy fails validatePortOverlapForClientTrafficPolicy for a listener (for example, a non-TLS ListenerSet listener sharing a port with a Gateway listener) and a Gateway-wide ClientTrafficPolicy also targets the parent, this insertion never runs because it is inside the success branch. The later Gateway-wide pass then sees the ListenerSet listener as unclaimed and translates the broader policy onto it instead of reporting it as overridden, breaking the intended ListenerSet-wide > Gateway-wide precedence. Claim the ListenerSet listener before port validation, as sectionName policies already do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The more specific policy should only override when it is accepted/effective; if it fails validation, allowing the Gateway-wide policy to apply as fallback seems reasonable.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I believe they are all useful. The majority of them were added 1 or 2 at a time as changes were requested and PR feedback came in. Each time the code was changed to address a bug or invariant that was not being enforced, I added a test to ensure the fix worked as intended. I think the new practice of adding the comment at the top of the test file to describe the purpose of the test, in the future, can help us decide at the end of a review if some tests can be removed at the end. |
| - lastTransitionTime: null | ||
| message: There are existing ClientTrafficPolicies that are overriding these |
There was a problem hiding this comment.
nits:
Would the following message provide a better user experience?
message: There are existing ClientTrafficPolicies that are overriding these sections [envoy-gateway/ext-listeners/ext-http-1 envoy-gateway/ext-listeners/ext-http-2]
|
LGTM, thanks! I left one nits comment(not-blocking). we could do follow-up PR. |
Signed-off-by: Isaac Wilson <[email protected]>
| for _, passKind := range []gwapiv1.Kind{resource.KindListenerSet, resource.KindGateway} { | ||
| for i, currPolicy := range clientTrafficPolicies { | ||
| policyName := utils.NamespacedName(currPolicy) | ||
| targetRefs := resolvePolicyTargetsForGatewayAndListenerSet( | ||
| currPolicy.Spec.PolicyTargetReferences, | ||
| gateways, | ||
| resources.ListenerSets, | ||
| resources.ReferenceGrants, | ||
| egv1a1.GroupName, | ||
| egv1a1.KindClientTrafficPolicy, | ||
| currPolicy.Namespace, | ||
| t.GetNamespace, | ||
| ) |
There was a problem hiding this comment.
Non-blocking: one last perf observation is that we we're processing every CTP twice, once for ListenerSets and a second for Gateways. I think we can split the clientTrafficPolicies loop and resolve policy targets before the passKind loop.
Something like this:
policyTargets := make([][]policyTargetReferenceWithSectionName, len(clientTrafficPolicies))
for i, p := range clientTrafficPolicies {
policyTargets[i] = resolvePolicyTargetsForGatewayAndListenerSet(
p.Spec.PolicyTargetReferences, gateways, resources.ListenerSets,
resources.ReferenceGrants, egv1a1.GroupName, egv1a1.KindClientTrafficPolicy,
p.Namespace, t.GetNamespace,
)
}
for _, passKind := range []gwapiv1.Kind{resource.KindListenerSet, resource.KindGateway} {
for i := range clientTrafficPolicies {
for _, currTarget := range policyTargets[i] {
...
}
}
}
|
/retest |
envoyproxy#9026) * feat(policy): support ListenerSet as targetRef for ClientTrafficPolicy Allow ClientTrafficPolicy to target a ListenerSet in addition to a Gateway, enabling per-listener-group traffic policy without applying settings cluster-wide. Supports both whole-ListenerSet and sectionName-scoped targeting with correct conflict detection. Signed-off-by: apkatsikas <[email protected]> * refactor(policy): merge sectionName CTP processing passes into one loop Signed-off-by: apkatsikas <[email protected]> * refactor(policy): collapse no-sectionName CTP processing passes into one loop Signed-off-by: apkatsikas <[email protected]> * chore: regenerate helm CRD snapshots for ListenerSet targetRef CEL rules Signed-off-by: apkatsikas <[email protected]> * fix(policy): enforce mutual exclusion between ListenerSet-wide and Gateway-wide CTPs When a ListenerSet-wide CTP applies to a listener, record the individual listener name in policyMap so a later Gateway-wide CTP correctly skips it. When a Gateway-wide CTP runs first (AllSections="/"), skip LS listeners in the ListenerSet-wide pass. Covers both interaction orderings with new golden file tests. Signed-off-by: apkatsikas <[email protected]> * refactor(policy): unify targetRef resolution into a single function Replace the separate resolveClientTrafficPolicyTargetRefForListenerSet with a ctpResolvedTarget struct and a single resolveClientTrafficPolicyTargetRef that dispatches on targetRef.Kind internally, reducing the three-return signature to the conventional (result, error) pair. Signed-off-by: apkatsikas <[email protected]> * fix(policy): enforce ListenerSet-wide > Gateway-wide priority regardless of input order Process ListenerSet-wide CTPs before Gateway-wide in the no-sectionName pass by iterating twice with a processLS filter, matching the more-specific-wins principle already established by sectionName policies. Adds golden file tests covering both input orderings. Signed-off-by: apkatsikas <[email protected]> * chore: add release notes for ListenerSet targetRef support in CTP Signed-off-by: apkatsikas <[email protected]> * fix(policy): remove internal sentinel from CTP Overridden status message When a ListenerSet-wide CTP was overridden by a Gateway-wide CTP, the Overridden message incorrectly included the internal listenerset/... scope sentinel alongside real listener names, e.g.: sections [ext-http-1 ext-http-2 listenerset/envoy-gateway/ext-listeners] Fix by splitting policyMap into two maps: - policyMap: whole-scope sentinels only (Conflicted detection) - claimedListeners: listener names only (skip logic + Overridden messages) The message now correctly shows only real listener names. Signed-off-by: apkatsikas <[email protected]> * refactor(policy): rename listenersDontBelongToLS to positive listenersBelongToLS Signed-off-by: apkatsikas <[email protected]> * refactor(policy): hoist passKind check before handledPolicies registration Update fixture to reflect new output ordering: LS-wide policies are now appended to res in pass 1, GW-wide in pass 2. Conditions and XdsIR unchanged. Signed-off-by: apkatsikas <[email protected]> * fix(policy): scope claimedSections keys to prevent false conflicts across ListenerSets Listeners with the same name in different ListenerSets (or a Gateway listener sharing a name with a ListenerSet listener) would incorrectly produce spurious Conflicted/Overridden status and skip translation. Scope claim keys by LS identity to isolate claims per target. Add test fixtures covering wide and sectionName variants of the collision scenario. Signed-off-by: apkatsikas <[email protected]> * chore: fix copyloopvar lint error in claimedSections loop Signed-off-by: apkatsikas <[email protected]> * refactor: precompute listenerSet map to avoid O(n) scan per CTP Signed-off-by: apkatsikas <[email protected]> * fix(policy): validate port overlap per-target scope for LS-wide CTPs Introduces ctpAttachScope enum (scopeSpecificSection, scopeEntireGateway, scopeEntireListenerSet) to replace the attachedToGateway bool in validatePortOverlapForClientTrafficPolicy. LS-wide CTPs now correctly allow same-port HTTP listeners within the same ListenerSet, matching the existing gateway-wide behavior. Signed-off-by: apkatsikas <[email protected]> * test: add CTP LS sectionName conflict/override coverage Signed-off-by: apkatsikas <[email protected]> * fix(policy): use ListenerSet as ancestorRef when CTP targets a ListenerSet Per GEP-713, the ancestorRef should be the ancestor where status meaningfully differs. When a CTP directly targets a ListenerSet, that is the ListenerSet itself, not the parent Gateway. Adds a mixed-type test confirming two distinct ancestor entries when a single CTP targets both a Gateway and a ListenerSet. Signed-off-by: apkatsikas <[email protected]> * fix(policy): support ListenerSet targetSelectors in ClientTrafficPolicy Add resolvePolicyTargetsForGatewayAndListenerSet helper that runs selector resolution against both Gateways and ListenerSets, fixing the gap where targetSelectors with kind: ListenerSet silently matched nothing. Add two test fixtures covering the LS-only and mixed Gateway+ListenerSet selector cases. Signed-off-by: apkatsikas <[email protected]> * test(cel): update CTP CEL validation tests for ListenerSet kind Fix stale error message strings that still referenced Gateway-only, and add a positive test case for kind: ListenerSet. Signed-off-by: apkatsikas <[email protected]> * fix: resolve appendAssign lint error in resolvePolicyTargetsForGatewayAndListenerSet Signed-off-by: apkatsikas <[email protected]> * fix(policy): add trailing slash to ListenerSet prefix to prevent false prefix matches in port overlap validation A ListenerSet named ext would falsely match listeners from ext-b in strings.Index comparisons. Adding a trailing slash to the extracted prefix ensures exact boundary matching. Adds a test fixture covering this scenario. Signed-off-by: apkatsikas <[email protected]> * test(e2e): add e2e test for ClientTrafficPolicy targeting ListenerSet Signed-off-by: apkatsikas <[email protected]> * fix(e2e): wait for route parents before dialing in ListenerSetClientTrafficPolicy test Signed-off-by: apkatsikas <[email protected]> * fix(e2e): poll until listener accepts TLS before version checks in ListenerSetClientTrafficPolicy test Signed-off-by: apkatsikas <[email protected]> --------- Signed-off-by: apkatsikas <[email protected]> Signed-off-by: Isaac Wilson <[email protected]> Co-authored-by: Isaac Wilson <[email protected]>
What type of PR is this?
feature
What this PR does / why we need it
Adds
ListenerSetas a validtargetRefkind forClientTrafficPolicy, alongside the existingGateway.Priority follows the existing more-specific-wins model: sectionName policies > ListenerSet-wide > Gateway-wide.
Changes:
kind == 'Gateway'→kind in ['Gateway', 'ListenerSet']before Gateway-wide to enforce priority regardless of input order
claimedScopestracks whole-scope claims (Gateway-wide, ListenerSet-wide);claimedSectionstracks individual listeners claimed by sectionName or ListenerSet-wide policies, keyed by a scope-qualified key (lsPrefix + listenerNamefor ListenerSet listeners, bare name for Gateway listeners) so that same-named listeners across different ListenerSets — or a Gateway listener sharing a name with a ListenerSet listener — do not produce false Conflicted/Overridden statusmake manifests,make helm-template.gateway-crds-helm)Known gap / follow-up:
ListenerSetis missing from the upstreamsectionNamefield description ingwapiv1.LocalPolicyTargetReferenceWithSectionName, PR opened here. CEL rules in this PR enforce valid kinds at admission time regardless.Local verification: KIND cluster with dev EG image, CTP with
tls.minVersion: "1.3"targeting a ListenerSet HTTPS listener:Gateway's own HTTP listener was unaffected — only the ListenerSet's HTTPS listener had the TLS restriction applied.
Which issue(s) this PR fixes
#8092
Release Notes
Yes
Reviewing the diff
clienttrafficpolicy.go- to review only meaningful changes, append?w=1to the GitHub PR URL.