Prevent webhooks from affecting admission requests for webhooks

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 -

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"))
 	}

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
+}

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 {

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) }

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)

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)

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}