fix: avoid rejection when users are classified as administrators (#1941)

* fix(controller): decode old object for delete requests

Signed-off-by: Oliver Bähler <oliverbaehler@hotmail.com>

* chore: modernize golang

Signed-off-by: Oliver Bähler <oliverbaehler@hotmail.com>

* chore: modernize golang

Signed-off-by: Oliver Bähler <oliverbaehler@hotmail.com>

* chore: modernize golang

Signed-off-by: Oliver Bähler <oliverbaehler@hotmail.com>

* fix: avoid rejection when users are classified as administrators

Signed-off-by: Oliver Baehler <oliver@sudo-i.net>

---------

Signed-off-by: Oliver Bähler <oliverbaehler@hotmail.com>
Signed-off-by: Oliver Baehler <oliver@sudo-i.net>

charts/capsule/templates/configuration.yaml

@@ -67,7 +67,7 @@ spec:
       {{- with .Values.webhooks.hooks.namespaces }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: namespaces.projectcapsule.dev
+      - name: namespaces.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -111,7 +111,7 @@ spec:
       {{- with .Values.webhooks.hooks.customresources }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: customresources.projectcapsule.dev
+      - name: customresources.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -153,7 +153,7 @@ spec:
       {{- with .Values.webhooks.hooks.cordoning }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: cordoning.projectcapsule.dev
+      - name: cordoning.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -187,7 +187,7 @@ spec:
       {{- with .Values.webhooks.hooks.devices }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: devices.projectcapsule.dev
+      - name: devices.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -229,7 +229,7 @@ spec:
       {{- with .Values.webhooks.hooks.gateways }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: gateway.projectcapsule.dev
+      - name: gateway.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -270,7 +270,7 @@ spec:
       {{- with .Values.webhooks.hooks.ingresses }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: ingress.projectcapsule.dev
+      - name: ingress.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -313,7 +313,7 @@ spec:
       {{- with .Values.webhooks.hooks.nodes }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: nodes.projectcapsule.dev
+      - name: nodes.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -352,7 +352,7 @@ spec:
       {{- with .Values.webhooks.hooks.pods }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: pods.projectcapsule.dev
+      - name: pods.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -394,7 +394,7 @@ spec:
       {{- with .Values.webhooks.hooks.persistentvolumeclaims }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: pvc.projectcapsule.dev
+      - name: pvc.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -435,7 +435,7 @@ spec:
       {{- with .Values.webhooks.hooks.services }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: services.projectcapsule.dev
+      - name: services.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -476,7 +476,7 @@ spec:
       {{- with (mergeOverwrite .Values.webhooks.hooks.managed .Values.webhooks.hooks.tenantResourceObjects) }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: managed.tenant.projectcapsule.dev
+      - name: managed.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -507,7 +507,7 @@ spec:
       {{- with  .Values.webhooks.hooks.replications }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: replications.projectcapsule.dev
+      - name: replications.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -538,7 +538,7 @@ spec:
       {{- with .Values.webhooks.hooks.tenants }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: tenants.projectcapsule.dev
+      - name: tenants.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -580,7 +580,7 @@ spec:
       {{- with .Values.webhooks.hooks.resourcepools.pools }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: resourcepools.projectcapsule.dev
+      - name: resourcepools.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -621,7 +621,7 @@ spec:
       {{- with .Values.webhooks.hooks.resourcepools.pools }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: resourcepoolclaims.projectcapsule.dev
+      - name: resourcepoolclaims.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -663,7 +663,7 @@ spec:
       {{- with .Values.webhooks.hooks.serviceaccounts }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: serviceaccounts.projectcapsule.dev
+      - name: serviceaccounts.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -704,7 +704,7 @@ spec:
       {{- with .Values.webhooks.hooks.config }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: config.projectcapsule.dev
+      - name: config.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -744,7 +744,7 @@ spec:
       {{- with .Values.webhooks.hooks.customquotas }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: namespaced.custom-quotas.projectcapsule.dev
+      - name: namespaced.custom-quotas.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -776,7 +776,7 @@ spec:
       {{- with .Values.webhooks.hooks.globalcustomquotas }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: cluster.custom-quotas.projectcapsule.dev
+      - name: cluster.custom-quotas.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -808,7 +808,7 @@ spec:
       {{- with .Values.webhooks.hooks.calculations }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: calculation.custom-quotas.projectcapsule.dev
+      - name: calculation.custom-quotas.validating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -858,7 +858,7 @@ spec:
       {{- with (mergeOverwrite .Values.webhooks.hooks.namespaces .Values.webhooks.hooks.namespaceOwnerReference) }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: namespaces.tenants.projectcapsule.dev
+      - name: namespaces.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -901,7 +901,7 @@ spec:
       {{- with .Values.webhooks.hooks.persistentvolumeclaims }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: pvc.projectcapsule.dev
+      - name: pvc.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -944,7 +944,7 @@ spec:
       {{- with (mergeOverwrite .Values.webhooks.hooks.pods .Values.webhooks.hooks.defaults.pods) }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: pod.defaults.projectcapsule.dev
+      - name: pod.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -984,7 +984,7 @@ spec:
       {{- with (mergeOverwrite .Values.webhooks.hooks.persistentvolumeclaims .Values.webhooks.hooks.defaults.pvc) }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: storage.defaults.projectcapsule.dev
+      - name: storage.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -1024,7 +1024,7 @@ spec:
       {{- with (mergeOverwrite .Values.webhooks.hooks.ingresses .Values.webhooks.hooks.defaults.ingress) }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: ingress.defaults.projectcapsule.dev
+      - name: ingress.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -1066,7 +1066,7 @@ spec:
       {{- with .Values.webhooks.hooks.gateways }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: gateway.defaults.projectcapsule.dev
+      - name: gateway.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -1107,7 +1107,7 @@ spec:
       {{- with .Values.webhooks.hooks.resourcepools.pools }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: resourcepools.projectcapsule.dev
+      - name: resourcepools.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -1149,7 +1149,7 @@ spec:
       {{- with .Values.webhooks.hooks.resourcepools.claims }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: resourcepoolclaims.projectcapsule.dev
+      - name: resourcepoolclaims.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -1191,7 +1191,7 @@ spec:
       {{- with .Values.webhooks.hooks.tenants }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: tenants.projectcapsule.dev
+      - name: tenants.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}
@@ -1234,7 +1234,7 @@ spec:
       {{- with .Values.webhooks.hooks.metadata }}
         {{- if .enabled }}
           {{- $any = true }}
-      - name: metadata.misc.projectcapsule.dev
+      - name: metadata.mutating.projectcapsule.dev
         {{- with .opts }}
         opts:
           {{- toYaml . |  nindent 10 }}

e2e/config_administrators_test.go

@@ -103,6 +103,86 @@ var _ = Describe("Administrators", Ordered, Label("namespace", "permissions", "a
 
 	})
 
+	It("interaction with non-tenant namespaces", func() {
+		ctx := context.TODO()
+
+		ns1 := NewNamespace("", map[string]string{})
+
+		By("creating namespace with explicit empty labels", func() {
+			NamespaceCreation(ns1, admin, defaultTimeoutInterval).Should(Succeed())
+		})
+
+		By("verifying no ownerReferences and no tenant label", func() {
+			ExpectNamespaceNotAssignedToTenant(ctx, ns1.Name)
+		})
+
+		By("updating unassigned namespace as administrator", func() {
+			Eventually(func() error {
+				current := NewNamespace(ns1.Name)
+
+				if err := k8sClient.Get(ctx, types.NamespacedName{Name: ns1.Name}, current); err != nil {
+					return err
+				}
+
+				original := current.DeepCopy()
+
+				if current.Annotations == nil {
+					current.Annotations = map[string]string{}
+				}
+
+				current.Annotations["e2e.capsule.clastix.io/admin-non-tenant-update"] = "true"
+
+				return impersonationClient(admin.Name, nil).Patch(ctx, current, client.MergeFrom(original))
+			}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
+		})
+
+		By("verifying namespace is still not assigned after administrator update", func() {
+			ExpectNamespaceNotAssignedToTenant(ctx, ns1.Name)
+		})
+
+		ns2 := NewNamespace("")
+
+		By("creating namespace with nil labels", func() {
+			NamespaceCreation(ns2, admin, defaultTimeoutInterval).Should(Succeed())
+		})
+
+		By("verifying no ownerReferences and no tenant label", func() {
+			ExpectNamespaceNotAssignedToTenant(ctx, ns2.Name)
+		})
+
+		By("updating unassigned namespace with nil labels as administrator", func() {
+			Eventually(func() error {
+				current := NewNamespace(ns2.Name)
+
+				if err := k8sClient.Get(ctx, types.NamespacedName{Name: ns2.Name}, current); err != nil {
+					return err
+				}
+
+				original := current.DeepCopy()
+
+				if current.Annotations == nil {
+					current.Annotations = map[string]string{}
+				}
+
+				current.Annotations["e2e.capsule.clastix.io/admin-non-tenant-update"] = "true"
+
+				return impersonationClient(admin.Name, nil).Patch(ctx, current, client.MergeFrom(original))
+			}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
+		})
+
+		By("verifying namespace is still not assigned after administrator update", func() {
+			ExpectNamespaceNotAssignedToTenant(ctx, ns2.Name)
+		})
+
+		By("deleting namespace", func() {
+			Expect(k8sClient.Delete(ctx, ns2)).Should(Succeed())
+		})
+
+		By("deleting namespace", func() {
+			Expect(k8sClient.Delete(ctx, ns1)).Should(Succeed())
+		})
+	})
+
 	It("capsule is triggered for administrators based on namespace label", func() {
 		By("creating namespace with faulty label", func() {
 			ns := NewNamespace("", map[string]string{

e2e/tenant_protected_webhook_test.go

@@ -37,9 +37,10 @@ var _ = Describe("Deleting a tenant with protected annotation", Ordered, Label("
 
 	JustAfterEach(func() {
 		Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: tnt.GetName()}, tnt)).Should(Succeed())
-		tnt.Spec.PreventDeletion = false
-		Expect(k8sClient.Update(context.TODO(), tnt)).Should(Succeed())
-		Expect(k8sClient.Delete(context.TODO(), tnt)).Should(Succeed())
+
+		UpdateTenantEventually(tnt, func(t *capsulev1beta2.Tenant) {
+			t.Spec.PreventDeletion = false
+		})
 	})
 
 	It("should fail", func() {

e2e/utils_test.go

@@ -500,6 +500,17 @@ func PatchNamespaceEventually(ns *corev1.Namespace, mutator func(*corev1.Namespa
 	}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
 }
 
+func ExpectNamespaceNotAssignedToTenant(ctx context.Context, name string) {
+	ns := &corev1.Namespace{}
+
+	Eventually(func(g Gomega) {
+		g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: name}, ns)).To(Succeed())
+
+		g.Expect(ns.GetOwnerReferences()).To(BeEmpty(), "namespace must not have ownerReferences")
+		g.Expect(ns.GetLabels()).NotTo(HaveKey(meta.TenantLabel), "namespace must not have tenant label")
+	}).WithTimeout(defaultTimeoutInterval).Should(Succeed())
+}
+
 func TenantNamespaceList(tnt *capsulev1beta2.Tenant, timeout time.Duration) AsyncAssertion {
 	t := &capsulev1beta2.Tenant{}
 	return Eventually(func() []string {

internal/webhook/namespace/mutation/assignment.go

@@ -48,6 +48,12 @@ func (h *ownerReferenceHandler) OnCreate(
 			return errResponse
 		}
 
+		// Administrators are allowed to create namespaces that are not managed
+		// by Capsule. In that case there is intentionally no tenant to assign.
+		if tnt == nil && user.IsAdmin() {
+			return ad.Allow("")
+		}
+
 		if tnt == nil {
 			response := admission.Denied(
 				"Unable to assign namespace to tenant. Please use " +

internal/webhook/namespace/mutation/metadata.go

@@ -48,7 +48,7 @@ func (h *metadataHandler) OnCreate(
 		}
 
 		if tnt == nil {
-			return ad.Deny("Unable to assign namespace to tenant.")
+			return nil
 		}
 
 		labels, annotations, err := tenant.BuildNamespaceMetadataForTenant(ns, tnt)
@@ -105,7 +105,7 @@ func (h *metadataHandler) OnUpdate(
 		}
 
 		if tnt == nil {
-			return ad.Deny("Unable to assign namespace to tenant.")
+			return nil
 		}
 
 		labels, annotations, err := tenant.BuildNamespaceMetadataForTenant(newNs, tnt)

internal/webhook/namespace/validation/handler.go

@@ -183,10 +183,6 @@ func (h *handler) OnUpdate(
 			}
 		}
 
-		if terminating := h.rejectOnTermination(ctx, c, ns, newTenant); terminating != nil {
-			return terminating
-		}
-
 		tnt := newTenant
 		if !user.IsAdmin() {
 			tnt = oldTenant
@@ -196,6 +192,10 @@ func (h *handler) OnUpdate(
 			return nil
 		}
 
+		if terminating := h.rejectOnTermination(ctx, c, ns, newTenant); terminating != nil {
+			return terminating
+		}
+
 		for _, hndl := range h.handlers {
 			if response := hndl.OnUpdate(c, reader, user, ns, oldNs, decoder, recorder, tnt)(ctx, req); response != nil {
 				return response
@@ -212,6 +212,10 @@ func (h *handler) rejectOnTermination(
 	ns *corev1.Namespace,
 	t *capsulev1beta2.Tenant,
 ) *admission.Response {
+	if t == nil {
+		return nil
+	}
+
 	tnt := &capsulev1beta2.Tenant{}
 
 	_ = c.Get(ctx, types.NamespacedName{Name: t.GetName()}, tnt)

internal/webhook/utils/tenant_get.go

@@ -55,6 +55,10 @@ func GetNamespaceTenant(
 		return nil, &response
 	}
 
+	if len(tnts) == 0 && user.IsAdmin() {
+		return nil, nil
+	}
+
 	if len(tnts) == 0 {
 		return nil, ad.Deny("You do not have any Tenant assigned: please, reach out to the system administrators")
 	}

pkg/runtime/admission/utils.go

@@ -15,6 +15,12 @@ func Deny(message string) *admission.Response {
 	return &response
 }
 
+func Allow(message string) *admission.Response {
+	response := admission.Allowed(message)
+
+	return &response
+}
+
 func normalizePath(p string) string {
 	if p == "" {
 		return ""