-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ Allow to wire an object mutation handler #2932
base: main
Are you sure you want to change the base?
✨ Allow to wire an object mutation handler #2932
Conversation
/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 |
ab06f04
to
922ee77
Compare
@@ -76,7 +88,14 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { | |||
return Errored(http.StatusBadRequest, err) | |||
} | |||
|
|||
// Default the object | |||
return h.handler.Handle(ctx, req.Object.Raw, obj) |
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 we're going to introduce another way to implement mutation, I wonder if we should go all the way:
- Should we just hand over ctx and the entire request? (~ l.83)
- Should we call this interface Mutator & the builder func WithMutator
I think then it would also work well/better for resource updates
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.
@vincepri @alvaroaleman Opinions?
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.
I wanted to avoid doing h.decoder.Decode(req, obj)
every time, but sure, better leave everything up to the developer.
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, I don't quite follow how the new thing helps with the problem at hand. The problem at hand is you need the raw request or not? Wouldn't it be better to change the existing CustomDefaulter
interface to add it as an arg in there?
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.
Yes, I do need the raw request. But this is not the biggest problem, because the request is embedded in the context.
But the reason why I need the raw request is to be able to return json patches. And the CustomDefaulter currently just expects changes to the object. The creation of patches is completely hidden from the defaulter.
We could change the interface to return patches, but that's a breaking change. Isn't it better to add a fully manual mechanism to building webhook handlers?
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.
My thinking was that we can lose information during json.Unmarshal and json.Marshal (either if the CRDs are not prefectly designed with omitempty, or if we have the wrong version of the CRD because it's not ours). This can lead to incorrect / unintended patches
So I thought if we provide a low-level way to handle this better, let's go all the way and give folks all the info (req) and allow them to return the patches directly
Additional context here:
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.
Somewhat related issue #2800
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.
Would this work: #2800 (comment) ?
5fd81ad
to
cd7afc5
Compare
df4be17
to
56c3467
Compare
The panic seems to be on a different suite /retest |
56c3467
to
a4f023c
Compare
/hold cancel |
@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. |
If you are under pressure to implement a fix now in Kueue, I would highly recommend to implement an alternative on Kueue side. Otherwise we need some time until folks have time to review this PR. None of us is paid to maintain controller-runtime. So we are working on CR when we find the time |
Having read through this discussion, the best path forward imo would be a small proposal that outlines the goals/non-goals and the problem we're trying to solve for all Controller Runtime users, not just this specific use case or what the current code does. |
See updated description. |
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.
@alculquicondor Thanks for the description and extra context. The problem statement makes sense to me, although the proposed solution bring in a lot of questions for most users of this library.
The provided goal A mechanism to wire an admission.Handler to process mutations
is an implementation detail, rather an an actual goal. Also to note, our design proposals are collected in the root repository and discussed separately from the actual implementation.
pkg/builder/webhook_test.go
Outdated
func mutatorFactoryForTestDefaulter(scheme *runtime.Scheme) admission.HandlerFactory { | ||
return func(obj runtime.Object, _ admission.Decoder) admission.Handler { | ||
return admission.WithCustomDefaulter(scheme, obj, &TestCustomDefaulter{}).Handler | ||
} | ||
} |
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.
This is very confusing. We're using a mutator, but returning a custom defaulter handler?
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.
This is proving that you can implement a custom defaulter using the mutationhandler.
I could add a completely different handler, but I didn't want to spend too much time in the test before getting green light on the implementation approach.
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.
I moved this somewhere else, but I'm still using a CustomDefaulter. Let me know if you prefer adding a different handler.
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.
I added a custom handler.
pkg/builder/webhook.go
Outdated
// WithMutatorFactory takes an admission.HandlerFactory, a MutatingWebhook will be wired for the handler that this factory creates. | ||
func (blder *WebhookBuilder) WithMutatorFactory(factory admission.HandlerFactory) *WebhookBuilder { | ||
blder.mutatorFactory = factory | ||
return blder | ||
} |
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.
Adding this makes the WithDefaulter
a noop, which doesn't return an error and it's a misleading UX
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.
Since this function doesn't return an error, I'll leave it to the Complete
function to report the error. Unless you are ok with a panic?
pkg/webhook/admission/webhook.go
Outdated
// WithHandlerFactory creates a new Webhook for a handler factory. | ||
func WithHandlerFactory(scheme *runtime.Scheme, obj runtime.Object, factory HandlerFactory) *Webhook { |
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.
Why is this called Factory?
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.
Because it knows how to create a handler. It allows you to receive a decoder from the WebhookBuilder.
Another option could be just to accept a Handler (renaming the method to WithMutationHandler
) and pass the decoder through the context.
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.
I decided to remove the factory and leave it to the implementation to decide how to build the object.
Up to you if you prefer me to write it there. This change seems so minimal that it shouldn't need such a permanent location for the design. |
44738ee
to
d48b844
Compare
There seems to be a difference in how the logger is set up. I'm investigating. |
d48b844
to
a375c55
Compare
/hold cancel This is ready for review now |
/retest |
a375c55
to
50c2afb
Compare
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.
LGTM
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could explicitly add the use case of go types that might be in a different version than the CRD.
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. |
Proposal
Allow wiring an
admission.Handler
to process mutations in a webhook, as part of thebuilder.WebhookBuilder
.Motivation
Currently, mutations can be done through a
CustomDefaulter
which 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.Handler
to 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.