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

[jiachengxu]

This PR adds support of k8s.io/api/admission/v1 in webhook package and moves support of v1beta1 to legacy.
This is because k8s.io/api/admission.k8s.io/v1beta1 is deprecated in kubernetes v1.16 in favor of admission.k8s.io/v1.
controller-tools v0.4.0 can already generate WebhookConfigurations in v1, and more and more folks are trying to upgrade to that, but then they figured out that controller-runtime is not compatible with it. This PR tackles that.
Fixes #1123 #1161
Ref: kubernetes-sigs/controller-tools#469

[k8s-ci-robot]

Hi @jiachengxu. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jiachengxu
To complete the pull request process, please assign pwittrock after the PR has been reviewed.
You can assign the PR to them by writing /assign @pwittrock in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jiachengxu]

cc @vincepri @alvaroaleman @estroz

[alvaroaleman]

/ok-to-test

[jiachengxu]

/test pull-controller-runtime-test-master

[alvaroaleman]

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

[alvaroaleman]

(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)

[jiachengxu]

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!

[alvaroaleman]

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

[DirectXMan12]

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.

[DirectXMan12]

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

[DirectXMan12]

At the very least, the high-level builder doesn't surface the payload to the user, so we should be able to paper over things.

That being said: currently, v1 and v1beta1 are identical (there's a change from should to must in the comments, but it doesn't change the actual field type), which means that we can safely just deserialize v1beta1 into a v1 struct and call it a day, without the duplication. Since you can't (modulo deprecated fields in the future, but by that time v1beta1 will prob be removed) remove fields from a stable API, we should be good on this into the future if we just do that.

[DirectXMan12]

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.

[DirectXMan12]

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

[DirectXMan12]

NB: v1 changed "should copy the UID" to "must copy the UID" in the response, so we need to make sure we're doing that

[vincepri]

Closing in favor of #1284

/close

[k8s-ci-robot]

@jiachengxu: PR needs rebase.

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]

@vincepri: Closed this PR.

In response to this:

Closing in favor of #1284

/close

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.