diff --git a/cmd/manager/main.go b/cmd/manager/main.go index ebb685244..40b006820 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -41,6 +41,7 @@ import ( v1a2 "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" "sigs.k8s.io/hierarchical-namespaces/internal/config" "sigs.k8s.io/hierarchical-namespaces/internal/forest" + "sigs.k8s.io/hierarchical-namespaces/internal/mutators" "sigs.k8s.io/hierarchical-namespaces/internal/reconcilers" "sigs.k8s.io/hierarchical-namespaces/internal/stats" "sigs.k8s.io/hierarchical-namespaces/internal/validators" @@ -213,6 +214,10 @@ func startControllers(mgr ctrl.Manager, certsCreated chan struct{}) { validators.Create(mgr, f) } + // Create mutating admission controllers. + setupLog.Info("Registering mutating webhook") + mutators.Create(mgr) + // Create all reconciling controllers setupLog.Info("Creating controllers", "maxReconciles", maxReconciles) if err := reconcilers.Create(mgr, f, maxReconciles, false); err != nil { diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index ef9d91a2d..dbc4cb14d 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -1,4 +1,32 @@ +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: MutatingWebhookConfiguration +metadata: + creationTimestamp: null + name: mutating-webhook-configuration +webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-namespace + failurePolicy: Ignore + name: namespacelabel.hnc.x-k8s.io + rules: + - apiGroups: + - "" + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - namespaces + sideEffects: None + --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration @@ -8,7 +36,6 @@ metadata: webhooks: - admissionReviewVersions: - v1 - - v1beta1 clientConfig: service: name: webhook-service @@ -29,7 +56,6 @@ webhooks: sideEffects: None - admissionReviewVersions: - v1 - - v1beta1 clientConfig: service: name: webhook-service @@ -50,7 +76,6 @@ webhooks: sideEffects: None - admissionReviewVersions: - v1 - - v1beta1 clientConfig: service: name: webhook-service @@ -72,7 +97,6 @@ webhooks: sideEffects: None - admissionReviewVersions: - v1 - - v1beta1 clientConfig: service: name: webhook-service @@ -94,7 +118,6 @@ webhooks: sideEffects: None - admissionReviewVersions: - v1 - - v1beta1 clientConfig: service: name: webhook-service diff --git a/internal/mutators/namespace.go b/internal/mutators/namespace.go new file mode 100644 index 000000000..5f2e705b1 --- /dev/null +++ b/internal/mutators/namespace.go @@ -0,0 +1,74 @@ +package mutators + +import ( + "context" + "encoding/json" + "net/http" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" + "sigs.k8s.io/hierarchical-namespaces/internal/config" +) + +// NamespaceIncludedLabelServingPath is where the mutator will run. Must be kept +// in sync with the kubebuilder markers below. +const ( + NamespaceIncludedLabelServingPath = "/mutate-namespace" +) + +// Note: the mutating webhook FAILS OPEN. This means that if the webhook goes +// down, all further changes are allowed. (An empty line has to be kept below +// the kubebuilder marker for the controller-gen to generate manifests.) +// +// +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 NamespaceIncludedLabel struct { + Log logr.Logger + decoder *admission.Decoder +} + +// Handle implements the mutating webhook. +func (n *NamespaceIncludedLabel) Handle(ctx context.Context, req admission.Request) admission.Response { + log := n.Log.WithValues("namespace", req.Name) + ns := &corev1.Namespace{} + err := n.decoder.Decode(req, ns) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + n.handle(log, ns) + marshaledNS, err := json.Marshal(ns) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } + return admission.PatchResponseFromRaw(req.Object.Raw, marshaledNS) +} + +// handle 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 (n *NamespaceIncludedLabel) handle(log logr.Logger, ns *corev1.Namespace) { + // Early exit if the namespace is excluded. + if config.ExcludedNamespaces[ns.Name] { + return + } + + // Add label if the namespace doesn't have it. + if _, hasLabel := ns.Labels[api.LabelIncludedNamespace]; !hasLabel { + if ns.Labels == nil { + ns.Labels = map[string]string{} + } + log.Info("Not an excluded namespace; set included-namespace label.") + ns.Labels[api.LabelIncludedNamespace] = "true" + } +} + +// InjectDecoder injects the decoder. +func (n *NamespaceIncludedLabel) InjectDecoder(d *admission.Decoder) error { + n.decoder = d + return nil +} diff --git a/internal/mutators/namespace_test.go b/internal/mutators/namespace_test.go new file mode 100644 index 000000000..5f009fadd --- /dev/null +++ b/internal/mutators/namespace_test.go @@ -0,0 +1,49 @@ +package mutators + +import ( + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" + "sigs.k8s.io/hierarchical-namespaces/internal/config" +) + +func TestMutateNamespaceIncludedLabel(t *testing.T) { + m := &NamespaceIncludedLabel{} + l := zap.New() + config.ExcludedNamespaces = map[string]bool{"excluded": true} + + tests := []struct { + name string + nsn string + lval string + expectlval string + }{ + {name: "Set label on non-excluded namespace without label", nsn: "a", expectlval: "true"}, + {name: "No operation on non-excluded namespace with label (e.g. from a yaml file)", nsn: "a", lval: "true", expectlval: "true"}, + {name: "No operation on non-excluded namespace with label with wrong value(e.g. from a yaml file)", nsn: "a", lval: "wrong", expectlval: "wrong"}, + {name: "No operation on excluded namespace without label", nsn: "excluded"}, + {name: "No operation on excluded namespace with label (e.g. from a yaml file)", nsn: "excluded", lval: "true", expectlval: "true"}, + {name: "No operation on excluded namespace with label with wrong value (e.g. from a yaml file)", nsn: "excluded", lval: "wrong", expectlval: "wrong"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + nsInst := &corev1.Namespace{} + nsInst.Name = tc.nsn + if tc.lval != "" { + nsInst.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.lval}) + } + + // Test + m.handle(l, nsInst) + + // Report + g.Expect(nsInst.Labels[api.LabelIncludedNamespace]).Should(Equal(tc.expectlval)) + }) + } +} diff --git a/internal/mutators/setup.go b/internal/mutators/setup.go new file mode 100644 index 000000000..51be1ba0f --- /dev/null +++ b/internal/mutators/setup.go @@ -0,0 +1,14 @@ +package mutators + +import ( + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook" +) + +// Create creates the mutator. This function is called from main.go. +func Create(mgr ctrl.Manager) { + // Create mutator for namespace `included-namespace` label. + mgr.GetWebhookServer().Register(NamespaceIncludedLabelServingPath, &webhook.Admission{Handler: &NamespaceIncludedLabel{ + Log: ctrl.Log.WithName("mutators").WithName("NamespaceIncludedLabel"), + }}) +} diff --git a/internal/reconcilers/hierarchy_config.go b/internal/reconcilers/hierarchy_config.go index 9a7ffbe46..9e5c09374 100644 --- a/internal/reconcilers/hierarchy_config.go +++ b/internal/reconcilers/hierarchy_config.go @@ -298,7 +298,7 @@ func (r *HierarchyConfigReconciler) syncWithForest(log logr.Logger, nsInst *core r.syncConditions(log, inst, ns, deletingCRD, hadCrit) // Sync the tree labels. This should go last since it can depend on the conditions. - nsCustomerLabelUpdated := r.syncLabel(log, nsInst, ns) + nsCustomerLabelUpdated := r.syncTreeLabels(log, nsInst, ns) return initial || nsCustomerLabelUpdated } @@ -461,8 +461,8 @@ func (r *HierarchyConfigReconciler) syncAnchors(log logr.Logger, ns *forest.Name } } -// Sync namespace tree labels and other labels. Return true if the labels are updated. -func (r *HierarchyConfigReconciler) syncLabel(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) bool { +// Sync namespace tree labels. Return true if the labels are updated. +func (r *HierarchyConfigReconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) bool { if ns.IsExternal() { metadata.SetLabel(nsInst, nsInst.Name+api.LabelTreeDepthSuffix, "0") return false @@ -504,7 +504,7 @@ func (r *HierarchyConfigReconciler) syncLabel(log logr.Logger, nsInst *corev1.Na // Update the labels in the forest so that we can quickly access the labels and // compare if they match the given selector if ns.SetLabels(nsInst.Labels) { - log.Info("Namespace labels have been updated.") + log.Info("Namespace tree labels have been updated.") return true } return false diff --git a/internal/validators/anchor.go b/internal/validators/anchor.go index 9a5e68d55..6ea3991fd 100644 --- a/internal/validators/anchor.go +++ b/internal/validators/anchor.go @@ -23,7 +23,7 @@ const ( // Note: the validating webhook FAILS CLOSE. This means that if the webhook goes down, all further // changes are forbidden. // -// +kubebuilder:webhook:admissionReviewVersions=v1;v1beta1,path=/validate-hnc-x-k8s-io-v1alpha2-subnamespaceanchors,mutating=false,failurePolicy=fail,groups="hnc.x-k8s.io",resources=subnamespaceanchors,sideEffects=None,verbs=create;delete,versions=v1alpha2,name=subnamespaceanchors.hnc.x-k8s.io +// +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;delete,versions=v1alpha2,name=subnamespaceanchors.hnc.x-k8s.io type Anchor struct { Log logr.Logger diff --git a/internal/validators/hierarchy.go b/internal/validators/hierarchy.go index 750e53cae..fc507fd90 100644 --- a/internal/validators/hierarchy.go +++ b/internal/validators/hierarchy.go @@ -34,7 +34,7 @@ const ( // changes to the hierarchy are forbidden. However, new objects will still be propagated according // to the existing hierarchy (unless the reconciler is down too). // -// +kubebuilder:webhook:admissionReviewVersions=v1;v1beta1,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 +// +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 Hierarchy struct { Log logr.Logger diff --git a/internal/validators/hncconfig.go b/internal/validators/hncconfig.go index dd0788a74..49e4b9ed9 100644 --- a/internal/validators/hncconfig.go +++ b/internal/validators/hncconfig.go @@ -28,7 +28,7 @@ const ( // Note: the validating webhook FAILS CLOSE. This means that if the webhook goes down, all further // changes are denied. // -// +kubebuilder:webhook:admissionReviewVersions=v1;v1beta1,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 +// +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 HNCConfig struct { Log logr.Logger diff --git a/internal/validators/namespace.go b/internal/validators/namespace.go index 8d76672ee..43b87bbc4 100644 --- a/internal/validators/namespace.go +++ b/internal/validators/namespace.go @@ -24,7 +24,7 @@ const ( // Note: the validating webhook FAILS CLOSE. This means that if the webhook goes down, all further // changes are forbidden. // -// +kubebuilder:webhook:admissionReviewVersions=v1;v1beta1,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 +// +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 Namespace struct { Log logr.Logger @@ -34,8 +34,10 @@ type Namespace struct { // nsRequest defines the aspects of the admission.Request that we care about. type nsRequest struct { - ns *corev1.Namespace - op k8sadm.Operation + nnm string + newns *corev1.Namespace + oldns *corev1.Namespace + op k8sadm.Operation } // Handle implements the validation webhook. @@ -72,7 +74,7 @@ func (v *Namespace) handle(req *nsRequest) admission.Response { v.Forest.Lock() defer v.Forest.Unlock() - ns := v.Forest.Get(req.ns.Name) + ns := v.Forest.Get(req.nnm) switch req.op { case k8sadm.Create: @@ -107,60 +109,71 @@ func (v *Namespace) handle(req *nsRequest) admission.Response { } // illegalIncludedNamespaceLabel checks if there's any illegal use of the -// included-namespace label on namespaces. -// TODO these validating rules will be removed and converted into a mutating -// webhook. See https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/37 +// included-namespace label on namespaces. It only checks a Create or an Update +// request, so the newns is not empty in the request. func (v *Namespace) illegalIncludedNamespaceLabel(req *nsRequest) admission.Response { - labelValue, hasLabel := req.ns.Labels[api.LabelIncludedNamespace] - isExcluded := config.ExcludedNamespaces[req.ns.Name] - - // An excluded namespace should not have the included-namespace label. - // - // Note: this only blocks the request if the excluded namespace is newly - // created or is updated with a newly added included-namespace label because - // existing illegal included-namespace label should have already been removed - // by our reconciler. For example, even when the VWHConfiguration is removed, - // adding the label to an excluded namespace would pass but the label is - // immediately removed; when the VWHConfiguration is there but the reconcilers - // are down, any request gets denied anyway. + labelValue, hasLabel := req.newns.Labels[api.LabelIncludedNamespace] + var oldLabelValue string + var oldHasLabel bool + if req.oldns != nil { + oldLabelValue, oldHasLabel = req.oldns.Labels[api.LabelIncludedNamespace] + } + isExcluded := config.ExcludedNamespaces[req.newns.Name] + + // An excluded namespaces should not have included-namespace label, but we + // only block illegal requests if there's an illegal change on the label. We + // won't block any other namespace changes if the namespace already has the + // illegal label. if isExcluded && hasLabel { - // Todo add label concept and add `See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label for detail` - // to the message below. Github issue - https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/9 - msg := fmt.Sprintf("You cannot enforce webhook rules on this excluded namespace using the %q label.", api.LabelIncludedNamespace) - return deny(metav1.StatusReasonForbidden, msg) + // Block the request only if the excluded namespace is newly created or is + // updated with an included-namespace label. Although any existing illegal + // included-namespace label should have already been removed by our + // reconciler, we want to decouple this from reconciler behaviour. + if oldHasLabel && labelValue != oldLabelValue || !oldHasLabel { + // Todo add label concept and add `See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label for detail` + // to the message below. Github issue - https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/9 + msg := fmt.Sprintf("You cannot enforce webhook rules on this excluded namespace using the %q label.", api.LabelIncludedNamespace) + return deny(metav1.StatusReasonForbidden, msg) + } } - // An included-namespace should not have the included-namespace label with a - // value other than "true" at any time. We will allow updating an included - // namespace by removing its included-namespace label, since people or system - // may apply namespace manifests (without the label) automatically and it - // doesn't make sense to block their attempts after the first time. - // - // Note: this only blocks the request if the included namespace is just - // created or updated with a wrong value for the included-namespace label, - // because our reconciler should already set it correctly before the request. - if !isExcluded && hasLabel && labelValue != "true" { - // Todo add label concept and add `See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label for detail` - // to the message below. Github issue - https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/9 - msg := fmt.Sprintf("You cannot unset the value of the %q label. Only HNC can edit this label on namespaces.", api.LabelIncludedNamespace) - return deny(metav1.StatusReasonForbidden, msg) + // An included-namespace should have the included-namespace label with the + // right value, but we only block illegal requests if there's an illegal + // change on the label. We won't block any other namespace changes if the + // the label is already illegal. + // Note: since we have a mutating webhook to set the correct label if it's + // missing before this, we only need to check if the label value is correct. + if !isExcluded && labelValue != "true" { + // Blocks only if the label is changed. + if oldLabelValue != labelValue { + // Todo add label concept and add `See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label for detail` + // to the message below. Github issue - https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/9 + msg := fmt.Sprintf("You cannot change the value of the %q label. It has to be set as true on a non-excluded namespace.", api.LabelIncludedNamespace) + return deny(metav1.StatusReasonForbidden, msg) + } } return allow("") } +// nameExistsInExternalHierarchy only applies to the Create operation since +// namespace name cannot be updated. Therefore newns won't be nil in the request. func (v *Namespace) nameExistsInExternalHierarchy(req *nsRequest) admission.Response { for _, nm := range v.Forest.GetNamespaceNames() { - if _, ok := v.Forest.Get(nm).ExternalTreeLabels[req.ns.Name]; ok { - msg := fmt.Sprintf("The namespace name %q is reserved by the external hierarchy manager %q.", req.ns.Name, v.Forest.Get(nm).Manager) + if _, ok := v.Forest.Get(nm).ExternalTreeLabels[req.newns.Name]; ok { + msg := fmt.Sprintf("The namespace name %q is reserved by the external hierarchy manager %q.", req.newns.Name, v.Forest.Get(nm).Manager) return deny(metav1.StatusReasonAlreadyExists, msg) } } return allow("") } +// 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. Therefore newns won't be nil in the request. func (v *Namespace) conflictBetweenParentAndExternalManager(req *nsRequest, ns *forest.Namespace) admission.Response { - mgr := req.ns.Annotations[api.AnnotationManagedBy] + mgr := req.newns.Annotations[api.AnnotationManagedBy] if mgr != "" && mgr != api.MetaGroup && ns.Parent() != nil { msg := fmt.Sprintf("Namespace %q 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.Name(), ns.Parent().Name(), mgr) @@ -169,8 +182,10 @@ func (v *Namespace) conflictBetweenParentAndExternalManager(req *nsRequest, ns * return allow("") } +// cannotDeleteSubnamespace only applies to the Delete operation, so the oldns +// won't be nil in the request. func (v *Namespace) cannotDeleteSubnamespace(req *nsRequest) admission.Response { - parent := req.ns.Annotations[api.SubnamespaceOf] + parent := req.oldns.Annotations[api.SubnamespaceOf] // Early exit if the namespace is not a subnamespace. if parent == "" { return allow("") @@ -178,9 +193,9 @@ func (v *Namespace) cannotDeleteSubnamespace(req *nsRequest) admission.Response // If the anchor doesn't exist, we want to allow it to be deleted anyway. // See issue https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/847. - anchorExists := v.Forest.Get(parent).HasAnchor(req.ns.Name) + anchorExists := v.Forest.Get(parent).HasAnchor(req.oldns.Name) if anchorExists { - msg := fmt.Sprintf("The namespace %s is a subnamespace. Please delete the anchor from the parent namespace %s to delete the subnamespace.", req.ns.Name, parent) + msg := fmt.Sprintf("The namespace %s is a subnamespace. Please delete the anchor from the parent namespace %s to delete the subnamespace.", req.oldns.Name, parent) return deny(metav1.StatusReasonForbidden, msg) } return allow("") @@ -203,28 +218,41 @@ func (v *Namespace) illegalCascadingDeletion(ns *forest.Namespace) admission.Res // 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 *Namespace) decodeRequest(log logr.Logger, in admission.Request) (*nsRequest, error) { - ns := &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 { + newns := &corev1.Namespace{} + oldns := &corev1.Namespace{} + var nnm string + + // Get the old namespace instance for a non-create request, because the old + // namespace instance is empty for a create request. + if in.Operation != k8sadm.Create { + // Use DecodeRaw() from req.OldObject, since Decode() only uses req.Object. if in.OldObject.Raw == nil { - // See https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/688. OldObject can be nil in - // K8s 1.14 and earlier. + // See https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/688. + // OldObject can be nil in K8s 1.14 and earlier. return nil, nil } - log.V(1).Info("Decoding a delete request.") - err = v.decoder.DecodeRaw(in.OldObject, ns) - } else { - err = v.decoder.Decode(in, ns) + log.V(1).Info("Decoding a non-create request to get the old namespace instance.", "operation", in.Operation) + if err := v.decoder.DecodeRaw(in.OldObject, oldns); err != nil { + return nil, err + } + nnm = oldns.Name } - if err != nil { - return nil, err + + // Get the new namespace instance for a non-delete request, because the new + // namespace instance is empty for a delete request. + if in.Operation != k8sadm.Delete { + log.V(1).Info("Decoding a non-delete request to get the get namespace instance.", "operation", in.Operation) + if err := v.decoder.Decode(in, newns); err != nil { + return nil, err + } + nnm = oldns.Name } return &nsRequest{ - ns: ns, - op: in.Operation, + nnm: nnm, + newns: newns, + oldns: oldns, + op: in.Operation, }, nil } diff --git a/internal/validators/namespace_test.go b/internal/validators/namespace_test.go index 050ab5b91..1b3b2eb1f 100644 --- a/internal/validators/namespace_test.go +++ b/internal/validators/namespace_test.go @@ -6,9 +6,9 @@ import ( . "github.com/onsi/gomega" k8sadm "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/hierarchical-namespaces/internal/config" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" + "sigs.k8s.io/hierarchical-namespaces/internal/config" "sigs.k8s.io/hierarchical-namespaces/internal/foresttest" ) @@ -39,13 +39,15 @@ func TestDeleteSubNamespace(t *testing.T) { g := NewWithT(t) // Create a namespace instance a and add the subnamespace-of annotation. + nnm := "a" sub := &corev1.Namespace{} - sub.Name = "a" + sub.Name = nnm setSubAnnotation(sub, tc.subnamespaceOf) req := &nsRequest{ - ns: sub, - op: k8sadm.Delete, + nnm: nnm, + oldns: sub, + op: k8sadm.Delete, } // Construct the forest @@ -70,17 +72,19 @@ func TestDeleteSubNamespace(t *testing.T) { func TestDeleteOwnerNamespace(t *testing.T) { f := foresttest.Create("-AA") vns := &Namespace{Forest: f} - a := f.Get("a") + nnm := "a" + a := f.Get(nnm) aInst := &corev1.Namespace{} - aInst.Name = "a" + aInst.Name = nnm b := f.Get("b") c := f.Get("c") t.Run("Delete a namespace with subnamespaces", func(t *testing.T) { g := NewWithT(t) req := &nsRequest{ - ns: aInst, - op: k8sadm.Delete, + nnm: nnm, + oldns: aInst, + op: k8sadm.Delete, } // Test @@ -136,8 +140,9 @@ func TestCreateNamespace(t *testing.T) { t.Run("Create namespace with an already existing name in external hierarchy", func(t *testing.T) { g := NewWithT(t) req := &nsRequest{ - ns: ns, - op: k8sadm.Create, + nnm: nm, + newns: ns, + op: k8sadm.Create, } // Test @@ -204,8 +209,9 @@ func TestUpdateNamespaceManagedBy(t *testing.T) { } req := &nsRequest{ - ns: tc.nsInst, - op: k8sadm.Update, + nnm: tc.nsInst.Name, + newns: tc.nsInst, + op: k8sadm.Update, } // Test @@ -236,42 +242,98 @@ func TestIllegalIncludedNamespaceNamespace(t *testing.T) { tests := []struct { name string excludedNS bool + oldlval string lval string op k8sadm.Operation fail bool }{ - {name: "allow creating non-excluded namespace without label", op: k8sadm.Create}, - {name: "allow creating non-excluded namespace with label (e.g. from a yaml file)", lval: "true", op: k8sadm.Create}, - {name: "deny creating non-excluded namespace with label with wrong value(e.g. from a yaml file)", lval: "else", op: k8sadm.Create, fail: true}, + // Note: for all the test cases below, you can consider without label as 0, + // with correct label as 1 and with wrong label as 2, so the test cases are + // generated as 0->0, 0->1, 0->2; 1->0, 1->1, 1->2; 2->0, 2->1, 2->2. The + // test cases are divided into 3 blocks - Create, Update and Delete. There's + // no old namespace instance for Create and there's no new namespace instance + // for Delete. + + // Create operations on included namespaces. + {name: "allow creating included namespace without label (this case should never happen in reality since mutator adds the correct label)", op: k8sadm.Create}, + {name: "allow creating included namespace with correct label (e.g. from a yaml file or added by mutator)", lval: "true", op: k8sadm.Create}, + {name: "deny creating included namespace with wrong label(e.g. from a yaml file)", lval: "wrong", op: k8sadm.Create, fail: true}, + + // Create operations on excluded namespaces. {name: "allow creating excluded namespace without label", excludedNS: true, op: k8sadm.Create}, - {name: "deny creating excluded namespace with label", excludedNS: true, lval: "true", op: k8sadm.Create, fail: true}, - - {name: "allow updating non-excluded namespace with correct label (which is actually set by HC reconciler)", lval: "true", op: k8sadm.Update}, - {name: "allow updating non-excluded namespace without label", op: k8sadm.Update}, - {name: "deny updating non-excluded namespace with incorrect label", lval: "else", op: k8sadm.Update, fail: true}, - {name: "allow updating excluded namespace without label", excludedNS: true, op: k8sadm.Update}, - {name: "deny updating excluded namespace with label", excludedNS: true, lval: "true", op: k8sadm.Update, fail: true}, - - {name: "allow deleting non-excluded namespace without label", op: k8sadm.Delete}, - {name: "allow deleting non-excluded namespace with label", lval: "true", op: k8sadm.Delete}, - {name: "allow creating excluded namespace without label", excludedNS: true, op: k8sadm.Delete}, - {name: "allow creating excluded namespace with label", excludedNS: true, lval: "true", op: k8sadm.Delete}, + {name: "deny creating excluded namespace with correct label", excludedNS: true, lval: "true", op: k8sadm.Create, fail: true}, + {name: "deny creating excluded namespace with wrong label", excludedNS: true, lval: "wrong", op: k8sadm.Create, fail: true}, + + // Update operations on included namespaces. + {name: "allow other updates on included namespace without label (this case should never happen with mutator adding the correct label)", op: k8sadm.Update}, + {name: "allow adding correct label to included namespace", lval: "true", op: k8sadm.Update}, + {name: "deny adding wrong label to included namespace", lval: "wrong", op: k8sadm.Update, fail: true}, + + {name: "deny removing the correct label from included namespace (this case should never happen in reality since mutator adds the correct label)", oldlval: "true", op: k8sadm.Update, fail: true}, + {name: "allow other updates on included namespace with correct label or removing the correct label (mutator adds it back)", oldlval: "true", lval: "true", op: k8sadm.Update}, + {name: "deny updating included namespace to have wrong label", oldlval: "true", lval: "wrong", op: k8sadm.Update, fail: true}, + + {name: "deny removing wrong label from included namespace (this case should never happen in reality since mutator adds the correct label)", oldlval: "wrong", op: k8sadm.Update, fail: true}, + {name: "allow updating included namespace to have correct label or just removing wrong label (mutator adds correct label)", oldlval: "wrong", lval: "true", op: k8sadm.Update}, + {name: "allow other updates on included namespace even with wrong label", oldlval: "wrong", lval: "wrong", op: k8sadm.Update}, + {name: "deny updating included namespace wrong label to another wrong label", oldlval: "wrong1", lval: "wrong2", op: k8sadm.Update, fail: true}, + + // Update operations on excluded namespaces. + {name: "allow other updates on excluded namespace without label", excludedNS: true, op: k8sadm.Update}, + {name: "deny adding illegal label (with correct value) to excluded namespace", excludedNS: true, lval: "true", op: k8sadm.Update, fail: true}, + {name: "deny adding illegal label (with wrong value) to excluded namespace", excludedNS: true, lval: "wrong", op: k8sadm.Update, fail: true}, + + {name: "allow removing illegal label (with correct value) from excluded namespace", excludedNS: true, oldlval: "true", op: k8sadm.Update}, + {name: "allow other updates on excluded namespace with illegal label (with correct value)", excludedNS: true, oldlval: "true", lval: "true", op: k8sadm.Update}, + {name: "deny updating illegal label value (correct to wrong value) on excluded namespace", excludedNS: true, oldlval: "true", lval: "wrong", op: k8sadm.Update, fail: true}, + + {name: "allow removing illegal label (with wrong value) from excluded namespace", excludedNS: true, oldlval: "wrong", op: k8sadm.Update}, + {name: "deny updating illegal label value (wrong to correct value) on excluded namespace", excludedNS: true, oldlval: "wrong", lval: "true", op: k8sadm.Update, fail: true}, + {name: "allow other updates on excluded namespace with illegal label (with wrong value)", excludedNS: true, oldlval: "wrong", lval: "wrong", op: k8sadm.Update}, + {name: "deny updating illegal label value (wrong to another wrong value) on excluded namespace", excludedNS: true, oldlval: "wrong1", lval: "wrong2", op: k8sadm.Update, fail: true}, + + // Delete operations on included namespaces. + {name: "allow deleting included namespace without label", op: k8sadm.Delete}, + {name: "allow deleting included namespace with label", oldlval: "true", op: k8sadm.Delete}, + {name: "allow deleting included namespace with wrong label", oldlval: "wrong", op: k8sadm.Delete}, + + // Delete operations on excluded namespaces. + {name: "allow deleting excluded namespace without label", excludedNS: true, op: k8sadm.Delete}, + {name: "allow deleting excluded namespace with label", excludedNS: true, oldlval: "true", op: k8sadm.Delete}, + {name: "allow deleting excluded namespace with wrong label", excludedNS: true, oldlval: "wrong", op: k8sadm.Delete}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - nsInst := &corev1.Namespace{} - nsInst.Name = "a" + nnm := "a" if tc.excludedNS { - nsInst.Name = "excluded" + nnm = "excluded" } + + newns := &corev1.Namespace{} + newns.Name = nnm if tc.lval != "" { - nsInst.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.lval}) + newns.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.lval}) } + oldns := &corev1.Namespace{} + oldns.Name = nnm + if tc.oldlval != "" { + oldns.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.oldlval}) + } + + if tc.op == k8sadm.Create { + oldns = nil + } + if tc.op == k8sadm.Delete { + newns = nil + } + req := &nsRequest{ - ns: nsInst, - op: tc.op, + nnm: nnm, + oldns: oldns, + newns: newns, + op: tc.op, } // Test diff --git a/internal/validators/object.go b/internal/validators/object.go index a55a47209..510ecdff9 100644 --- a/internal/validators/object.go +++ b/internal/validators/object.go @@ -40,7 +40,7 @@ const ( // file if you want to change the webhook `rules` and better make the rules // here the same as what's in the webhook_patch.yaml. // -// +kubebuilder:webhook:admissionReviewVersions=v1;v1beta1,path=/validate-objects,mutating=false,failurePolicy=fail,groups="*",resources="*",sideEffects=None,verbs=create;update;delete,versions="*",name=objects.hnc.x-k8s.io +// +kubebuilder:webhook:admissionReviewVersions=v1,path=/validate-objects,mutating=false,failurePolicy=fail,groups="*",resources="*",sideEffects=None,verbs=create;update;delete,versions="*",name=objects.hnc.x-k8s.io type Object struct { Log logr.Logger diff --git a/internal/validators/setup.go b/internal/validators/setup.go index 5503a1e87..55e578e08 100644 --- a/internal/validators/setup.go +++ b/internal/validators/setup.go @@ -14,6 +14,7 @@ import ( const ( serviceName = "hnc-webhook-service" vwhName = "hnc-validating-webhook-configuration" + mwhName = "hnc-mutating-webhook-configuration" caName = "hnc-ca" caOrganization = "hnc" secretNamespace = "hnc-system" @@ -45,6 +46,9 @@ func CreateCertsIfNeeded(mgr ctrl.Manager, novalidation, internalCert, restartOn Webhooks: []cert.WebhookInfo{{ Type: cert.Validating, Name: vwhName, + }, { + Type: cert.Mutating, + Name: mwhName, }}, RestartOnSecretRefresh: restartOnSecretRefresh, })