Skip to content

Conversation

@camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Nov 15, 2025

Closes: #5208

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 15, 2025
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2025
@camilamacedo86 camilamacedo86 force-pushed the add-optional-strict-manager branch 3 times, most recently from 1932ccb to 8eea334 Compare November 15, 2025 17:02
@camilamacedo86 camilamacedo86 changed the title WIP: 📖 : Add doc about Strict Server-Side Field Validation 📖 : Add doc about Strict Server-Side Field Validation Nov 15, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2025
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Nov 15, 2025

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?
Your feedback would be very much appreciated! 🙏

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.
And who request this one: @guettli

Could you please folks approve this one if you are OK with.
PS.: I am adding a hold to allow everybody LGTM and etc and give proper time to everybody review

/hold

Thanks a lot! 💛

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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@alvaroaleman alvaroaleman Nov 16, 2025

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

Copy link
Member Author

@camilamacedo86 camilamacedo86 Nov 16, 2025

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?

Copy link
Contributor

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

@camilamacedo86 camilamacedo86 changed the title 📖 : Add doc about Strict Server-Side Field Validation 📖 Add doc about Strict Server-Side Field Validation Nov 16, 2025
@camilamacedo86 camilamacedo86 force-pushed the add-optional-strict-manager branch 2 times, most recently from cef707f to 9a7c270 Compare November 16, 2025 12:19
@camilamacedo86 camilamacedo86 force-pushed the add-optional-strict-manager branch from 9a7c270 to 70d360a Compare November 16, 2025 12:20
@@ -0,0 +1,214 @@
# Strict Field Validation
Copy link
Member

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

Copy link
Member Author

@camilamacedo86 camilamacedo86 Nov 17, 2025

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 🚀 ⭐

Copy link
Contributor

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.

Copy link
Member

@sbueringer sbueringer Nov 17, 2025

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

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document WithFieldValidation

5 participants