📖 update webhooks for core types to match controller-runtime v0.15 #3399
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Hi @mythi. 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
There was a problem hiding this comment.
I am not sure if I understand this changes.
See that the Handler still existing for 0.15.0: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/webhook/admission#Handler
The only breaking change that I saw is that now we have admission.Warnings in the method signature, see:
This change should be in placed in the docs as well for master since after we do the changes we must to run make generate and then the docs are updated.
So, do we still needing this PR?
|
@camilamacedo86 I believe it's still needed. |
Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>
|
@camilamacedo86 anything I can/need to do here? |
|
Hi @mythi,
By using the master branch and checking the samples code in the master, what issues are you still face?
We do not use sigs.k8s.io/controller-runtime/pkg/runtime/inject. |
@camilamacedo86 hmm but this PR is not about the scaffolding process but this page https://book.kubebuilder.io/reference/webhook-for-core-types.html which is wrong. |
|
Hi @mythi,
It seems make sense. When it no longer be a draft could you please either ping in the slack so that we won a help from the community in the review? |
|
I think you are very skilled in this area, could you please give a hand in the review of this PR? |
|
|
||
| ## Update main.go | ||
|
|
||
| Now you need to register your handler in the webhook server. | ||
|
|
||
| ```go | ||
| mgr.GetWebhookServer().Register("/mutate-v1-pod", &webhook.Admission{Handler: &podAnnotator{Client: mgr.GetClient()}}) | ||
| if err := builder.WebhookManagedBy(mgr). |
There was a problem hiding this comment.
Maybe we should add a func to podAnnotator to set it up with Manager - as we do for controllers?
func (r *podAnnotator) SetupWithManager(mgr ctrl.Manager) error
IMO the code will be easier to read. An advanced user can always in-line it...
There was a problem hiding this comment.
AFAIU, the same is covered by the new snippet. That is used with:
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{})
...
if err := builder.WebhookManagedBy(mgr).
...
There was a problem hiding this comment.
Ok, but if using kubebuilder to create the webhook, I think it will generate a SetupWithManager func.
There was a problem hiding this comment.
Ok, but if using kubebuilder to create the webhook, I think it will generate a SetupWithManager func.
@erikgb is correct. For example, see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/api/v1/captain_webhook.go
Even though they do the same thing, I agree that for the Kubebuilder documentation we should ensure we are accurately reflecting what is scaffolded by default by Kubebuilder.
There was a problem hiding this comment.
this should be covered now. apologies for the silence!
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mythi 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 |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
mythi
changed the title
|
@erikgb @camilamacedo86 the review comments are addressed, and this is ready for another review |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
lgtm
|
|
||
| ## Update main.go | ||
|
|
||
| Now you need to register your handler in the webhook server. | ||
|
|
||
| ```go | ||
| mgr.GetWebhookServer().Register("/mutate-v1-pod", &webhook.Admission{Handler: &podAnnotator{Client: mgr.GetClient()}}) | ||
| if err := builder.WebhookManagedBy(mgr). |
There was a problem hiding this comment.
Ok, but if using kubebuilder to create the webhook, I think it will generate a SetupWithManager func.
@erikgb is correct. For example, see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/api/v1/captain_webhook.go
Even though they do the same thing, I agree that for the Kubebuilder documentation we should ensure we are accurately reflecting what is scaffolded by default by Kubebuilder.
There was a problem hiding this comment.
Requested changes:
- Ensure that the changes in the doc are aligned with the default scaffold done by KB
From @erikgb and @everettraven :
@erikgb is correct. For example, see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/api/v1/captain_webhook.go
Even though they do the same thing, I agree that for the Kubebuilder documentation we should ensure we are accurately reflecting what is scaffolded by default by Kubebuilder.
k8s-ci-robot
added
the
needs-rebase
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages 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
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
|
/remove-lifecycle rotten |
k8s-ci-robot
removed
the
lifecycle/rotten
mythi
force-pushed
the
cr-0.15
branch
2 times, most recently
from
f66aa72
to
3781a81
Compare
k8s-ci-robot
removed
the
needs-rebase
| return admission.Errored(http.StatusInternalServerError, err) | ||
| } | ||
| return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod) | ||
| return nil |
There was a problem hiding this comment.
before it could return an error because it was patching the value
so, if an error were faced it will be returned now I am not sure if we we are patching the value.
Did you tested it out?
It seems that it is not doing the same.
There was a problem hiding this comment.
Did you tested it out?
The code is same as in https://github.com/kubernetes-sigs/controller-runtime/blob/76d3d0826fa9dca267c70c68c706f6de40084043/examples/builtins/mutatingwebhook.go so I'm assuming it's tested by controller-runtime. FWIW, my own webhook follows the same flow also.
There was a problem hiding this comment.
It is not the same
Before it was use the client to perform the changes on the cluster side and return its result (nil OR error)
Now, it adds the value to the pod, do nothing and return nil always
Also, we are changing how to do.
So, anyway, for any tutorial example we should test things out, ensure that it is right.
Could you please test this one?
There was a problem hiding this comment.
It is not the same
I'm pretty sure it's the same. I just copied the Default() func and compared with what is in this file and there are no functional changes.
There was a problem hiding this comment.
But the example that we have here does
marshaledPod, err := json.Marshal(pod)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}
return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod)
It would not add too much value just have the sample controller-runtime right?
Would still possible to do the same with the new changes?
There was a problem hiding this comment.
It would not add too much value just have the sample controller-runtime right?
That's how it's been. I'm just a messenger and making the same changes that controller-runtime has between controller-runtime release-0.14 and release-0.15.
There was a problem hiding this comment.
Hi @mythi
Sure, and we appreciate it a lot.
What I am trying to say is that we should update the doc to keep the same example that we have before but now addressing the controller-runtime changes. That would provide more value than change the current example to copy and paste an example from there.
So that we could ensure that we are able to do the same but in another way and that the example that we have is valid.
There was a problem hiding this comment.
That would provide more value than change the current example to copy and paste an example from there.
What I tried to say was that the current example is also a copy and paste and that the diff here is how controller-runtime addressed their changes in the example. I kinda understand what you're asking me to change but the counterargument is why would we want to diverge from the example given elsewhere (especially since we used to be aligned).
Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>
|
Would love to see docs updated. The example code works nicely, but the docs are misleading (to me). They don't align with the code example. |
Just to clarify: when you say "example", you mean the controller-runtime example the book points to? This PR is exactly about getting those aligned. |
|
HI @mythi How, we should change this doc?
We should not:
I hope that clarifies. |
I don't think the current example meets any of these bullets either. However, if that is desired, we could close this and someone can pick up re-writing this page according to the new ask. |
|
@mythi: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
camilamacedo86
added
the
help wanted
Fixes: #3393