✨ Allow to wire an object mutation handler #2932
Conversation
k8s-ci-robot
added
do-not-merge/hold
k8s-ci-robot
added
the
size/M
|
/assign @sbueringer |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alculquicondor 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 |
alculquicondor
force-pushed
the
patch_defaulter
branch
from
ab06f04
to
922ee77
Compare
k8s-ci-robot
added
the
needs-rebase
alculquicondor
force-pushed
the
patch_defaulter
branch
2 times, most recently
from
5fd81ad
to
cd7afc5
Compare
k8s-ci-robot
added
size/L
alculquicondor
force-pushed
the
patch_defaulter
branch
3 times, most recently
from
df4be17
to
56c3467
Compare
|
The panic seems to be on a different suite /retest |
k8s-ci-robot
removed
the
needs-rebase
alculquicondor
force-pushed
the
patch_defaulter
branch
from
56c3467
to
a4f023c
Compare
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@sbueringer I addressed all the comments, PTAL. |
|
Can we move this forward? In Kueue, we are risking dropping fields all the time because we don't control the versions of the CRDs that we have webhooks for. |
I understand, but c-r is a widely used project and this is an external interface change. Please have some patience until we find time to properly review it. |
|
Just to be clear. This can be implemented entirely outside of controller-runtime. |
I understand that, but it's a lot of repetitive code to maintain. I was hoping that the change was not controversial, but if you don't like the high level idea, please let me know so we can propose other alternatives. Or just implement the whole thing in Kueue. |
alculquicondor
force-pushed
the
patch_defaulter
branch
from
a375c55
to
50c2afb
Compare
There was a problem hiding this comment.
So if I understand it correctly, the root of the issue is a mismatch between the go types in the project and what is configured as the CRD, resulting in the patch removing fields it doesn't know about.
Can you use unstructured.Unstructured in your CustomDefaulter to avoid this?
| @@ -65,6 +66,12 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder { | |||
| return blder | |||
| } | |||
|
|
|||
| // WithMutationHandler takes an admission.Handler, a MutatingWebhook will be wired for it. | |||
| func (blder *WebhookBuilder) WithMutationHandler(h admission.Handler) *WebhookBuilder { | |||
There was a problem hiding this comment.
Its super not obvious when and why to use this versus the defaulter and almost guaranteed to lead to questions
There was a problem hiding this comment.
Can we leave it as "more advanced use cases where you want control over the construction of patches"?
There was a problem hiding this comment.
Or we could explicitly add the use case of go types that might be in a different version than the CRD.
There was a problem hiding this comment.
Please write something along the lines of that this is a low-level plug point for advanced users that need more control over how exactly the patch is being constructed and if in doubt, people should always use WithDefaulter instead.
|
How about #2800 (comment) instead? |
I don't think so. I need to modify either the raw object or the returning patches.
That was my first proposal, which leads to different problems. |
|
/lgtm We probably want some godoc around when to use WithMutationHandler vs WithDefaulter directly on these funcs + a comment on each that only one of them can be used at the same time |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: a9928964ed3d2fc7a109c288d3e64412ca2019ac
|
|
@sbueringer thank you for reviewing as we are going to use it for Kueue webhooks to register the LossLessDefaulter ( docs, and code). We are also working on improving it to be selective. I'm wondering what your view is on including it as an opt-in defaulter in the controller-runtime code? It would relieve Kueue from maintaining code which is below its "business-level" logic. Also, it would allow other projects to use it, in case they also have similar issues. Should we open an issue to discuss this further? |
This seems extreme. The tl,dr of the problem still seems to be that we don't know of a good solution, so instead we add a low-level plug point so people can work around the problem. If we were to add something like what you've built in Kueue we effectively bless it and call it a good/appropriate solution and I don't think that is what it is. |
alvaroaleman
added
the
tide/merge-method-squash
|
@alvaroaleman, I think the proposal in kubernetes-sigs/kueue#3194 is actually generic and it doesn't seem to have any of the problems of earlier proposals. It just removes any |
|
Maybe it would be better to first try to get the current PR merged and then we can discuss on a new issue if we want to add some built-in handlers? Except if it's all or nothing from your side |
|
Yeah. that's for sure, we should first move it to the point where we can merge it into Kueue. I'm also perfectly ok to incubate it for some time (like a release or more) in Kueue, so that we detect potential issues. Once it is merged into Kueue and we feel comfortable about it I would like to follow up with an issue in controller-runtime to discuss the transfer and potential gaps. WDYT? |
|
Yes please, let's get this PR merged, as there will always be cases where people need to have fine grained control over patches. The current way of doing that requires a lot of duplicated code from controller-runtime. |
No guarantees about the outcome, but sure we can absolutely open an issue and discuss |
k8s-ci-robot
removed
the
lgtm
|
@alvaroaleman any other comment or ask before approval? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: c3432160bf30e616ab33c71016d0a7dc91a4036e
|
|
/close Merged #2982 instead |
|
@sbueringer: Closed this PR. 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. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
Proposal
Allow wiring an
admission.Handlerto process mutations in a webhook, as part of thebuilder.WebhookBuilder.Motivation
Currently, mutations can be done through a
CustomDefaulterwhich accepts and modifies aruntime.Object. The creation of patches is hidden from the developer. The jsonpatch is created from the difference of the request's raw yaml and the marshalled object modified by theCustomDefaulter.This works for webhooks that are always released along with CRD changes. However, it doesn't work for webhooks written for CRDs from other projects. An example of this is Kueue, which writes webhooks for k8s Jobs, kubeflow, kuberay and others. Whenever there is a new version of the third party CRD, the hidden controller-runtime handler will produce a patch that removes all unknown fields.
A mechanism to fully override the handler allows developers to write patches that are safer against these version changes.
Goals
Non Goals
admission.Handlerto process validations. This could easily be added in a follow up proposal, but it doesn't seem to provide as much value as processing mutations.Alternatives
My first proposal was #2931, which is to round-trip the raw object to get rid of unknown fields, and generate a jsondiff from this object.
However, @sbueringer pointed out that the round-trip might also add fields that didn't exist in the raw object is the go types are missing omitempty, which might lead to patches that can't be applied to the raw object in the apiserver. So the suggestion was to give full control to developers over the handler to build patches as they see fit.