Reorganize admission webhook code #55132

caesarxuchao · 2017-11-06T02:47:15Z

Moved client and kubeconfig related code to webhook/config;
Moved the rule matcher to webhook/rules;
Left TODOs saying we are going to move some other common utilities;
Other code is moved to webhook/validation.

This is to prepare adding the mutating webhook. See #54892.

k8s-ci-robot · 2017-11-06T02:47:17Z

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

caesarxuchao · 2017-11-06T02:47:32Z

/sig api-machinery

lavalamp · 2017-11-14T00:15:45Z

pkg/kubeapiserver/admission/initializer.go

@@ -21,7 +21,7 @@ import (

 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apiserver/pkg/admission"
-	"k8s.io/apiserver/pkg/admission/plugin/webhook"
+	webhookconfig "k8s.io/apiserver/pkg/admission/plugin/webhook/config"


I think all of these interface types should go in "k8s.io/apiserver/pkg/admission/options" or something like that. They're available to all admission plugins, not just webhooks.

lavalamp · 2017-11-14T00:18:01Z

pkg/kubeapiserver/admission/initializer.go

@@ -62,7 +62,7 @@ type WantsQuotaConfiguration interface {
 // WantsServiceResolver defines a fuction that accepts a ServiceResolver for
 // admission plugins that need to make calls to services.
 type WantsServiceResolver interface {


I am surprised that these "WantsX" interfaces are defined here rather than in the generic layer, but that's a can of worms for some other time.

They scatter in both directories.

$ git grep "type Wants.* interface" pkg/kubeapiserver/admission/initializer.go:35:type WantsInternalKubeClientSet interface { pkg/kubeapiserver/admission/initializer.go:41:type WantsInternalKubeInformerFactory interface { pkg/kubeapiserver/admission/initializer.go:47:type WantsCloudConfig interface { pkg/kubeapiserver/admission/initializer.go:52:type WantsRESTMapper interface { pkg/kubeapiserver/admission/initializer.go:57:type WantsQuotaConfiguration interface { pkg/kubeapiserver/admission/initializer.go:64:type WantsServiceResolver interface { pkg/kubeapiserver/admission/initializer.go:76:type WantsAuthenticationInfoResolverWrapper interface { staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go:28:type WantsExternalKubeClientSet interface { staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go:34:type WantsExternalKubeInformerFactory interface { staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go:40:type WantsAuthorizer interface { staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go:46:type WantsScheme interface {

Hm, maybe some of those aren't generic. I think auth info, service resolver, and rest mapper probably are. Don't fix it in this PR :)

lavalamp · 2017-11-14T00:18:36Z

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

@@ -43,7 +43,7 @@ type defaultAuthenticationInfoResolver struct {
 	kubeconfig clientcmdapi.Config
 }

-func newDefaultAuthenticationInfoResolver(kubeconfigFile string) (AuthenticationInfoResolver, error) {
+func NewDefaultAuthenticationInfoResolver(kubeconfigFile string) (AuthenticationInfoResolver, error) {


lavalamp · 2017-11-14T00:19:12Z

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

+	ErrNeedServiceOrURL = errors.New("webhook configuration must have either service or URL")
+)
+
+type ClientManager struct {


Godoc on public types/funcs in this file?

lavalamp · 2017-11-14T00:20:29Z

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

+	}, nil
+}
+
+func (cm *ClientManager) SetAuthenticationInfoResolverWrapper(wrapper AuthenticationInfoResolverWrapper) {


Why is this passed to new and also here?

The one passed to new is AuthenticationInfoResolver. I don't see an easy way to remove either.

lavalamp · 2017-11-14T00:22:36Z

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

-	KubeConfigFile string `json:"kubeConfigFile"`
+import "fmt"
+
+type ErrCallingWebhook struct {


// ErrCallingWebhook is returned for transport-layer errors calling webhooks. It represents a failure to talk to the webhook, not the webhook rejecting a request.

Or something.

lavalamp · 2017-11-14T00:23:37Z

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

 // ResolveEndpoint constructs a service URL from a given namespace and name
 // note that the name and namespace are required and by default all created addresses use HTTPS scheme.
 // for example:
 //  name=ross namespace=andromeda resolves to https://ross.andromeda.svc:443
-func (sr defaultServiceResolver) ResolveEndpoint(namespace, name string) (*url.URL, error) {
+func (sr *defaultServiceResolver) ResolveEndpoint(namespace, name string) (*url.URL, error) {


Why make this a pointer? It has no member variables.

lavalamp · 2017-11-14T00:24:39Z

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

@@ -24,12 +23,14 @@ import (
 	"k8s.io/apiserver/pkg/admission"
 )

-type RuleMatcher struct {
+// Matcher determines if the Attr matches the Rule.


Nice, very readable as rules.Matcher.

lavalamp · 2017-11-14T00:30:48Z

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

 	if err != nil {
 		return nil, err
 	}

-	cache, err := lru.New(defaultCacheSize)
+	cm, err := config.NewClientManager(authInfoResolver, config.NewDefaultServiceResolver())


won't these parameters be nil when this line is called?

No, these are the default values.

lavalamp · 2017-11-14T00:32:59Z

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

@@ -195,7 +153,7 @@ func (a *GenericAdmissionWebhook) ValidateInitialization() error {
 	if a.hookSource == nil {
 		return fmt.Errorf("the GenericAdmissionWebhook admission plugin requires a Kubernetes client to be provided")
 	}
-	if a.negotiatedSerializer == nil {
+	if a.clientManager.GetNegotiatedSerializer() == nil {


Maybe define a func (c *ClientManager) Valid() error and call it here. Shouldn't this be checking the resolvers, too?

…code Automatic merge from submit-queue. 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>. Reorganizing more webhook code ref: kubernetes/enhancements#492 Continue on kubernetes/kubernetes#55132. With this PR, all code shared between the mutating and validating webhook plugins is extracted into its own package. Kubernetes-commit: c3e4084066105ff632668deb3732cdb276b0e622

Automatic merge from submit-queue (batch tested with PRs 55963, 55790, 55670, 55931). 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>. admission/webhook: move webhook initializer into plugin Follow-up of kubernetes/kubernetes#55132. Non-generic plugin intitializers should go into the admission plugin itself. No need leak that into the generic apiserver. Keeping them contained in the webhook package makes it easier (it was already possible) for extension apiservers to provide one, but we don't need to leak it into the generic plugin initializer. Kubernetes-commit: 7fd2b627766613829908fff4785c40f8f013577e