feat: support defaultChildMergeType for BackendTrafficPolicy - #9374
feat: support defaultChildMergeType for BackendTrafficPolicy#9374H34rtUnd3rB14d3 wants to merge 10 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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: e22e13f6f6
ℹ️ 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 policy.Spec.MergeType == nil { | ||
| if policy.Spec.MergeType == nil && !t.anyGatewayMergeDefault(policy, parentRefCtxs) { |
There was a problem hiding this comment.
When defaultMergeType sends an otherwise-unset policy into the per-listener merge path, MergeGateways mode makes t.IRKey(gwNN) the same for every Gateway and the later route application filters only by listener sectionName. For a route attached to two Gateways that both have a listener named http but different parent BackendTrafficPolicies, the first merged parent policy is applied to both listeners and the second is skipped because the route traffic is already set. Include the Gateway identity when applying defaulted merged route policies in merged-Gateway IR.
Useful? React with 👍 / 👎.
| // +kubebuilder:validation:Enum=StrategicMerge;JSONMerge | ||
| // +optional | ||
| DefaultMergeType *MergeType `json:"defaultMergeType,omitempty"` |
There was a problem hiding this comment.
The enum marker protects EnvoyProxy CRs, but this same EnvoyProxySpec is also used as the EnvoyGateway default spec, which is loaded through ValidateEnvoyGateway rather than CRD validation. A config such as envoyProxy.backendTrafficPolicy.defaultMergeType: Replace is therefore accepted and effectiveMergeType will use Replace, causing the child policy to replace the parent while statuses record it as merged. Add runtime validation for the EnvoyGateway/default-spec path as well.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9374 +/- ##
==========================================
- Coverage 76.36% 76.35% -0.01%
==========================================
Files 261 261
Lines 44537 44592 +55
==========================================
+ Hits 34011 34050 +39
- Misses 8279 8294 +15
- Partials 2247 2248 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // BackendTrafficPolicy defines defaults applied to BackendTrafficPolicy resources | ||
| // attached to Gateways that use this EnvoyProxy. | ||
| // +optional | ||
| BackendTrafficPolicy *PolicyDefaults `json:"backendTrafficPolicy,omitempty"` |
There was a problem hiding this comment.
should we add some things like following for better extension?
policyMergeType:
backendTrafficPolicy:
securitryPolicy: #we could implament this in the future.cc @envoyproxy/gateway-maintainers WDYT?
There was a problem hiding this comment.
IMHO, if we decide to do that, I'd name the parent policyDefaults rather than policyMergeType, since each entry also carries excludeLabel and may grow beyond merge settings later.
There was a problem hiding this comment.
policyDefaults sounds good to me, cc @envoyproxy/gateway-maintainers
There was a problem hiding this comment.
I propose this:
policyDefaults:
backendTrafficPolicy:
mergeType: StrategicMerge
mergeExcludeLabel: gateway.envoyproxy.io/skip-merge-default
foo: bar # additional BTP-specific default
securityPolicy:
mergeType: StrategicMerge
mergeExcludeLabel: gateway.envoyproxy.io/skip-merge-defaultand these structs:
type PolicyDefaults struct {
BackendTrafficPolicy *BackendTrafficPolicyDefaults `json:"backendTrafficPolicy,omitempty"`
// SecurityPolicy *SecurityPolicyDefaults `json:"securityPolicy,omitempty"` // future
}
// Per-policy and extensible: BTP-only defaults live here alongside the shared merge settings.
type BackendTrafficPolicyDefaults struct {
MergeSettings `json:",inline"`
}
type MergeSettings struct {
MergeType *MergeType `json:"mergeType,omitempty"`
MergeExcludeLabel *string `json:"mergeExcludeLabel,omitempty"`
}This way every policy can reuse MergeSettings while still having its own policy-specific defaults, and we don't need to nest the settings any deeper.
There was a problem hiding this comment.
sound goo to me, can you join the meeting and discuss with other maintainers?
cc @envoyproxy/gateway-maintainers
There was a problem hiding this comment.
+1 - done. defaultChildMergeType is now accepted only on whole-Gateway targets (no sectionName) and rejected on Listener-scoped targets via CEL, with a test.
The Gateway-level default still applies to all children, including routes under a Listener.
There was a problem hiding this comment.
cc @arkodg could you take a look at the API shape here?
There was a problem hiding this comment.
However, supporting a different defaultChildMergeType at different parent levels adds complexity and becomes confusing without a clear benefit. I'd restrict it to the top parent policy only and reject it on any intermediate parent.
sounds good to me! wait for comments from other maintainers.
There was a problem hiding this comment.
Friendly ping @arkodg. Just checking if there are any updates on the API review. Happy to address any feedback if needed.
There was a problem hiding this comment.
this warrants a longer & wider discussion, hey @zhaohuabing lets bring this into a future community meeting
A route-level BackendTrafficPolicy that leaves mergeType unset currently replaces its parent policy rather than merging into it. That means anyone who wants merging has to remember to set mergeType on every policy. This adds a backendTrafficPolicy section to the EnvoyProxy API with a defaultMergeType field. When a route-level policy doesn't set its own mergeType, it picks up this default and merges into the parent instead of replacing it. An explicit mergeType still wins, an optional excludeLabel lets a policy opt out, and policies in the control-plane namespace are left untouched. defaultMergeType only accepts StrategicMerge or JSONMerge. Fixes envoyproxy#9334 Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
Rename the EnvoyProxy policy-defaults type from BackendTrafficPolicyDefaults to a reusable PolicyDefaults, so other xPolicies can adopt the same shape. The backendTrafficPolicy field is unchanged; only the type name changes. Also fixes two review findings: - Validate defaultMergeType on the EnvoyGateway default EnvoyProxySpec. That spec is loaded via config (ValidateEnvoyGateway), not CRD admission, so the enum does not apply and a value like Replace could slip through. effectiveMergeType also defensively ignores a non-merge value, so a stray default can never report a "merged" status while actually replacing. - Scope a defaulted merged route policy to its own Gateway's listeners. In MergeGateways mode all Gateways share one IR keyed by section name, so a route attached to two Gateways with different parent policies received the first Gateway's merge on both. applyTrafficFeatureToRoute now filters listeners by the target Gateway. Adds a merged-gateways golden fixture. Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
Add a unit test for applyTrafficFeatureToRoute exercising the TCP and UDP listener skip branches when a route policy targets a different Gateway under the shared MergeGateways IR. The HTTP path is already covered by the merged-gateways golden fixture; this brings the new scoping code to full patch coverage. Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
…ergeSettings Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
e942762 to
0e3e12d
Compare
Replace the EnvoyProxy policyDefaults API with a defaultChildMergeType field on Gateway-targeting BackendTrafficPolicies, following GEP-713. A child policy without its own mergeType merges into the closest parent policy using its defaultChildMergeType; an explicit mergeType (including Replace, allowed by a separate change) always wins. Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
0d3188c to
0d61eab
Compare
Reject defaultChildMergeType on Listener (sectionName) targets via CEL so the child merge default is declared only at the top-level Gateway. The default that children inherit always comes from the Gateway-level policy Add CEL, unit, and golden test coverage. Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
0d61eab to
72c5161
Compare
…voyproxy Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru> # Conflicts: # internal/gatewayapi/backendtrafficpolicy.go
…voyproxy Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru> # Conflicts: # api/v1alpha1/backendtrafficpolicy_types.go # charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml # charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml # internal/gatewayapi/backendtrafficpolicy.go # site/content/en/latest/api/extension_types.md # site/content/en/latest/concepts/gateway_api_extensions/backend-traffic-policy.md # test/helm/gateway-crds-helm/all.out.yaml # test/helm/gateway-crds-helm/e2e.out.yaml # test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml
H34rtUnd3rB14d3
changed the title
…voyproxy Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
The deduplication index treats a route policy with mergeType unset as replacing its parent, which no longer implies a replace once the parent sets defaultChildMergeType. Pin the resulting behavior in a golden fixture so a change on either side surfaces as a diff. Signed-off-by: Maksim Kuchkovskiy <K.Maksim.E@yandex.ru>
|
Noticed a small interaction with MergeBackends while rebasing on main: the dedup index treats mergeType unset as "replaces parent", which isn't quite true anymore when the parent sets defaultChildMergeType. The result is just a missed dedup (the route gets its own cluster even though its effective settings match), not a wrong config. Added a golden fixture to pin the current behavior. If you think there's a better way to handle this, I'm happy to adjust the implementation. |
|
Hi @H34rtUnd3rB14d3 feel free to add this to the community agenda and join the meeting to discuss. |
5 participants
What this PR does / why we need it:
Right now, if a route-level BackendTrafficPolicy doesn't set mergeType, it replaces the
gateway- or listener-level policy instead of merging into it. So if you keep a baseline policy on
the Gateway and add route-level policies on top, you have to remember to put mergeType on every
single one of them, or the baseline silently gets dropped for that route.
This PR lets the parent policy declare that default once, following GEP-713:
With that in place, a route-level policy that doesn't set its own mergeType picks up the default
and merges into its parent. A few rules that keep it predictable:
An explicit mergeType on the child always wins; mergeType: Replace is an explicit opt-out.
defaultChildMergeType may only be set on a policy targeting an entire Gateway. It is rejected by CEL on Listener-scoped targets (sectionName) and on ListenerSet targets, so the default is declared at a single top-level parent instead of at several intermediate ones.
The default and the merge target are resolved separately: a child merges into the closest parent policy in its attachment hierarchy (a Listener- or ListenerSet-level policy if one exists, otherwise the Gateway-level policy), while the default strategy always comes from the Gateway-level policy. This keeps a Listener-level policy from silently suppressing the Gateway's default.
defaultChildMergeType only accepts StrategicMerge or JSONMerge — the field's enum rejects Replace, since defaulting to a replace wouldn't do anything.
Nothing is written to the policy object — the default is resolved at translation time, so
kubectl get btp -o yaml still shows no mergeType. The effect is visible in the policy
status (Merged / Overridden) and in the generated Envoy config. When no parent sets
defaultChildMergeType, behavior is exactly the same as today, so this is fully backward compatible.
Implementation is in internal/gatewayapi/backendtrafficpolicy.go (resolveDefaultChildMergeType /
effectiveMergeType / anyParentPolicyMergeDefault); no new webhook, CRD, or controller configuration.
Tests: unit tests for the resolution helpers, golden translator fixtures covering the basic merge,
Replace opt-out, multi-parent, MergeGateways scoping, and the Gateway-default-with-Listener-parent
case, plus CEL validation for the field and its target restrictions. Verified end to end in a Kind
cluster (live CEL rejection, policy statuses, and the merged Envoy cluster config, including a control
test confirming the merge stops when the default is removed).
Which issue(s) this PR fixes:
Fixes #9334
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.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.)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.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.