Replace webhook implementation with WebhookManagedBy. (#27)

Author: ryotarai

internal/anchor/validator.go

@@ -8,8 +8,11 @@ import (
 
 	"github.com/go-logr/logr"
 	k8sadm "k8s.io/api/admission/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
@@ -30,45 +33,75 @@ const (
 // +kubebuilder:webhook:admissionReviewVersions=v1,path=/validate-hnc-x-k8s-io-v1alpha2-subnamespaceanchors,mutating=false,failurePolicy=fail,groups="hnc.x-k8s.io",resources=subnamespaceanchors,sideEffects=None,verbs=create;update;delete,versions=v1alpha2,name=subnamespaceanchors.hnc.x-k8s.io
 
 type Validator struct {
-	Log     logr.Logger
-	Forest  *forest.Forest
-	decoder admission.Decoder
+	Log    logr.Logger
+	Forest *forest.Forest
 }
 
-// req defines the aspects of the admission.Request that we care about.
-type anchorRequest struct {
-	anchor *api.SubnamespaceAnchor
-	op     k8sadm.Operation
+func NewValidator(forest *forest.Forest) *Validator {
+	return &Validator{
+		Log:    ctrl.Log.WithName("anchor").WithName("validate"),
+		Forest: forest,
+	}
 }
 
-// Handle implements the validation webhook.
-func (v *Validator) Handle(ctx context.Context, req admission.Request) admission.Response {
-	log := v.Log.WithValues("ns", req.Namespace, "nm", req.Name, "op", req.Operation, "user", req.UserInfo.Username)
-	// Early exit since the HNC SA can do whatever it wants.
-	if webhooks.IsHNCServiceAccount(&req.AdmissionRequest.UserInfo) {
+func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	return v.validateAnchor(ctx, obj, k8sadm.Create)
+}
+
+func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+	return v.validateAnchor(ctx, newObj, k8sadm.Update)
+}
+
+func (v *Validator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	return v.validateAnchor(ctx, obj, k8sadm.Delete)
+}
+
+func (v *Validator) validateAnchor(ctx context.Context, obj runtime.Object, operation k8sadm.Operation) (admission.Warnings, error) {
+	req, err := admission.RequestFromContext(ctx)
+	if err != nil {
+		return nil, apierrors.NewInternalError(err)
+	}
+
+	log := v.Log.WithValues("ns", req.Namespace, "nm", req.Name, "op", operation, "user", req.UserInfo.Username)
+
+	if webhooks.IsHNCServiceAccount(&req.UserInfo) {
 		log.V(1).Info("Allowed change by HNC SA")
-		return webhooks.Allow("HNC SA")
+		return nil, nil
 	}
 
-	decoded, err := v.decodeRequest(log, req)
-	if err != nil {
-		log.Error(err, "Couldn't decode request")
-		return webhooks.DenyBadRequest(err)
+	anchor, ok := obj.(*api.SubnamespaceAnchor)
+	if !ok {
+		return nil, apierrors.NewInternalError(fmt.Errorf("expected a SubnamespaceAnchor but got a %T", obj))
+	}
+
+	anchorReq := &anchorRequest{
+		anchor: anchor,
+		op:     operation,
 	}
 
-	resp := v.handle(decoded)
-	if !resp.Allowed {
-		log.Info("Denied", "code", resp.Result.Code, "reason", resp.Result.Reason, "message", resp.Result.Message)
-	} else {
-		log.V(1).Info("Allowed", "message", resp.Result.Message)
+	if err := v.handleValidation(anchorReq); err != nil {
+		if !apierrors.IsInvalid(err) && !apierrors.IsConflict(err) && !apierrors.IsForbidden(err) {
+			log.Error(err, "Validation failed")
+		} else {
+			log.Info("Denied", "reason", err)
+		}
+		return nil, err
 	}
-	return resp
+
+	log.V(1).Info("Allowed")
+	return nil, nil
+}
+
+// req defines the aspects of the admission.Request that we care about.
+type anchorRequest struct {
+	anchor *api.SubnamespaceAnchor
+	op     k8sadm.Operation
 }
 
-// handle implements the non-boilerplate logic of this validator, allowing it to be more easily unit
+// handleValidation implements the non-boilerplate logic of this validator, allowing it to be more easily unit
 // tested (ie without constructing a full admission.Request). It validates that the request is allowed
 // based on the current in-memory state of the forest.
-func (v *Validator) handle(req *anchorRequest) admission.Response {
+func (v *Validator) handleValidation(req *anchorRequest) error {
 	v.Forest.Lock()
 	defer v.Forest.Unlock()
 	pnm := req.anchor.Namespace
@@ -83,28 +116,28 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 			allErrs := field.ErrorList{
 				field.Invalid(fldPath, cnm, msg),
 			}
-			return webhooks.DenyInvalid(api.SubnamespaceAnchorGK, cnm, allErrs)
+			return apierrors.NewInvalid(api.SubnamespaceAnchorGK, cnm, allErrs)
 		}
 	}
 
 	labelErrs := config.ValidateManagedLabels(req.anchor.Spec.Labels)
 	annotationErrs := config.ValidateManagedAnnotations(req.anchor.Spec.Annotations)
 	allErrs := append(labelErrs, annotationErrs...)
 	if len(allErrs) > 0 {
-		return webhooks.DenyInvalid(api.SubnamespaceAnchorGK, req.anchor.Name, allErrs)
+		return apierrors.NewInvalid(api.SubnamespaceAnchorGK, req.anchor.Name, allErrs)
 	}
 
 	switch req.op {
 	case k8sadm.Create:
 		// Can't create subnamespaces in unmanaged namespaces
 		if why := config.WhyUnmanaged(pnm); why != "" {
 			err := fmt.Errorf("cannot create a subnamespace in the unmanaged namespace %q (%s)", pnm, why)
-			return webhooks.DenyForbidden(api.SubnamespaceAnchorGR, pnm, err)
+			return apierrors.NewForbidden(api.SubnamespaceAnchorGR, pnm, err)
 		}
 		// Can't create subnamespaces using unmanaged namespace names
 		if why := config.WhyUnmanaged(cnm); why != "" {
 			err := fmt.Errorf("cannot create a subnamespace using the unmanaged namespace name %q (%s)", cnm, why)
-			return webhooks.DenyForbidden(api.SubnamespaceAnchorGR, cnm, err)
+			return apierrors.NewForbidden(api.SubnamespaceAnchorGR, cnm, err)
 		}
 
 		// Can't create anchors for existing namespaces, _unless_ it's for a subns with a missing
@@ -113,7 +146,7 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 			childIsMissingAnchor := (cns.Parent().Name() == pnm && cns.IsSub)
 			if !childIsMissingAnchor {
 				err := errors.New("cannot create a subnamespace using an existing namespace")
-				return webhooks.DenyConflict(api.SubnamespaceAnchorGR, cnm, err)
+				return apierrors.NewConflict(api.SubnamespaceAnchorGR, cnm, err)
 			}
 		}
 
@@ -122,40 +155,12 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 		// unless allowCascadingDeletion is set.
 		if req.anchor.Status.State == api.Ok && cns.ChildNames() != nil && !cns.AllowsCascadingDeletion() {
 			err := fmt.Errorf("subnamespace %s is not a leaf and doesn't allow cascading deletion. Please set allowCascadingDeletion flag or make it a leaf first", cnm)
-			return webhooks.DenyForbidden(api.SubnamespaceAnchorGR, cnm, err)
+			return apierrors.NewForbidden(api.SubnamespaceAnchorGR, cnm, err)
 		}
 
 	default:
 		// nop for updates etc
 	}
 
-	return webhooks.Allow("")
-}
-
-// decodeRequest gets the information we care about into a simple struct that's easy to both a) use
-// and b) factor out in unit tests.
-func (v *Validator) decodeRequest(log logr.Logger, in admission.Request) (*anchorRequest, error) {
-	anchor := &api.SubnamespaceAnchor{}
-	var err error
-	// For DELETE request, use DecodeRaw() from req.OldObject, since Decode() only
-	// uses req.Object, which will be empty for a DELETE request.
-	if in.Operation == k8sadm.Delete {
-		log.V(1).Info("Decoding a delete request.")
-		err = v.decoder.DecodeRaw(in.OldObject, anchor)
-	} else {
-		err = v.decoder.Decode(in, anchor)
-	}
-	if err != nil {
-		return nil, err
-	}
-
-	return &anchorRequest{
-		anchor: anchor,
-		op:     in.Operation,
-	}, nil
-}
-
-func (v *Validator) InjectDecoder(d admission.Decoder) error {
-	v.decoder = d
 	return nil
 }

internal/anchor/validator_test.go

@@ -5,7 +5,6 @@ import (
 
 	. "github.com/onsi/gomega"
 	k8sadm "k8s.io/api/admission/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
 	"sigs.k8s.io/hierarchical-namespaces/internal/config"
@@ -47,11 +46,14 @@ func TestCreateSubnamespaces(t *testing.T) {
 			}
 
 			// Test
-			got := v.handle(req)
+			err := v.handleValidation(req)
 
 			// Report
-			logResult(t, got.AdmissionResponse.Result)
-			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
+			if tc.fail {
+				g.Expect(err).Should(HaveOccurred())
+			} else {
+				g.Expect(err).ShouldNot(HaveOccurred())
+			}
 		})
 	}
 }
@@ -82,11 +84,14 @@ func TestDeleteSubnamespaces(t *testing.T) {
 			}
 
 			// Test
-			got := v.handle(req)
+			err := v.handleValidation(req)
 
 			// Report
-			logResult(t, got.AdmissionResponse.Result)
-			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
+			if tc.fail {
+				g.Expect(err).Should(HaveOccurred())
+			} else {
+				g.Expect(err).ShouldNot(HaveOccurred())
+			}
 		})
 	}
 }
@@ -152,11 +157,14 @@ func TestManagedMeta(t *testing.T) {
 			}
 
 			// Test
-			got := v.handle(req)
+			err := v.handleValidation(req)
 
 			// Report
-			logResult(t, got.AdmissionResponse.Result)
-			g.Expect(got.AdmissionResponse.Allowed).Should(Equal(tc.allowed))
+			if tc.allowed {
+				g.Expect(err).ShouldNot(HaveOccurred())
+			} else {
+				g.Expect(err).Should(HaveOccurred())
+			}
 		})
 	}
 }
@@ -207,15 +215,14 @@ func TestAllowCascadingDeleteSubnamespaces(t *testing.T) {
 			}
 
 			// Test
-			got := v.handle(req)
+			err := v.handleValidation(req)
 
 			// Report
-			logResult(t, got.AdmissionResponse.Result)
-			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
+			if tc.fail {
+				g.Expect(err).Should(HaveOccurred())
+			} else {
+				g.Expect(err).ShouldNot(HaveOccurred())
+			}
 		})
 	}
 }
-
-func logResult(t *testing.T, result *metav1.Status) {
-	t.Logf("Got reason %q, code %d, msg %q", result.Reason, result.Code, result.Message)
-}