feat(api): support Replace mergeType in BackendTrafficPolicy and SecurityPolicy#9195
feat(api): support Replace mergeType in BackendTrafficPolicy and SecurityPolicy#9195mehara-rothila wants to merge 7 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
3c38de5 to
a71606f
Compare
![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: 3c38de5808
ℹ️ 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".
a71606f to
800b626
Compare
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
800b626 to
67abb12
Compare
mehara-rothila
changed the title
a462e20 to
fe6e1b9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9195 +/- ##
==========================================
+ Coverage 75.53% 75.59% +0.05%
==========================================
Files 252 252
Lines 41646 41742 +96
==========================================
+ Hits 31457 31554 +97
+ Misses 8062 8061 -1
Partials 2127 2127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fe6e1b9 to
a5607ee
Compare
|
@zhaohuabing I regenerated the three test/helm/gateway-crds-helm/*.out.yaml snapshots that were missing the mergeType CRD change, that was the only thing failing gen-check. The workflows on the latest push are waiting for approval; could you approve the run when you get a chance? Thanks! |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5607eea67
ℹ️ 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".
|
Hi @mehara-rothila sorry for the delay in reviewing this. Overall, this PR looks good. Could you please resolve the conflicts? Thanks! |
15f61b5 to
1879ad7
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1879ad7714
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23e21a257a
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@zhaohuabing Thanks for the review! The conflicts are resolved and the branch is rebased onto the latest main. Since then, the Codex review comments have also been addressed:
Could you please take another look when you get a chance? The CI workflows may also need approval to run since this PR comes from a fork. |
Signed-off-by: mehara-rothila <rothilamehara22@gmail.com>
…ergeType When a route-scoped BackendTrafficPolicy or SecurityPolicy uses mergeType Replace, the later Gateway/Listener policy pass used to back-fill TCP/UDP route fields (load balancer, DNS, timeouts, etc.) and re-apply TCP authorization that the replacing policy had intentionally omitted, making Replace ineffective for those routes. Track TCP/UDP IR routes claimed by a Replace-merged route policy and skip them in the parent policy pass, mirroring the whole-struct guard that HTTP routes already have via ir.HTTPRoute.Traffic/Security. Signed-off-by: mehara-rothila <rothilamehara22@gmail.com>
deabb79 to
1339b07
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1339b07b74
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba81167de6
ℹ️ 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".
With the field-owner merge pattern, references are resolved after the merge and scoped to the owning policy, so a route policy using mergeType Replace is not rejected by an invalid responseOverride body valueRef on the parent Gateway policy it discards. Add a regression fixture covering this case. Signed-off-by: mehara-rothila <rothilamehara22@gmail.com>
The mergeType field contract says it cannot be set when targeting a parent resource (Gateway), and the Gateway translation path ignores it, but unlike SecurityPolicy there was no spec-level validation enforcing this, so a Gateway-targeting policy could set a no-op mergeType and still be accepted. Add the same spec-level CEL rule SecurityPolicy uses, covering targetRef, targetRefs, and targetSelectors, regenerate the CRDs, and add CEL validation tests. Signed-off-by: mehara-rothila <rothilamehara22@gmail.com>
The TCP loop in translateSecurityPolicyForRoute visits every IR route on the listener, so a Replace claim recorded there could suppress the parent policy's authorization on routes belonging to other TCPRoutes sharing the listener. Only claim IR routes whose destination name matches the targeted route's prefix, mirroring the BackendTrafficPolicy path. Signed-off-by: mehara-rothila <rothilamehara22@gmail.com>
The new CEL rule rejects mergeType on Gateway/listener-targeting BackendTrafficPolicies, which were previously accepted with the field silently ignored. Document this as a breaking change. Signed-off-by: mehara-rothila <rothilamehara22@gmail.com>
7e22aa7 to
ed8af11
Compare
|
@zhaohuabing This is ready for another look when you get a chance:
Thanks! |
|
Heads up on CI: Happy to take another look if a rerun still shows something specific to this branch. |
|
Key risk: Replace in shared mergeInternal leaks into the K8s patch path mergeInternal is shared by two very different callers:
KubernetesPatchSpec.Type is a *MergeType with no +kubebuilder:validation:Enum (shared_types.go:579), so type: Replace is admitted by the CRD. Before this PR, such a value hit mergeInternal's default and failed loudly with unsupported merge type: Replace. After this PR, it silently replaces the entire generated resource with the (usually partial) patch value — e.g. a DaemonSet patch would wipe the generated pod spec. The proxy Deployment/Service/HPA/PDB paths are protected by envoyproxy_validate.go (which rejects any type ≠ JSONMerge/StrategicMerge), but I found two unguarded gaps:
So a previously safe (fail-loud) misconfiguration becomes a silent, destructive full-object replacement on those paths. It's an unusual config, but it's a regression introduced as a side effect of a policy-focused PR. Recommended fix (pick one):
|
23893a5 to
1b7dc05
Compare
mergeInternal is shared by utils.Merge (policy merge, the target of this PR) and utils.MergeWithPatch (KubernetesPatchSpec patches for the Envoy proxy and rate limit Deployment/Service/DaemonSet/HPA). Adding Replace to mergeInternal let a KubernetesPatchSpec with type: Replace silently replace the entire generated resource with the partial patch value (for example wiping a generated DaemonSet pod spec) instead of failing loudly as it did before this PR. KubernetesPatchSpec.Type had no enum, so type: Replace was admitted by the CRD; the DaemonSet and rate limit patch paths also lack the runtime patch-type validation that the proxy Deployment/Service/HPA/PDB paths have. Guard both layers: - Add +kubebuilder:validation:Enum=StrategicMerge;JSONMerge to KubernetesPatchSpec.Type so Replace is rejected at admission. - Reject Replace in MergeWithPatch so every resource patch path fails loudly at reconciliation regardless of per-resource validation. Replace remains valid for the policy mergeType fields, which use utils.Merge and have their own enums. Signed-off-by: mehara-rothila <rothilamehara22@gmail.com>
1b7dc05 to
17c0797
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Fixed in 17c0797. You were right that adding Replace to the shared mergeInternal leaked into the KubernetesPatchSpec path. Guarded both layers:
I confirmed the PDB path is unaffected: it uses Added a MergeWithPatch unit test, a CEL-validation test for the DaemonSet Replace rejection, and a breaking-change release note. MergeWithPatch returns the zero value on that error to match the other merge errors. |
What type of PR is this?
api/feat: support Replace mergeType in BackendTrafficPolicy and SecurityPolicy
What this PR does / why we need it:
This PR adds support for the
ReplacemergeTypeinBackendTrafficPolicy(BTP) andSecurityPolicy(SP) resources.Previously:
Replacevia CEL validation rules (self != 'Replace').unsupported merge typeerror whenReplacewas specified.This PR:
Replaceas a valid type.Replacemerge logic ininternal/utils/merge.go, allowing route-level policies to completely discard parent/gateway-level policies whenmergeType: Replaceis specified.Replacemerge type.controller-genand post-processes the Helm templates.Replacemerge type.Which issue(s) this PR fixes:
Fixes #8728
Release Notes: Yes