diff --git a/pkg/apis/admissionregistration/types.go b/pkg/apis/admissionregistration/types.go index 5e81ce43c92..d2986a4d11b 100644 --- a/pkg/apis/admissionregistration/types.go +++ b/pkg/apis/admissionregistration/types.go @@ -266,26 +266,60 @@ const ( // WebhookClientConfig contains the information to make a TLS // connection with the webhook type WebhookClientConfig struct { - // Service is a reference to the service for this webhook. If there is only - // one port open for the service, that port will be used. If there are multiple - // ports open, port 443 will be used if it is open, otherwise it is an error. - // Required - Service ServiceReference + // `url` gives the location of the webhook, in standard URL form + // (`[scheme://]host:port/path`). Exactly one of `url` or `service` + // must be specified. + // + // The `host` should not refer to a service running in the cluster; use + // the `service` field instead. The host might be resolved via external + // DNS in some apiservers (e.g., `kube-apiserver` cannot resolve + // in-cluster DNS as that would be a layering violation). `host` may + // also be an IP address. + // + // Please note that using `localhost` or `127.0.0.1` as a `host` is + // risky unless you take great care to run this webhook on all hosts + // which run an apiserver which might need to make calls to this + // webhook. Such installs are likely to be non-portable, i.e., not easy + // to turn up in a new cluster. + // + // If the scheme is present, it must be "https://". + // + // A path is optional, and if present may be any string permissible in + // a URL. You may use the path to pass an arbitrary string to the + // webhook, for example, a cluster identifier. + // + // +optional + URL *string - // URLPath is an optional field that specifies the URL path to use when posting the AdmissionReview object. - URLPath string + // `service` is a reference to the service for this webhook. Either + // `service` or `url` must be specified. + // + // If the webhook is running within the cluster, then you should use `service`. + // + // If there is only one port open for the service, that port will be + // used. If there are multiple ports open, port 443 will be used if it + // is open, otherwise it is an error. + // + // +optional + Service *ServiceReference - // CABundle is a PEM encoded CA bundle which will be used to validate webhook's server certificate. - // Required + // `caBundle` is a PEM encoded CA bundle which will be used to validate + // the webhook's server certificate. + // Required. CABundle []byte } // ServiceReference holds a reference to Service.legacy.k8s.io type ServiceReference struct { - // Namespace is the namespace of the service + // `namespace` is the namespace of the service. // Required Namespace string - // Name is the name of the service + // `name` is the name of the service. // Required Name string + + // `path` is an optional URL path which will be sent in any request to + // this service. + // +optional + Path *string } diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 11df7ac4f57..f22fa38c650 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -18,6 +18,7 @@ package validation import ( "fmt" + "net/url" "strings" genericvalidation "k8s.io/apimachinery/pkg/api/validation" @@ -192,29 +193,69 @@ func validateWebhook(hook *admissionregistration.Webhook, fldPath *field.Path) f allErrors = append(allErrors, field.NotSupported(fldPath.Child("failurePolicy"), *hook.FailurePolicy, supportedFailurePolicies.List())) } - if len(hook.ClientConfig.URLPath) != 0 { - allErrors = append(allErrors, validateURLPath(fldPath.Child("clientConfig", "urlPath"), hook.ClientConfig.URLPath)...) - } - if hook.NamespaceSelector != nil { allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, fldPath.Child("namespaceSelector"))...) } + allErrors = append(allErrors, validateWebhookClientConfig(fldPath.Child("clientConfig"), &hook.ClientConfig)...) + return allErrors } -func validateURLPath(fldPath *field.Path, urlPath string) field.ErrorList { +func validateWebhookClientConfig(fldPath *field.Path, cc *admissionregistration.WebhookClientConfig) field.ErrorList { var allErrors field.ErrorList + if (cc.URL == nil) == (cc.Service == nil) { + allErrors = append(allErrors, field.Required(fldPath.Child("url"), "exactly one of url or service is required")) + } + + if cc.URL != nil { + const form = "; desired format: https://host[/path]" + if u, err := url.Parse(*cc.URL); err != nil { + allErrors = append(allErrors, field.Required(fldPath.Child("url"), "url must be a valid URL: "+err.Error()+form)) + } else { + if u.Scheme != "" && u.Scheme != "https" { + allErrors = append(allErrors, field.Required(fldPath.Child("url"), "'https' is the only allowed URL scheme"+form)) + } + if len(u.Host) == 0 { + allErrors = append(allErrors, field.Required(fldPath.Child("url"), "host must be provided"+form)) + } + } + } + + if cc.Service != nil { + allErrors = append(allErrors, validateWebhookService(fldPath.Child("service"), cc.Service)...) + } + return allErrors +} + +func validateWebhookService(fldPath *field.Path, svc *admissionregistration.ServiceReference) field.ErrorList { + var allErrors field.ErrorList + + if len(svc.Name) == 0 { + allErrors = append(allErrors, field.Required(fldPath.Child("name"), "service name is required")) + } + + if len(svc.Namespace) == 0 { + allErrors = append(allErrors, field.Required(fldPath.Child("namespace"), "service namespace is required")) + } + + if svc.Path == nil { + return allErrors + } + + // TODO: replace below with url.Parse + verifying that host is empty? + + urlPath := *svc.Path if urlPath == "/" || len(urlPath) == 0 { return allErrors } if urlPath == "//" { - allErrors = append(allErrors, field.Invalid(fldPath, urlPath, "segment[0] may not be empty")) + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "segment[0] may not be empty")) return allErrors } if !strings.HasPrefix(urlPath, "/") { - allErrors = append(allErrors, field.Invalid(fldPath, urlPath, "must start with a '/'")) + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "must start with a '/'")) } urlPathToCheck := urlPath[1:] @@ -224,12 +265,12 @@ func validateURLPath(fldPath *field.Path, urlPath string) field.ErrorList { steps := strings.Split(urlPathToCheck, "/") for i, step := range steps { if len(step) == 0 { - allErrors = append(allErrors, field.Invalid(fldPath, urlPath, fmt.Sprintf("segment[%d] may not be empty", i))) + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d] may not be empty", i))) continue } failures := validation.IsDNS1123Subdomain(step) for _, failure := range failures { - allErrors = append(allErrors, field.Invalid(fldPath, urlPath, fmt.Sprintf("segment[%d]: %v", i, failure))) + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d]: %v", i, failure))) } } diff --git a/pkg/apis/admissionregistration/validation/validation_test.go b/pkg/apis/admissionregistration/validation/validation_test.go index 7fb0103ea79..c7d27ab8635 100644 --- a/pkg/apis/admissionregistration/validation/validation_test.go +++ b/pkg/apis/admissionregistration/validation/validation_test.go @@ -231,6 +231,8 @@ func TestValidateInitializerConfiguration(t *testing.T) { } } +func strPtr(s string) *string { return &s } + func newValidatingWebhookConfiguration(hooks []admissionregistration.Webhook) *admissionregistration.ValidatingWebhookConfiguration { return &admissionregistration.ValidatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ @@ -243,6 +245,9 @@ func newValidatingWebhookConfiguration(hooks []admissionregistration.Webhook) *a // TODO: Add TestValidateMutatingWebhookConfiguration to test validation for mutating webhooks. func TestValidateValidatingWebhookConfiguration(t *testing.T) { + validClientConfig := admissionregistration.WebhookClientConfig{ + URL: strPtr("https://example.com"), + } tests := []struct { name string config *admissionregistration.ValidatingWebhookConfiguration @@ -253,13 +258,16 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, }, { - Name: "k8s.io", + Name: "k8s.io", + ClientConfig: validClientConfig, }, { - Name: "", + Name: "", + ClientConfig: validClientConfig, }, }), expectedError: `webhooks[1].name: Invalid value: "k8s.io": should be a domain with at least three segments separated by dots, webhooks[2].name: Required value`, @@ -357,7 +365,8 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, Rules: []admissionregistration.RuleWithOperations{ { Operations: []admissionregistration.OperationType{"CREATE"}, @@ -376,7 +385,8 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, Rules: []admissionregistration.RuleWithOperations{ { Operations: []admissionregistration.OperationType{"CREATE"}, @@ -396,7 +406,8 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, Rules: []admissionregistration.RuleWithOperations{ { Operations: []admissionregistration.OperationType{"CREATE"}, @@ -416,7 +427,8 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, Rules: []admissionregistration.RuleWithOperations{ { Operations: []admissionregistration.OperationType{"CREATE"}, @@ -435,7 +447,8 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, Rules: []admissionregistration.RuleWithOperations{ { Operations: []admissionregistration.OperationType{"CREATE"}, @@ -455,7 +468,8 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, Rules: []admissionregistration.RuleWithOperations{ { Operations: []admissionregistration.OperationType{"CREATE"}, @@ -475,7 +489,8 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { - Name: "webhook.k8s.io", + Name: "webhook.k8s.io", + ClientConfig: validClientConfig, FailurePolicy: func() *admissionregistration.FailurePolicyType { r := admissionregistration.FailurePolicyType("other") return &r @@ -485,94 +500,202 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { expectedError: `webhooks[0].failurePolicy: Unsupported value: "other": supported values: "Fail", "Ignore"`, }, { - name: "URLPath must start with slash", + name: "both service and URL missing", + config: newValidatingWebhookConfiguration( + []admissionregistration.Webhook{ + { + Name: "webhook.k8s.io", + ClientConfig: admissionregistration.WebhookClientConfig{}, + }, + }), + expectedError: `exactly one of`, + }, + { + name: "both service and URL provided", config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { Name: "webhook.k8s.io", ClientConfig: admissionregistration.WebhookClientConfig{ - URLPath: "foo/", + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + }, + URL: strPtr("example.com/k8s/webhook"), }, }, }), - expectedError: `clientConfig.urlPath: Invalid value: "foo/": must start with a '/'`, + expectedError: `[0].clientConfig.url: Required value: exactly one of url or service is required`, }, { - name: "URLPath accepts slash", + name: "blank URL", config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { Name: "webhook.k8s.io", ClientConfig: admissionregistration.WebhookClientConfig{ - URLPath: "/", + URL: strPtr(""), + }, + }, + }), + expectedError: `[0].clientConfig.url: Required value: host must be provided`, + }, + { + name: "wrong scheme", + config: newValidatingWebhookConfiguration( + []admissionregistration.Webhook{ + { + Name: "webhook.k8s.io", + ClientConfig: admissionregistration.WebhookClientConfig{ + URL: strPtr("http://example.com"), + }, + }, + }), + expectedError: `https`, + }, + { + name: "missing host", + config: newValidatingWebhookConfiguration( + []admissionregistration.Webhook{ + { + Name: "webhook.k8s.io", + ClientConfig: admissionregistration.WebhookClientConfig{ + URL: strPtr("https:///fancy/webhook"), + }, + }, + }), + expectedError: `host must be provided`, + }, + { + name: "just totally wrong", + config: newValidatingWebhookConfiguration( + []admissionregistration.Webhook{ + { + Name: "webhook.k8s.io", + ClientConfig: admissionregistration.WebhookClientConfig{ + URL: strPtr("arg#backwards=thisis?html.index/port:host//:https"), + }, + }, + }), + expectedError: `host must be provided`, + }, + { + name: "path must start with slash", + config: newValidatingWebhookConfiguration( + []admissionregistration.Webhook{ + { + Name: "webhook.k8s.io", + ClientConfig: admissionregistration.WebhookClientConfig{ + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + Path: strPtr("foo/"), + }, + }, + }, + }), + expectedError: `clientConfig.service.path: Invalid value: "foo/": must start with a '/'`, + }, + { + name: "path accepts slash", + config: newValidatingWebhookConfiguration( + []admissionregistration.Webhook{ + { + Name: "webhook.k8s.io", + ClientConfig: admissionregistration.WebhookClientConfig{ + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + Path: strPtr("/"), + }, }, }, }), expectedError: ``, }, { - name: "URLPath accepts no trailing slash", + name: "path accepts no trailing slash", config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { Name: "webhook.k8s.io", ClientConfig: admissionregistration.WebhookClientConfig{ - URLPath: "/foo", + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + Path: strPtr("/foo"), + }, }, }, }), expectedError: ``, }, { - name: "URLPath fails //", + name: "path fails //", config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { Name: "webhook.k8s.io", ClientConfig: admissionregistration.WebhookClientConfig{ - URLPath: "//", + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + Path: strPtr("//"), + }, }, }, }), - expectedError: `clientConfig.urlPath: Invalid value: "//": segment[0] may not be empty`, + expectedError: `clientConfig.service.path: Invalid value: "//": segment[0] may not be empty`, }, { - name: "URLPath no empty step", + name: "path no empty step", config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { Name: "webhook.k8s.io", ClientConfig: admissionregistration.WebhookClientConfig{ - URLPath: "/foo//bar/", + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + Path: strPtr("/foo//bar/"), + }, }, }, }), - expectedError: `clientConfig.urlPath: Invalid value: "/foo//bar/": segment[1] may not be empty`, + expectedError: `clientConfig.service.path: Invalid value: "/foo//bar/": segment[1] may not be empty`, }, { - name: "URLPath no empty step 2", + name: "path no empty step 2", config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { Name: "webhook.k8s.io", ClientConfig: admissionregistration.WebhookClientConfig{ - URLPath: "/foo/bar//", + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + Path: strPtr("/foo/bar//"), + }, }, }, }), - expectedError: `clientConfig.urlPath: Invalid value: "/foo/bar//": segment[2] may not be empty`, + expectedError: `clientConfig.service.path: Invalid value: "/foo/bar//": segment[2] may not be empty`, }, { - name: "URLPath no non-subdomain", + name: "path no non-subdomain", config: newValidatingWebhookConfiguration( []admissionregistration.Webhook{ { Name: "webhook.k8s.io", ClientConfig: admissionregistration.WebhookClientConfig{ - URLPath: "/apis/foo.bar/v1alpha1/--bad", + Service: &admissionregistration.ServiceReference{ + Namespace: "ns", + Name: "n", + Path: strPtr("/apis/foo.bar/v1alpha1/--bad"), + }, }, }, }), - expectedError: `clientConfig.urlPath: Invalid value: "/apis/foo.bar/v1alpha1/--bad": segment[3]: a DNS-1123 subdomain`, + expectedError: `clientConfig.service.path: Invalid value: "/apis/foo.bar/v1alpha1/--bad": segment[3]: a DNS-1123 subdomain`, }, } for _, test := range tests { diff --git a/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go b/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go index 9232c1eb34f..23645f05b64 100644 --- a/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go +++ b/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go @@ -272,26 +272,60 @@ const ( // WebhookClientConfig contains the information to make a TLS // connection with the webhook type WebhookClientConfig struct { - // Service is a reference to the service for this webhook. If there is only - // one port open for the service, that port will be used. If there are multiple - // ports open, port 443 will be used if it is open, otherwise it is an error. - // Required - Service ServiceReference `json:"service" protobuf:"bytes,1,opt,name=service"` + // `url` gives the location of the webhook, in standard URL form + // (`[scheme://]host:port/path`). Exactly one of `url` or `service` + // must be specified. + // + // The `host` should not refer to a service running in the cluster; use + // the `service` field instead. The host might be resolved via external + // DNS in some apiservers (e.g., `kube-apiserver` cannot resolve + // in-cluster DNS as that would be a layering violation). `host` may + // also be an IP address. + // + // Please note that using `localhost` or `127.0.0.1` as a `host` is + // risky unless you take great care to run this webhook on all hosts + // which run an apiserver which might need to make calls to this + // webhook. Such installs are likely to be non-portable, i.e., not easy + // to turn up in a new cluster. + // + // If the scheme is present, it must be "https://". + // + // A path is optional, and if present may be any string permissible in + // a URL. You may use the path to pass an arbitrary string to the + // webhook, for example, a cluster identifier. + // + // +optional + URL *string `json:"url,omitempty"` - // URLPath is an optional field that specifies the URL path to use when posting the AdmissionReview object. - URLPath string `json:"urlPath" protobuf:"bytes,3,opt,name=urlPath"` + // `service` is a reference to the service for this webhook. Either + // `service` or `url` must be specified. + // + // If the webhook is running within the cluster, then you should use `service`. + // + // If there is only one port open for the service, that port will be + // used. If there are multiple ports open, port 443 will be used if it + // is open, otherwise it is an error. + // + // +optional + Service *ServiceReference `json:"service" protobuf:"bytes,1,opt,name=service"` - // CABundle is a PEM encoded CA bundle which will be used to validate webhook's server certificate. - // Required + // `caBundle` is a PEM encoded CA bundle which will be used to validate + // the webhook's server certificate. + // Required. CABundle []byte `json:"caBundle" protobuf:"bytes,2,opt,name=caBundle"` } // ServiceReference holds a reference to Service.legacy.k8s.io type ServiceReference struct { - // Namespace is the namespace of the service + // `namespace` is the namespace of the service. // Required Namespace string `json:"namespace" protobuf:"bytes,1,opt,name=namespace"` - // Name is the name of the service + // `name` is the name of the service. // Required Name string `json:"name" protobuf:"bytes,2,opt,name=name"` + + // `path` is an optional URL path which will be sent in any request to + // this service. + // +optional + Path *string `json:"path,omitempty"` } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go index 96f5b1ea34f..1eba62672d8 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go @@ -20,6 +20,7 @@ package webhook import ( "context" "encoding/json" + "errors" "fmt" "io" "net" @@ -55,6 +56,10 @@ const ( defaultCacheSize = 200 ) +var ( + ErrNeedServiceOrURL = errors.New("webhook configuration must have either service or URL") +) + type ErrCallingWebhook struct { WebhookName string Reason error @@ -395,47 +400,71 @@ func toStatusErr(name string, result *metav1.Status) *apierrors.StatusError { func (a *GenericAdmissionWebhook) hookClient(h *v1alpha1.Webhook) (*rest.RESTClient, error) { cacheKey, err := json.Marshal(h.ClientConfig) - if err != nil { - return nil, err - } if client, ok := a.cache.Get(string(cacheKey)); ok { return client.(*rest.RESTClient), nil } - serverName := h.ClientConfig.Service.Name + "." + h.ClientConfig.Service.Namespace + ".svc" - restConfig, err := a.authInfoResolver.ClientConfigFor(serverName) + complete := func(cfg *rest.Config) (*rest.RESTClient, error) { + cfg.TLSClientConfig.CAData = h.ClientConfig.CABundle + cfg.ContentConfig.NegotiatedSerializer = a.negotiatedSerializer + cfg.ContentConfig.ContentType = runtime.ContentTypeJSON + client, err := rest.UnversionedRESTClientFor(cfg) + if err == nil { + a.cache.Add(string(cacheKey), client) + } + return client, err + } + + if svc := h.ClientConfig.Service; svc != nil { + serverName := svc.Name + "." + svc.Namespace + ".svc" + restConfig, err := a.authInfoResolver.ClientConfigFor(serverName) + if err != nil { + return nil, err + } + cfg := rest.CopyConfig(restConfig) + host := serverName + ":443" + cfg.Host = "https://" + host + if svc.Path != nil { + cfg.APIPath = *svc.Path + } + cfg.TLSClientConfig.ServerName = serverName + + delegateDialer := cfg.Dial + if delegateDialer == nil { + delegateDialer = net.Dial + } + cfg.Dial = func(network, addr string) (net.Conn, error) { + if addr == host { + u, err := a.serviceResolver.ResolveEndpoint(svc.Namespace, svc.Name) + if err != nil { + return nil, err + } + addr = u.Host + } + return delegateDialer(network, addr) + } + + return complete(cfg) + } + + if h.ClientConfig.URL == nil { + return nil, &ErrCallingWebhook{WebhookName: h.Name, Reason: ErrNeedServiceOrURL} + } + + u, err := url.Parse(*h.ClientConfig.URL) + if err != nil { + return nil, &ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Unparsable URL: %v", err)} + } + + restConfig, err := a.authInfoResolver.ClientConfigFor(u.Host) if err != nil { return nil, err } cfg := rest.CopyConfig(restConfig) - host := serverName + ":443" - cfg.Host = "https://" + host - cfg.APIPath = h.ClientConfig.URLPath - cfg.TLSClientConfig.ServerName = serverName - cfg.TLSClientConfig.CAData = h.ClientConfig.CABundle - cfg.ContentConfig.NegotiatedSerializer = a.negotiatedSerializer - cfg.ContentConfig.ContentType = runtime.ContentTypeJSON + cfg.Host = u.Host + cfg.APIPath = u.Path + // TODO: test if this is needed: cfg.TLSClientConfig.ServerName = u.Host - delegateDialer := cfg.Dial - if delegateDialer == nil { - delegateDialer = net.Dial - } - - cfg.Dial = func(network, addr string) (net.Conn, error) { - if addr == host { - u, err := a.serviceResolver.ResolveEndpoint(h.ClientConfig.Service.Namespace, h.ClientConfig.Service.Name) - if err != nil { - return nil, err - } - addr = u.Host - } - return delegateDialer(network, addr) - } - - client, err := rest.UnversionedRESTClientFor(cfg) - if err == nil { - a.cache.Add(string(cacheKey), client) - } - return client, err + return complete(cfg) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go index 25308fa230c..d16c3605fa0 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go @@ -89,6 +89,33 @@ func (f fakeNamespaceLister) Get(name string) (*corev1.Namespace, error) { return nil, errors.NewNotFound(corev1.Resource("namespaces"), name) } +// ccfgSVC returns a client config using the service reference mechanism. +func ccfgSVC(urlPath string) registrationv1alpha1.WebhookClientConfig { + return registrationv1alpha1.WebhookClientConfig{ + Service: ®istrationv1alpha1.ServiceReference{ + Name: "webhook-test", + Namespace: "default", + Path: &urlPath, + }, + CABundle: caCert, + } +} + +type urlConfigGenerator struct { + baseURL *url.URL +} + +// ccfgURL returns a client config using the URL mechanism. +func (c urlConfigGenerator) ccfgURL(urlPath string) registrationv1alpha1.WebhookClientConfig { + u2 := *c.baseURL + u2.Path = urlPath + urlString := u2.String() + return registrationv1alpha1.WebhookClientConfig{ + URL: &urlString, + CABundle: caCert, + } +} + // TestAdmit tests that GenericAdmissionWebhook#Admit works as expected func TestAdmit(t *testing.T) { scheme := runtime.NewScheme() @@ -148,6 +175,8 @@ func TestAdmit(t *testing.T) { UID: "webhook-test", } + ccfgURL := urlConfigGenerator{serverURL}.ccfgURL + type test struct { hookSource fakeHookSource path string @@ -155,6 +184,15 @@ func TestAdmit(t *testing.T) { errorContains string } + matchEverythingRules := []registrationv1alpha1.RuleWithOperations{{ + Operations: []registrationv1alpha1.OperationType{registrationv1alpha1.OperationAll}, + Rule: registrationv1alpha1.Rule{ + APIGroups: []string{"*"}, + APIVersions: []string{"*"}, + Resources: []string{"*/*"}, + }, + }} + policyFail := registrationv1alpha1.Fail policyIgnore := registrationv1alpha1.Ignore @@ -163,7 +201,7 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "nomatch", - ClientConfig: newFakeHookClientConfig("disallow"), + ClientConfig: ccfgSVC("disallow"), Rules: []registrationv1alpha1.RuleWithOperations{{ Operations: []registrationv1alpha1.OperationType{registrationv1alpha1.Create}, }}, @@ -175,8 +213,8 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "allow", - ClientConfig: newFakeHookClientConfig("allow"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("allow"), + Rules: matchEverythingRules, }}, }, expectAllow: true, @@ -185,8 +223,8 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "disallow", - ClientConfig: newFakeHookClientConfig("disallow"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("disallow"), + Rules: matchEverythingRules, }}, }, errorContains: "without explanation", @@ -195,8 +233,8 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "disallowReason", - ClientConfig: newFakeHookClientConfig("disallowReason"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("disallowReason"), + Rules: matchEverythingRules, }}, }, errorContains: "you shall not pass", @@ -205,7 +243,7 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "disallow", - ClientConfig: newFakeHookClientConfig("disallow"), + ClientConfig: ccfgSVC("disallow"), Rules: newMatchEverythingRules(), NamespaceSelector: &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{{ @@ -222,7 +260,7 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "disallow", - ClientConfig: newFakeHookClientConfig("disallow"), + ClientConfig: ccfgSVC("disallow"), Rules: newMatchEverythingRules(), NamespaceSelector: &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{{ @@ -239,18 +277,18 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "internalErr A", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, FailurePolicy: &policyIgnore, }, { Name: "internalErr B", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, FailurePolicy: &policyIgnore, }, { Name: "internalErr C", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, FailurePolicy: &policyIgnore, }}, }, @@ -260,16 +298,16 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "internalErr A", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, }, { Name: "internalErr B", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, }, { Name: "internalErr C", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, }}, }, expectAllow: false, @@ -278,23 +316,45 @@ func TestAdmit(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "internalErr A", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, FailurePolicy: &policyFail, }, { Name: "internalErr B", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, FailurePolicy: &policyFail, }, { Name: "internalErr C", - ClientConfig: newFakeHookClientConfig("internalErr"), - Rules: newMatchEverythingRules(), + ClientConfig: ccfgSVC("internalErr"), + Rules: matchEverythingRules, FailurePolicy: &policyFail, }}, }, expectAllow: false, }, + "match & allow (url)": { + hookSource: fakeHookSource{ + hooks: []registrationv1alpha1.Webhook{{ + Name: "allow", + ClientConfig: ccfgURL("allow"), + Rules: matchEverythingRules, + }}, + }, + expectAllow: true, + }, + "match & disallow (url)": { + hookSource: fakeHookSource{ + hooks: []registrationv1alpha1.Webhook{{ + Name: "disallow", + ClientConfig: ccfgURL("disallow"), + Rules: matchEverythingRules, + }}, + }, + errorContains: "without explanation", + }, + // No need to test everything with the url case, since only the + // connection is different. } for name, tt := range table { @@ -378,6 +438,7 @@ func TestAdmitCachedClient(t *testing.T) { Name: "webhook-test", UID: "webhook-test", } + ccfgURL := urlConfigGenerator{serverURL}.ccfgURL type test struct { name string @@ -393,7 +454,7 @@ func TestAdmitCachedClient(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "cache1", - ClientConfig: newFakeHookClientConfig("allow"), + ClientConfig: ccfgSVC("allow"), Rules: newMatchEverythingRules(), FailurePolicy: &policyIgnore, }}, @@ -406,7 +467,7 @@ func TestAdmitCachedClient(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "cache2", - ClientConfig: newFakeHookClientConfig("internalErr"), + ClientConfig: ccfgSVC("internalErr"), Rules: newMatchEverythingRules(), FailurePolicy: &policyIgnore, }}, @@ -419,7 +480,33 @@ func TestAdmitCachedClient(t *testing.T) { hookSource: fakeHookSource{ hooks: []registrationv1alpha1.Webhook{{ Name: "cache3", - ClientConfig: newFakeHookClientConfig("allow"), + ClientConfig: ccfgSVC("allow"), + Rules: newMatchEverythingRules(), + FailurePolicy: &policyIgnore, + }}, + }, + expectAllow: true, + expectCache: false, + }, + { + name: "cache 4", + hookSource: fakeHookSource{ + hooks: []registrationv1alpha1.Webhook{{ + Name: "cache4", + ClientConfig: ccfgURL("allow"), + Rules: newMatchEverythingRules(), + FailurePolicy: &policyIgnore, + }}, + }, + expectAllow: true, + expectCache: true, + }, + { + name: "cache 5", + hookSource: fakeHookSource{ + hooks: []registrationv1alpha1.Webhook{{ + Name: "cache5", + ClientConfig: ccfgURL("allow"), Rules: newMatchEverythingRules(), FailurePolicy: &policyIgnore, }}, @@ -581,17 +668,6 @@ func TestToStatusErr(t *testing.T) { } } -func newFakeHookClientConfig(urlPath string) registrationv1alpha1.WebhookClientConfig { - return registrationv1alpha1.WebhookClientConfig{ - Service: registrationv1alpha1.ServiceReference{ - Name: "webhook-test", - Namespace: "default", - }, - URLPath: urlPath, - CABundle: caCert, - } -} - func newMatchEverythingRules() []registrationv1alpha1.RuleWithOperations { return []registrationv1alpha1.RuleWithOperations{{ Operations: []registrationv1alpha1.OperationType{registrationv1alpha1.OperationAll}, diff --git a/test/e2e/apimachinery/webhook.go b/test/e2e/apimachinery/webhook.go index 08e02b82435..30febfc2923 100644 --- a/test/e2e/apimachinery/webhook.go +++ b/test/e2e/apimachinery/webhook.go @@ -218,6 +218,8 @@ func deployWebhookAndService(f *framework.Framework, image string, context *cert framework.ExpectNoError(err, "waiting for service %s/%s have %d endpoint", namespace, serviceName, 1) } +func strPtr(s string) *string { return &s } + func registerWebhook(f *framework.Framework, context *certContext) { client := f.ClientSet By("Registering the webhook via the AdmissionRegistration API") @@ -239,11 +241,11 @@ func registerWebhook(f *framework.Framework, context *certContext) { }, }}, ClientConfig: v1alpha1.WebhookClientConfig{ - Service: v1alpha1.ServiceReference{ + Service: &v1alpha1.ServiceReference{ Namespace: namespace, Name: serviceName, + Path: strPtr("/pods"), }, - URLPath: "/pods", CABundle: context.signingCert, }, }, @@ -268,11 +270,11 @@ func registerWebhook(f *framework.Framework, context *certContext) { }, }, ClientConfig: v1alpha1.WebhookClientConfig{ - Service: v1alpha1.ServiceReference{ + Service: &v1alpha1.ServiceReference{ Namespace: namespace, Name: serviceName, + Path: strPtr("/configmaps"), }, - URLPath: "/configmaps", CABundle: context.signingCert, }, }, diff --git a/test/integration/etcd/etcd_storage_path_test.go b/test/integration/etcd/etcd_storage_path_test.go index 9d8e77c3d34..95f865568d4 100644 --- a/test/integration/etcd/etcd_storage_path_test.go +++ b/test/integration/etcd/etcd_storage_path_test.go @@ -382,11 +382,11 @@ var etcdStorageData = map[schema.GroupVersionResource]struct { expectedEtcdPath: "/registry/initializerconfigurations/ic1", }, gvr("admissionregistration.k8s.io", "v1alpha1", "validatingwebhookconfigurations"): { - stub: `{"metadata":{"name":"hook1","creationTimestamp":null},"webhooks":[{"name":"externaladmissionhook.k8s.io","clientConfig":{"service":{"namespace":"","name":""},"caBundle":null},"rules":[{"operations":["CREATE"],"apiGroups":["group"],"apiVersions":["version"],"resources":["resource"]}],"failurePolicy":"Ignore"}]}`, + stub: `{"metadata":{"name":"hook1","creationTimestamp":null},"webhooks":[{"name":"externaladmissionhook.k8s.io","clientConfig":{"service":{"namespace":"ns","name":"n"},"caBundle":null},"rules":[{"operations":["CREATE"],"apiGroups":["group"],"apiVersions":["version"],"resources":["resource"]}],"failurePolicy":"Ignore"}]}`, expectedEtcdPath: "/registry/validatingwebhookconfigurations/hook1", }, gvr("admissionregistration.k8s.io", "v1alpha1", "mutatingwebhookconfigurations"): { - stub: `{"metadata":{"name":"hook1","creationTimestamp":null},"webhooks":[{"name":"externaladmissionhook.k8s.io","clientConfig":{"service":{"namespace":"","name":""},"caBundle":null},"rules":[{"operations":["CREATE"],"apiGroups":["group"],"apiVersions":["version"],"resources":["resource"]}],"failurePolicy":"Ignore"}]}`, + stub: `{"metadata":{"name":"hook1","creationTimestamp":null},"webhooks":[{"name":"externaladmissionhook.k8s.io","clientConfig":{"service":{"namespace":"ns","name":"n"},"caBundle":null},"rules":[{"operations":["CREATE"],"apiGroups":["group"],"apiVersions":["version"],"resources":["resource"]}],"failurePolicy":"Ignore"}]}`, expectedEtcdPath: "/registry/mutatingwebhookconfigurations/hook1", }, // --