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..0b17203c9 --- /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" +) + +// NamespaceMutatorServingPath is where the mutator will run. Must be kept in +// sync with the kubebuilder markers below. +const ( + NamespaceMutatorServingPath = "/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 Namespace struct { + Log logr.Logger + decoder *admission.Decoder +} + +// Handle implements the mutating webhook. +func (m *Namespace) Handle(ctx context.Context, req admission.Request) admission.Response { + log := m.Log.WithValues("namespace", req.Name) + ns := &corev1.Namespace{} + err := m.decoder.Decode(req, ns) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + m.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 (m *Namespace) 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 (m *Namespace) InjectDecoder(d *admission.Decoder) error { + m.decoder = d + return nil +} diff --git a/internal/mutators/namespace_test.go b/internal/mutators/namespace_test.go new file mode 100644 index 000000000..1c1755d74 --- /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 := &Namespace{} + 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..bbc62c3e7 --- /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(NamespaceMutatorServingPath, &webhook.Admission{Handler: &Namespace{ + Log: ctrl.Log.WithName("mutators").WithName("Namespace"), + }}) +} 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..e0b36b523 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,9 @@ type Namespace struct { // nsRequest defines the aspects of the admission.Request that we care about. type nsRequest struct { - ns *corev1.Namespace - op k8sadm.Operation + ns *corev1.Namespace + oldns *corev1.Namespace + op k8sadm.Operation } // Handle implements the validation webhook. @@ -107,22 +108,20 @@ 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. func (v *Namespace) illegalIncludedNamespaceLabel(req *nsRequest) admission.Response { + // 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 allow("") + } + } 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. + // An excluded namespaces should not have included-namespace 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 @@ -130,25 +129,22 @@ func (v *Namespace) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp 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" { + // An included-namespace should have the included-namespace label with the + // right value. + // 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" { // 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) + 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. 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 { @@ -159,6 +155,10 @@ func (v *Namespace) nameExistsInExternalHierarchy(req *nsRequest) admission.Resp 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. func (v *Namespace) conflictBetweenParentAndExternalManager(req *nsRequest, ns *forest.Namespace) admission.Response { mgr := req.ns.Annotations[api.AnnotationManagedBy] if mgr != "" && mgr != api.MetaGroup && ns.Parent() != nil { @@ -169,6 +169,7 @@ func (v *Namespace) conflictBetweenParentAndExternalManager(req *nsRequest, ns * return allow("") } +// cannotDeleteSubnamespace only applies to the Delete operation. func (v *Namespace) cannotDeleteSubnamespace(req *nsRequest) admission.Response { parent := req.ns.Annotations[api.SubnamespaceOf] // Early exit if the namespace is not a subnamespace. @@ -200,19 +201,18 @@ func (v *Namespace) illegalCascadingDeletion(ns *forest.Namespace) admission.Res return 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. +// 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 *Namespace) 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. + + // 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 { - 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. - return nil, nil - } log.V(1).Info("Decoding a delete request.") err = v.decoder.DecodeRaw(in.OldObject, ns) } else { @@ -222,9 +222,20 @@ func (v *Namespace) decodeRequest(log logr.Logger, in admission.Request) (*nsReq 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, - op: in.Operation, + ns: ns, + oldns: oldns, + op: in.Operation, }, nil } diff --git a/internal/validators/namespace_test.go b/internal/validators/namespace_test.go index 050ab5b91..b44aec66a 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" ) @@ -236,42 +236,94 @@ 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: "deny creating included namespace without label (this case should never happen in reality since mutator adds the correct label)", op: k8sadm.Create, fail: true}, + {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" } + + ns := &corev1.Namespace{} + ns.Name = nnm if tc.lval != "" { - nsInst.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.lval}) + ns.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.lval}) } + oldns := &corev1.Namespace{} + if tc.op == k8sadm.Update { + oldns.Name = nnm + if tc.oldlval != "" { + oldns.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.oldlval}) + } + } else { + oldns = nil + } + req := &nsRequest{ - ns: nsInst, - op: tc.op, + oldns: oldns, + ns: ns, + 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, })