-
Notifications
You must be signed in to change notification settings - Fork 1.6k
📖 Add doc about Strict Server-Side Field Validation #5215
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
base: master
Are you sure you want to change the base?
📖 Add doc about Strict Server-Side Field Validation #5215
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1932ccb to
8eea334
Compare
|
Hi @sbueringer and @alvaroaleman 👋 Since we are documenting a feature of controller-runtime — how to use it, when to use it, and related details — could you please give a hand with the review? Also, could you please let me know what you think? I am ping @dlipovetsky since seems to be the author of the feature as well. Could you please folks approve this one if you are OK with. /hold Thanks a lot! 💛 |
docs/book/src/reference/reference.md
Outdated
| execute any custom logic related to a resource before it gets deleted from | ||
| Kubernetes cluster. | ||
| - [Strict Field Validation](strict-field-validation.md) | ||
| Reject unknown fields instead of silently dropping them. Useful for development |
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.
What does this have to do with upgrade ordering? Is this referring to CRDs being upgraded before controllers? That is always expected to happen IMHO.
Could also mention that this has no effect for server-side apply, there validation is always strict.
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.
Hi @alvaroaleman,
Thank you for the quick reply. The above dev statement wasn’t entirely accurate — it was added by mistake, so please ignore it. I’ve already fixed it.
However, please note:
While CRDs should be updated first, in many scenarios this is still best-effort rather than guaranteed. This can become a concern, especially during the lifecycle and upgrade process.
I’ve updated the documentation to address this properly.
I hope this helps clarify the concerns as well.
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.
While CRDs should be updated first, in many scenarios this is still best-effort rather than guaranteed.
IMHO if that fails, it is much preferrable to have errors rather than the apisever silently dropping the fields
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.
IMHO if that fails, it is much preferrable to have errors rather than the apisever silently dropping the fields
Depends on the scenario, right? It really varies based on how the project is distributed, configured, and used (Helm, GitOps, DevOps setups, etc.).
The docs aim to clarify:
- What this feature is and what it solves;
- Where it works well and where it may not;
- What users need to test themselves — especially upgrades and lifecycle — when using Argo CD, Flux, Helm, third-party CRDs etc. Any caveats/concerns they should be aware of;
- How it plays with the Kubebuilder scaffold and which options might cause issues.
This way users/maintainers know it exists, see it as a valid option and make an informed decision.
Also: is there anything about the feature that we’re not explaining, missing, or getting wrong? Any key detail we should add?
Unless I misunderstood something — otherwise this wouldn’t be a feature and would just be the default behavior, right? Why wasn’t it added by default? What concerns were raised? Any other key concerns/considerations I might not be thinking about?
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.
@camilamacedo86 @alvaroaleman maybe we should make it easier for developers to use WithFieldValidation before documenting it.
I started a thread about that in #controller-runtime
cef707f to
9a7c270
Compare
9a7c270 to
70d360a
Compare
| @@ -0,0 +1,214 @@ | |||
| # Strict Field Validation | |||
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.
Sorry was just quickly reading over it so I might have missed this. This seems to focus only on CRDs?
If strict validation is used with builtin-resources this can mean that the controller is only compatible with exactly the Kubernetes version it was compiled against, which is often not desired
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.
That is exactly the kind of amazing help that I am looking for !!! 🙌
It is something that we must clarify.
Thank you, I will try to add it.
You are amazing 🚀 ⭐
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.
If strict validation is used with builtin-resources this can mean that the controller is only compatible with exactly the Kubernetes version it was compiled against, which is often not desired
I heard that Server-Side Apply always uses strict field validation. How can that work? I guess that SSA does a Patch, not an Update.
I guess that strict field validation works (even with other Kubernetes versions), when you use Patch, and the schema matches for the fields of that patch.
Are there docs about that? I would like to learn more about that.
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.
SSA does neither Patch nor Update it does Apply. If you send a field that doesn't exist it will fail
No idea about docs to be honest, maybe somewhere on the Kubernetes website
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.
But good point, maybe it's not too bad to use it with builtin types. You shouldn't try to set fields that might not exist on the Kubernetes versions you run against anyway. And Update is a bad idea in general anyway.
Closes: #5208