fix: correct tenant ownerships resolution with deduplicaiton (#2059)

* chore * perfromance improvements Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * perfromance improvements Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * feat(performance): removed duplicate client calls from all admission paths Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * fix: correct tenant ownerships resolution with deduplicaiton Signed-off-by: Oliver Baehler <oliver@sudo-i.net> --------- Signed-off-by: Oliver Baehler <oliver@sudo-i.net>
2026-08-19 20:46:42 +00:00 · 2026-07-28 19:30:00 +02:00
parent 2252c530f4
commit 613b43aa21
5 changed files with 598 additions and 7 deletions
@@ -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())
+		})
+	},
+)
@@ -120,7 +120,9 @@ func resolveTenantByClosestNamespacePrefix(
 			ambiguous = false

 		case len(prefix) == matchedPrefixLen:
-			ambiguous = true
+			if matched.GetName() != tnts[i].GetName() {
+				ambiguous = true
+			}
 		}
 	}

@@ -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
+}
@@ -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))
@@ -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)
 	}
 }