Add MutatingWebhookConfiguration type #55282

mbohlool · 2017-11-08T01:37:55Z

As part of Mutating Webhook support, this PR adds the configuration for Mutating webhooks. It also renames existing ReadOnly webhook configurations from ExternalAdmissionHookConfiguration to ValidatingWebhookConfiguration. As part of the process some sub-types are also renamed.

Lastly, the mutating webhook configurations are sorted by name to make the serial executing of them deterministic.

ref: kubernetes/enhancements#492

The dynamic admission webhook is split into two kinds, mutating and validating.

k8s-ci-robot · 2017-11-08T01:37:56Z

@mbohlool: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

cheftako · 2017-11-08T01:56:43Z

pkg/apis/admissionregistration/validation/validation.go

-	for i, hook := range e.ExternalAdmissionHooks {
-		allErrors = append(allErrors, validateExternalAdmissionHook(&hook, field.NewPath("externalAdmissionHooks").Index(i))...)
+	for i, hook := range e.Webhooks {
+		allErrors = append(allErrors, validateWebhook(&hook, field.NewPath("externalAdmissionHooks").Index(i))...)


Do we really need to maintain the old path? I realize the migration may be painful but it would be nice if we could keep the naming consistent.

that is an old code that should not exists anymore, please let me know if you see any more of these.

cheftako · 2017-11-08T02:00:47Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

 		e.ErrStatus.Reason = "LoadingConfiguration"
 		e.ErrStatus.Details.Causes = append(e.ErrStatus.Details.Causes, metav1.StatusCause{
-			Type:    "ExternalAdmissionHookConfigurationFailure",
+			Type:    "ValidatingWebhookConfigurationFailure",
 			Message: "An error has occurred while refreshing the externalAdmissionHook configuration, no resources can be created/updated/deleted/connected until a refresh succeeds.",


Again can we keep the naming consistent?

sttts · 2017-11-08T16:23:17Z

/cc @deads2k

sttts · 2017-11-08T16:29:12Z

staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go

+			}
+			return nil, err
+		}
+		return mergeMutatingWebhookConfigurations(list), nil


Isn't this list coming from the lister, i.e. a local cache? Does line 83 mutate that object?

Our standard listers create temporary lists at least. But it doesn't look like it's specified that the callee is the owner of the list. In any case, mergeMutatingWebhookConfigurations does not give any hint that it mutates its arguments.

This is copied from the existing one-- if the existing one is broken we should fix it both places.

This modifies the List object but the List object is constructed each time?

the function should not mutate that list. Will fix.

@lavalamp Even if the list object is constructed each time, the function should either document that it modifies its parameters or not modify them. It is easier not to modify them. Also existing code does not have this issue because we do not sort validating webhooks. They are called in parallel so the order does not matter.

ah, thanks for the explanation.

sttts · 2017-11-08T16:39:34Z

staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go

+	List(opts metav1.ListOptions) (*v1alpha1.MutatingWebhookConfigurationList, error)
+}
+
+type MutatingWebhookConfigurationManager struct {


godoc. Is this a controller of some sort?

It collects the mutating webhook objects so that they can be called. Yes, a doc string would be good.

This is the same as ValidatingWebhookConfigurationManager which was ExternalAdmissionHookConfigurationManager. I would like to not add stuff to this PR and keep it pure renaming and duplicating. Any enhancement should be done in a following PR in my opinion.

Noticed that as well after commenting.

lavalamp · 2017-11-08T22:22:39Z

pkg/apis/admissionregistration/types.go

@@ -118,35 +118,65 @@ const (
 // +genclient:nonNamespaced
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

-// ExternalAdmissionHookConfiguration describes the configuration of initializers.
-type ExternalAdmissionHookConfiguration struct {
+// ValidatingWebhookConfiguration describes the configuration of and admission webhook that accept or reject and object without changing it.


lavalamp · 2017-11-08T22:23:20Z

pkg/apis/admissionregistration/types.go

-	ExternalAdmissionHooks []ExternalAdmissionHook
+	// +patchMergeKey=name
+	// +patchStrategy=merge
+	Webhooks []Webhook


Webhook or AdmissionHook? I have a very very mild preference for the latter.

We remove admission from the type name because it was the duplicate of (part of) the package name. I would argue the same here that I prefer Webhook for that reason.

+1 for Webhook

lavalamp · 2017-11-08T22:26:15Z

pkg/kubectl/scheme/install.go

@@ -106,7 +106,7 @@ func init() {
 	if err := announced.NewGroupMetaFactory(
 		&announced.GroupMetaFactoryArgs{
 			GroupName:              admissionregistrationv1alpha1.GroupName,
-			RootScopedKinds:        sets.NewString("InitializerConfiguration", "ExternalAdmissionHookConfiguration"),
+			RootScopedKinds:        sets.NewString("InitializerConfiguration", "ValidatingWebhookConfiguration"),


needs to list the mutating one, also?

Ouch, is this the new scheme of kubectl? Do we really decided to rewrite the group installers here. This hurts.

lavalamp · 2017-11-08T22:30:07Z

pkg/registry/admissionregistration/validatingwebhookconfiguration/storage/storage.go

+		ObjectNameFunc: func(obj runtime.Object) (string, error) {
+			return obj.(*admissionregistration.ValidatingWebhookConfiguration).Name, nil
+		},
+		DefaultQualifiedResource: admissionregistration.Resource("externaladmissionhookconfigurations"),


this string probably needs to be updated?

lavalamp · 2017-11-08T22:36:54Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go

@@ -153,7 +153,7 @@ func (a *GenericAdmissionWebhook) SetScheme(scheme *runtime.Scheme) {

 // WantsExternalKubeClientSet defines a function which sets external ClientSet for admission plugins that need it
 func (a *GenericAdmissionWebhook) SetExternalKubeClientSet(client clientset.Interface) {


The name of this plugin needs to be changed too, but maybe that is another PR?

Yes, another PR is better.

lavalamp · 2017-11-08T22:39:18Z

test/integration/etcd/etcd_storage_path_test.go

@@ -382,7 +382,7 @@ var etcdStorageData = map[schema.GroupVersionResource]struct {
 		expectedEtcdPath: "/registry/initializerconfigurations/ic1",
 	},
 	gvr("admissionregistration.k8s.io", "v1alpha1", "externaladmissionhookconfigurations"): {


s/externaladmissionhookconfigurations/validatingwebhookconfigurations/?

And also duplicate this entry for the mutating ones?

lavalamp · 2017-11-08T22:43:58Z

pkg/registry/admissionregistration/mutatingwebhookconfiguration/storage/storage.go

 		ObjectNameFunc: func(obj runtime.Object) (string, error) {
-			return obj.(*admissionregistration.ExternalAdmissionHookConfiguration).Name, nil
+			return obj.(*admissionregistration.ValidatingWebhookConfiguration).Name, nil
 		},
 		DefaultQualifiedResource: admissionregistration.Resource("externaladmissionhookconfigurations"),


Update this string?

lavalamp · 2017-11-08T22:44:31Z

I left a few comments, looks close.

lavalamp · 2017-11-08T22:44:43Z

Wrong button :)

k8s-ci-robot · 2017-11-09T22:13:19Z

@mbohlool: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

lavalamp · 2017-11-09T23:10:33Z

/lgtm

k8s-github-robot · 2017-11-09T23:11:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 492

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

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [lavalamp]
~~cmd/kube-apiserver/OWNERS~~ [lavalamp]
~~docs/api-reference/OWNERS~~ [lavalamp]
~~hack/OWNERS~~ [lavalamp]
~~pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]
~~test/OWNERS~~ [lavalamp]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mbohlool · 2017-11-09T23:49:33Z

The change was only automated doc update. Applying lgtm label again.

mbohlool · 2017-11-10T00:20:23Z

/test pull-kubernetes-unit

mbohlool · 2017-11-10T01:36:30Z

I am going on vacation today. Would be nice if we can set higher priority on this PR so that submit queue merge it sooner. @caesarxuchao

caesarxuchao · 2017-11-10T01:39:20Z

@mbohlool is going to be away for weeks. I'll add the "queue/blocks-others" to get this one merged soon. We can iterate in future PRs if there are more concerns.

k8s-github-robot · 2017-11-10T05:05:51Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-10T05:39:23Z

Automatic merge from submit-queue (batch tested with PRs 55268, 55282, 55419, 48340, 54829). If you want to cherry-pick this change to another branch, please follow the instructions here.

sttts · 2017-11-10T08:43:02Z

staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go

+func mergeMutatingWebhookConfigurations(
+	list *v1alpha1.MutatingWebhookConfigurationList,
+) *v1alpha1.MutatingWebhookConfiguration {
+	configurations := append([]v1alpha1.MutatingWebhookConfiguration{}, list.Items...)


mbohlool assigned lavalamp Nov 8, 2017

mbohlool requested review from lavalamp and caesarxuchao November 8, 2017 01:37

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels Nov 8, 2017

cheftako reviewed Nov 8, 2017

View reviewed changes

mbohlool force-pushed the webhooks branch 2 times, most recently from 0070052 to 1bcd0b3 Compare November 8, 2017 08:15

k8s-ci-robot requested a review from deads2k November 8, 2017 16:23

sttts reviewed Nov 8, 2017

View reviewed changes

mbohlool force-pushed the webhooks branch from 1bcd0b3 to c6c39a7 Compare November 8, 2017 22:18

lavalamp reviewed Nov 8, 2017

View reviewed changes

mbohlool force-pushed the webhooks branch from c6c39a7 to f2bdf8b Compare November 8, 2017 22:27

lavalamp reviewed Nov 8, 2017

View reviewed changes

lavalamp closed this Nov 8, 2017

lavalamp reopened this Nov 8, 2017

mbohlool removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 9, 2017

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 9, 2017

mbohlool added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 9, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2017

mbohlool force-pushed the webhooks branch from 21c5c6d to ae2c229 Compare November 9, 2017 22:46

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

Update generated files for MutatingWebhookConfiguration

4568e05

mbohlool force-pushed the webhooks branch from ae2c229 to 4568e05 Compare November 9, 2017 23:34

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

mbohlool removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 9, 2017

mbohlool added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

mbohlool added this to the v1.8 milestone Nov 10, 2017

caesarxuchao added the queue/blocks-others label Nov 10, 2017

mbohlool modified the milestones: v1.8, v1.9 Nov 10, 2017

k8s-github-robot merged commit 61f2108 into kubernetes:master Nov 10, 2017

sttts reviewed Nov 10, 2017

View reviewed changes

dependabot-preview bot mentioned this pull request May 8, 2018

Bump kubernetes from 4.0.0 to 6.0.0 ministryofjustice/analytics-platform-unidler#3

Merged

dependabot-preview bot mentioned this pull request May 16, 2018

Bump kubernetes from 3.0.0 to 6.0.0 ministryofjustice/analytics-platform-control-panel#161

Closed

		@@ -153,7 +153,7 @@ func (a GenericAdmissionWebhook) SetScheme(scheme runtime.Scheme) {

		// WantsExternalKubeClientSet defines a function which sets external ClientSet for admission plugins that need it
		func (a *GenericAdmissionWebhook) SetExternalKubeClientSet(client clientset.Interface) {

Add MutatingWebhookConfiguration type #55282

Add MutatingWebhookConfiguration type #55282

Conversation

mbohlool commented Nov 8, 2017 • edited by lavalamp Loading

k8s-ci-robot commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Nov 8, 2017

lavalamp commented Nov 8, 2017

k8s-ci-robot commented Nov 9, 2017

lavalamp commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

mbohlool commented Nov 9, 2017

mbohlool commented Nov 10, 2017

mbohlool commented Nov 10, 2017

caesarxuchao commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

Choose a reason for hiding this comment

mbohlool commented Nov 8, 2017 •

edited by lavalamp

Loading