enhancement(pni-crd): enforce immutable spec and add property for IP constraints #4143
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds immutability validation to the PodNetworkInstance CRD's spec field using Kubernetes CEL validation rules. The goal is to prevent modifications to the PNI spec after creation, which allows PNIs to retry reconciliation after failures when IP allocations become available.
- Adds kubebuilder XValidation annotation to enforce
self.spec == oldSelf.spec - Updates generated CRD manifest with x-kubernetes-validations
- Adds documentation comments explaining the validation rule and its requirements
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crd/multitenancy/api/v1alpha1/podnetworkinstance.go |
Adds kubebuilder XValidation annotation to PodNetworkInstance type to enforce spec immutability |
crd/multitenancy/manifests/multitenancy.acn.azure.com_podnetworkinstances.yaml |
Updates generated CRD manifest with x-kubernetes-validations rule to enforce spec immutability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crd/multitenancy/manifests/multitenancy.acn.azure.com_podnetworkinstances.yaml
Outdated
Show resolved
Hide resolved
|
This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days |
|
Pull request closed due to inactivity. |
| // +kubebuilder:metadata:labels=owner= | ||
| // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.status` | ||
| // | ||
| // Enforce immutability of .spec. |
There was a problem hiding this comment.
lets remove the 'code comments' for the kubebuilder validation rule from the CRD description compiled by kubebuilder? the consumer of the CRD doesn't need to know this is output from the tool, and the rule is already self-descriptive in the yaml.
There was a problem hiding this comment.
if we need to document the rule in more detail, that should be part of the message of the rule.
There was a problem hiding this comment.
Removed the comments.
| // PodIPReservationSize is the number of IP address to statically reserve | ||
| // +kubebuilder:default=0 | ||
| PodIPReservationSize int `json:"podIPReservationSize,omitempty"` | ||
| // IPConstraint specifies criteria for selecting IP addresses from the PodNetwork's subnet |
There was a problem hiding this comment.
shouldn't this have additional kubebuilder markers? at min. // +kubebuilder:validation:Optional.
check podnetwork for examples, https://book.kubebuilder.io/reference/markers/crd-validation.html?highlight=required#crd-validation for reference
There was a problem hiding this comment.
also document example format / usage.
There was a problem hiding this comment.
I had added this validation for cidr check, but the CEL validation cost was too high.
// +kubebuilder:validation:XValidation:rule="self == '' || isIP(self) || isCIDR(self)",message="must be a valid IPv4/IPv6 address or CIDR"
So, I added this MaxLength:
// +kubebuilder:validation:MaxLength=43
But the cost is still too high, copilot is suggesting to add MaxItems on PodNetworkConfig array -> but this is incorrect.
adding just the Optional validation for now, which I had missed earlier
There was a problem hiding this comment.
done, added the optional validation + description with example and format
kmurudi
left a comment
kmurudi
left a comment
There was a problem hiding this comment.
immutable spec changes lgtm
aranta-rokade
force-pushed
the
arantarokade/pni-immutable
branch
from
acc6ecb to
2344003
Compare
aranta-rokade
changed the title
aranta-rokade
changed the title
|
|
||
| // PodNetworkConfig describes a template for how to attach a PodNetwork to a Pod | ||
| //nolint:lll | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.ipConstraint) || size(self.ipConstraint) == 0 || self.podIPReservationSize == 1",message="ipConstraint can only be specified when podIPReservationSize is 1" |
There was a problem hiding this comment.
Since ipConstraint has +kubebuilder:default="", the field is always present post-defaulting. This makes has(self.ipConstraint) redundant in the XValidation rule. Simplified the rule to validate purely on value semantics: empty string is always allowed, and non-empty ipConstraint is only allowed when podIPReservationSize == 1. This keeps the invariant clear and aligns with CRD defaulting behavior.
Reason for Change:
Issue Fixed:
Making PNI CRD immutable, allows PNIs to retry reconcile after failure, for the cases when IP allocations become available.
The new property is added in preparation to support new feature to enforce an IP constraint on the subnet. Reservations will be added from the IP or cidr provided in the IPConstraint field.
Tested PNI - I will add these test cases on dncrc UT
<style> </style>