✨ Change CRD generation logic to honor k8s:immutable #1216
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lalitc375 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @lalitc375! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @lalitc375. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
| func (Immutable) ApplyToSchema(schema *apiext.JSONSchemaProps) error { | ||
| schema.XValidations = append(schema.XValidations, apiext.ValidationRule{ | ||
| Message: "Value is immutable", | ||
| Rule: "self == oldSelf", |
There was a problem hiding this comment.
Won't this error out in the create case as oldSelf will be undefined?
There was a problem hiding this comment.
It won't, there is always a "oldSelf" since the rule is attached to the field and only takes effect when the field exists.
There was a problem hiding this comment.
since the rule is attached to the field and only takes effect when the field exists.
Not quite true, but it's still safe.
Rules that include oldSelf are known as transition rules, and the CEL implementation detects that a rule is a transition rule.
It will only run a transition rule when it has a value for both self and oldSelf. So only on updates, not when the field first appears.
But, you could have a rule without oldSelf, in which case, the rule would run even when the field first appears.
There's also optionalOldSelf which allows transition rules to run when there is no value for oldSelf, but you have to handle those differently.
Which brings me to... The semantic here is that I could have an object with a field we say is immutable. What does it actually mean to be immutable?
There are two interpretations IMO:
- Once a value has been set for X, it cannot be changed (omitted -> set is allowed on updated)
- Value for X can only be set on create (omitted counts as a value)
This implements the first of those, how does it work in the declarative validation project?
If it is the latter, we would need an optionalOldSelf rule and implement a different validation rule
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
/cc @aaron-prindle
PTAL if this align with stand alone +k8s:immutable tag in latest design.
| func (Immutable) ApplyToSchema(schema *apiext.JSONSchemaProps) error { | ||
| schema.XValidations = append(schema.XValidations, apiext.ValidationRule{ | ||
| Message: "Value is immutable", | ||
| Rule: "self == oldSelf", |
There was a problem hiding this comment.
It won't, there is always a "oldSelf" since the rule is attached to the field and only takes effect when the field exists.
|
@yongruilin: GitHub didn't allow me to request PR reviews from the following users: aaron-prindle. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
cc @JoelSpeed |
This change adds logic to understand and interpret k8s:immutable tag in controller-gen. This tag is getting introduced for native types in K8s as part of 5073-declarative-validation-with-validation-gen KEP.
On detecting the tag, A CEL validation is added for the corresponding field.