[DISCUSSION] Webhook prototypes for external types #2170
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Hi @kopiczko. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kopiczko 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 |
|
|
||
| //+kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=fail,sideEffects=None,groups=core,resources=pods,verbs=create;update,versions=v1,name=mpod.kb.io,admissionReviewVersions={v1,v1beta1} | ||
|
|
||
| type PodDefaulter struct { |
There was a problem hiding this comment.
To keep this in line with nomenclature,
| type PodDefaulter struct { | |
| type PodMutator struct { |
There was a problem hiding this comment.
I named it after admission.Defaulter in controller-runtime. But I don't have a strong option on the name here.
| // Option 1. Extract .Validate*() methods to make it resemble | ||
| // admission.Validator to make controller-runtime users feel at home. | ||
| // Initially I wanted them to be private methods (i.e. .validate*()) | ||
| // but since .default() is not possible (see option 1. in defaulter) | ||
| // I went with this. | ||
| // | ||
| // Two questions that come to mind with this approach: | ||
| // | ||
| // 1. Should admission.Denied be supported? | ||
| // 2. Should there be something in admission.Allowed message? | ||
|
|
||
| var validateErr error | ||
| switch req.Operation { | ||
| case admissionv1.Create: | ||
| validateErr = v.ValidateCreate(r) | ||
| case admissionv1.Update: | ||
| old := &corev1.Pod{} | ||
| if err := v.decoder.DecodeRaw(req.OldObject, old); err != nil { | ||
| return admission.Errored(http.StatusBadRequest, err) | ||
| } | ||
| validateErr = v.ValidateUpdate(r, old) | ||
| case admissionv1.Delete: | ||
| validateErr = v.ValidateDelete(r) | ||
| } | ||
|
|
||
| if validateErr != nil { | ||
| return admission.Errored(http.StatusBadRequest, validateErr) | ||
| } | ||
| return admission.Allowed("") | ||
|
|
||
| // End of option 1. | ||
|
|
||
| // Option 2. Simply add TODO here and remove .validate**() methods. | ||
|
|
||
| // TODO(user): fill in your validation logic upon object creation. | ||
| return admission.Errored(http.StatusBadRequest, errors.New("not implemented")) | ||
|
|
||
| // End of option 2. |
There was a problem hiding this comment.
Any opinions on the options here?
There was a problem hiding this comment.
Option 1 (in both mutating and validating webhook scaffolds).
| func (v *PodValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { | ||
| hookServer := mgr.GetWebhookServer() | ||
| hookServer.Register("/validate-v1-pod", &webhook.Admission{Handler: v}) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
@estroz SetupWebhookWithManager is already there. There is also some example wiring in the main.go.
There was a problem hiding this comment.
Yes I saw that. I think we could come up with a convenience wrapper for a set of handlers to register them in a loop. Not necessary right now though.
| // | ||
| // Two questions that come to mind with this approach: | ||
| // | ||
| // 1. Should admission.Denied be supported? |
There was a problem hiding this comment.
Take a peek at admission.Validator code.
| // Two questions that come to mind with this approach: | ||
| // | ||
| // 1. Should admission.Denied be supported? | ||
| // 2. Should there be something in admission.Allowed message? |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: 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/test-infra repository. |
This PR is a follow up of the discussion started in Slack: https://kubernetes.slack.com/archives/CAR30FCJZ/p1619778173399700
Towards #1999
This PR is not meant to be merged. It's here for the discussion purposes.
This is a prototype of webhook scaffolding for core types. This can be reused for non-core types or non-core types may still be required to fulfil admission.Defaulter/Validator interfaces.
In this prototype there are two options proposed as outlined in the comments. It is focused only on the webhook scaffolding and setup. Nothing more.