api: add CEL validation for service IP fields - #9818
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: 80b7ef2887
ℹ️ 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".
| // +kubebuilder:validation:XValidation:message="allocateLoadBalancerNodePorts can only be set for LoadBalancer type",rule="!has(self.allocateLoadBalancerNodePorts) || self.type == 'LoadBalancer'" | ||
| // +kubebuilder:validation:XValidation:message="loadBalancerSourceRanges can only be set for LoadBalancer type",rule="!has(self.loadBalancerSourceRanges) || self.type == 'LoadBalancer'" | ||
| // +kubebuilder:validation:XValidation:message="loadBalancerIP can only be set for LoadBalancer type",rule="!has(self.loadBalancerIP) || self.type == 'LoadBalancer'" | ||
| // +kubebuilder:validation:XValidation:message="loadBalancerSourceRanges must contain valid CIDR values",rule="!has(self.loadBalancerSourceRanges) || self.loadBalancerSourceRanges.all(r, isCIDR(r))" |
There was a problem hiding this comment.
Because LoadBalancerSourceRanges has no MaxItems constraint, this .all(...) expression has unbounded estimated CEL cost. Kubernetes validates that cost when the generated EnvoyProxy CRD is installed or upgraded and rejects rules whose worst-case cost exceeds the per-expression budget, so applying the Helm CRDs fails before any EnvoyProxy can be admitted. Add an appropriate MaxItems bound (and regenerate the CRDs) before iterating over this field.
Useful? React with 👍 / 👎.
Signed-off-by: Kise Ryota <kiseryota.contact@gmail.com>
80b7ef2 to
cbb0f30
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9818 +/- ##
==========================================
+ Coverage 76.26% 81.28% +5.01%
==========================================
Files 261 263 +2
Lines 44115 40862 -3253
==========================================
- Hits 33646 33214 -432
+ Misses 8245 7648 -597
+ Partials 2224 0 -2224 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // it happens outside of kubernetes and has to be supported and handled by the platform provider. | ||
| // This field may only be set for services with type LoadBalancer and will be cleared if the type | ||
| // is changed to any other type. | ||
| // +kubebuilder:validation:MaxItems=16 |
There was a problem hiding this comment.
lets make this 64 to avoid breaking users
There was a problem hiding this comment.
raising MaxItems alone causes CRD install to fail CEL estimated cost for loadBalancerSourceRanges.all(r, isCIDR(r)), because the string items have no maxLength and are treated as unbounded.
I went with MaxItems=64 plus MaxLength on the items using defined string type, how about this approach instead?
Raising MaxItems alone exceeds CEL estimated cost for isCIDR validation; bound item string length so 64 stays within budget. Signed-off-by: Kise Ryota <kiseryota.contact@gmail.com>
ec8c781 to
fc43488
Compare
|
|
||
| // LoadBalancerSourceRange is a CIDR string allowed in LoadBalancerSourceRanges. | ||
| // MaxLength bounds CEL cost estimation for isCIDR validation over this list. | ||
| // +kubebuilder:validation:MaxLength=64 |
There was a problem hiding this comment.
instead of this, what about // +kubebuilder:validation:items:Format=cidr
There was a problem hiding this comment.
That sounds good, but I didn't know about it, so I'll check it out.
2 participants
What this PR does / why we need it:
This PR adds CEL validation for
KubernetesServiceSpecservice IP fields.It adds
isCIDR()validation forloadBalancerSourceRanges, so invalid CIDR values are rejected by CRD admission before reaching the webhook validation path. This also aligns validation with Kubernetes CEL CIDR semantics, where CIDRsmust be network-address CIDRs. For example,
192.168.0.0/24is valid, while192.168.0.1/24is rejected because host bits are set.ref: https://pkg.go.dev/k8s.io/apiserver/pkg/cel/library#CIDR
It also replaces the existing IPv4 regex for
loadBalancerIPwith Kubernetes CEL IP validation:ref: https://pkg.go.dev/k8s.io/apiserver/pkg/cel/library#IP
This keeps the existing IPv4-only behavior while using the built-in CEL IP parser instead of a hand-written regex.
Which issue(s) this PR fixes:
part-of: #9686
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/.