Skip to content
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.

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

Webhook configurations can choose which version of Review request they accept #74998

Merged
merged 3 commits into from Mar 8, 2019

Conversation

@mbohlool
Copy link
Member

mbohlool commented Mar 5, 2019

This is a task from Extensibility GA

Currently apiserver sends AdmissionReview to Admission Webhooks in v1beta1. We are going to introduce AdmissionReview v1 and a prerequisite for that is to have a mechanism for the webhook to define versions they understand. This PR addresses that.

The mechanism is also added to CRD Conversion Webhook stack.

- Add mechanism for Admission Webhooks to specify which version of AdmissionReview they support
- Add mechanism for CRD Conversion Webhooks to specify which version of ConversionReview they support

/cc @liggitt @roycaihw @sttts @caesarxuchao

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 6, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@mbohlool mbohlool force-pushed the mbohlool:pippin branch 2 times, most recently from 13091f8 to 74c1ff0 Mar 6, 2019

@liggitt liggitt added the api-review label Mar 6, 2019

@liggitt liggitt self-assigned this Mar 6, 2019

@liggitt liggitt added this to In progress in API Reviews Mar 6, 2019

@liggitt liggitt added this to the v1.14 milestone Mar 6, 2019

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Mar 6, 2019

/test pull-kubernetes-bazel-test

@mbohlool mbohlool force-pushed the mbohlool:pippin branch from 74c1ff0 to f6f41a3 Mar 6, 2019

@@ -44,4 +44,8 @@ func SetDefaults_Webhook(obj *admissionregistrationv1beta1.Webhook) {
obj.TimeoutSeconds = new(int32)
*obj.TimeoutSeconds = 30
}

if len(obj.AdmissionReviewVersions) == 0 {
obj.AdmissionReviewVersions = []string{"v1beta1"}

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

prefer SchemeGroupVersion.Version?

} else {
seen := map[string]bool{}
for i, v := range versions {
if !isAcceptedAdmissionReviewVersion(v) {

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

we don't want to prevent versions we don't understand, or that will block a webhook that supports v1beta1 and v1 from registering itself

return "v1beta1" == v
}

func validateAdmissionReviewVersions(versions []string, fldPath *field.Path) field.ErrorList {

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

this function is still getting called on update (*Strategy#ValidateUpdate -> ValidateValidatingWebhookConfiguration -> validateAdmissionReviewVersions), but lacks the information to know whether it should require a recognized version

we should limit *Strategy#ValidateUpdate to calling Validate*Update, and not have the strategy also call the Validate* method directly (that would let us unit test Validate* and Validate*Update methods and have that actually reflect what validation would be done on create and update)

This comment has been minimized.

@mbohlool

mbohlool Mar 6, 2019

Author Member

ValidateUpdate should not call Validate*. If you check current code, both methods it calls doing the same thing and we basically duplicate validation errors.

strategy#ValidateUpdate calls to both ValidateValidatingWebhookConfiguration and ValidateValidatingWebhookConfigurationUpdate. ValidateValidatingWebhookConfigurationUpdate calls ValidateValidatingWebhookConfiguration on the newObject which is the same as first call strategy#ValidateUpdate did to ValidateValidatingWebhookConfiguration. I will fix strategy.

allErrors = append(allErrors, validateWebhook(&hook, fldPath)...)
oldHook, exists := oldWebhooks[hook.Name]
if exists {
// Only validate admissionReviewVersions if they were valid before

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

skipping this if the old webhook didn't have recognized versions means we also skip required and duplicate checking, which isn't great

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

it might be simpler to do this at the object level rather than the webhook level (being more fine-grained doesn't do a lot for us, and makes the code way more complicated):

  • set hadRecognizedVersions to true if all webhooks in the old object provided recognized versions
  • change validateAdmissionReviewVersions to take requireRecognizedVersion bool and pass in hadRecognizedVersions here, and pass in true from ValidateMutatingWebhookConfiguration (you did something similar in validateCustomResourceDefinitionSpec and it worked well)
@@ -66,6 +66,7 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} {
Strategy: apiextensions.NoneConverter,
}
}
obj.Conversion.ConversionReviewVersions = []string{"v1beta1"}

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

only if len == 0?

This comment has been minimized.

@mbohlool

mbohlool Mar 6, 2019

Author Member

Hmm, is the point of this function to make the object valid? any value other than []string{"v1beta1"} is not valid.

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

fuzzing doesn't care about validation, just about round-trippability. len(0) doesn't round-trip because empty lists get defaulted

@@ -68,6 +68,9 @@ func SetDefaults_CustomResourceDefinitionSpec(obj *CustomResourceDefinitionSpec)
Strategy: NoneConverter,
}
}
if len(obj.Conversion.ConversionReviewVersions) == 0 {
obj.Conversion.ConversionReviewVersions = []string{"v1beta1"}

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

prefer SchemeGroupVersion.Version

@@ -108,6 +108,11 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.Webhook,
}
}

// Currently dispatcher only supports `v1beta1` AdmissionReview
if !util.HasAdmissionReviewVersion("v1beta1", h) {

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

prefer v1beta1.SchemeGroupVersion.Version, and add a TODO to make the dispatcher capable of sending/receiving multiple versions

@@ -94,6 +94,11 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta
}
}

// Currently dispatcher only supports `v1beta1` AdmissionReview
if !util.HasAdmissionReviewVersion("v1beta1", h) {

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

use constant, and add TODO to make this capable of sending/receiving multiple versions

@@ -216,6 +230,11 @@ func (c *webhookConverter) ConvertToVersion(in runtime.Object, target runtime.Gr
}
}

// Currently converter only supports `v1beta1` ConversionReview
if !c.hasConversionReviewVersion("v1beta1") {

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

use constant and add TODO

@@ -250,6 +283,9 @@ func ValidateCustomResourceConversion(conversion *apiextensions.CustomResourceCo
allErrs = append(allErrs, webhook.ValidateWebhookService(fldPath.Child("webhookClientConfig").Child("service"), cc.Service.Name, cc.Service.Namespace, cc.Service.Path)...)
}
}
if mustValidateConversionReviewVersions {

This comment has been minimized.

@liggitt

liggitt Mar 6, 2019

Member

required and duplicate checking is still worthwhile in all cases, this should only govern whether we require a recognized version

@liggitt liggitt moved this from In progress to Changes requested in API Reviews Mar 6, 2019

@mbohlool mbohlool force-pushed the mbohlool:pippin branch from f6f41a3 to 3db0f4d Mar 6, 2019

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Mar 7, 2019

@liggitt comment addressed.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 7, 2019

/test pull-kubernetes-godeps

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Mar 8, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 8, 2019

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 8, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mbohlool

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt liggitt removed the api-review label Mar 8, 2019

@liggitt liggitt removed this from Changes requested in API Reviews Mar 8, 2019

mbohlool added some commits Mar 5, 2019

@mbohlool mbohlool force-pushed the mbohlool:pippin branch from 631b1d6 to cbe0002 Mar 8, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 8, 2019

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 8, 2019

squashed, retagging
/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Mar 8, 2019

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 8, 2019

v1.14 release lead here, this is a size/XXL PR that is attempting to land right before Code Freeze... are you really sure this is a good idea?

The vast majority of the size is generated changes and thorough unit tests.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 8, 2019

flake #75159

/retest

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Mar 8, 2019

/retest

1 similar comment
@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Mar 8, 2019

/retest

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Mar 8, 2019

/test pull-kubernetes-godeps

@k8s-ci-robot k8s-ci-robot merged commit e318642 into kubernetes:master Mar 8, 2019

17 checks passed

cla/linuxfoundation mbohlool authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

mbohlool added a commit to mbohlool/kubernetes.github.io that referenced this pull request Mar 13, 2019

mbohlool added a commit to mbohlool/kubernetes.github.io that referenced this pull request Mar 14, 2019

mbohlool added a commit to mbohlool/kubernetes.github.io that referenced this pull request Mar 15, 2019

k8s-ci-robot added a commit to kubernetes/website that referenced this pull request Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.