From f3b3e0771e47890f42b42a48381c6f0ab3dac565 Mon Sep 17 00:00:00 2001 From: Yiqi Gao Date: Tue, 18 May 2021 14:48:09 -0400 Subject: [PATCH] Add MWH and update VWH on included-namespace label Part of 9. Fixes 37. Add mutating webhook to set the correct `included-namespace` label on non-excluded namespaces if it's missing. Leave all other situation as is and let the validating webhook to do the validation. Update namespace validator to allow all other changes if the illegal included-namespace label is unchanged. Since namespace mutator ensures the label exists on included namespaces, update rules to ignore the case when the label is missing on the included namespaces. Add old instance field to the namespace validator. Decode it from request to compare with the new instance. Add more test cases since we now have more combination of cases to test. Update the namespace tree label update logs and function name in the HC reconciler to make it clear that the HC reconciler only updates the tree labels and won't update included-namespace label if the MWH works fine. Tested by `make test` with new test cases. Also tested manually by applying namespace manifests with different labels, viewed the logs and verified the MWH and VWH work fine. --- cmd/manager/main.go | 5 ++ config/webhook/manifests.yaml | 33 ++++++-- internal/mutators/namespace.go | 74 ++++++++++++++++++ internal/mutators/namespace_test.go | 49 ++++++++++++ internal/mutators/setup.go | 14 ++++ internal/reconcilers/hierarchy_config.go | 8 +- internal/validators/anchor.go | 2 +- internal/validators/hierarchy.go | 2 +- internal/validators/hncconfig.go | 2 +- internal/validators/namespace.go | 85 ++++++++++++--------- internal/validators/namespace_test.go | 96 ++++++++++++++++++------ internal/validators/object.go | 2 +- internal/validators/setup.go | 4 + 13 files changed, 304 insertions(+), 72 deletions(-) create mode 100644 internal/mutators/namespace.go create mode 100644 internal/mutators/namespace_test.go create mode 100644 internal/mutators/setup.go 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, })