admission: wire through validating-only admission #54484

sttts · 2017-10-24T13:38:42Z

Based on #54232.

This is important and required for beta because it affects the shape of the webhook admission plugins that are going to be produced and is needed to make sure that our existing chain continues to properly verify and protect the API objects based on their final state after webhook admission mutators run.

We discussed this in the October 11 API machinery call with @erictune and @caesarxuchao and we agreed to do this as a requirement for beta. See this part of the recording: https://www.youtube.com/watch?v=mrgDPHyr4VY#t=325 .

deads2k · 2017-10-24T14:46:13Z

/assign caesarxuchao
@lavalamp

k8s-ci-robot · 2017-10-24T14:46:53Z

@sttts: 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.

deads2k · 2017-10-26T13:55:23Z

pkg/registry/apps/statefulset/registry.go

@@ -73,15 +73,15 @@ func (s *storage) GetStatefulSet(ctx genericapirequest.Context, statefulSetID st
 }

 func (s *storage) CreateStatefulSet(ctx genericapirequest.Context, statefulSet *apps.StatefulSet) (*apps.StatefulSet, error) {
-	obj, err := s.Create(ctx, statefulSet, false)
+	obj, err := s.Create(ctx, statefulSet, rest.ValidateAllObjectFunc, false)


So I've been reading through this code. I think it is a mistake to make this intermediate registry for use by the scale client. It makes what is happening far less obvious. Your choice here is safe, but only because of how this is used.

Please open an issue.

It's boilerplate code essentially doing a type cast, not more. I agree this complicates more than it helps.

Following the old behaviour, I wired the validation functions through now from the subresource registries via the intermediate registries to the storage.

deads2k · 2017-10-26T13:56:45Z

pkg/registry/certificates/certificates/registry.go

@@ -56,12 +56,12 @@ func (s *storage) ListCSRs(ctx genericapirequest.Context, options *metainternalv
 }

 func (s *storage) CreateCSR(ctx genericapirequest.Context, csr *certificates.CertificateSigningRequest) error {


file looks dead. please delete instead.

We have plenty of these which are not called. This will be fixed in a follow-up via #54693.

deads2k · 2017-10-26T13:57:30Z

pkg/registry/core/configmap/registry.go

@@ -69,7 +69,7 @@ func (s *storage) GetConfigMap(ctx genericapirequest.Context, name string, optio
 }

 func (s *storage) CreateConfigMap(ctx genericapirequest.Context, cfg *api.ConfigMap) (*api.ConfigMap, error) {


looks dead, please delete instead.

Follow-up via #54693

deads2k · 2017-10-26T13:58:26Z

pkg/registry/core/endpoint/registry.go

@@ -66,7 +66,7 @@ func (s *storage) GetEndpoints(ctx genericapirequest.Context, name string, optio
 }

 func (s *storage) UpdateEndpoints(ctx genericapirequest.Context, endpoints *api.Endpoints) error {
-	_, _, err := s.Update(ctx, endpoints.Name, rest.DefaultUpdatedObjectInfo(endpoints))
+	_, _, err := s.Update(ctx, endpoints.Name, rest.DefaultUpdatedObjectInfo(endpoints), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc)


only safe because of how it is called. Needs a comment.

deads2k · 2017-10-26T13:58:58Z

pkg/registry/core/namespace/registry.go

@@ -67,12 +67,12 @@ func (s *storage) GetNamespace(ctx genericapirequest.Context, namespaceName stri
 }

 func (s *storage) CreateNamespace(ctx genericapirequest.Context, namespace *api.Namespace) error {


appears dead. please delete instead.

Follow-up via #54693

deads2k · 2017-10-26T13:59:37Z

pkg/registry/core/node/registry.go

@@ -56,12 +56,12 @@ func (s *storage) ListNodes(ctx genericapirequest.Context, options *metainternal
 }

 func (s *storage) CreateNode(ctx genericapirequest.Context, node *api.Node) error {


appears dead. please delete instead

Follow-up via #54693

deads2k · 2017-10-26T14:00:43Z

pkg/registry/core/replicationcontroller/registry.go

@@ -74,15 +74,15 @@ func (s *storage) GetController(ctx genericapirequest.Context, controllerID stri
 }

 func (s *storage) CreateController(ctx genericapirequest.Context, controller *api.ReplicationController) (*api.ReplicationController, error) {


same comment about scale here. Needs a big warning and a followup issue.

deads2k · 2017-10-26T14:01:53Z

pkg/registry/core/secret/registry.go

@@ -67,12 +67,12 @@ func (s *storage) GetSecret(ctx genericapirequest.Context, name string, options
 }

 func (s *storage) CreateSecret(ctx genericapirequest.Context, secret *api.Secret) (*api.Secret, error) {


comment that its only safe due to usage and that we need to move to a loopback connection for SA token authentication.

issue for the loopback thing.

Also dead code fixed via #54693

deads2k · 2017-10-26T14:03:02Z

pkg/registry/core/service/registry.go

@@ -58,7 +58,7 @@ func (s *storage) ListServices(ctx genericapirequest.Context, options *metainter
 }

 func (s *storage) CreateService(ctx genericapirequest.Context, svc *api.Service) (*api.Service, error) {


only safe due to usage. Issue to move this one to point of use.

This is actually used. Wired through the createValidation from service REST Create.

deads2k · 2017-10-26T14:03:43Z

pkg/registry/core/serviceaccount/registry.go

@@ -67,12 +67,12 @@ func (s *storage) GetServiceAccount(ctx genericapirequest.Context, name string,
 }

 func (s *storage) CreateServiceAccount(ctx genericapirequest.Context, serviceAccount *api.ServiceAccount) error {


only safe due to usage. This one should be removed in favor of loopback too.

Also dead code fixed via #54693

deads2k · 2017-10-26T14:06:14Z

pkg/registry/extensions/deployment/storage/storage.go

@@ -225,6 +225,7 @@ func (r *ScaleREST) Update(ctx genericapirequest.Context, name string, objInfo r
 		return nil, false, err
 	}

+	// TODO: should we pass admission here?


Yes, but you should do it by eliminating the use of the registry or changing the registry signature to accept the validation functions.

Are the validation/update validation functions passed into this function going to be about Deployments or Scale types? I would naively expect the latter, in which case this code probably needs some drastic revision. (file an issue "scale (all?) subresources evade admission checking") rather than fix it in this PR.

I would naively expect the former. As a webhook author, should I care about the way a user changes the replicas value? I would naively add an admission step for the full resource.

lavalamp 20 hours ago
Are the validation/update validation functions passed into this function going to be about Deployments or Scale types? I would naively expect the latter, in which case this code probably needs some drastic revision. (file an issue "scale (all?) subresources evade admission checking") rather than fix it in this PR.

sttts 2 hours ago
I would naively expect the former. As a webhook author, should I care about the way a user changes the replicas value? I would naively add an admission step for the full resource.

In our code today, the same admission chain is called for subresources as is called for normal resources. This gives admission plugins the flexibility to decide if they care about subresource modifications (not all will), but if they do, it gives the full before/after context.

I think for now we should preserve the existing behavior (which I called out in a comment). I'm open to discussing a change in behavior, but I'd like to do it outside the context of this pull. @lavalamp I think you agree based on "(file an issue "scale (all?) subresources evade admission checking") rather than fix it in this PR."

Comment is outdated. Will remove the TODO.

fejta-bot · 2017-11-01T23:23:57Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-11-02T02:14:56Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-11-02T05:47:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-11-02T10:14:40Z

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

k8s-github-robot · 2017-11-02T11:02:33Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue (batch tested with PRs 55331, 55272, 55228, 49763, 55242). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. apiserver: add validating admission tests Follow-up of #54484 This includes tests - in endpoint tests, - in generic registry, - in patch handler, - in admission chain.

Automatic merge from submit-queue (batch tested with PRs 55331, 55272, 55228, 49763, 55242). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. apiserver: add validating admission tests Follow-up of kubernetes/kubernetes#54484 This includes tests - in endpoint tests, - in generic registry, - in patch handler, - in admission chain. Kubernetes-commit: ee7f1b6e74fab4ed77592f29264575c634e4f2c1

k8s-github-robot assigned hongchaodeng and nhlfr Oct 24, 2017

sttts force-pushed the sttts-split-psp branch from 6f750c4 to daf8666 Compare October 24, 2017 13:47

sttts assigned liggitt and deads2k and unassigned hongchaodeng and nhlfr Oct 24, 2017

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

sttts force-pushed the sttts-split-psp branch from daf8666 to 4266322 Compare October 24, 2017 13:48

k8s-ci-robot assigned caesarxuchao Oct 24, 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 Oct 24, 2017

sttts force-pushed the sttts-split-psp branch from 85a34c0 to 52de214 Compare October 24, 2017 15:38

deads2k reviewed Oct 26, 2017

View reviewed changes

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

sttts force-pushed the sttts-split-psp branch from b8fac4e to 1bb2cb2 Compare November 2, 2017 07:45

deads2k and others added 4 commits November 2, 2017 08:45

add wiring for validating admission

02e16cb

admission: { -> Mutating}Admit(admission.Attributes)

970d255

admission: split MutationInterface out of Interface

d4f48c9

admission: complete plumbing of validation admission

74b4223

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

sttts added 2 commits November 2, 2017 09:29

admission: wire create+update validation func into kube registries

2452aff

admission: rename Validate{ -> Initialization}, Validat{ingAdmit -> e}

aedcf68

sttts force-pushed the sttts-split-psp branch from 1bb2cb2 to 35bb682 Compare November 2, 2017 08:33

Update bazel

35bb682

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

k8s-github-robot merged commit 40212c1 into kubernetes:master Nov 2, 2017

This was referenced Nov 6, 2017

Admission metrics #55086

Closed

apiserver: add validating admission tests #55228

Merged

admission: fold create+update validation funcs into update ObjectInfo #55240

Closed

		@@ -56,12 +56,12 @@ func (s storage) ListCSRs(ctx genericapirequest.Context, options metainternalv
		}

		func (s storage) CreateCSR(ctx genericapirequest.Context, csr certificates.CertificateSigningRequest) error {

		@@ -69,7 +69,7 @@ func (s *storage) GetConfigMap(ctx genericapirequest.Context, name string, optio
		}

		func (s storage) CreateConfigMap(ctx genericapirequest.Context, cfg api.ConfigMap) (*api.ConfigMap, error) {

		@@ -67,12 +67,12 @@ func (s *storage) GetNamespace(ctx genericapirequest.Context, namespaceName stri
		}

		func (s storage) CreateNamespace(ctx genericapirequest.Context, namespace api.Namespace) error {

		@@ -56,12 +56,12 @@ func (s storage) ListNodes(ctx genericapirequest.Context, options metainternal
		}

		func (s storage) CreateNode(ctx genericapirequest.Context, node api.Node) error {

		@@ -74,15 +74,15 @@ func (s *storage) GetController(ctx genericapirequest.Context, controllerID stri
		}

		func (s storage) CreateController(ctx genericapirequest.Context, controller api.ReplicationController) (*api.ReplicationController, error) {

		@@ -67,12 +67,12 @@ func (s *storage) GetSecret(ctx genericapirequest.Context, name string, options
		}

		func (s storage) CreateSecret(ctx genericapirequest.Context, secret api.Secret) (*api.Secret, error) {

		@@ -58,7 +58,7 @@ func (s storage) ListServices(ctx genericapirequest.Context, options metainter
		}

		func (s storage) CreateService(ctx genericapirequest.Context, svc api.Service) (*api.Service, error) {

		@@ -67,12 +67,12 @@ func (s *storage) GetServiceAccount(ctx genericapirequest.Context, name string,
		}

		func (s storage) CreateServiceAccount(ctx genericapirequest.Context, serviceAccount api.ServiceAccount) error {

admission: wire through validating-only admission #54484

admission: wire through validating-only admission #54484

Conversation

sttts commented Oct 24, 2017 • edited by deads2k Loading

deads2k commented Oct 24, 2017

k8s-ci-robot commented Oct 24, 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

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

fejta-bot commented Nov 1, 2017

fejta-bot commented Nov 2, 2017

fejta-bot commented Nov 2, 2017

k8s-github-robot commented Nov 2, 2017

k8s-github-robot commented Nov 2, 2017

sttts commented Oct 24, 2017 •

edited by deads2k

Loading