Don't Merge: Example of simpler interface for Webhooks #292
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pwittrock 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 |
k8s-ci-robot
added
the
approved
| type Validator interface { | ||
| runtime.Object | ||
| ValidateCreate() error | ||
| ValidateUpdate(old runtime.Object) error |
There was a problem hiding this comment.
How are we going to handle Delele and Connect?
There was a problem hiding this comment.
Do we need to initially?
There was a problem hiding this comment.
Maybe not.
Create and Update are most common and should be must-have;
Delete is less common and should be good-to-have;
I haven't heard a real use case of Connect.
There was a problem hiding this comment.
ValidateUpdate(old runtime.Object) error
IIRC if the webhook is registered as ValidatingWebhookConfiguration, old object will not be included in the AdmissionReview request.
But a nil object may not be a problem here.
| @@ -125,10 +126,11 @@ type Server struct { | |||
| // They can be nil, if there is no webhook registered under it. | |||
| webhookConfigurations []runtime.Object | |||
|
|
|||
| // manager is the manager that this webhook server will be registered. | |||
| manager manager.Manager | |||
There was a problem hiding this comment.
Happy to see we will drop manager and instead to use scheme and restMapper explictly.
| } | ||
|
|
||
| return types.Response{}, false | ||
| } |
There was a problem hiding this comment.
This is fantastic. I like the way it hides all the webhook machinery from the end-users. Resource author just need to define the validate and default methods on their types and rest is taken care by the machinery (exactly the way it should be).
This is a huge step in making defaulting and validation accessible.
| copy.Default() | ||
|
|
||
| // Create the patch | ||
| return admission.PatchResponse(obj, copy), true |
There was a problem hiding this comment.
This causes roundtripping issue, we are moving away from this #256 .
|
@mengqiy is working on extracting manifest generation out of controller-runtime that has an overlap with this, so has volunteered to tackle this PR. |
This code isn't polished enough to be merged.
Sending to show folks the vision.