diff --git a/api/v1beta2/tenantowner_types.go b/api/v1beta2/tenantowner_types.go index 57d83563..96f7378c 100644 --- a/api/v1beta2/tenantowner_types.go +++ b/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. diff --git a/api/v1beta2/tenantowner_types_test.go b/api/v1beta2/tenantowner_types_test.go new file mode 100644 index 00000000..a22696bd --- /dev/null +++ b/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) + } +} diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index a5d24bc4..7e8cbcdd 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/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. diff --git a/charts/capsule/crds/capsule.clastix.io_tenantowners.yaml b/charts/capsule/crds/capsule.clastix.io_tenantowners.yaml index bf554c0a..06821547 100644 --- a/charts/capsule/crds/capsule.clastix.io_tenantowners.yaml +++ b/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 diff --git a/e2e/namespace_hijacking_test.go b/e2e/config_namespace_hijacking_test.go similarity index 94% rename from e2e/namespace_hijacking_test.go rename to e2e/config_namespace_hijacking_test.go index 1c1cfa6c..1a328775 100644 --- a/e2e/namespace_hijacking_test.go +++ b/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()) }) }) diff --git a/e2e/tenantowner_status_test.go b/e2e/tenantowner_status_test.go index 8e8adff7..9f478f3e 100644 --- a/e2e/tenantowner_status_test.go +++ b/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, diff --git a/e2e/tenantowner_test.go b/e2e/tenantowner_test.go index f799ba86..e4c71568 100644 --- a/e2e/tenantowner_test.go +++ b/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, diff --git a/internal/controllers/cfg/status/manager.go b/internal/controllers/cfg/status/manager.go index 4fd0a2df..a376fa2a 100644 --- a/internal/controllers/cfg/status/manager.go +++ b/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 } diff --git a/internal/webhook/namespace/mutation/metadata.go b/internal/webhook/namespace/mutation/metadata.go index 63fa4461..7efd972c 100644 --- a/internal/webhook/namespace/mutation/metadata.go +++ b/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 } diff --git a/internal/webhook/namespace/validation/handler.go b/internal/webhook/namespace/validation/handler.go index 1d823b21..4f509cc4 100644 --- a/internal/webhook/namespace/validation/handler.go +++ b/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) }