From 58b43ad27d00191cf5291d8508dc346f1924b785 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Mon, 5 Mar 2018 16:35:52 -0800 Subject: [PATCH] Prevent webhooks from affecting admission requests for webhooks --- .../admissionregistration/v1beta1/types.go | 4 + .../plugin/webhook/mutating/admission.go | 4 + .../admission/plugin/webhook/rules/rules.go | 12 ++ .../plugin/webhook/validating/admission.go | 6 +- test/e2e/apimachinery/webhook.go | 165 +++++++++++++++++- test/images/webhook/Makefile | 7 +- test/images/webhook/main.go | 14 ++ test/utils/image/manifest.go | 2 +- 8 files changed, 205 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go b/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go index be5c1339ac..9a4a1ddb4f 100644 --- a/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go +++ b/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go @@ -135,6 +135,10 @@ type Webhook struct { // Rules describes what operations on what resources/subresources the webhook cares about. // The webhook cares about an operation if it matches _any_ Rule. + // However, in order to prevent ValidatingAdmissionWebhooks and MutatingAdmissionWebhooks + // from putting the cluster in a state which cannot be recovered from without completely + // disabling the plugin, ValidatingAdmissionWebhooks and MutatingAdmissionWebhooks are never called + // on admission requests for ValidatingWebhookConfiguration and MutatingWebhookConfiguration objects. Rules []RuleWithOperations `json:"rules,omitempty" protobuf:"bytes,3,rep,name=rules"` // FailurePolicy defines how unrecognized errors from the admission endpoint are handled - diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/admission.go index 73e213a6f1..9982c82a3b 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/admission.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/admission.go @@ -186,6 +186,10 @@ func (a *MutatingWebhook) loadConfiguration(attr admission.Attributes) *v1beta1. // Admit makes an admission decision based on the request attributes. func (a *MutatingWebhook) Admit(attr admission.Attributes) error { + if rules.IsWebhookConfigurationResource(attr) { + return nil + } + if !a.WaitForReady() { return admission.NewForbidden(attr, fmt.Errorf("not yet ready to handle request")) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/rules/rules.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/rules/rules.go index eb99357569..096ab5021d 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/rules/rules.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/rules/rules.go @@ -93,3 +93,15 @@ func (r *Matcher) resource() bool { } return false } + +// IsWebhookConfigurationResource determines if an admission.Attributes object is describing +// the admission of a ValidatingWebhookConfiguration or a MutatingWebhookConfiguration +func IsWebhookConfigurationResource(attr admission.Attributes) bool { + gvk := attr.GetKind() + if gvk.Group == "admissionregistration.k8s.io" { + if gvk.Kind == "ValidatingWebhookConfiguration" || gvk.Kind == "MutatingWebhookConfiguration" { + return true + } + } + return false +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/admission.go index 8e8c7448fc..fc77e5b955 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/admission.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/admission.go @@ -174,6 +174,10 @@ func (a *ValidatingAdmissionWebhook) loadConfiguration(attr admission.Attributes // Validate makes an admission decision based on the request attributes. func (a *ValidatingAdmissionWebhook) Validate(attr admission.Attributes) error { + if rules.IsWebhookConfigurationResource(attr) { + return nil + } + if !a.WaitForReady() { return admission.NewForbidden(attr, fmt.Errorf("not yet ready to handle request")) } @@ -266,7 +270,7 @@ func (a *ValidatingAdmissionWebhook) Validate(attr admission.Attributes) error { return errs[0] } -// TODO: factor into a common place along with the validating webhook version. +// TODO: factor into a common place along with the mutating webhook version. func (a *ValidatingAdmissionWebhook) shouldCallHook(h *v1beta1.Webhook, attr admission.Attributes) (bool, *apierrors.StatusError) { var matches bool for _, r := range h.Rules { diff --git a/test/e2e/apimachinery/webhook.go b/test/e2e/apimachinery/webhook.go index a139bf8911..ad85111134 100644 --- a/test/e2e/apimachinery/webhook.go +++ b/test/e2e/apimachinery/webhook.go @@ -57,6 +57,9 @@ const ( podMutatingWebhookConfigName = "e2e-test-mutating-webhook-pod" crdMutatingWebhookConfigName = "e2e-test-mutating-webhook-config-crd" webhookFailClosedConfigName = "e2e-test-webhook-fail-closed" + webhookForWebhooksConfigName = "e2e-test-webhook-for-webhooks-config" + removableValidatingHookName = "e2e-test-should-be-removable-validating-webhook-config" + removableMutatingHookName = "e2e-test-should-be-removable-mutating-webhook-config" skipNamespaceLabelKey = "skip-webhook-admission" skipNamespaceLabelValue = "yes" @@ -141,6 +144,12 @@ var _ = SIGDescribe("AdmissionWebhook", func() { testMutatingPodWebhook(f) }) + It("Should not be able to prevent deleting validating-webhook-configurations or mutating-webhook-configurations", func() { + webhookCleanup := registerWebhookForWebhookConfigurations(f, context) + defer webhookCleanup() + testWebhookForWebhookConfigurations(f) + }) + It("Should mutate crd", func() { testcrd, err := framework.CreateTestCRD(f) if err != nil { @@ -375,7 +384,7 @@ func registerWebhook(f *framework.Framework, context *certContext) func() { }) framework.ExpectNoError(err, "registering webhook config %s with namespace %s", configName, namespace) - // The webhook configuration is honored in 1s. + // The webhook configuration is honored in 10s. time.Sleep(10 * time.Second) return func() { @@ -437,7 +446,7 @@ func registerMutatingWebhookForConfigMap(f *framework.Framework, context *certCo }) framework.ExpectNoError(err, "registering mutating webhook config %s with namespace %s", configName, namespace) - // The webhook configuration is honored in 1s. + // The webhook configuration is honored in 10s. time.Sleep(10 * time.Second) return func() { client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(configName, nil) } } @@ -493,7 +502,7 @@ func registerMutatingWebhookForPod(f *framework.Framework, context *certContext) }) framework.ExpectNoError(err, "registering mutating webhook config %s with namespace %s", configName, namespace) - // The webhook configuration is honored in 1s. + // The webhook configuration is honored in 10s. time.Sleep(10 * time.Second) return func() { client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(configName, nil) } @@ -709,6 +718,152 @@ func testFailClosedWebhook(f *framework.Framework) { } } +func registerWebhookForWebhookConfigurations(f *framework.Framework, context *certContext) func() { + var err error + client := f.ClientSet + By("Registering a webhook on ValidatingWebhookConfiguration and MutatingWebhookConfiguration objects, via the AdmissionRegistration API") + + namespace := f.Namespace.Name + configName := webhookForWebhooksConfigName + failurePolicy := v1beta1.Fail + + // This webhook will deny all requests to Delete admissionregistration objects + _, err = client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Create(&v1beta1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: configName, + }, + Webhooks: []v1beta1.Webhook{ + { + Name: "deny-webhook-configuration-deletions.k8s.io", + Rules: []v1beta1.RuleWithOperations{{ + Operations: []v1beta1.OperationType{v1beta1.Delete}, + Rule: v1beta1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + APIVersions: []string{"*"}, + Resources: []string{ + "validatingwebhookconfigurations", + "mutatingwebhookconfigurations", + }, + }, + }}, + ClientConfig: v1beta1.WebhookClientConfig{ + Service: &v1beta1.ServiceReference{ + Namespace: namespace, + Name: serviceName, + Path: strPtr("/always-deny"), + }, + CABundle: context.signingCert, + }, + FailurePolicy: &failurePolicy, + }, + }, + }) + + framework.ExpectNoError(err, "registering webhook config %s with namespace %s", configName, namespace) + + // The webhook configuration is honored in 10s. + time.Sleep(10 * time.Second) + return func() { + err := client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Delete(configName, nil) + framework.ExpectNoError(err, "deleting webhook config %s with namespace %s", configName, namespace) + } +} + +// This test assumes that the deletion-rejecting webhook defined in +// registerWebhookForWebhookConfigurations is in place. +func testWebhookForWebhookConfigurations(f *framework.Framework) { + var err error + client := f.ClientSet + By("Creating a validating-webhook-configuration object") + + namespace := f.Namespace.Name + failurePolicy := v1beta1.Ignore + + _, err = client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Create(&v1beta1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: removableValidatingHookName, + }, + Webhooks: []v1beta1.Webhook{ + { + Name: "should-be-removable-validating-webhook.k8s.io", + Rules: []v1beta1.RuleWithOperations{{ + Operations: []v1beta1.OperationType{v1beta1.Create}, + Rule: v1beta1.Rule{ + APIGroups: []string{"*"}, + APIVersions: []string{"*"}, + Resources: []string{"*"}, + }, + }}, + ClientConfig: v1beta1.WebhookClientConfig{ + Service: &v1beta1.ServiceReference{ + Namespace: namespace, + Name: serviceName, + // This path not recognized by the webhook service, + // so the call to this webhook will always fail, + // but because the failure policy is ignore, it will + // have no effect on admission requests. + Path: strPtr(""), + }, + CABundle: nil, + }, + FailurePolicy: &failurePolicy, + }, + }, + }) + framework.ExpectNoError(err, "registering webhook config %s with namespace %s", removableValidatingHookName, namespace) + + // The webhook configuration is honored in 10s. + time.Sleep(10 * time.Second) + + By("Deleting the validating-webhook-configuration, which should be possible to remove") + + err = client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Delete(removableValidatingHookName, nil) + framework.ExpectNoError(err, "deleting webhook config %s with namespace %s", removableValidatingHookName, namespace) + + By("Creating a mutating-webhook-configuration object") + + _, err = client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Create(&v1beta1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: removableMutatingHookName, + }, + Webhooks: []v1beta1.Webhook{ + { + Name: "should-be-removable-mutating-webhook.k8s.io", + Rules: []v1beta1.RuleWithOperations{{ + Operations: []v1beta1.OperationType{v1beta1.Create}, + Rule: v1beta1.Rule{ + APIGroups: []string{"*"}, + APIVersions: []string{"*"}, + Resources: []string{"*"}, + }, + }}, + ClientConfig: v1beta1.WebhookClientConfig{ + Service: &v1beta1.ServiceReference{ + Namespace: namespace, + Name: serviceName, + // This path not recognized by the webhook service, + // so the call to this webhook will always fail, + // but because the failure policy is ignore, it will + // have no effect on admission requests. + Path: strPtr(""), + }, + CABundle: nil, + }, + FailurePolicy: &failurePolicy, + }, + }, + }) + framework.ExpectNoError(err, "registering webhook config %s with namespace %s", removableMutatingHookName, namespace) + + // The webhook configuration is honored in 10s. + time.Sleep(10 * time.Second) + + By("Deleting the mutating-webhook-configuration, which should be possible to remove") + + err = client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(removableMutatingHookName, nil) + framework.ExpectNoError(err, "deleting webhook config %s with namespace %s", removableMutatingHookName, namespace) +} + func createNamespace(f *framework.Framework, ns *v1.Namespace) error { return wait.PollImmediate(100*time.Millisecond, 30*time.Second, func() (bool, error) { _, err := f.ClientSet.CoreV1().Namespaces().Create(ns) @@ -849,7 +1004,7 @@ func registerWebhookForCRD(f *framework.Framework, context *certContext, testcrd }) framework.ExpectNoError(err, "registering crd webhook config %s with namespace %s", configName, namespace) - // The webhook configuration is honored in 1s. + // The webhook configuration is honored in 10s. time.Sleep(10 * time.Second) return func() { client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Delete(configName, nil) @@ -909,7 +1064,7 @@ func registerMutatingWebhookForCRD(f *framework.Framework, context *certContext, }) framework.ExpectNoError(err, "registering crd webhook config %s with namespace %s", configName, namespace) - // The webhook configuration is honored in 1s. + // The webhook configuration is honored in 10s. time.Sleep(10 * time.Second) return func() { client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(configName, nil) } diff --git a/test/images/webhook/Makefile b/test/images/webhook/Makefile index b2582f416e..868e5b60fa 100644 --- a/test/images/webhook/Makefile +++ b/test/images/webhook/Makefile @@ -12,9 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +IMAGE = gcr.io/kubernetes-e2e-test-images/k8s-sample-admission-webhook-amd64 +TAG = 1.9v2 + build: CGO_ENABLED=0 GOOS=linux go build -a -installsuffix cgo -o webhook . - docker build --no-cache -t gcr.io/kubernetes-e2e-test-images/k8s-sample-admission-webhook-amd64:1.9v1 . + docker build --no-cache -t $(IMAGE):$(TAG) . rm -rf webhook push: - docker push gcr.io/kubernetes-e2e-test-images/k8s-sample-admission-webhook-amd64:1.9v1 + docker push $(IMAGE):$(TAG) diff --git a/test/images/webhook/main.go b/test/images/webhook/main.go index bdf68ba9c7..4607fe279b 100644 --- a/test/images/webhook/main.go +++ b/test/images/webhook/main.go @@ -67,6 +67,15 @@ func toAdmissionResponse(err error) *v1beta1.AdmissionResponse { } } +// Deny all requests made to this function. +func alwaysDeny(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { + glog.V(2).Info("calling always-deny") + reviewResponse := v1beta1.AdmissionResponse{} + reviewResponse.Allowed = false + reviewResponse.Result = &metav1.Status{Message: "this webhook denies all requests"} + return &reviewResponse +} + // only allow pods to pull images from specific registry. func admitPods(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { glog.V(2).Info("admitting pods") @@ -295,6 +304,10 @@ func serve(w http.ResponseWriter, r *http.Request, admit admitFunc) { } } +func serveAlwaysDeny(w http.ResponseWriter, r *http.Request) { + serve(w, r, alwaysDeny) +} + func servePods(w http.ResponseWriter, r *http.Request) { serve(w, r, admitPods) } @@ -324,6 +337,7 @@ func main() { config.addFlags() flag.Parse() + http.HandleFunc("/always-deny", serveAlwaysDeny) http.HandleFunc("/pods", servePods) http.HandleFunc("/mutating-pods", serveMutatePods) http.HandleFunc("/configmaps", serveConfigmaps) diff --git a/test/utils/image/manifest.go b/test/utils/image/manifest.go index 85b0e5fd5d..1157d975ac 100644 --- a/test/utils/image/manifest.go +++ b/test/utils/image/manifest.go @@ -48,7 +48,7 @@ func (i *ImageConfig) SetVersion(version string) { } var ( - AdmissionWebhook = ImageConfig{e2eRegistry, "k8s-sample-admission-webhook", "1.9v1", true} + AdmissionWebhook = ImageConfig{e2eRegistry, "k8s-sample-admission-webhook", "1.9v2", true} APIServer = ImageConfig{e2eRegistry, "k8s-aggregator-sample-apiserver", "1.7v2", true} AppArmorLoader = ImageConfig{gcRegistry, "apparmor-loader", "0.1", false} BusyBox = ImageConfig{gcRegistry, "busybox", "1.24", false}