🐛 fix nil Decoder in multiValidating and multiMutating handlers by im…

…plementing DecoderInjector (#1502)

* multiValidating and multiMutating handlers implement DecoderInjector

* set fields before injecting in controllerManager

pkg/manager/internal.go

@@ -225,6 +225,9 @@ func (cm *controllerManager) Add(r Runnable) error {
 
 // Deprecated: use the equivalent Options field to set a field. This method will be removed in v0.10.
 func (cm *controllerManager) SetFields(i interface{}) error {
+	if err := cm.cluster.SetFields(i); err != nil {
+		return err
+	}
 	if _, err := inject.InjectorInto(cm.SetFields, i); err != nil {
 		return err
 	}
@@ -234,9 +237,6 @@ func (cm *controllerManager) SetFields(i interface{}) error {
 	if _, err := inject.LoggerInto(cm.logger, i); err != nil {
 		return err
 	}
-	if err := cm.cluster.SetFields(i); err != nil {
-		return err
-	}
 
 	return nil
 }

pkg/webhook/admission/multi.go

@@ -77,6 +77,16 @@ func (hs multiMutating) InjectFunc(f inject.Func) error {
 	return nil
 }
 
+// InjectDecoder injects the decoder into the handlers.
+func (hs multiMutating) InjectDecoder(d *Decoder) error {
+	for _, handler := range hs {
+		if _, err := InjectDecoderInto(d, handler); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // MultiMutatingHandler combines multiple mutating webhook handlers into a single
 // mutating webhook handler.  Handlers are called in sequential order, and the first
 // `allowed: false`	response may short-circuit the rest.  Users must take care to
@@ -125,3 +135,13 @@ func (hs multiValidating) InjectFunc(f inject.Func) error {
 
 	return nil
 }
+
+// InjectDecoder injects the decoder into the handlers.
+func (hs multiValidating) InjectDecoder(d *Decoder) error {
+	for _, handler := range hs {
+		if _, err := InjectDecoderInto(d, handler); err != nil {
+			return err
+		}
+	}
+	return nil
+}

pkg/webhook/webhook_integration_test.go

@@ -76,7 +76,32 @@ var _ = Describe("Webhook", func() {
 
 			ctx, cancel := context.WithCancel(context.Background())
 			go func() {
-				_ = server.Start(ctx)
+				err = server.Start(ctx)
+				Expect(err).NotTo(HaveOccurred())
+			}()
+
+			Eventually(func() bool {
+				err = c.Create(context.TODO(), obj)
+				return errors.ReasonForError(err) == metav1.StatusReason("Always denied")
+			}, 1*time.Second).Should(BeTrue())
+
+			cancel()
+			close(done)
+		})
+		It("should reject create request for multi-webhook that rejects all requests", func(done Done) {
+			m, err := manager.New(cfg, manager.Options{
+				Port:    testenv.WebhookInstallOptions.LocalServingPort,
+				Host:    testenv.WebhookInstallOptions.LocalServingHost,
+				CertDir: testenv.WebhookInstallOptions.LocalServingCertDir,
+			}) // we need manager here just to leverage manager.SetFields
+			Expect(err).NotTo(HaveOccurred())
+			server := m.GetWebhookServer()
+			server.Register("/failing", &webhook.Admission{Handler: admission.MultiValidatingHandler(&rejectingValidator{})})
+
+			ctx, cancel := context.WithCancel(context.Background())
+			go func() {
+				err = server.Start(ctx)
+				Expect(err).NotTo(HaveOccurred())
 			}()
 
 			Eventually(func() bool {
@@ -99,7 +124,8 @@ var _ = Describe("Webhook", func() {
 
 			ctx, cancel := context.WithCancel(context.Background())
 			go func() {
-				_ = server.StartStandalone(ctx, scheme.Scheme)
+				err := server.StartStandalone(ctx, scheme.Scheme)
+				Expect(err).NotTo(HaveOccurred())
 			}()
 
 			Eventually(func() bool {
@@ -170,8 +196,18 @@ var _ = Describe("Webhook", func() {
 })
 
 type rejectingValidator struct {
+	d *admission.Decoder
+}
+
+func (v *rejectingValidator) InjectDecoder(d *admission.Decoder) error {
+	v.d = d
+	return nil
 }
 
 func (v *rejectingValidator) Handle(ctx context.Context, req admission.Request) admission.Response {
+	var obj appsv1.Deployment
+	if err := v.d.Decode(req, &obj); err != nil {
+		return admission.Denied(err.Error())
+	}
 	return admission.Denied(fmt.Sprint("Always denied"))
 }