Skip to content

⚠ pkg/webhook: Add support for k8s.io/api/admission/v1 #1233

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

⚠ pkg/webhook: Add support for k8s.io/api/admission/v1 #1233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ linters-settings:
lll:
line-length: 170
dupl:
threshold: 400
# The threshold is increased because we have some duplicated test code for pkg/webhook to support
# k8s.io/api/admission/v1beta1, we can change this back when we drop the support and remove the legacy code completely.
# threshold: 400
threshold: 1500
issues:
# don't skip warning about doc comments
exclude-use-default: false
Expand Down
1 change: 1 addition & 0 deletions examples/scratch-env/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,7 @@ k8s.io/kube-openapi v0.0.0-20200805222855-6aeccd4b50c6 h1:+WnxoVtG8TMiudHBSEtrVL
k8s.io/kube-openapi v0.0.0-20200805222855-6aeccd4b50c6/go.mod h1:UuqjUnNftUyPE5H64/qeyjQoUZhGpeFDVdxjTeEVN2o=
k8s.io/utils v0.0.0-20200729134348-d5654de09c73 h1:uJmqzgNWG7XyClnU/mLPBWwfKKF1K8Hf8whTseBgJcg=
k8s.io/utils v0.0.0-20200729134348-d5654de09c73/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
k8s.io/utils v0.0.0-20200912215256-4140de9c8800/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.9/go.mod h1:dzAXnQbTRyDlZPJX2SUPEqvnB+j7AJjtlox7PEwigU0=
sigs.k8s.io/structured-merge-diff/v4 v4.0.1 h1:YXTMot5Qz/X1iBRJhAt+vI+HVttY0WkSqqhKxQ0xVbA=
Expand Down
4 changes: 4 additions & 0 deletions pkg/builder/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ limitations under the License.
//
// Projects built with the builder package can trivially be rebased on top of the underlying
// packages if the project requires more customized behavior in the future.
//
// NOTE: there are some functions and files with `legacy` suffix, which are used to support `k8s.io/api/admission.k8s.io/v1beta1` api.
// Since `k8s.io/api/admission.k8s.io/v1beta1` is deprecated in kubernetes v1.16 in favor of admission.k8s.io/v1, please consider
// to upgrade if possible.
package builder

import (
Expand Down
72 changes: 72 additions & 0 deletions pkg/builder/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,17 @@ func (blder *WebhookBuilder) Complete() error {
return blder.registerWebhooks()
}

// CompleteLegacy builds the webhook to support `k8s.io/api/admission.k8s.io/v1beta1` api.
// Since `k8s.io/api/admission.k8s.io/v1beta1` is deprecated in kubernetes v1.16 in favor of admission.k8s.io/v1, please consider
// to upgrade if possible.
func (blder *WebhookBuilder) CompleteLegacy() error {
Copy link
Member

@alvaroaleman alvaroaleman Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR now basically duplicates the whole webhook code. I see two problems with that:
a) We duplicate a lot of code
b) Controller authors might now know what webhook version is available to their end users (managed kube is very slow)

What about we build a v1beta1 <-> v1 round tripping function, internally only handle one of the two and always expose two handlers, one for v1beta1, one for v1, one of which will convert the request and the result?

/cc @DirectXMan12

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I see that I agreed to the current approach in #1123, I am very sorry about that. I didn't realize that this means we have duplicate so much code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that I was pretty busy with my work in the last few days.

We duplicate a lot of code

Yeah, that's true. However, I think duplicating code makes it easier to drop the support in the future. In our case, we are going to drop support of v1beta1 in the future instead of supporting both v1 and v1beta1, we can just remove the files and functions with the Legacy suffix, and it will work.
Also, building a fancy round-tripping function may be a good choice, but it may need more interface designing & discussion. Since more and more folks found that controller-runtime cannot work with controller-tool v0.4.0 for webhook v1 version, I wanted to implement something fast and workable for them.
Finally, if we still want to have more discussion and designing around this, how about we just release a new version of controller-tool with this PR: kubernetes-sigs/controller-tools#474, so at least user can specify the AdmissionReviewVersion to v1beta1 and migrate to webhook v1. Then we figure out how we want to support AdmissionReviewVersion v1 in controller-runtime. (/cc @vincepri)

Controller authors might now know what webhook version is available to their end users (managed kube is very slow)

If I got this correctly, you meant controller authors might need to know which version of AdmissionReviewVersion that the end users' clusters support. I think they need to know this anyway if they want to introduce break changes and migrate from legacy v1beta1 to v1? Because I think with two handlers v1 and v1beta1 if v1beta1 of AdmissionReview API is deprecated in the cluster, controller authors might still need to migrate handlers to v1? Please correct if I got this wrong.

Finally, please don't feel sorry about our previous discussion :) We are all trying to make the community better!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, building a fancy round-tripping function may be a good choice, but it may need more interface designing & discussion.

I don't think it's fancy, its just copying all fields of one type into the equivalent fields of the other type and write a fuzz test that roundtripps using something like https://github.com/google/gofuzz. Since this is not going to be part of an external interface, I don't think there will be much designing and discussion involved.

I think they need to know this anyway if they want to introduce break changes and migrate from legacy v1beta1 to v1? Because I think with two handlers v1 and v1beta1 if v1beta1 of AdmissionReview API is deprecated in the cluster, controller authors might still need to migrate handlers to v1? Please correct if I got this wrong.

From what I can see they are actually identical, so its irrelevant for authors: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/admission/v1/types.go#L40 https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/admission/v1beta1/types.go#L45

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, for the builder, none of the underlying webhook payload is surfaced to the user, so the builder should handle both cases automatically for the user.

The "lower-level" variants may be a different story.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, if they're currently identical, we can just deserialize into v1 safely.

// Set the Config
blder.loadRestConfig()

// Set the Webhook if needed
return blder.registerWebhooksLegacy()
}

func (blder *WebhookBuilder) loadRestConfig() {
if blder.config == nil {
blder.config = blder.mgr.GetConfig()
Expand Down Expand Up @@ -145,6 +156,67 @@ func (blder *WebhookBuilder) registerConversionWebhook() error {
return nil
}

func (blder *WebhookBuilder) registerWebhooksLegacy() error {
// Create webhook(s) for each type
var err error
blder.gvk, err = apiutil.GVKForObject(blder.apiType, blder.mgr.GetScheme())
if err != nil {
return err
}

blder.registerDefaultingWebhookLegacy()
blder.registerValidatingWebhookLegacy()

err = blder.registerConversionWebhook()
if err != nil {
return err
}
return nil
}

// registerDefaultingWebhook registers a defaulting webhook if th
func (blder *WebhookBuilder) registerDefaultingWebhookLegacy() {
defaulter, isDefaulter := blder.apiType.(admission.Defaulter)
if !isDefaulter {
log.Info("skip registering a mutating webhook, admission.Defaulter interface is not implemented", "GVK", blder.gvk)
return
}
mwh := admission.DefaultingWebhookForLegacy(defaulter)
if mwh != nil {
path := generateMutatePath(blder.gvk)

// Checking if the path is already registered.
// If so, just skip it.
if !blder.isAlreadyHandled(path) {
log.Info("Registering a mutating webhook",
"GVK", blder.gvk,
"path", path)
blder.mgr.GetWebhookServer().Register(path, mwh)
}
}
}

func (blder *WebhookBuilder) registerValidatingWebhookLegacy() {
validator, isValidator := blder.apiType.(admission.Validator)
if !isValidator {
log.Info("skip registering a validating webhook, admission.Validator interface is not implemented", "GVK", blder.gvk)
return
}
vwh := admission.ValidatingWebhookForLegacy(validator)
if vwh != nil {
path := generateValidatePath(blder.gvk)

// Checking if the path is already registered.
// If so, just skip it.
if !blder.isAlreadyHandled(path) {
log.Info("Registering a validating webhook",
"GVK", blder.gvk,
"path", path)
blder.mgr.GetWebhookServer().Register(path, vwh)
}
}
}

func (blder *WebhookBuilder) isAlreadyHandled(path string) bool {
if blder.mgr.GetWebhookServer().WebhookMux == nil {
return false
Expand Down
Loading