fix: allow administrator operations on tenant namespaces (#2037)

fix: allow administrator operations on tenant namespaces 

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

api/v1beta2/tenantowner_types.go

@@ -18,8 +18,13 @@ type TenantOwnerSpec struct {
 	// Adds the given subject as capsule user. When enabled this subject does not have to be
 	// mentioned in the CapsuleConfiguration as Capsule User. In almost all scenarios Tenant Owners
 	// must be Capsule Users.
-	//+kubebuilder:default:=true
-	Aggregate bool `json:"aggregate"`
+	// +kubebuilder:default=true
+	// +optional
+	Aggregate *bool `json:"aggregate,omitempty"`
+}
+
+func (s TenantOwnerSpec) AggregateEnabled() bool {
+	return s.Aggregate == nil || *s.Aggregate
 }
 
 // TenantOwnerStatus defines the observed state of TenantOwner.

api/v1beta2/tenantowner_types_test.go

@@ -0,0 +1,49 @@
+// Copyright 2020-2026 Project Capsule Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package v1beta2
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+
+	"k8s.io/utils/ptr"
+)
+
+func TestTenantOwnerAggregateEnabled(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name      string
+		aggregate *bool
+		want      bool
+	}{
+		{name: "unset defaults true", want: true},
+		{name: "explicit true", aggregate: ptr.To(true), want: true},
+		{name: "explicit false", aggregate: ptr.To(false), want: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			spec := TenantOwnerSpec{Aggregate: tt.aggregate}
+			if got := spec.AggregateEnabled(); got != tt.want {
+				t.Fatalf("AggregateEnabled() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestTenantOwnerAggregateFalseIsSerialized(t *testing.T) {
+	t.Parallel()
+
+	data, err := json.Marshal(TenantOwnerSpec{Aggregate: ptr.To(false)})
+	if err != nil {
+		t.Fatalf("marshal TenantOwnerSpec: %v", err)
+	}
+	if !strings.Contains(string(data), `"aggregate":false`) {
+		t.Fatalf("explicit false was omitted from JSON: %s", data)
+	}
+}

api/v1beta2/zz_generated.deepcopy.go

@@ -1896,6 +1896,11 @@ func (in *TenantOwnerList) DeepCopyObject() runtime.Object {
 func (in *TenantOwnerSpec) DeepCopyInto(out *TenantOwnerSpec) {
 	*out = *in
 	in.CoreOwnerSpec.DeepCopyInto(&out.CoreOwnerSpec)
+	if in.Aggregate != nil {
+		in, out := &in.Aggregate, &out.Aggregate
+		*out = new(bool)
+		**out = **in
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TenantOwnerSpec.

charts/capsule/crds/capsule.clastix.io_tenantowners.yaml

@@ -77,7 +77,6 @@ spec:
                 description: Name of the entity.
                 type: string
             required:
-            - aggregate
             - kind
             - name
             type: object

e2e/config_namespace_hijacking_test.go

@@ -26,7 +26,7 @@ import (
 	"github.com/projectcapsule/capsule/pkg/tenant"
 )
 
-var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("namespace", "hijack"), func() {
+var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("config", "namespace", "hijack"), func() {
 	t1 := &capsulev1beta2.Tenant{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "e2e-ns-attack-1",
@@ -257,7 +257,42 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 		return fmt.Sprintf("random-tenant-%d", rand.Int()), types.UID(fmt.Sprintf("%d", rand.Int()))
 	}
 
+	createUnmanagedNamespace := func() *corev1.Namespace {
+		ns := NewNamespace("")
+		Expect(k8sClient.Create(context.TODO(), ns)).To(Succeed())
+		DeferCleanup(func() { EventuallyDeletion(ns) })
+
+		return ns
+	}
+
+	waitForTenantNamespacesDeletion := func(tenantNames ...string) {
+		names := make(map[string]struct{}, len(tenantNames))
+		for _, name := range tenantNames {
+			names[name] = struct{}{}
+		}
+
+		Eventually(func(g Gomega) {
+			list := &corev1.NamespaceList{}
+			g.Expect(k8sClient.List(context.TODO(), list)).To(Succeed())
+
+			for i := range list.Items {
+				for _, ref := range tenant.TenantOwnerReferences(&list.Items[i]) {
+					_, belongsToTestTenant := names[ref.Name]
+					g.Expect(belongsToTestTenant).To(
+						BeFalse(),
+						"namespace %q still references a previous incarnation of Tenant %q (UID %q)",
+						list.Items[i].Name,
+						ref.Name,
+						ref.UID,
+					)
+				}
+			}
+		}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
+	}
+
 	JustBeforeEach(func() {
+		waitForTenantNamespacesDeletion(t1.Name, t2.Name, t3.Name)
+
 		EventuallyCreation(func() error {
 			t1.ResourceVersion = ""
 
@@ -285,6 +320,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 		EventuallyDeletion(t1)
 		EventuallyDeletion(t2)
 		EventuallyDeletion(t3)
+		waitForTenantNamespacesDeletion(t1.Name, t2.Name, t3.Name)
 	})
 
 	It("Owners can not hijack Tenant ownership through namespaces/status", func() {
@@ -363,7 +399,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 		_, err = cs.CoreV1().Namespaces().UpdateStatus(
 			context.TODO(),
 			hijacked,
-			metav1.UpdateOptions{},
+			metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}},
 		)
 
 		if err != nil {
@@ -448,7 +484,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 		_, err = cs.CoreV1().Namespaces().Finalize(
 			context.TODO(),
 			hijacked,
-			metav1.UpdateOptions{},
+			metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}},
 		)
 
 		if err != nil {
@@ -489,7 +525,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 			_, err = cs.CoreV1().Namespaces().Update(
 				context.TODO(),
 				current,
-				metav1.UpdateOptions{},
+				metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}},
 			)
 
 			Expect(err).To(HaveOccurred())
@@ -514,8 +550,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 		tenantA := getTenant(t1.Name)
 		tenantB := getTenant(t2.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -575,8 +610,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Tenant A owners can not adopt unmanaged namespaces into Tenant B", func() {
 		tenantB := getTenant(t3.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -606,8 +640,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not hijack unmanaged namespaces with controller ownerReference flags", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -641,8 +674,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not smuggle Tenant ownerReference beside unrelated ownerReferences", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -764,8 +796,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not hijack unmanaged namespaces using JSONPatch add label", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -791,8 +822,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not hijack unmanaged namespaces using JSONPatch add ownerReference", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -819,8 +849,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not hijack unmanaged namespaces with matching label and forged ownerReference UID", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -851,8 +880,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not hijack unmanaged namespaces using server-side apply", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -902,8 +930,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 			deleteNamespaceStatusRBACForOwner(tnt)
 		}, tenant)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -923,7 +950,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 				UID:        tenant.GetUID(),
 			}}
 
-			_, _ = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{})
+			_, _ = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
 
 			patch := []byte(fmt.Sprintf(
 				`{"metadata":{"labels":{"%s":"%s"}}}`,
@@ -947,8 +974,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not hijack unmanaged namespaces with valid ownerReference and mismatching label", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -1134,8 +1160,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 	It("Owners can not patch unmanaged namespaces into a Tenant", func() {
 		tenant := getTenant(t1.Name)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -1194,7 +1219,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 				},
 			}
 
-			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{})
+			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
 			if err != nil {
 				expectOriginalTenantOwnership(ns.Name, tenant)
 
@@ -1236,7 +1261,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 
 			statusNs.Labels[meta.TenantLabel] = randomName
 
-			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{})
+			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
 			if err != nil {
 				expectOriginalTenantOwnership(ns.Name, tenant)
 
@@ -1287,7 +1312,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 				},
 			}
 
-			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{})
+			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
 			if err != nil {
 				retrievedNs := getNamespace(ns.Name)
 
@@ -1314,8 +1339,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 			deleteNamespaceStatusRBACForOwner(tnt)
 		}, tenant)
 
-		unmanaged := NewNamespace("")
-		Expect(k8sClient.Create(context.TODO(), unmanaged)).Should(Succeed())
+		unmanaged := createUnmanagedNamespace()
 
 		for _, owner := range t1.Spec.Owners {
 			cs := ownerClient(owner.UserSpec)
@@ -1337,7 +1361,7 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 				},
 			}
 
-			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{})
+			_, err = cs.CoreV1().Namespaces().UpdateStatus(context.TODO(), statusNs, metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}})
 			if err != nil {
 				expectNoTenantOwnership(unmanaged.Name, tenant)
 
@@ -1346,6 +1370,13 @@ var _ = Describe("creating several Namespaces for a Tenant", Ordered, Label("nam
 
 			expectNoTenantOwnership(unmanaged.Name, tenant)
 		}
+
+		Expect(k8sClient.Delete(context.TODO(), unmanaged)).To(Succeed())
+		Eventually(func() bool {
+			err := k8sClient.Get(context.TODO(), types.NamespacedName{Name: unmanaged.Name}, &corev1.Namespace{})
+
+			return apierrors.IsNotFound(err)
+		}, defaultTimeoutInterval, defaultPollInterval).Should(BeTrue())
 	})
 })
 

e2e/tenantowner_status_test.go

@@ -11,6 +11,7 @@ import (
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
 
 	capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2"
 	capmeta "github.com/projectcapsule/capsule/pkg/api/meta"
@@ -65,7 +66,7 @@ var _ = Describe("TenantOwner status tracks matched Tenants", Ordered, Label("te
 			},
 		},
 		Spec: capsulev1beta2.TenantOwnerSpec{
-			Aggregate: true,
+			Aggregate: ptr.To(true),
 			CoreOwnerSpec: rbac.CoreOwnerSpec{
 				UserSpec: rbac.UserSpec{
 					Kind: rbac.UserOwner,
@@ -216,7 +217,7 @@ var _ = Describe("TenantOwner status with 10 matched Tenants", Ordered, Label("t
 			},
 		},
 		Spec: capsulev1beta2.TenantOwnerSpec{
-			Aggregate: true,
+			Aggregate: ptr.To(true),
 			CoreOwnerSpec: rbac.CoreOwnerSpec{
 				UserSpec: rbac.UserSpec{
 					Kind: rbac.UserOwner,
@@ -301,7 +302,7 @@ var _ = Describe("TenantOwner status fan-out: 10 TenantOwners matched by 1 Tenan
 					},
 				},
 				Spec: capsulev1beta2.TenantOwnerSpec{
-					Aggregate: true,
+					Aggregate: ptr.To(true),
 					CoreOwnerSpec: rbac.CoreOwnerSpec{
 						UserSpec: rbac.UserSpec{
 							Kind: rbac.UserOwner,

e2e/tenantowner_test.go

@@ -10,6 +10,7 @@ import (
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2"
@@ -135,7 +136,7 @@ var _ = Describe("Owners", Ordered, Label("config", "tenantowner", "tenant", "pe
 			},
 		},
 		Spec: capsulev1beta2.TenantOwnerSpec{
-			Aggregate: true,
+			Aggregate: ptr.To(true),
 			CoreOwnerSpec: rbac.CoreOwnerSpec{
 				UserSpec: rbac.UserSpec{
 					Kind: rbac.GroupOwner,
@@ -157,7 +158,7 @@ var _ = Describe("Owners", Ordered, Label("config", "tenantowner", "tenant", "pe
 			},
 		},
 		Spec: capsulev1beta2.TenantOwnerSpec{
-			Aggregate: true,
+			Aggregate: ptr.To(true),
 			CoreOwnerSpec: rbac.CoreOwnerSpec{
 				UserSpec: rbac.UserSpec{
 					Kind: rbac.GroupOwner,
@@ -180,7 +181,7 @@ var _ = Describe("Owners", Ordered, Label("config", "tenantowner", "tenant", "pe
 			},
 		},
 		Spec: capsulev1beta2.TenantOwnerSpec{
-			Aggregate: true,
+			Aggregate: ptr.To(true),
 			CoreOwnerSpec: rbac.CoreOwnerSpec{
 				UserSpec: rbac.UserSpec{
 					Kind: rbac.ServiceAccountOwner,
@@ -201,7 +202,7 @@ var _ = Describe("Owners", Ordered, Label("config", "tenantowner", "tenant", "pe
 			},
 		},
 		Spec: capsulev1beta2.TenantOwnerSpec{
-			Aggregate: true,
+			Aggregate: ptr.To(true),
 			CoreOwnerSpec: rbac.CoreOwnerSpec{
 				UserSpec: rbac.UserSpec{
 					Kind: rbac.UserOwner,
@@ -224,7 +225,7 @@ var _ = Describe("Owners", Ordered, Label("config", "tenantowner", "tenant", "pe
 			},
 		},
 		Spec: capsulev1beta2.TenantOwnerSpec{
-			Aggregate: false,
+			Aggregate: ptr.To(false),
 			CoreOwnerSpec: rbac.CoreOwnerSpec{
 				UserSpec: rbac.UserSpec{
 					Kind: rbac.UserOwner,

internal/controllers/cfg/status/manager.go

@@ -116,7 +116,7 @@ func (r *Manager) SetupWithManager(
 				CreateFunc: func(e event.CreateEvent) bool {
 					to, ok := e.Object.(*capsulev1beta2.TenantOwner)
 
-					return ok && to.Spec.Aggregate
+					return ok && to.Spec.AggregateEnabled()
 				},
 				UpdateFunc: func(e event.UpdateEvent) bool {
 					oldTo, ok1 := e.ObjectOld.(*capsulev1beta2.TenantOwner)
@@ -126,7 +126,7 @@ func (r *Manager) SetupWithManager(
 						return false
 					}
 
-					if oldTo.Spec.Aggregate != newTo.Spec.Aggregate {
+					if oldTo.Spec.AggregateEnabled() != newTo.Spec.AggregateEnabled() {
 						return true
 					}
 
@@ -143,7 +143,7 @@ func (r *Manager) SetupWithManager(
 				DeleteFunc: func(e event.DeleteEvent) bool {
 					to, ok := e.Object.(*capsulev1beta2.TenantOwner)
 
-					return ok && to.Spec.Aggregate
+					return ok && to.Spec.AggregateEnabled()
 				},
 			}),
 		).
@@ -231,7 +231,7 @@ func (r *Manager) gatherCapsuleUsers(
 	for i := range toList.Items {
 		to := &toList.Items[i]
 
-		if !to.Spec.Aggregate {
+		if !to.Spec.AggregateEnabled() {
 			continue
 		}
 

internal/webhook/namespace/mutation/metadata.go

@@ -185,15 +185,6 @@ func (h *metadataHandler) resolveTenantForUpdate(
 			return nil, ad.ErroredResponse(err)
 		}
 
-		if tnt != nil {
-			return tnt, nil
-		}
-
-		tnt, err = tenant.GetTenantByLabels(ctx, reader, oldNs)
-		if err != nil {
-			return nil, ad.ErroredResponse(err)
-		}
-
 		return tnt, nil
 	}
 

internal/webhook/namespace/validation/handler.go

@@ -97,7 +97,7 @@ func (h *handler) OnDelete(
 		}
 
 		tnt, err := tenant.ResolveNamespaceTenant(ctx, reader, oldNs)
-		if err != nil {
+		if err != nil && !user.IsAdmin() {
 			return ad.ErroredResponse(err)
 		}
 
@@ -149,7 +149,7 @@ func (h *handler) OnUpdate(
 		}
 
 		oldTenant, err := tenant.ResolveNamespaceTenant(ctx, reader, oldNs)
-		if err != nil {
+		if err != nil && !user.IsAdmin() {
 			return ad.ErroredResponse(err)
 		}