From b783d03f44d5b59ed8f8b6cf9e0c3f4dfa73c7e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20B=C3=A4hler?= <26610571+oliverbaehler@users.noreply.github.com> Date: Tue, 2 Jun 2026 14:17:27 +0200 Subject: [PATCH] fix: avoid rejection when users are classified as administrators (#1941) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(controller): decode old object for delete requests Signed-off-by: Oliver Bähler * chore: modernize golang Signed-off-by: Oliver Bähler * chore: modernize golang Signed-off-by: Oliver Bähler * chore: modernize golang Signed-off-by: Oliver Bähler * fix: avoid rejection when users are classified as administrators Signed-off-by: Oliver Baehler --------- Signed-off-by: Oliver Bähler Signed-off-by: Oliver Baehler --- charts/capsule/templates/configuration.yaml | 60 +++++++------- e2e/config_administrators_test.go | 80 +++++++++++++++++++ e2e/tenant_protected_webhook_test.go | 7 +- e2e/utils_test.go | 11 +++ .../webhook/namespace/mutation/assignment.go | 6 ++ .../webhook/namespace/mutation/metadata.go | 4 +- .../webhook/namespace/validation/handler.go | 12 ++- internal/webhook/utils/tenant_get.go | 4 + pkg/runtime/admission/utils.go | 6 ++ 9 files changed, 151 insertions(+), 39 deletions(-) diff --git a/charts/capsule/templates/configuration.yaml b/charts/capsule/templates/configuration.yaml index 72d9a38a..930f00cd 100644 --- a/charts/capsule/templates/configuration.yaml +++ b/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 }} diff --git a/e2e/config_administrators_test.go b/e2e/config_administrators_test.go index f3b01eb2..d4dfd023 100644 --- a/e2e/config_administrators_test.go +++ b/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{ diff --git a/e2e/tenant_protected_webhook_test.go b/e2e/tenant_protected_webhook_test.go index b0b52c46..1c1d85f7 100644 --- a/e2e/tenant_protected_webhook_test.go +++ b/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() { diff --git a/e2e/utils_test.go b/e2e/utils_test.go index 9463967e..1b6063d7 100644 --- a/e2e/utils_test.go +++ b/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 { diff --git a/internal/webhook/namespace/mutation/assignment.go b/internal/webhook/namespace/mutation/assignment.go index 343b9db2..dbae0804 100644 --- a/internal/webhook/namespace/mutation/assignment.go +++ b/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 " + diff --git a/internal/webhook/namespace/mutation/metadata.go b/internal/webhook/namespace/mutation/metadata.go index 291f9d17..f182a17c 100644 --- a/internal/webhook/namespace/mutation/metadata.go +++ b/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) diff --git a/internal/webhook/namespace/validation/handler.go b/internal/webhook/namespace/validation/handler.go index c62e14c3..ce6c2e46 100644 --- a/internal/webhook/namespace/validation/handler.go +++ b/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) diff --git a/internal/webhook/utils/tenant_get.go b/internal/webhook/utils/tenant_get.go index 3169b59e..842e0558 100644 --- a/internal/webhook/utils/tenant_get.go +++ b/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") } diff --git a/pkg/runtime/admission/utils.go b/pkg/runtime/admission/utils.go index 7dfe0ca8..a777de8f 100644 --- a/pkg/runtime/admission/utils.go +++ b/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 ""