From 613b43aa21bb8125aca67ff5df4c95f433296504 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20B=C3=A4hler?= <26610571+oliverbaehler@users.noreply.github.com> Date: Tue, 28 Jul 2026 19:30:00 +0200 Subject: [PATCH] fix: correct tenant ownerships resolution with deduplicaiton (#2059) * chore * perfromance improvements Signed-off-by: Oliver Baehler * perfromance improvements Signed-off-by: Oliver Baehler * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler * fix: correct tenant ownerships resolution with deduplicaiton Signed-off-by: Oliver Baehler --------- Signed-off-by: Oliver Baehler --- ...orce_tenant_prefix_duplicate_group_test.go | 83 +++ internal/webhook/utils/tenant_get.go | 4 +- internal/webhook/utils/tenant_get_test.go | 485 ++++++++++++++++++ pkg/tenant/get_by.go | 24 +- pkg/tenant/get_by_test.go | 9 +- 5 files changed, 598 insertions(+), 7 deletions(-) create mode 100644 e2e/config_force_tenant_prefix_duplicate_group_test.go create mode 100644 internal/webhook/utils/tenant_get_test.go diff --git a/e2e/config_force_tenant_prefix_duplicate_group_test.go b/e2e/config_force_tenant_prefix_duplicate_group_test.go new file mode 100644 index 00000000..de8e8e41 --- /dev/null +++ b/e2e/config_force_tenant_prefix_duplicate_group_test.go @@ -0,0 +1,83 @@ +// Copyright 2020-2026 Project Capsule Authors +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2" + "github.com/projectcapsule/capsule/pkg/api/rbac" +) + +var _ = Describe( + "creating a prefixed Namespace when one Tenant is matched more than once", + Ordered, + Label("config", "tenant", "prefix", "issue-2058"), + func() { + const ( + ownerGroup = "e2e-prefix-duplicate-group" + username = "e2e-prefix-duplicate-group-user" + ) + + tnt := &capsulev1beta2.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "e2e-prefix-duplicate-group", + Labels: map[string]string{"env": "e2e"}, + }, + Spec: capsulev1beta2.TenantSpec{ + Owners: rbac.OwnerListSpec{{ + CoreOwnerSpec: rbac.CoreOwnerSpec{ + UserSpec: rbac.UserSpec{ + Kind: rbac.GroupOwner, + Name: ownerGroup, + }, + }, + }}, + }, + } + + JustBeforeEach(func() { + EventuallyCreation(func() error { + tnt.ResourceVersion = "" + + return k8sClient.Create(context.TODO(), tnt) + }).Should(Succeed()) + TenantReady(tnt, metav1.ConditionTrue, defaultTimeoutInterval) + + ModifyCapsuleConfigurationOpts(func(configuration *capsulev1beta2.CapsuleConfiguration) { + configuration.Spec.ForceTenantPrefix = true + }) + }) + + JustAfterEach(func() { + EventuallyDeletion(tnt) + + ModifyCapsuleConfigurationOpts(func(configuration *capsulev1beta2.CapsuleConfiguration) { + configuration.Spec.ForceTenantPrefix = false + }) + }) + + It("assigns the Namespace when the owning group is repeated in the request", func() { + namespace := NewNamespace(tnt.GetName() + "-namespace") + clientset := impersonationClientSet( + username, + withDefaultGroups([]string{ownerGroup, ownerGroup}), + ) + + Eventually(func() error { + _, err := clientset.CoreV1(). + Namespaces(). + Create(context.TODO(), namespace, metav1.CreateOptions{}) + + return err + }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) + + NamespaceIsPartOfTenant(tnt, namespace).Should(Succeed()) + }) + }, +) diff --git a/internal/webhook/utils/tenant_get.go b/internal/webhook/utils/tenant_get.go index b769a4a4..b929d1f2 100644 --- a/internal/webhook/utils/tenant_get.go +++ b/internal/webhook/utils/tenant_get.go @@ -120,7 +120,9 @@ func resolveTenantByClosestNamespacePrefix( ambiguous = false case len(prefix) == matchedPrefixLen: - ambiguous = true + if matched.GetName() != tnts[i].GetName() { + ambiguous = true + } } } diff --git a/internal/webhook/utils/tenant_get_test.go b/internal/webhook/utils/tenant_get_test.go new file mode 100644 index 00000000..44013718 --- /dev/null +++ b/internal/webhook/utils/tenant_get_test.go @@ -0,0 +1,485 @@ +// Copyright 2020-2026 Project Capsule Authors +// SPDX-License-Identifier: Apache-2.0 + +package utils + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2" + "github.com/projectcapsule/capsule/pkg/api/rbac" + "github.com/projectcapsule/capsule/pkg/runtime/configuration" + "github.com/projectcapsule/capsule/pkg/users" +) + +func TestGetNamespaceTenantResolvesMatchingPrefix(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + namespace string + tenants []*capsulev1beta2.Tenant + user users.AdmissionUser + want string + }{ + { + name: "issue 2058 group owner", + namespace: "dummy-namespace", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant("dummy", tenantGetTestOwner(rbac.GroupOwner, "dummy-team")), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + Groups: []string{"dummy-team"}, + }, + want: "dummy", + }, + { + name: "multiple available tenants", + namespace: "dummy-namespace", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant("dummy", tenantGetTestOwner(rbac.GroupOwner, "developer")), + tenantGetTestTenant("another", tenantGetTestOwner(rbac.GroupOwner, "developer")), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + Groups: []string{"developer"}, + }, + want: "dummy", + }, + { + name: "same tenant matched by repeated group", + namespace: "dummy-namespace", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant("dummy", tenantGetTestOwner(rbac.GroupOwner, "dummy-team")), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + Groups: []string{"dummy-team", "dummy-team"}, + }, + want: "dummy", + }, + { + name: "closest overlapping prefix", + namespace: "team-platform-namespace", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant("team", tenantGetTestOwner(rbac.UserOwner, "alice")), + tenantGetTestTenant("team-platform", tenantGetTestOwner(rbac.UserOwner, "alice")), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + }, + want: "team-platform", + }, + { + name: "same tenant matched by user and group", + namespace: "dummy-namespace", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant( + "dummy", + tenantGetTestOwner(rbac.UserOwner, "alice"), + tenantGetTestOwner(rbac.GroupOwner, "dummy-team"), + ), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + Groups: []string{"dummy-team"}, + }, + want: "dummy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + cl, cfg := tenantGetTestClient(t, true, tt.tenants...) + + got, response := GetNamespaceTenant( + ctx, + cl, + cl, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: tt.namespace}}, + tt.user, + cfg, + nil, + ) + if response != nil { + t.Fatalf("GetNamespaceTenant() unexpected response: %#v", response.Result) + } + if got == nil || got.Name != tt.want { + t.Fatalf("GetNamespaceTenant() = %#v, want Tenant %q", got, tt.want) + } + }) + } +} + +func TestGetNamespaceTenantResponses(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + forcePrefix bool + namespace string + tenants []*capsulev1beta2.Tenant + user users.AdmissionUser + wantTenant string + wantMessage string + }{ + { + name: "single tenant requires configured prefix", + forcePrefix: true, + namespace: "workloads", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant("dummy", tenantGetTestOwner(rbac.UserOwner, "alice")), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + }, + wantMessage: "The Namespace name must start with 'dummy-' when ForceTenantPrefix is enabled in the Tenant.", + }, + { + name: "multiple tenants require a matching prefix", + forcePrefix: true, + namespace: "workloads", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant("dummy", tenantGetTestOwner(rbac.UserOwner, "alice")), + tenantGetTestTenant("another", tenantGetTestOwner(rbac.UserOwner, "alice")), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + }, + wantMessage: "The Namespace prefix used doesn't match any available Tenant", + }, + { + name: "tenant override disables configured prefix", + forcePrefix: true, + namespace: "workloads", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant( + "dummy", + tenantGetTestOwner(rbac.UserOwner, "alice"), + tenantGetTestPrefixOverride(false), + ), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + }, + wantTenant: "dummy", + }, + { + name: "tenant override enables prefix", + forcePrefix: false, + namespace: "workloads", + tenants: []*capsulev1beta2.Tenant{ + tenantGetTestTenant( + "dummy", + tenantGetTestOwner(rbac.UserOwner, "alice"), + tenantGetTestPrefixOverride(true), + ), + }, + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + }, + wantMessage: "The Namespace name must start with 'dummy-' when ForceTenantPrefix is enabled in the Tenant.", + }, + { + name: "capsule user without tenant is denied", + forcePrefix: true, + namespace: "workloads", + user: users.AdmissionUser{ + Type: users.AdmissionUserCapsule, + Username: "alice", + }, + wantMessage: "You do not have any Tenant assigned: please, reach out to the system administrators", + }, + { + name: "administrator without tenant is not assigned", + forcePrefix: true, + namespace: "workloads", + user: users.AdmissionUser{ + Type: users.AdmissionUserAdmin, + Username: "admin", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + cl, cfg := tenantGetTestClient(t, tt.forcePrefix, tt.tenants...) + + got, response := GetNamespaceTenant( + ctx, + cl, + cl, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: tt.namespace}}, + tt.user, + cfg, + nil, + ) + + if tt.wantMessage != "" { + if response == nil { + t.Fatal("GetNamespaceTenant() response = nil, want denial") + } + if response.Result.Message != tt.wantMessage { + t.Fatalf("GetNamespaceTenant() message = %q, want %q", response.Result.Message, tt.wantMessage) + } + if got != nil { + t.Fatalf("GetNamespaceTenant() Tenant = %#v, want nil", got) + } + + return + } + + if response != nil { + t.Fatalf("GetNamespaceTenant() unexpected response: %#v", response.Result) + } + if tt.wantTenant == "" { + if got != nil { + t.Fatalf("GetNamespaceTenant() Tenant = %#v, want nil", got) + } + + return + } + if got == nil || got.Name != tt.wantTenant { + t.Fatalf("GetNamespaceTenant() = %#v, want Tenant %q", got, tt.wantTenant) + } + }) + } +} + +func TestResolveTenantByClosestNamespacePrefix(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + namespace string + tenantNames []string + want string + wantAmbiguous bool + }{ + { + name: "no match", + namespace: "workloads", + tenantNames: []string{"team", "platform"}, + }, + { + name: "prefix requires delimiter", + namespace: "teamworkloads", + tenantNames: []string{"team"}, + }, + { + name: "one match", + namespace: "team-workloads", + tenantNames: []string{"team", "platform"}, + want: "team", + }, + { + name: "closest match independent of order", + namespace: "team-platform-workloads", + tenantNames: []string{"team", "team-platform"}, + want: "team-platform", + }, + { + name: "duplicate candidate is not ambiguous", + namespace: "team-workloads", + tenantNames: []string{"team", "team"}, + want: "team", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + tenants := make([]capsulev1beta2.Tenant, 0, len(tt.tenantNames)) + for _, name := range tt.tenantNames { + tenants = append(tenants, capsulev1beta2.Tenant{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + }) + } + + got, ambiguous := resolveTenantByClosestNamespacePrefix(tt.namespace, tenants) + if ambiguous != tt.wantAmbiguous { + t.Fatalf("resolveTenantByClosestNamespacePrefix() ambiguous = %t, want %t", ambiguous, tt.wantAmbiguous) + } + if tt.want == "" { + if got != nil { + t.Fatalf("resolveTenantByClosestNamespacePrefix() = %#v, want nil", got) + } + + return + } + if got == nil || got.Name != tt.want { + t.Fatalf("resolveTenantByClosestNamespacePrefix() = %#v, want Tenant %q", got, tt.want) + } + }) + } +} + +func TestValidateNamespacePrefix(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + forcePrefix bool + override *bool + namespace string + want bool + }{ + { + name: "disabled", + namespace: "workloads", + want: true, + }, + { + name: "configured and matching", + forcePrefix: true, + namespace: "team-workloads", + want: true, + }, + { + name: "configured and not matching", + forcePrefix: true, + namespace: "workloads", + want: false, + }, + { + name: "tenant disables configuration", + forcePrefix: true, + override: boolPointer(false), + namespace: "workloads", + want: true, + }, + { + name: "tenant enables prefix", + override: boolPointer(true), + namespace: "workloads", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + _, cfg := tenantGetTestClient(t, tt.forcePrefix) + tenant := &capsulev1beta2.Tenant{ + ObjectMeta: metav1.ObjectMeta{Name: "team"}, + Spec: capsulev1beta2.TenantSpec{ + ForceTenantPrefix: tt.override, + }, + } + + got := validateNamespacePrefix( + cfg, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: tt.namespace}}, + tenant, + ) + if got != tt.want { + t.Fatalf("validateNamespacePrefix() = %t, want %t", got, tt.want) + } + }) + } +} + +func tenantGetTestClient( + t *testing.T, + forcePrefix bool, + tenants ...*capsulev1beta2.Tenant, +) (client.Client, configuration.Configuration) { + t.Helper() + + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := capsulev1beta2.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + const configurationName = "capsule" + + objects := make([]client.Object, 0, len(tenants)+1) + objects = append(objects, &capsulev1beta2.CapsuleConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: configurationName}, + Spec: capsulev1beta2.CapsuleConfigurationSpec{ + ForceTenantPrefix: forcePrefix, + }, + }) + for _, tenant := range tenants { + objects = append(objects, tenant) + } + + cl := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + WithIndex(&capsulev1beta2.Tenant{}, ".spec.owner.ownerkind", func(obj client.Object) []string { + return tenantGetTestOwnerKeys(obj.(*capsulev1beta2.Tenant).Status.Owners) + }). + Build() + + return cl, configuration.NewCapsuleConfiguration( + context.Background(), + cl, + cl, + nil, + configurationName, + ) +} + +func tenantGetTestTenant(name string, opts ...func(*capsulev1beta2.Tenant)) *capsulev1beta2.Tenant { + tenant := &capsulev1beta2.Tenant{ObjectMeta: metav1.ObjectMeta{Name: name}} + for _, opt := range opts { + opt(tenant) + } + + return tenant +} + +func tenantGetTestOwner(kind rbac.OwnerKind, name string) func(*capsulev1beta2.Tenant) { + return func(tenant *capsulev1beta2.Tenant) { + owner := rbac.CoreOwnerSpec{UserSpec: rbac.UserSpec{Kind: kind, Name: name}} + tenant.Status.Owners = append(tenant.Status.Owners, owner) + } +} + +func tenantGetTestPrefixOverride(value bool) func(*capsulev1beta2.Tenant) { + return func(tenant *capsulev1beta2.Tenant) { + tenant.Spec.ForceTenantPrefix = boolPointer(value) + } +} + +func tenantGetTestOwnerKeys(owners rbac.OwnerStatusListSpec) []string { + keys := make([]string, 0, len(owners)) + for _, owner := range owners { + keys = append(keys, owner.Kind.String()+":"+owner.Name) + } + + return keys +} + +func boolPointer(value bool) *bool { + return &value +} diff --git a/pkg/tenant/get_by.go b/pkg/tenant/get_by.go index a6c2497e..1ae937b7 100644 --- a/pkg/tenant/get_by.go +++ b/pkg/tenant/get_by.go @@ -178,7 +178,23 @@ func GetTenantByUserInfo( ns *corev1.Namespace, user users.AdmissionUser, ) (sortedTenants, error) { - var tenants sortedTenants + tenants := make(sortedTenants, 0) + seen := make(map[string]struct{}) + + // A requester can match the same Tenant through multiple owner identities + // (for example, both directly and through one or more groups). + appendUnique := func(items []capsulev1beta2.Tenant) { + for i := range items { + name := items[i].GetName() + if _, ok := seen[name]; ok { + continue + } + + seen[name] = struct{}{} + + tenants = append(tenants, items[i]) + } + } // User tenants. userTntList := &capsulev1beta2.TenantList{} @@ -191,7 +207,7 @@ func GetTenantByUserInfo( return nil, err } - tenants = userTntList.Items + appendUnique(userTntList.Items) // ServiceAccount tenants. if strings.HasPrefix(user.Username, "system:serviceaccount:") { @@ -205,7 +221,7 @@ func GetTenantByUserInfo( return nil, err } - tenants = append(tenants, saTntList.Items...) + appendUnique(saTntList.Items) } // Group tenants. @@ -221,7 +237,7 @@ func GetTenantByUserInfo( return nil, err } - tenants = append(tenants, groupTntList.Items...) + appendUnique(groupTntList.Items) } sort.Sort(sort.Reverse(tenants)) diff --git a/pkg/tenant/get_by_test.go b/pkg/tenant/get_by_test.go index 66ef8735..09cb7016 100644 --- a/pkg/tenant/get_by_test.go +++ b/pkg/tenant/get_by_test.go @@ -158,6 +158,11 @@ func TestGetTenantByUserInfo(t *testing.T) { tenantObject("short", withSpecOwner(rbac.UserOwner, "alice")), tenantObject("very-long-name", withSpecOwner(rbac.GroupOwner, "developers")), tenantObject("service", withSpecOwner(rbac.ServiceAccountOwner, users.ServiceAccountUsername("tenant-a", "builder"))), + tenantObject( + "shared", + withSpecOwner(rbac.ServiceAccountOwner, users.ServiceAccountUsername("tenant-a", "builder")), + withSpecOwner(rbac.GroupOwner, "developers"), + ), ) got, err := tenant.GetTenantByUserInfo(ctx, cl, nil, nil, users.AdmissionUser{ @@ -173,8 +178,8 @@ func TestGetTenantByUserInfo(t *testing.T) { names = append(names, tnt.Name) } - if !reflect.DeepEqual(names, []string{"very-long-name", "service"}) { - t.Fatalf("GetTenantByUserInfo() names = %#v, want sorted matching tenants", names) + if !reflect.DeepEqual(names, []string{"very-long-name", "service", "shared"}) { + t.Fatalf("GetTenantByUserInfo() names = %#v, want unique sorted matching tenants", names) } }