diff --git a/internal/anchor/validator.go b/internal/anchor/validator.go index 46e98879e..564e9900a 100644 --- a/internal/anchor/validator.go +++ b/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,7 +116,7 @@ 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) } } @@ -91,7 +124,7 @@ func (v *Validator) handle(req *anchorRequest) admission.Response { 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 { @@ -99,12 +132,12 @@ func (v *Validator) handle(req *anchorRequest) admission.Response { // 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 } diff --git a/internal/anchor/validator_test.go b/internal/anchor/validator_test.go index 09a29e917..7fd4e8705 100644 --- a/internal/anchor/validator_test.go +++ b/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) -} diff --git a/internal/hierarchyconfig/validator.go b/internal/hierarchyconfig/validator.go index f1dea00dc..26f2e2835 100644 --- a/internal/hierarchyconfig/validator.go +++ b/internal/hierarchyconfig/validator.go @@ -7,15 +7,15 @@ import ( "strings" "github.com/go-logr/logr" - k8sadm "k8s.io/api/admission/v1" authnv1 "k8s.io/api/authentication/v1" authzv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" @@ -38,10 +38,15 @@ const ( // +kubebuilder:webhook:admissionReviewVersions=v1,path=/validate-hnc-x-k8s-io-v1alpha2-hierarchyconfigurations,mutating=false,failurePolicy=fail,groups="hnc.x-k8s.io",resources=hierarchyconfigurations,sideEffects=None,verbs=create;update,versions=v1alpha2,name=hierarchyconfigurations.hnc.x-k8s.io type Validator struct { - Log logr.Logger - Forest *forest.Forest - server serverClient - decoder admission.Decoder + Forest *forest.Forest + server serverClient +} + +func NewValidator(forest *forest.Forest, client client.Client) *Validator { + return &Validator{ + Forest: forest, + server: &realClient{client: client}, + } } // serverClient represents the checks that should typically be performed against the apiserver, but @@ -55,13 +60,19 @@ type serverClient interface { IsAdmin(ctx context.Context, ui *authnv1.UserInfo, nnm string) (bool, error) } -// request defines the aspects of the admission.Request that we care about. -type request struct { - hc *api.HierarchyConfiguration - ui *authnv1.UserInfo +func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + return v.handle(ctx, obj) +} + +func (v *Validator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + return v.handle(ctx, obj) } -// Handle implements the validation webhook. +func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + return v.handle(ctx, newObj) +} + +// handle implements the validation webhook. // // During updates, the validator currently ignores the existing state of the object (`oldObject`). // The reason is that most of the checks being performed are on the state of the entire forest, not @@ -84,25 +95,6 @@ type request struct { // possible to introduce structural failures, as described in the user docs. // // Authz false positives are prevented as described by the comments to `getServerChecks`. -func (v *Validator) Handle(ctx context.Context, req admission.Request) admission.Response { - log := v.Log.WithValues("ns", req.Namespace, "op", req.Operation, "user", req.UserInfo.Username) - decoded, err := v.decodeRequest(req) - if err != nil { - log.Error(err, "Couldn't decode request") - return webhooks.DenyBadRequest(err) - } - - resp := v.handle(ctx, log, 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) - } - return resp -} - -// handle implements the non-boilerplate logic of this validator, allowing it to be more easily unit -// tested (ie without constructing a full admission.Request). // // This follows the standard HNC pattern of: // - Load a bunch of stuff from the apiserver @@ -111,47 +103,57 @@ func (v *Validator) Handle(ctx context.Context, req admission.Request) admission // // This minimizes the amount of time that the forest is locked, allowing different threads to // proceed in parallel. -func (v *Validator) handle(ctx context.Context, log logr.Logger, req *request) admission.Response { +func (v *Validator) handle(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + req, err := admission.RequestFromContext(ctx) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + + log := admission.DefaultLogConstructor(logf.FromContext(ctx), &req) + // Early exit: the HNC SA can do whatever it wants. This is because if an illegal HC already // exists on the K8s server, we need to be able to update its status even though the rest of the // object wouldn't pass legality. We should probably only give the HNC SA the ability to modify // the _status_, though. TODO: https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/80. - if webhooks.IsHNCServiceAccount(req.ui) { - return allow("HNC SA") + if webhooks.IsHNCServiceAccount(&req.UserInfo) { + return nil, nil } - if why := config.WhyUnmanaged(req.hc.Namespace); why != "" { - err := fmt.Errorf("namespace %q is not managed by HNC (%s) and cannot be set as a child of another namespace", req.hc.Namespace, why) - return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err) + hc, ok := obj.(*api.HierarchyConfiguration) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("expected a HierarchyConfiguration but got a %T", obj)) } - if why := config.WhyUnmanaged(req.hc.Spec.Parent); why != "" { - err := fmt.Errorf("namespace %q is not managed by HNC (%s) and cannot be set as the parent of another namespace", req.hc.Spec.Parent, why) - return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err) + + if why := config.WhyUnmanaged(hc.Namespace); why != "" { + return nil, apierrors.NewForbidden(api.HierarchyConfigurationGR, api.Singleton, fmt.Errorf("namespace %q is not managed by HNC (%s) and cannot be set as a child of another namespace", hc.Namespace, why)) + } + if why := config.WhyUnmanaged(hc.Spec.Parent); why != "" { + return nil, apierrors.NewForbidden(api.HierarchyConfigurationGR, api.Singleton, fmt.Errorf("namespace %q is not managed by HNC (%s) and cannot be set as the parent of another namespace", hc.Spec.Parent, why)) } - labelErrs := config.ValidateManagedLabels(req.hc.Spec.Labels) - annotationErrs := config.ValidateManagedAnnotations(req.hc.Spec.Annotations) + labelErrs := config.ValidateManagedLabels(hc.Spec.Labels) + annotationErrs := config.ValidateManagedAnnotations(hc.Spec.Annotations) allErrs := append(labelErrs, annotationErrs...) if len(allErrs) > 0 { - return webhooks.DenyInvalid(api.HierarchyConfigurationGK, req.hc.Name, allErrs) + return nil, apierrors.NewInvalid(api.HierarchyConfigurationGK, hc.Name, allErrs) } // Do all checks that require holding the in-memory lock. Generate a list of server checks we // should perform once the lock is released. - serverChecks, resp := v.checkForest(req.hc) - if !resp.Allowed { - return resp + serverChecks, err := v.checkForest(hc) + if err != nil { + return nil, err } // Ensure that the server's in the right state to make the changes. - return v.checkServer(ctx, log, req.ui, serverChecks) + return nil, v.checkServer(ctx, log, &req.UserInfo, serverChecks) } // checkForest validates that the request is allowed based on the current in-memory state of the // forest. If it is, it returns a list of checks we need to perform against the apiserver in order // to be allowed to make the change; these checks are executed _after_ the in-memory lock is // released. -func (v *Validator) checkForest(hc *api.HierarchyConfiguration) ([]serverCheck, admission.Response) { +func (v *Validator) checkForest(hc *api.HierarchyConfiguration) ([]serverCheck, error) { v.Forest.Lock() defer v.Forest.Unlock() @@ -161,59 +163,54 @@ func (v *Validator) checkForest(hc *api.HierarchyConfiguration) ([]serverCheck, newParent := v.Forest.Get(hc.Spec.Parent) // Check problems on the namespace itself - if resp := v.checkNS(ns); !resp.Allowed { - return nil, resp + if err := v.checkNS(ns); err != nil { + return nil, err } // Check problems on the parents - if resp := v.checkParent(ns, curParent, newParent); !resp.Allowed { - return nil, resp + if err := v.checkParent(ns, curParent, newParent); err != nil { + return nil, err } // The structure looks good. Get the list of namespaces we need server checks on. - return v.getServerChecks(curParent, newParent), allow("") + return v.getServerChecks(curParent, newParent), nil } // checkNS looks for problems with the current namespace that should prevent changes. -func (v *Validator) checkNS(ns *forest.Namespace) admission.Response { +func (v *Validator) checkNS(ns *forest.Namespace) error { // Wait until the namespace has been synced if !ns.Exists() { - msg := fmt.Sprintf("HNC has not reconciled namespace %q yet - please try again in a few moments.", ns.Name()) - return webhooks.DenyServiceUnavailable(msg) + return apierrors.NewServiceUnavailable(fmt.Sprintf("HNC has not reconciled namespace %q yet - please try again in a few moments.", ns.Name())) } // Deny the request if the namespace has a halted root - but not if it's halted itself, since we // may be trying to resolve the halted condition. haltedRoot := ns.GetHaltedRoot() if haltedRoot != "" && haltedRoot != ns.Name() { - err := fmt.Errorf("ancestor %q of namespace %q has a critical condition, which must be resolved before any changes can be made to the hierarchy configuration", haltedRoot, ns.Name()) - return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err) + return apierrors.NewForbidden(api.HierarchyConfigurationGR, api.Singleton, fmt.Errorf("ancestor %q of namespace %q has a critical condition, which must be resolved before any changes can be made to the hierarchy configuration", haltedRoot, ns.Name())) } - return allow("") + return nil } // checkParent validates if the parent is legal based on the current in-memory state of the forest. -func (v *Validator) checkParent(ns, curParent, newParent *forest.Namespace) admission.Response { +func (v *Validator) checkParent(ns, curParent, newParent *forest.Namespace) error { if ns.IsExternal() && newParent != nil { - err := fmt.Errorf("namespace %q is managed by %q, not HNC, so it cannot have a parent in HNC", ns.Name(), ns.Manager) - return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err) + return apierrors.NewForbidden(api.HierarchyConfigurationGR, api.Singleton, fmt.Errorf("namespace %q is managed by %q, not HNC, so it cannot have a parent in HNC", ns.Name(), ns.Manager)) } if curParent == newParent { - return allow("parent unchanged") + return nil } // Prevent changing parent of a subnamespace if ns.IsSub { - err := fmt.Errorf("illegal parent: Cannot set the parent of %q to %q because it's a subnamespace of %q", ns.Name(), newParent.Name(), curParent.Name()) - return webhooks.DenyConflict(api.HierarchyConfigurationGR, api.Singleton, err) + return apierrors.NewForbidden(api.HierarchyConfigurationGR, api.Singleton, fmt.Errorf("illegal parent: Cannot set the parent of %q to %q because it's a subnamespace of %q", ns.Name(), newParent.Name(), curParent.Name())) } // non existence of parent namespace -> not allowed if newParent != nil && !newParent.Exists() { - err := fmt.Errorf("requested parent %q does not exist", newParent.Name()) - return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err) + return apierrors.NewForbidden(api.HierarchyConfigurationGR, api.Singleton, fmt.Errorf("requested parent %q does not exist", newParent.Name())) } // Is this change structurally legal? Note that this can "leak" information about the hierarchy @@ -222,8 +219,7 @@ func (v *Validator) checkParent(ns, curParent, newParent *forest.Namespace) admi // have visibility into its ancestry and descendents, and this check can only fail if the new // parent conflicts with something in the _existing_ hierarchy. if reason := ns.CanSetParent(newParent); reason != "" { - err := fmt.Errorf("illegal parent: %s", reason) - return webhooks.DenyConflict(api.HierarchyConfigurationGR, api.Singleton, err) + return apierrors.NewConflict(api.HierarchyConfigurationGR, api.Singleton, fmt.Errorf("illegal parent: %s", reason)) } // Prevent overwriting source objects in the descendants after the hierarchy change. @@ -231,11 +227,10 @@ func (v *Validator) checkParent(ns, curParent, newParent *forest.Namespace) admi msg := "Cannot update hierarchy because it would overwrite the following object(s):\n" msg += " * " + strings.Join(co, "\n * ") + "\n" msg += "To fix this, please rename or remove the conflicting objects first." - err := errors.New(msg) - return webhooks.DenyConflict(api.HierarchyConfigurationGR, api.Singleton, err) + return apierrors.NewConflict(api.HierarchyConfigurationGR, api.Singleton, errors.New(msg)) } - return allow("") + return nil } // getConflictingObjects returns a list of namespaced objects if there's any conflict. @@ -393,9 +388,9 @@ func (v *Validator) getServerChecks(curParent, newParent *forest.Namespace) []se } // checkServer executes the list of requested checks. -func (v *Validator) checkServer(ctx context.Context, log logr.Logger, ui *authnv1.UserInfo, reqs []serverCheck) admission.Response { +func (v *Validator) checkServer(ctx context.Context, log logr.Logger, ui *authnv1.UserInfo, reqs []serverCheck) error { if v.server == nil { - return allow("") // unit test; TODO put in fake + return nil // unit test; TODO put in fake } // TODO: parallelize? @@ -405,54 +400,26 @@ func (v *Validator) checkServer(ctx context.Context, log logr.Logger, ui *authnv log.Info("Checking existence", "object", req.nnm, "reason", req.reason) exists, err := v.server.Exists(ctx, req.nnm) if err != nil { - err = fmt.Errorf("while checking existance for %q, the %s: %w", req.nnm, req.reason, err) - return webhooks.DenyInternalError(err) + return apierrors.NewInternalError(fmt.Errorf("while checking existance for %q, the %s: %w", req.nnm, req.reason, err)) } if exists { - msg := fmt.Sprintf("HNC has not reconciled namespace %q yet - please try again in a few moments.", req.nnm) - return webhooks.DenyServiceUnavailable(msg) + return apierrors.NewServiceUnavailable(fmt.Sprintf("HNC has not reconciled namespace %q yet - please try again in a few moments.", req.nnm)) } case checkAuthz: log.Info("Checking authz", "object", req.nnm, "reason", req.reason) allowed, err := v.server.IsAdmin(ctx, ui, req.nnm) if err != nil { - err = fmt.Errorf("while checking authz for %q, the %s: %w", req.nnm, req.reason, err) - return webhooks.DenyInternalError(err) + return apierrors.NewInternalError(fmt.Errorf("while checking authz for %q, the %s: %w", req.nnm, req.reason, err)) } if !allowed { - return webhooks.DenyUnauthorized(fmt.Sprintf("User %s is not authorized to modify the subtree of %s, which is the %s", - ui.Username, req.nnm, req.reason)) + return apierrors.NewUnauthorized(fmt.Sprintf("user %s is not authorized to modify the subtree of %s, which is the %s", ui.Username, req.nnm, req.reason)) } } } - return 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(in admission.Request) (*request, error) { - hc := &api.HierarchyConfiguration{} - if err := v.decoder.Decode(in, hc); err != nil { - return nil, err - } - - return &request{ - hc: hc, - ui: &in.UserInfo, - }, nil -} - -func (v *Validator) InjectClient(c client.Client) error { - v.server = &realClient{client: c} - return nil -} - -func (v *Validator) InjectDecoder(d admission.Decoder) error { - v.decoder = d return nil } @@ -505,15 +472,3 @@ func (r *realClient) IsAdmin(ctx context.Context, ui *authnv1.UserInfo, nnm stri // Extract the interesting result return sar.Status.Allowed, err } - -// allow is a replacement for controller-runtime's admission.Allowed() that allows you to set the -// message (human-readable) as opposed to the reason (machine-readable). -func allow(msg string) admission.Response { - return admission.Response{AdmissionResponse: k8sadm.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{ - Code: 0, - Message: msg, - }, - }} -} diff --git a/internal/hierarchyconfig/validator_test.go b/internal/hierarchyconfig/validator_test.go index ab7116104..2252f4c31 100644 --- a/internal/hierarchyconfig/validator_test.go +++ b/internal/hierarchyconfig/validator_test.go @@ -2,25 +2,32 @@ package hierarchyconfig import ( "context" + "errors" "testing" . "github.com/onsi/gomega" + admissionv1 "k8s.io/api/admission/v1" authn "k8s.io/api/authentication/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" - "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" "sigs.k8s.io/hierarchical-namespaces/internal/config" "sigs.k8s.io/hierarchical-namespaces/internal/foresttest" "sigs.k8s.io/hierarchical-namespaces/internal/objects" ) +func newContext(ctx context.Context) context.Context { + return admission.NewContextWithRequest(ctx, admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{}, + }) +} + func TestManagedMeta(t *testing.T) { f := foresttest.Create("-") // a h := &Validator{Forest: f} - l := zap.New() // For this test we accept any label or annotation not starting with 'h', // to allow almost any meta - except the hnc.x-k8s.io labels/annotations, // which cannot be managed anyway. And allows us to use that for testing. @@ -70,12 +77,13 @@ func TestManagedMeta(t *testing.T) { hc.ObjectMeta.Namespace = "a" hc.Spec.Labels = tc.labels hc.Spec.Annotations = tc.annotations - req := &request{hc: hc} - - got := h.handle(context.Background(), l, req) - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(Equal(tc.allowed)) + _, err := h.handle(newContext(t.Context()), hc) + if tc.allowed { + g.Expect(err).To(Succeed()) + } else { + g.Expect(err).To(HaveOccurred()) + } }) } } @@ -83,7 +91,6 @@ func TestManagedMeta(t *testing.T) { func TestStructure(t *testing.T) { f := foresttest.Create("-a-") // a <- b; c h := &Validator{Forest: f} - l := zap.New() // For this unit test, we only set `kube-system` as an excluded namespace. config.SetNamespaces("", "kube-system") @@ -114,15 +121,16 @@ func TestStructure(t *testing.T) { hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{Parent: tc.pnm}} hc.ObjectMeta.Name = api.Singleton hc.ObjectMeta.Namespace = tc.nnm - req := &request{hc: hc} // Test - got := h.handle(context.Background(), l, req) + _, err := h.handle(newContext(t.Context()), hc) // Report - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail)) - g.Expect(got.Result.Message).Should(ContainSubstring(tc.msgContains)) + if tc.fail { + g.Expect(err).To(MatchError(ContainSubstring(tc.msgContains))) + } else { + g.Expect(err).To(Succeed()) + } }) } } @@ -130,7 +138,6 @@ func TestStructure(t *testing.T) { func TestChangeParentOnManagedBy(t *testing.T) { f := foresttest.Create("-a-c") // a <- b; c <- d h := &Validator{Forest: f} - l := zap.New() // Make c and d external namespaces f.Get("c").Manager = "external-tool" @@ -159,14 +166,16 @@ func TestChangeParentOnManagedBy(t *testing.T) { hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{Parent: tc.pnm}} hc.ObjectMeta.Name = api.Singleton hc.ObjectMeta.Namespace = tc.nnm - req := &request{hc: hc} // Test - got := h.handle(context.Background(), l, req) + _, err := h.handle(newContext(t.Context()), hc) // Report - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail)) + if tc.fail { + g.Expect(err).To(Not(Succeed())) + } else { + g.Expect(err).To(Succeed()) + } }) } } @@ -188,7 +197,6 @@ func TestChangeParentWithConflict(t *testing.T) { foresttest.CreateSecret("conflict", "d", f) h := &Validator{Forest: f} - l := zap.New() tests := []struct { name string @@ -210,14 +218,16 @@ func TestChangeParentWithConflict(t *testing.T) { hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{Parent: tc.pnm}} hc.ObjectMeta.Name = api.Singleton hc.ObjectMeta.Namespace = tc.nnm - req := &request{hc: hc} // Test - got := h.handle(context.Background(), l, req) + _, err := h.handle(newContext(t.Context()), hc) // Report - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail)) + if tc.fail { + g.Expect(err).To(Not(Succeed())) + } else { + g.Expect(err).To(Succeed()) + } }) } } @@ -243,7 +253,6 @@ func TestConflictItemWithPropagateNoneLabel(t *testing.T) { foresttest.CreateSecret("conflict", "d", f) h := &Validator{Forest: f} - l := zap.New() tests := []struct { name string nnm string @@ -261,14 +270,16 @@ func TestConflictItemWithPropagateNoneLabel(t *testing.T) { hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{Parent: tc.pnm}} hc.ObjectMeta.Name = api.Singleton hc.ObjectMeta.Namespace = tc.nnm - req := &request{hc: hc} // Test - got := h.handle(context.Background(), l, req) + _, err := h.handle(newContext(t.Context()), hc) // Report - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail)) + if tc.fail { + g.Expect(err).To(Not(Succeed())) + } else { + g.Expect(err).To(Succeed()) + } }) } @@ -305,28 +316,27 @@ func TestAuthz(t *testing.T) { g := NewWithT(t) f := foresttest.Create(tc.forest) h := &Validator{Forest: f, server: tc.server} - l := zap.New() // Create request hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{Parent: tc.to}} hc.ObjectMeta.Name = api.Singleton hc.ObjectMeta.Namespace = tc.nm - req := &request{hc: hc, ui: &authn.UserInfo{Username: "jen"}} // Test - got := h.handle(context.Background(), l, req) + _, err := h.handle(newContext(t.Context()), hc) // Report - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Result.Code).Should(Equal(tc.code)) + if tc.code > 0 { + var apiStatus apierrors.APIStatus + g.Expect(errors.As(err, &apiStatus)).Should(BeTrue()) + g.Expect(apiStatus.Status().Code).Should(Equal(tc.code)) + } else { + g.Expect(err).To(Succeed()) + } }) } } -func logResult(t *testing.T, result *metav1.Status) { - t.Logf("Got reason %q, code %d, msg %q", result.Reason, result.Code, result.Message) -} - // fakeServer implements serverClient. It's implemented as a string separated by a colon (":") with // the following meanings: // * Anything *before* the colon passes the IsAdmin check diff --git a/internal/hncconfig/validator.go b/internal/hncconfig/validator.go index 4f043574e..53d246be4 100644 --- a/internal/hncconfig/validator.go +++ b/internal/hncconfig/validator.go @@ -8,9 +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/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" @@ -32,56 +34,78 @@ const ( // +kubebuilder:webhook:admissionReviewVersions=v1,path=/validate-hnc-x-k8s-io-v1alpha2-hncconfigurations,mutating=false,failurePolicy=fail,groups="hnc.x-k8s.io",resources=hncconfigurations,sideEffects=None,verbs=create;update;delete,versions=v1alpha2,name=hncconfigurations.hnc.x-k8s.io type Validator struct { - Log logr.Logger - Forest *forest.Forest - mapper resourceMapper - decoder admission.Decoder + Log logr.Logger + Forest *forest.Forest + mapper resourceMapper } type gvkSet map[schema.GroupVersionKind]api.SynchronizationMode -func (v *Validator) Handle(ctx context.Context, req admission.Request) admission.Response { - log := v.Log.WithValues("nm", req.Name, "op", req.Operation, "user", req.UserInfo.Username) - if webhooks.IsHNCServiceAccount(&req.AdmissionRequest.UserInfo) { - return webhooks.Allow("HNC SA") +func NewValidator(forest *forest.Forest) *Validator { + return &Validator{ + Log: ctrl.Log.WithName("hncconfig").WithName("validate"), + Forest: forest, } +} + +func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + return v.handle(ctx, obj, k8sadm.Create) +} - if req.Operation == k8sadm.Delete { +func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + return v.handle(ctx, newObj, k8sadm.Update) +} + +func (v *Validator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + return v.handle(ctx, obj, k8sadm.Delete) +} + +func (v *Validator) handle(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("nm", req.Name, "op", operation, "user", req.UserInfo.Username) + if webhooks.IsHNCServiceAccount(&req.UserInfo) { + return nil, nil + } + + if operation == k8sadm.Delete { if req.Name == api.HNCConfigSingleton { err := errors.New("deleting the 'config' object is forbidden") - return webhooks.DenyForbidden(api.HNCConfigurationGR, api.HNCConfigSingleton, err) + return nil, apierrors.NewForbidden(api.HNCConfigurationGR, api.HNCConfigSingleton, err) } else { - // We allow deleting other objects. We should never enter this case with the CRD validation. We introduced - // the CRD validation in v0.6. Before that, it was protected by the validation controller. If users somehow - // bypassed the validation controller and created objects of other names, those objects would still have an - // obsolete condition and we will allow users to delete the objects. - return webhooks.Allow("") + return nil, nil } } - inst := &api.HNCConfiguration{} - if err := v.decoder.Decode(req, inst); err != nil { - log.Error(err, "Couldn't decode request") - return webhooks.DenyBadRequest(err) + inst, ok := obj.(*api.HNCConfiguration) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("expected a HNCConfiguration but got a %T", obj)) } - resp := v.handle(inst) - 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.validateInstance(inst); 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 } -// handle implements the validation logic of this validator for Create and Update operations, +// validateInstance implements the validation logic of this validator for Create and Update operations, // allowing it to be more easily unit tested (ie without constructing a full admission.Request). -func (v *Validator) handle(inst *api.HNCConfiguration) admission.Response { +func (v *Validator) validateInstance(inst *api.HNCConfiguration) error { ts := gvkSet{} // Convert all valid types from GR to GVK. If any type is invalid, e.g. not // exist in the apiserver, wrong configuration, deny the request. - if rp := v.validateTypes(inst, ts); !rp.Allowed { - return rp + if err := v.validateTypes(inst, ts); err != nil { + return err } // Lastly, check if changing a type to "Propagate" mode would cause @@ -89,7 +113,7 @@ func (v *Validator) handle(inst *api.HNCConfiguration) admission.Response { return v.checkForest(ts) } -func (v *Validator) validateTypes(inst *api.HNCConfiguration, ts gvkSet) admission.Response { +func (v *Validator) validateTypes(inst *api.HNCConfiguration, ts gvkSet) error { allErrs := field.ErrorList{} for i, r := range inst.Spec.Resources { gr := schema.GroupResource{Group: r.Group, Resource: r.Resource} @@ -118,12 +142,12 @@ func (v *Validator) validateTypes(inst *api.HNCConfiguration, ts gvkSet) admissi ts[gvk] = r.Mode } if len(allErrs) > 0 { - return webhooks.DenyInvalid(api.HNCConfigurationGK, api.HNCConfigSingleton, allErrs) + return apierrors.NewInvalid(api.HNCConfigurationGK, api.HNCConfigSingleton, allErrs) } - return webhooks.Allow("") + return nil } -func (v *Validator) checkForest(ts gvkSet) admission.Response { +func (v *Validator) checkForest(ts gvkSet) error { v.Forest.Lock() defer v.Forest.Unlock() @@ -139,11 +163,11 @@ func (v *Validator) checkForest(ts gvkSet) admission.Response { msg += "\nTo fix this, please rename or remove the conflicting objects first." err := errors.New(msg) // TODO(erikgb): Invalid field error better? - return webhooks.DenyConflict(api.HNCConfigurationGR, api.Singleton, err) + return apierrors.NewConflict(api.HNCConfigurationGR, api.Singleton, err) } } - return webhooks.Allow("") + return nil } // checkConflictsForGVK looks for conflicts from top down for each tree. @@ -224,16 +248,11 @@ func (a ancestorObjects) add(onm string, ns *forest.Namespace) { a[onm] = append(a[onm], ns.Name()) } -func (v *Validator) InjectConfig(cf *rest.Config) error { - mapper, err := apimeta.NewGroupKindMapper(cf) +func (v *Validator) SetupWithManager(mgr ctrl.Manager) error { + mapper, err := apimeta.NewGroupKindMapper(mgr.GetConfig()) if err != nil { return err } v.mapper = mapper return nil } - -func (v *Validator) InjectDecoder(d admission.Decoder) error { - v.decoder = d - return nil -} diff --git a/internal/hncconfig/validator_test.go b/internal/hncconfig/validator_test.go index 28f603d86..22e9f845c 100644 --- a/internal/hncconfig/validator_test.go +++ b/internal/hncconfig/validator_test.go @@ -8,7 +8,6 @@ import ( . "github.com/onsi/gomega" k8sadm "k8s.io/api/admission/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -32,32 +31,33 @@ var ( func TestDeletingConfigObject(t *testing.T) { t.Run("Delete config object", func(t *testing.T) { g := NewWithT(t) - req := admission.Request{AdmissionRequest: k8sadm.AdmissionRequest{ + ctx := admission.NewContextWithRequest(context.Background(), admission.Request{AdmissionRequest: k8sadm.AdmissionRequest{ Operation: k8sadm.Delete, Name: api.HNCConfigSingleton, - }} + }}) validator := &Validator{Log: zap.New()} + hc := &api.HNCConfiguration{} - got := validator.Handle(context.Background(), req) + _, err := validator.ValidateDelete(ctx, hc) - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(BeFalse()) + g.Expect(err).Should(HaveOccurred()) + g.Expect(err.Error()).Should(ContainSubstring("deleting the 'config' object is forbidden")) }) } func TestDeletingOtherObject(t *testing.T) { - t.Run("Delete config object", func(t *testing.T) { + t.Run("Delete other object", func(t *testing.T) { g := NewWithT(t) - req := admission.Request{AdmissionRequest: k8sadm.AdmissionRequest{ + ctx := admission.NewContextWithRequest(context.Background(), admission.Request{AdmissionRequest: k8sadm.AdmissionRequest{ Operation: k8sadm.Delete, Name: "other", - }} + }}) validator := &Validator{Log: zap.New()} + hc := &api.HNCConfiguration{} - got := validator.Handle(context.Background(), req) + _, err := validator.ValidateDelete(ctx, hc) - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(BeTrue()) + g.Expect(err).ShouldNot(HaveOccurred()) }) } @@ -95,10 +95,13 @@ func TestRBACTypes(t *testing.T) { c := &api.HNCConfiguration{Spec: api.HNCConfigurationSpec{Resources: tc.configs}} c.Name = api.HNCConfigSingleton - got := validator.handle(c) + err := validator.validateInstance(c) - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(Equal(tc.allow)) + if tc.allow { + g.Expect(err).ShouldNot(HaveOccurred()) + } else { + g.Expect(err).Should(HaveOccurred()) + } }) } } @@ -157,10 +160,13 @@ func TestNonRBACTypes(t *testing.T) { Log: zap.New(), } - got := validator.handle(c) + err := validator.validateInstance(c) - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(Equal(tc.allow)) + if tc.allow { + g.Expect(err).ShouldNot(HaveOccurred()) + } else { + g.Expect(err).Should(HaveOccurred()) + } }) } } @@ -224,12 +230,16 @@ func TestPropagateConflict(t *testing.T) { } f.Get(string(ns)).SetSourceObject(inst) } - got := validator.handle(c) + err := validator.validateInstance(c) - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(Equal(tc.allow)) + if tc.allow { + g.Expect(err).ShouldNot(HaveOccurred()) + } else { + g.Expect(err).Should(HaveOccurred()) + } if tc.errContain != "" { - g.Expect(strings.Contains(got.AdmissionResponse.Result.Message, tc.errContain)) + g.Expect(err).Should(HaveOccurred()) + g.Expect(err.Error()).Should(ContainSubstring(tc.errContain)) } }) } @@ -247,7 +257,3 @@ func (f fakeResourceMapper) NamespacedKindFor(gr schema.GroupResource) (schema.G } return gr2gvk[gr], nil } - -func logResult(t *testing.T, result *metav1.Status) { - t.Logf("Got reason %q, code %d, msg %q", result.Reason, result.Code, result.Message) -} diff --git a/internal/hrq/hrqvalidator.go b/internal/hrq/hrqvalidator.go index 54ba7f814..000767570 100644 --- a/internal/hrq/hrqvalidator.go +++ b/internal/hrq/hrqvalidator.go @@ -8,7 +8,10 @@ import ( "github.com/go-logr/logr" v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -35,9 +38,15 @@ const ( // +kubebuilder:webhook:admissionReviewVersions=v1;v1beta1,path=/validate-hnc-x-k8s-io-v1alpha2-hrq,mutating=false,failurePolicy=fail,groups="hnc.x-k8s.io",resources=hierarchicalresourcequotas,sideEffects=None,verbs=create;update,versions=v1alpha2,name=hrq.hnc.x-k8s.io type HRQ struct { - server serverClient - Log logr.Logger - decoder admission.Decoder + server serverClient + Log logr.Logger +} + +func NewHRQ(c client.Client) *HRQ { + return &HRQ{ + server: &realClient{client: client.NewDryRunClient(c)}, + Log: ctrl.Log.WithName("validators").WithName("HierarchicalResourceQuota"), + } } // serverClient represents the checks that should typically be performed against @@ -47,28 +56,59 @@ type serverClient interface { validate(ctx context.Context, inst *v1.ResourceQuota) error } -func (v *HRQ) Handle(ctx context.Context, req admission.Request) admission.Response { +func (v *HRQ) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + req, err := admission.RequestFromContext(ctx) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + log := logutils.WithRID(v.Log).WithValues("nm", req.Name, "nnm", req.Namespace) - inst := &api.HierarchicalResourceQuota{} - if err := v.decoder.Decode(req, inst); err != nil { - log.Error(err, "Couldn't decode request") - return deny(metav1.StatusReasonBadRequest, err.Error()) + inst, ok := obj.(*api.HierarchicalResourceQuota) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("expected a HierarchicalResourceQuota but got a %T", obj)) } - resp := v.handle(ctx, log, inst) - if resp.Allowed { - log.V(1).Info("Allowed", "message", resp.Result.Message) - } else { - log.Info("Denied", "code", resp.Result.Code, "reason", resp.Result.Reason, "message", resp.Result.Message) + if err := v.validate(ctx, log, inst); err != nil { + log.Info("Denied", "reason", err) + return nil, err } - return resp + + log.V(1).Info("Allowed") + return nil, nil +} + +func (v *HRQ) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + req, err := admission.RequestFromContext(ctx) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + + log := logutils.WithRID(v.Log).WithValues("nm", req.Name, "nnm", req.Namespace) + + inst, ok := newObj.(*api.HierarchicalResourceQuota) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("expected a HierarchicalResourceQuota but got a %T", newObj)) + } + + if err := v.validate(ctx, log, inst); err != nil { + log.Info("Denied", "reason", err) + return nil, err + } + + log.V(1).Info("Allowed") + return nil, nil +} + +func (v *HRQ) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + // HierarchicalResourceQuota deletion doesn't need validation for our purposes + return nil, nil } // Validate if the resources in the spec are valid resource types for quota. We // will dry-run writing the HRQ resources to an RQ to see if we get an error // from the apiserver or not. If yes, we will deny the HRQ request. -func (v *HRQ) handle(ctx context.Context, log logr.Logger, inst *api.HierarchicalResourceQuota) admission.Response { +func (v *HRQ) validate(ctx context.Context, log logr.Logger, inst *api.HierarchicalResourceQuota) error { rq := &v1.ResourceQuota{} rq.Namespace = inst.Namespace rq.Name = createRQName() @@ -76,10 +116,11 @@ func (v *HRQ) handle(ctx context.Context, log logr.Logger, inst *api.Hierarchica rq.Spec.ScopeSelector = inst.Spec.ScopeSelector log.V(1).Info("Validating resource types in the HRQ spec by writing them to a resource quota on apiserver", "limits", inst.Spec.Hard) if err := v.server.validate(ctx, rq); err != nil { - return denyInvalidField(fieldInfo, ignoreRQErr(err.Error())) + return apierrors.NewInvalid(api.GroupVersion.WithKind("HierarchicalResourceQuota").GroupKind(), inst.Name, + field.ErrorList{field.Invalid(field.NewPath("spec").Child("hard"), inst.Spec.Hard, ignoreRQErr(err.Error()))}) } - return allow("") + return nil } func createRQName() string { @@ -116,14 +157,3 @@ func (c *realClient) validate(ctx context.Context, inst *v1.ResourceQuota) error } return nil } - -func (v *HRQ) InjectClient(c client.Client) error { - // Create a dry-run client. - v.server = &realClient{client: client.NewDryRunClient(c)} - return nil -} - -func (r *HRQ) InjectDecoder(d admission.Decoder) error { - r.decoder = d - return nil -} diff --git a/internal/hrq/hrqvalidator_test.go b/internal/hrq/hrqvalidator_test.go index b74a93515..f058e2d58 100644 --- a/internal/hrq/hrqvalidator_test.go +++ b/internal/hrq/hrqvalidator_test.go @@ -3,6 +3,7 @@ package hrq import ( "context" "errors" + "strings" "testing" v1 "k8s.io/api/core/v1" @@ -42,9 +43,12 @@ func TestHRQSpec(t *testing.T) { hrq := &api.HierarchicalResourceQuota{} hrq.Spec.Hard = argsToResourceList(tc.hrqLimit...) v := HRQ{server: fakeServer("")} - got := v.handle(context.Background(), l, hrq) - if got.AdmissionResponse.Allowed == tc.fail || got.Result.Message != tc.msg { - t.Errorf("unexpected admission response. Expected: %t, %s; Got: %t, %s", !tc.fail, tc.msg, got.AdmissionResponse.Allowed, got.Result.Message) + err := v.validate(context.Background(), l, hrq) + if (err != nil) != tc.fail { + t.Errorf("unexpected validation result: got error=%v, expected failure=%v", err, tc.fail) + } + if err != nil && tc.msg != "" && !strings.Contains(err.Error(), tc.msg) { + t.Errorf("unexpected error message. Expected to contain: %s; Got: %s", tc.msg, err.Error()) } }) } diff --git a/internal/hrq/rqstatusvalidator.go b/internal/hrq/rqstatusvalidator.go index a2c577b67..c06b1e65a 100644 --- a/internal/hrq/rqstatusvalidator.go +++ b/internal/hrq/rqstatusvalidator.go @@ -2,13 +2,17 @@ package hrq import ( "context" + "fmt" "strings" "github.com/go-logr/logr" k8sadm "k8s.io/api/admission/v1" authnv1 "k8s.io/api/authentication/v1" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -34,14 +38,50 @@ const ( type ResourceQuotaStatus struct { Log logr.Logger Forest *forest.Forest + client client.Client +} + +func NewResourceQuotaStatus(forest *forest.Forest, client client.Client) *ResourceQuotaStatus { + return &ResourceQuotaStatus{ + Log: ctrl.Log.WithName("validators").WithName("ResourceQuota"), + Forest: forest, + client: client, + } +} - client client.Client - decoder admission.Decoder +func (r *ResourceQuotaStatus) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + // ResourceQuota creation doesn't need validation for our purposes + return nil, nil } -func (r *ResourceQuotaStatus) Handle(ctx context.Context, req admission.Request) admission.Response { +func (r *ResourceQuotaStatus) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + req, err := admission.RequestFromContext(ctx) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + log := logutils.WithRID(r.Log).WithValues("nm", req.Name, "nnm", req.Namespace) + rq, ok := newObj.(*v1.ResourceQuota) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("expected a ResourceQuota but got a %T", newObj)) + } + + if err := r.validateResourceQuotaStatus(ctx, log, &req.UserInfo, rq); err != nil { + log.Info("Denied", "reason", err) + return nil, err + } + + log.V(1).Info("Allowed") + return nil, nil +} + +func (r *ResourceQuotaStatus) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + // ResourceQuota deletion doesn't need validation for our purposes + return nil, nil +} + +func (r *ResourceQuotaStatus) validateResourceQuotaStatus(_ context.Context, log logr.Logger, userInfo *authnv1.UserInfo, inst *v1.ResourceQuota) error { // We only intercept and validate the HRQ-singleton RQ status changes from the // K8s Resource Quota Admission Controller. Once the admission controller // allows a request, it will issue a request to update the RQ status. We can @@ -49,54 +89,41 @@ func (r *ResourceQuotaStatus) Handle(ctx context.Context, req admission.Request) // (or denying) status update requests from the admission controller. // // Thus we will allow any non-HRQ-singleton RQ updates first. - if !strings.Contains(req.Name, api.ResourceQuotaSingletonName) { - return allow("non-HRQ-singleton RQ status change ignored") + if !strings.Contains(inst.Name, api.ResourceQuotaSingletonName) { + log.V(1).Info("non-HRQ-singleton RQ status change ignored") + return nil } // Then for HRQ-singleton RQ status updates, we will allow any request // attempted by other than K8s Resource Quota Admission Controller. - if !isResourceQuotaAdmissionControllerServiceAccount(&req.UserInfo) { + if !isResourceQuotaAdmissionControllerServiceAccount(userInfo) { log.V(1).Info("Request is not from Kubernetes Resource Quota Admission Controller; Allowed") - return allow("") - } - - inst := &v1.ResourceQuota{} - if err := r.decoder.Decode(req, inst); err != nil { - log.Error(err, "Couldn't decode request") - return deny(metav1.StatusReasonBadRequest, err.Error()) + return nil } - resp := r.handle(inst) - if resp.Allowed { - log.V(1).Info("Allowed", "message", resp.Result.Message, "usages", inst.Status.Used) - } else { - log.Info("Denied", "code", resp.Result.Code, "reason", resp.Result.Reason, "message", resp.Result.Message, "usages", inst.Status.Used) - } - return resp + return r.validateHRQResourceQuotaStatus(inst) } -func (r *ResourceQuotaStatus) handle(inst *v1.ResourceQuota) admission.Response { +func (r *ResourceQuotaStatus) validateHRQResourceQuotaStatus(inst *v1.ResourceQuota) error { r.Forest.Lock() defer r.Forest.Unlock() ns := r.Forest.Get(inst.Namespace) if err := ns.TryUseResources(inst.Status.Used, inst.GetName()); err != nil { - return deny(metav1.StatusReasonForbidden, err.Error()) + return apierrors.NewForbidden(v1.Resource("resourcequotas"), inst.Name, err) } - return allow("the usage update is in compliance with the ancestors' (including itself) HRQs") -} - -func (r *ResourceQuotaStatus) InjectClient(c client.Client) error { - r.client = c return nil } -func (r *ResourceQuotaStatus) InjectDecoder(d admission.Decoder) error { - r.decoder = d - return nil +func isResourceQuotaAdmissionControllerServiceAccount(user *authnv1.UserInfo) bool { + // Treat nil user same as admission controller SA so that unit tests do not need to + // specify admission controller SA. + return user == nil || user.Username == ResourceQuotaAdmissionControllerUsername } +// Helper functions for admission responses - these are shared between hrqvalidator.go and rqstatusvalidator.go + // allow is a replacement for controller-runtime's admission.Allowed() that allows you to set the // message (human-readable) as opposed to the reason (machine-readable). func allow(msg string) admission.Response { @@ -109,12 +136,6 @@ func allow(msg string) admission.Response { }} } -func isResourceQuotaAdmissionControllerServiceAccount(user *authnv1.UserInfo) bool { - // Treat nil user same as admission controller SA so that unit tests do not need to - // specify admission controller SA. - return user == nil || user.Username == ResourceQuotaAdmissionControllerUsername -} - // deny is a replacement for controller-runtime's admission.Denied() that allows you to set _both_ a // human-readable message _and_ a machine-readable reason, and also sets the code correctly instead // of hardcoding it to 403 Forbidden. diff --git a/internal/hrq/rqstatusvalidator_test.go b/internal/hrq/rqstatusvalidator_test.go index 316f6df3f..b60585e31 100644 --- a/internal/hrq/rqstatusvalidator_test.go +++ b/internal/hrq/rqstatusvalidator_test.go @@ -65,9 +65,9 @@ func TestRQStatusChange(t *testing.T) { usage, _ := f.Get(tc.namespace).GetLocalUsages(api.ResourceQuotaSingletonName) rqInst.Status.Used = utils.Add(delta, usage) - got := rqs.handle(rqInst) - if got.AdmissionResponse.Allowed == tc.fail { - t.Errorf("unexpected admission response") + err = rqs.validateHRQResourceQuotaStatus(rqInst) + if (err != nil) != tc.fail { + t.Errorf("unexpected validation result: got error=%v, expected failure=%v", err, tc.fail) } }) } diff --git a/internal/namespace/mutator.go b/internal/namespace/mutator.go index 5ee53a5c9..cc224aa2b 100644 --- a/internal/namespace/mutator.go +++ b/internal/namespace/mutator.go @@ -2,15 +2,14 @@ package namespaces import ( "context" - "encoding/json" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" "sigs.k8s.io/hierarchical-namespaces/internal/config" - "sigs.k8s.io/hierarchical-namespaces/internal/webhooks" ) const ( @@ -26,32 +25,31 @@ const ( // +kubebuilder:webhook:admissionReviewVersions=v1,path=/mutate-namespace,mutating=true,failurePolicy=ignore,groups="",resources=namespaces,sideEffects=None,verbs=create;update,versions=v1,name=namespacelabel.hnc.x-k8s.io type Mutator struct { - Log logr.Logger - decoder admission.Decoder + Log logr.Logger } -// Handle implements the mutating webhook. -func (m *Mutator) Handle(ctx context.Context, req admission.Request) admission.Response { - log := m.Log.WithValues("namespace", req.Name) - ns := &corev1.Namespace{} - if err := m.decoder.Decode(req, ns); err != nil { - log.Error(err, "Couldn't decode request") - return webhooks.DenyBadRequest(err) +func NewMutator() *Mutator { + return &Mutator{ + Log: ctrl.Log.WithName("namespace").WithName("mutate"), } +} - m.handle(log, ns) - marshaledNS, err := json.Marshal(ns) - if err != nil { - return webhooks.DenyInternalError(err) +func (m *Mutator) Default(ctx context.Context, obj runtime.Object) error { + ns, ok := obj.(*corev1.Namespace) + if !ok { + return nil } - return admission.PatchResponseFromRaw(req.Object.Raw, marshaledNS) + + log := m.Log.WithValues("namespace", ns.Name) + m.mutateNamespace(log, ns) + return nil } -// handle implements the non-boilerplate logic of this mutator, allowing it to +// mutateNamespace implements the non-boilerplate logic of this mutator, allowing it to // be more easily unit tested (ie without constructing a full admission.Request). // Currently, we only add `included-namespace` label to non-excluded namespaces // if the label is missing. -func (m *Mutator) handle(log logr.Logger, ns *corev1.Namespace) { +func (m *Mutator) mutateNamespace(log logr.Logger, ns *corev1.Namespace) { if !config.IsManagedNamespace(ns.Name) { return } @@ -65,9 +63,3 @@ func (m *Mutator) handle(log logr.Logger, ns *corev1.Namespace) { ns.Labels[api.LabelIncludedNamespace] = "true" } } - -// InjectDecoder injects the decoder. -func (m *Mutator) InjectDecoder(d admission.Decoder) error { - m.decoder = d - return nil -} diff --git a/internal/namespace/mutator_test.go b/internal/namespace/mutator_test.go index e0bd8e6a5..49dfe5f78 100644 --- a/internal/namespace/mutator_test.go +++ b/internal/namespace/mutator_test.go @@ -40,7 +40,7 @@ func TestMutateNamespaceIncludedLabel(t *testing.T) { } // Test - m.handle(l, nsInst) + m.mutateNamespace(l, nsInst) // Report g.Expect(nsInst.Labels[api.LabelIncludedNamespace]).Should(Equal(tc.expectlval)) diff --git a/internal/namespace/validator.go b/internal/namespace/validator.go index a7bbad837..760ee7c43 100644 --- a/internal/namespace/validator.go +++ b/internal/namespace/validator.go @@ -9,6 +9,9 @@ import ( "github.com/go-logr/logr" k8sadm "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" @@ -33,45 +36,84 @@ var ( // +kubebuilder:webhook:admissionReviewVersions=v1,path=/validate-v1-namespace,mutating=false,failurePolicy=fail,groups="",resources=namespaces,sideEffects=None,verbs=delete;create;update,versions=v1,name=namespaces.hnc.x-k8s.io type Validator struct { - Log logr.Logger - Forest *forest.Forest - decoder admission.Decoder + Log logr.Logger + Forest *forest.Forest } -// nsRequest defines the aspects of the admission.Request that we care about. -type nsRequest struct { - ns *corev1.Namespace - oldns *corev1.Namespace - op k8sadm.Operation +func NewValidator(forest *forest.Forest) *Validator { + return &Validator{ + Log: ctrl.Log.WithName("namespace").WithName("validate"), + Forest: forest, + } +} + +func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + return v.validateNamespace(ctx, obj, nil, k8sadm.Create) +} + +func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + return v.validateNamespace(ctx, newObj, oldObj, k8sadm.Update) } -// Handle implements the validation webhook. -func (v *Validator) Handle(ctx context.Context, req admission.Request) admission.Response { - log := v.Log.WithValues("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) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + return v.validateNamespace(ctx, obj, nil, k8sadm.Delete) +} + +func (v *Validator) validateNamespace(ctx context.Context, obj runtime.Object, oldObj 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("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) + ns, ok := obj.(*corev1.Namespace) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("expected a Namespace but got a %T", obj)) } - 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) + var oldns *corev1.Namespace + if oldObj != nil { + oldns, ok = oldObj.(*corev1.Namespace) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("expected a Namespace for oldObj but got a %T", oldObj)) + } } - return resp + + nsReq := &nsRequest{ + ns: ns, + oldns: oldns, + op: operation, + } + + if err := v.handleValidation(nsReq); 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 + } + + log.V(1).Info("Allowed") + return nil, nil } -// handle implements the non-boilerplate logic of this validator, allowing it to be more easily unit +// nsRequest defines the aspects of the admission.Request that we care about. +type nsRequest struct { + ns *corev1.Namespace + oldns *corev1.Namespace + op k8sadm.Operation +} + +// handleValidation implements the non-boilerplate logic of this validator, allowing it to be more easily unit // tested (ie without constructing a full admission.Request). -func (v *Validator) handle(req *nsRequest) admission.Response { +func (v *Validator) handleValidation(req *nsRequest) error { v.Forest.Lock() defer v.Forest.Unlock() @@ -79,50 +121,50 @@ func (v *Validator) handle(req *nsRequest) admission.Response { switch req.op { case k8sadm.Create: - if rsp := v.illegalIncludedNamespaceLabel(req); !rsp.Allowed { - return rsp + if err := v.illegalIncludedNamespaceLabel(req); err != nil { + return err } // This check only applies to the Create operation since namespace name // cannot be updated. - if rsp := v.nameExistsInExternalHierarchy(req); !rsp.Allowed { - return rsp + if err := v.nameExistsInExternalHierarchy(req); err != nil { + return err } - if rsp := v.illegalTreeLabel(req); !rsp.Allowed { - return rsp + if err := v.illegalTreeLabel(req); err != nil { + return err } case k8sadm.Update: - if rsp := v.illegalIncludedNamespaceLabel(req); !rsp.Allowed { - return rsp + if err := v.illegalIncludedNamespaceLabel(req); err != nil { + return err } // This check only applies to the Update operation. Creating a namespace // with external manager is allowed and we will prevent this conflict by not // allowing setting a parent when validating the HierarchyConfiguration. - if rsp := v.conflictBetweenParentAndExternalManager(req, ns); !rsp.Allowed { - return rsp + if err := v.conflictBetweenParentAndExternalManager(req, ns); err != nil { + return err } - if rsp := v.illegalTreeLabel(req); !rsp.Allowed { - return rsp + if err := v.illegalTreeLabel(req); err != nil { + return err } case k8sadm.Delete: - if rsp := v.cannotDeleteSubnamespace(req); !rsp.Allowed { - return rsp + if err := v.cannotDeleteSubnamespace(req); err != nil { + return err } - if rsp := v.illegalCascadingDeletion(ns); !rsp.Allowed { - return rsp + if err := v.illegalCascadingDeletion(ns); err != nil { + return err } } - return webhooks.Allow("") + return nil } // illegalTreeLabel checks if tree labels are being created or modified // by any user or service account since only HNC service account is // allowed to do so -func (v *Validator) illegalTreeLabel(req *nsRequest) admission.Response { +func (v *Validator) illegalTreeLabel(req *nsRequest) error { oldLabels := map[string]string{} if req.oldns != nil { oldLabels = req.oldns.Labels @@ -136,7 +178,7 @@ func (v *Validator) illegalTreeLabel(req *nsRequest) admission.Response { // Check if new HNC label tree key isn't being added if oldLabels[key] != val { err := fmt.Errorf("cannot set or modify tree label %q in namespace %q; these can only be managed by HNC", key, req.ns.Name) - return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err) + return apierrors.NewForbidden(namespaceGR, req.ns.Name, err) } } @@ -145,24 +187,24 @@ func (v *Validator) illegalTreeLabel(req *nsRequest) admission.Response { if strings.Contains(key, api.LabelTreeDepthSuffix) { if _, ok := req.ns.Labels[key]; !ok { err := fmt.Errorf("cannot remove tree label %q in namespace %q; these can only be managed by HNC", key, req.ns.Name) - return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err) + return apierrors.NewForbidden(namespaceGR, req.ns.Name, err) } } } - return webhooks.Allow("") + return nil } // illegalIncludedNamespaceLabel checks if there's any illegal use of the // included-namespace label on namespaces. It only checks a Create or an Update // request. -func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Response { +func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) error { // Early exit if there's no change on the label. labelValue, hasLabel := req.ns.Labels[api.LabelIncludedNamespace] if req.oldns != nil { oldLabelValue, oldHasLabel := req.oldns.Labels[api.LabelIncludedNamespace] if oldHasLabel == hasLabel && oldLabelValue == labelValue { - return webhooks.Allow("") + return nil } } @@ -173,7 +215,7 @@ func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp err := fmt.Errorf("you cannot enforce webhook rules on this unmanaged namespace using the %q label. "+ "See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label "+ "for detail", api.LabelIncludedNamespace) - return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err) + return apierrors.NewForbidden(namespaceGR, req.ns.Name, err) } // An included-namespace should have the included-namespace label with the @@ -184,15 +226,15 @@ func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp err := fmt.Errorf("you cannot change the value of the %q label. It has to be set as true on all managed namespaces. "+ "See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label "+ "for detail", api.LabelIncludedNamespace) - return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err) + return apierrors.NewForbidden(namespaceGR, req.ns.Name, err) } - return webhooks.Allow("") + return nil } // nameExistsInExternalHierarchy only applies to the Create operation since // namespace name cannot be updated. -func (v *Validator) nameExistsInExternalHierarchy(req *nsRequest) admission.Response { +func (v *Validator) nameExistsInExternalHierarchy(req *nsRequest) error { for _, nm := range v.Forest.GetNamespaceNames() { ns := v.Forest.Get(nm) if !ns.IsExternal() { @@ -201,32 +243,32 @@ func (v *Validator) nameExistsInExternalHierarchy(req *nsRequest) admission.Resp externalTreeLabels := ns.GetTreeLabels() if _, ok := externalTreeLabels[req.ns.Name]; ok { msg := fmt.Errorf("is reserved by the external hierarchy manager %q", v.Forest.Get(nm).Manager) - return webhooks.DenyConflict(namespaceGR, req.ns.Name, msg) + return apierrors.NewConflict(namespaceGR, req.ns.Name, msg) } } - return webhooks.Allow("") + return nil } // conflictBetweenParentAndExternalManager only applies to the Update operation. // Creating a namespace with external manager is allowed and we will prevent // this conflict by not allowing setting a parent when validating the // HierarchyConfiguration. -func (v *Validator) conflictBetweenParentAndExternalManager(req *nsRequest, ns *forest.Namespace) admission.Response { +func (v *Validator) conflictBetweenParentAndExternalManager(req *nsRequest, ns *forest.Namespace) error { mgr := req.ns.Annotations[api.AnnotationManagedBy] if mgr != "" && mgr != api.MetaGroup && ns.Parent() != nil { err := fmt.Errorf("is a child of %q. Namespaces with parents defined by HNC cannot also be managed externally. "+ "To manage this namespace with %q, first make it a root in HNC", ns.Parent().Name(), mgr) - return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err) + return apierrors.NewForbidden(namespaceGR, req.ns.Name, err) } - return webhooks.Allow("") + return nil } // cannotDeleteSubnamespace only applies to the Delete operation. -func (v *Validator) cannotDeleteSubnamespace(req *nsRequest) admission.Response { +func (v *Validator) cannotDeleteSubnamespace(req *nsRequest) error { parent := req.ns.Annotations[api.SubnamespaceOf] // Early exit if the namespace is not a subnamespace. if parent == "" { - return webhooks.Allow("") + return nil } // If the anchor doesn't exist, we want to allow it to be deleted anyway. @@ -234,64 +276,21 @@ func (v *Validator) cannotDeleteSubnamespace(req *nsRequest) admission.Response anchorExists := v.Forest.Get(parent).HasAnchor(req.ns.Name) if anchorExists { err := fmt.Errorf("is a subnamespace. Please delete the anchor from the parent namespace %s to delete the subnamespace", parent) - return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err) + return apierrors.NewForbidden(namespaceGR, req.ns.Name, err) } - return webhooks.Allow("") + return nil } -func (v *Validator) illegalCascadingDeletion(ns *forest.Namespace) admission.Response { +func (v *Validator) illegalCascadingDeletion(ns *forest.Namespace) error { if ns.AllowsCascadingDeletion() { - return webhooks.Allow("") + return nil } for _, cnm := range ns.ChildNames() { if v.Forest.Get(cnm).IsSub { err := errors.New("contains subnamespaces. Please remove all subnamespaces before deleting this namespace, or set 'allowCascadingDeletion' to delete them automatically") - return webhooks.DenyForbidden(namespaceGR, ns.Name(), err) + return apierrors.NewForbidden(namespaceGR, ns.Name(), err) } } - return webhooks.Allow("no subnamespaces found") -} - -// 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. For Create and Delete, -// the non-empty namespace instance will be put in the `ns` field. Only Update -// request would have a non-empty `oldns` field. -func (v *Validator) decodeRequest(log logr.Logger, in admission.Request) (*nsRequest, error) { - ns := &corev1.Namespace{} - oldns := &corev1.Namespace{} - 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, ns) - } else { - err = v.decoder.Decode(in, ns) - } - if err != nil { - return nil, err - } - - // Get the old namespace instance from an Update request. - if in.Operation == k8sadm.Update { - log.V(1).Info("Decoding an update request.") - if err = v.decoder.DecodeRaw(in.OldObject, oldns); err != nil { - return nil, err - } - } else { - oldns = nil - } - - return &nsRequest{ - ns: ns, - oldns: oldns, - op: in.Operation, - }, nil -} - -func (v *Validator) InjectDecoder(d admission.Decoder) error { - v.decoder = d return nil } diff --git a/internal/namespace/validator_test.go b/internal/namespace/validator_test.go index c55962267..aaa18d9b2 100644 --- a/internal/namespace/validator_test.go +++ b/internal/namespace/validator_test.go @@ -6,7 +6,6 @@ import ( . "github.com/onsi/gomega" k8sadm "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" "sigs.k8s.io/hierarchical-namespaces/internal/config" @@ -58,11 +57,14 @@ func TestDeleteSubNamespace(t *testing.T) { vns := &Validator{Forest: f} // Test - got := vns.handle(req) + err := vns.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()) + } }) } } @@ -84,35 +86,31 @@ func TestDeleteOwnerNamespace(t *testing.T) { } // Test - got := vns.handle(req) + err := vns.handleValidation(req) // Report - Shouldn't allow deleting the parent namespace. - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(BeFalse()) + g.Expect(err).Should(HaveOccurred()) // Set allowCascadingDeletion on one child. b.UpdateAllowCascadingDeletion(true) // Test - got = vns.handle(req) + err = vns.handleValidation(req) // Report - Still shouldn't allow deleting the parent namespace. - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(BeFalse()) + g.Expect(err).Should(HaveOccurred()) // Set allowCascadingDeletion on the other child too. c.UpdateAllowCascadingDeletion(true) // Test - got = vns.handle(req) + err = vns.handleValidation(req) // Report - Shouldn't allow deleting the parent namespace since parent namespace is not set to allow cascading deletion. - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(BeFalse()) + g.Expect(err).Should(HaveOccurred()) // Unset allowCascadingDeletion on one child but set allowCascadingDeletion on the parent itself. c.UpdateAllowCascadingDeletion(false) a.UpdateAllowCascadingDeletion(true) // Test - got = vns.handle(req) + err = vns.handleValidation(req) // Report - Should allow deleting the parent namespace with allowCascadingDeletion set on it. - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(BeTrue()) + g.Expect(err).ShouldNot(HaveOccurred()) }) } @@ -141,11 +139,10 @@ func TestCreateNamespace(t *testing.T) { } // Test - got := vns.handle(req) + err := vns.handleValidation(req) // Report - logResult(t, got.AdmissionResponse.Result) - g.Expect(got.AdmissionResponse.Allowed).Should(BeFalse()) + g.Expect(err).Should(HaveOccurred()) }) } @@ -209,11 +206,14 @@ func TestUpdateNamespaceManagedBy(t *testing.T) { } // Test - got := vns.handle(req) + err := vns.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()) + } }) } } @@ -327,18 +327,18 @@ func TestIllegalIncludedNamespaceNamespace(t *testing.T) { } // Test - got := vns.handle(req) + err := vns.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) -} func TestIllegalTreeOperations(t *testing.T) { f := foresttest.Create("-") vns := &Validator{Forest: f} @@ -379,10 +379,13 @@ func TestIllegalTreeOperations(t *testing.T) { } } - got := vns.illegalTreeLabel(req) + err := vns.illegalTreeLabel(req) - 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()) + } } } diff --git a/internal/setup/reconcilers.go b/internal/setup/reconcilers.go index 7b1419992..afc0b2924 100644 --- a/internal/setup/reconcilers.go +++ b/internal/setup/reconcilers.go @@ -35,7 +35,10 @@ func Create(log logr.Logger, mgr ctrl.Manager, f *forest.Forest, opts Options) { if !opts.NoWebhooks { log.Info("Registering validating webhook (won't work when running locally; use --no-webhooks)") - createWebhooks(mgr, f, opts) + if err := createWebhooks(mgr, f, opts); err != nil { + log.Error(err, "cannot create webhooks") + os.Exit(1) + } } log.Info("Registering reconcilers") diff --git a/internal/setup/webhooks.go b/internal/setup/webhooks.go index 076bdb2d0..f0ff60637 100644 --- a/internal/setup/webhooks.go +++ b/internal/setup/webhooks.go @@ -4,11 +4,14 @@ import ( "fmt" cert "github.com/open-policy-agent/cert-controller/pkg/rotator" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" "sigs.k8s.io/hierarchical-namespaces/internal/anchor" "sigs.k8s.io/hierarchical-namespaces/internal/config" "sigs.k8s.io/hierarchical-namespaces/internal/forest" @@ -57,24 +60,24 @@ func ManageCerts(mgr ctrl.Manager, setupFinished chan struct{}, restartOnSecretR } // createWebhooks creates all mutators and validators. -func createWebhooks(mgr ctrl.Manager, f *forest.Forest, opts Options) { +func createWebhooks(mgr ctrl.Manager, f *forest.Forest, opts Options) error { decoder := admission.NewDecoder(mgr.GetScheme()) // NOTE(ryotarai): The injecting mechanism is removed in https://github.com/kubernetes-sigs/controller-runtime/pull/2134 // For now, the decoder and client are injected manually, but we might want to replace this with sigs.k8s.io/controller-runtime/pkg/builder.WebhookManagedBy // Create webhook for Hierarchy - { - handler := &hierarchyconfig.Validator{ - Log: ctrl.Log.WithName("hierarchyconfig").WithName("validate"), - Forest: f, - } - handler.InjectDecoder(decoder) - handler.InjectClient(mgr.GetClient()) - mgr.GetWebhookServer().Register(hierarchyconfig.ServingPath, &webhook.Admission{Handler: handler}) + if err := builder.WebhookManagedBy(mgr). + For(&api.HierarchyConfiguration{}). + WithCustomPath(hierarchyconfig.ServingPath). + WithValidator(hierarchyconfig.NewValidator(f, mgr.GetClient())). + Complete(); err != nil { + return fmt.Errorf("failed to create webhook for hierarchyconfig: %w", err) } // Create webhooks for managed objects + // Note: We cannot use WebhookManagedBy here because objects validator handles all resource types dynamically (*) + // and WebhookManagedBy requires a specific type to be specified with For(). { handler := &objects.Validator{ Log: ctrl.Log.WithName("objects").WithName("validate"), @@ -86,65 +89,69 @@ func createWebhooks(mgr ctrl.Manager, f *forest.Forest, opts Options) { } // Create webhook for the config - { - handler := &hncconfig.Validator{ - Log: ctrl.Log.WithName("hncconfig").WithName("validate"), - Forest: f, - } - handler.InjectDecoder(decoder) - handler.InjectConfig(mgr.GetConfig()) - mgr.GetWebhookServer().Register(hncconfig.ServingPath, &webhook.Admission{Handler: handler}) + hnconfigValidator := hncconfig.NewValidator(f) + if err := hnconfigValidator.SetupWithManager(mgr); err != nil { + return fmt.Errorf("failed to setup hncconfig validator: %w", err) + } + if err := builder.WebhookManagedBy(mgr). + For(&api.HNCConfiguration{}). + WithCustomPath(hncconfig.ServingPath). + WithValidator(hnconfigValidator). + Complete(); err != nil { + return fmt.Errorf("failed to create webhook for hncconfig: %w", err) } // Create webhook for the subnamespace anchors. - { - handler := &anchor.Validator{ - Log: ctrl.Log.WithName("anchor").WithName("validate"), - Forest: f, - } - handler.InjectDecoder(decoder) - mgr.GetWebhookServer().Register(anchor.ServingPath, &webhook.Admission{Handler: handler}) + anchorValidator := anchor.NewValidator(f) + if err := builder.WebhookManagedBy(mgr). + For(&api.SubnamespaceAnchor{}). + WithCustomPath(anchor.ServingPath). + WithValidator(anchorValidator). + Complete(); err != nil { + return fmt.Errorf("failed to create webhook for anchor: %w", err) } // Create webhook for the namespaces (core type). - { - handler := &ns.Validator{ - Log: ctrl.Log.WithName("namespace").WithName("validate"), - Forest: f, - } - handler.InjectDecoder(decoder) - mgr.GetWebhookServer().Register(ns.ServingPath, &webhook.Admission{Handler: handler}) + nsValidator := ns.NewValidator(f) + if err := builder.WebhookManagedBy(mgr). + For(&corev1.Namespace{}). + WithCustomPath(ns.ServingPath). + WithValidator(nsValidator). + Complete(); err != nil { + return fmt.Errorf("failed to create webhook for namespace: %w", err) } // Create mutator for namespace `included-namespace` label. - { - handler := &ns.Mutator{ - Log: ctrl.Log.WithName("namespace").WithName("mutate"), - } - handler.InjectDecoder(decoder) - mgr.GetWebhookServer().Register(ns.MutatorServingPath, &webhook.Admission{Handler: handler}) + nsMutator := ns.NewMutator() + if err := builder.WebhookManagedBy(mgr). + For(&corev1.Namespace{}). + WithCustomPath(ns.MutatorServingPath). + WithDefaulter(nsMutator). + Complete(); err != nil { + return fmt.Errorf("failed to create mutator webhook for namespace: %w", err) } if opts.HRQ { // Create webhook for ResourceQuota status. - { - handler := &hrq.ResourceQuotaStatus{ - Log: ctrl.Log.WithName("validators").WithName("ResourceQuota"), - Forest: f, - } - handler.InjectDecoder(decoder) - handler.InjectClient(mgr.GetClient()) - mgr.GetWebhookServer().Register(hrq.ResourceQuotasStatusServingPath, &webhook.Admission{Handler: handler}) + rqStatusValidator := hrq.NewResourceQuotaStatus(f, mgr.GetClient()) + if err := builder.WebhookManagedBy(mgr). + For(&corev1.ResourceQuota{}). + WithCustomPath(hrq.ResourceQuotasStatusServingPath). + WithValidator(rqStatusValidator). + Complete(); err != nil { + return fmt.Errorf("failed to create webhook for ResourceQuota status: %w", err) } // Create webhook for HierarchicalResourceQuota spec. - { - handler := &hrq.HRQ{ - Log: ctrl.Log.WithName("validators").WithName("HierarchicalResourceQuota"), - } - handler.InjectDecoder(decoder) - handler.InjectClient(mgr.GetClient()) - mgr.GetWebhookServer().Register(hrq.HRQServingPath, &webhook.Admission{Handler: handler}) + hrqValidator := hrq.NewHRQ(mgr.GetClient()) + if err := builder.WebhookManagedBy(mgr). + For(&api.HierarchicalResourceQuota{}). + WithCustomPath(hrq.HRQServingPath). + WithValidator(hrqValidator). + Complete(); err != nil { + return fmt.Errorf("failed to create webhook for HierarchicalResourceQuota: %w", err) } } + + return nil }