-
Notifications
You must be signed in to change notification settings - Fork 899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enforced immutable fields using CEL rules #5682
Enforced immutable fields using CEL rules #5682
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run make generate
? It doesn't seem like the CRDs were updated with the result of the new comment marker.
Also, I'm pretty confident we have more immutable fields than this. There's probably some without // +immutable
markers.
@negz Also can I please know how can I find those fields that are supposed to be immutable but are not marked with //+immutable ? |
@NeerajNagure Unfortunately there's no easy way to do this. You'd need to be familiar enough with Crossplane's APIs to know which fields can't change. I don't think this needs to block this PR though, if we can come up with a pattern that works someone who is familiar with the APIs can copy it to the other fields that need it. |
@negz can you please review this? |
Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
…struct Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
623497c
to
edb536d
Compare
Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
@negz I have made the changes as suggested by you |
Thanks @NeerajNagure. Could you please add some details on how you've tested this change? Adding an E2E test would be ideal. |
Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
Signed-off-by: Neeraj Nagure <nagureneeraj@gmail.com>
@negz I have added 2 e2e tests that check if its possible to update immutable fields or not (Also those e2e tests pass successfully) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NeerajNagure!
Description of your changes
Added CEL based comment markers to enforce immutability on every field that was previously marked as // +immutable
Fixes #4128
I have:
make reviewable
to ensure this PR is ready for review.Added or updated unit tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.y
labels to auto-backport this PR.Need help with this checklist? See the cheat sheet.