diff --git a/controllers/authorino_controller.go b/controllers/authorino_controller.go index cd93cf71..23cfa529 100644 --- a/controllers/authorino_controller.go +++ b/controllers/authorino_controller.go @@ -151,12 +151,12 @@ func (r *AuthorinoReconciler) cleanupClusterScopedPermissions(ctx context.Contex // namespaced ones are garbage collected automatically by k8s because of the owner reference // Delete instance-specific ClusterRoleBindings - managerBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crName, reconcilers.AuthorinoManagerClusterRoleBindingName, reconcilers.AuthorinoManagerClusterRoleName, sa, labels) + managerBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crNamespacedName.Namespace, crName, reconcilers.AuthorinoManagerClusterRoleBindingName, reconcilers.AuthorinoManagerClusterRoleName, sa, labels) if err := r.Client.Delete(ctx, managerBinding); err != nil && !errors.IsNotFound(err) { r.Log.Error(err, "failed to delete ClusterRoleBinding", "name", managerBinding.Name) } - k8sAuthBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crName, reconcilers.AuthorinoK8sAuthClusterRoleBindingName, reconcilers.AuthorinoK8sAuthClusterRoleName, sa, labels) + k8sAuthBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crNamespacedName.Namespace, crName, reconcilers.AuthorinoK8sAuthClusterRoleBindingName, reconcilers.AuthorinoK8sAuthClusterRoleName, sa, labels) if err := r.Client.Delete(ctx, k8sAuthBinding); err != nil && !errors.IsNotFound(err) { r.Log.Error(err, "failed to delete ClusterRoleBinding", "name", k8sAuthBinding.Name) } diff --git a/controllers/authorino_controller_test.go b/controllers/authorino_controller_test.go index ba444188..08464aa7 100644 --- a/controllers/authorino_controller_test.go +++ b/controllers/authorino_controller_test.go @@ -120,7 +120,7 @@ var _ = Describe("Authorino controller", func() { var bindingNsdName types.NamespacedName if authorinoInstance.Spec.ClusterWide { binding = &k8srbac.ClusterRoleBinding{} - bindingNsdName = types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName} + bindingNsdName = types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName} } else { binding = &k8srbac.RoleBinding{} bindingNsdName = namespacedName(testAuthorinoNamespace, authorinoInstance.Name+"-authorino") @@ -132,7 +132,7 @@ var _ = Describe("Authorino controller", func() { // Authorino Auth ClusterRoleBinding k8sAuthBinding := &k8srbac.ClusterRoleBinding{} - k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName} + k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName} Eventually(func(ctx context.Context) error { return k8sClient.Get(ctx, k8sAuthBindingNsdName, k8sAuthBinding) @@ -378,7 +378,7 @@ var _ = Describe("Authorino controller", func() { // Authorino ClusterRoleBinding binding := &k8srbac.ClusterRoleBinding{} - bindingNsdName := types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName} + bindingNsdName := types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName} Eventually(func(g Gomega, ctx context.Context) { g.Expect(k8sClient.Get(ctx, bindingNsdName, binding)).ToNot(HaveOccurred()) @@ -389,7 +389,7 @@ var _ = Describe("Authorino controller", func() { // Authorino Auth ClusterRoleBinding k8sAuthBinding := &k8srbac.ClusterRoleBinding{} - k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName} + k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName} Eventually(func(g Gomega, ctx context.Context) { g.Expect(k8sClient.Get(ctx, k8sAuthBindingNsdName, k8sAuthBinding)).ToNot(HaveOccurred()) @@ -417,7 +417,7 @@ var _ = Describe("Authorino controller", func() { Expect(k8sClient.Create(ctx, authorinoInstance)).Should(Succeed()) // Update binding name for the new instance - bindingNsdName = types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName} + bindingNsdName = types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName} // manager cluster role binding should get service account added Eventually(func(g Gomega, ctx context.Context) { @@ -430,6 +430,173 @@ var _ = Describe("Authorino controller", func() { }) }) + // Regression coverage for + // https://github.com/Kuadrant/authorino-operator/issues/348: two cluster-wide + // Authorino instances that share the same CR name in different namespaces must + // each own a distinct, namespace-qualified k8s-auth ClusterRoleBinding and must + // not evict each other's ServiceAccount. + Context("Same-named cluster-wide instances across namespaces", func() { + const sharedName = "authorino" + const nsA = "authorino-ns-a" + const nsB = "authorino-ns-b" + + newClusterWideInstance := func(name, namespace string) *api.Authorino { + return &api.Authorino{ + TypeMeta: v1.TypeMeta{ + APIVersion: api.GroupVersion.String(), + Kind: "Authorino", + }, + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: api.AuthorinoSpec{ + Image: "example.com/authorino:latest", + Replicas: pointer.Int32(1), + ClusterWide: true, + Listener: api.Listener{Tls: api.Tls{Enabled: pointer.Bool(false)}}, + OIDCServer: api.OIDCServer{Tls: api.Tls{Enabled: pointer.Bool(false)}}, + }, + } + } + + k8sAuthBindingName := func(namespace, name string) types.NamespacedName { + return types.NamespacedName{Name: namespace + "." + name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName} + } + + BeforeEach(func(ctx context.Context) { + for _, ns := range []string{nsA, nsB} { + namespace := &k8score.Namespace{ObjectMeta: v1.ObjectMeta{Name: ns}} + if err := k8sClient.Create(ctx, namespace); err != nil && !apierrors.IsAlreadyExists(err) { + Expect(err).ToNot(HaveOccurred()) + } + } + }) + + It("Should give each instance its own binding without evicting the other", func(ctx context.Context) { + instanceA := newClusterWideInstance(sharedName, nsA) + instanceB := newClusterWideInstance(sharedName, nsB) + Expect(k8sClient.Create(ctx, instanceA)).To(Succeed()) + Expect(k8sClient.Create(ctx, instanceB)).To(Succeed()) + + subjectA := authorinoResources.GetSubjectForRoleBinding(authorinoResources.GetAuthorinoServiceAccount(nsA, sharedName, nil)) + subjectB := authorinoResources.GetSubjectForRoleBinding(authorinoResources.GetAuthorinoServiceAccount(nsB, sharedName, nil)) + + // Each instance provisions its own namespace-qualified binding pointing + // at its own ServiceAccount. + Eventually(func(g Gomega, ctx context.Context) { + bindingA := &k8srbac.ClusterRoleBinding{} + g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsA, sharedName), bindingA)).To(Succeed()) + g.Expect(bindingA.Subjects).To(ContainElement(subjectA)) + + bindingB := &k8srbac.ClusterRoleBinding{} + g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsB, sharedName), bindingB)).To(Succeed()) + g.Expect(bindingB.Subjects).To(ContainElement(subjectB)) + }).WithContext(ctx).Should(Succeed()) + + // Force instance A to reconcile by mutating a label, which propagates + // from the CR to its ClusterRoleBinding. + const propagatedLabel = "test.authorino.kuadrant.io/propagated" + Eventually(func(g Gomega, ctx context.Context) { + g.Expect(k8sClient.Get(ctx, namespacedName(nsA, sharedName), instanceA)).To(Succeed()) + if instanceA.Labels == nil { + instanceA.Labels = map[string]string{} + } + instanceA.Labels[propagatedLabel] = "yes" + g.Expect(k8sClient.Update(ctx, instanceA)).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + + // Wait until binding A reflects the change, confirming instance A has + // reconciled its permissions. + Eventually(func(g Gomega, ctx context.Context) { + bindingA := &k8srbac.ClusterRoleBinding{} + g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsA, sharedName), bindingA)).To(Succeed()) + g.Expect(bindingA.Labels).To(HaveKeyWithValue(propagatedLabel, "yes")) + }).WithContext(ctx).Should(Succeed()) + + // Instance B's binding must still contain only its own ServiceAccount + // (no cross-instance eviction). + Consistently(func(g Gomega, ctx context.Context) { + bindingB := &k8srbac.ClusterRoleBinding{} + g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsB, sharedName), bindingB)).To(Succeed()) + g.Expect(bindingB.Subjects).To(ConsistOf(subjectB)) + }, "5s", "1s").WithContext(ctx).Should(Succeed()) + }) + }) + + Context("Legacy ClusterRoleBinding migration", func() { + It("Should remove the pre-namespace-qualified ClusterRoleBinding after reconcile", func(ctx context.Context) { + authorinoInstance := newFullAuthorinoInstance() + authorinoInstance.Spec.ClusterWide = true + + // Simulate a binding created by an older operator version, whose name + // omits the namespace, owned by this instance's ServiceAccount. + sa := authorinoResources.GetAuthorinoServiceAccount(testAuthorinoNamespace, authorinoInstance.Name, nil) + legacyName := authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName + legacyBinding := &k8srbac.ClusterRoleBinding{ + ObjectMeta: v1.ObjectMeta{Name: legacyName}, + RoleRef: k8srbac.RoleRef{ + APIGroup: k8srbac.GroupName, + Kind: "ClusterRole", + Name: reconcilers.AuthorinoK8sAuthClusterRoleName, + }, + Subjects: []k8srbac.Subject{authorinoResources.GetSubjectForRoleBinding(sa)}, + } + Expect(k8sClient.Create(ctx, legacyBinding)).To(Succeed()) + + Expect(k8sClient.Create(ctx, authorinoInstance)).To(Succeed()) + + // The namespace-qualified binding is provisioned. + newBindingName := types.NamespacedName{Name: testAuthorinoNamespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName} + Eventually(func(g Gomega, ctx context.Context) { + g.Expect(k8sClient.Get(ctx, newBindingName, &k8srbac.ClusterRoleBinding{})).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + + // The legacy binding is cleaned up. + Eventually(func(g Gomega, ctx context.Context) { + err := k8sClient.Get(ctx, types.NamespacedName{Name: legacyName}, &k8srbac.ClusterRoleBinding{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }).WithContext(ctx).Should(Succeed()) + }) + + It("Should not remove a legacy ClusterRoleBinding owned by an instance in another namespace", func(ctx context.Context) { + authorinoInstance := newFullAuthorinoInstance() + authorinoInstance.Spec.ClusterWide = true + + // A legacy binding whose name collides with this instance's (same CR + // name) but which belongs to an instance in a different namespace: + // its subject references a ServiceAccount in that other namespace. + foreignSA := authorinoResources.GetAuthorinoServiceAccount("other-namespace", authorinoInstance.Name, nil) + legacyName := authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName + foreignBinding := &k8srbac.ClusterRoleBinding{ + ObjectMeta: v1.ObjectMeta{Name: legacyName}, + RoleRef: k8srbac.RoleRef{ + APIGroup: k8srbac.GroupName, + Kind: "ClusterRole", + Name: reconcilers.AuthorinoK8sAuthClusterRoleName, + }, + Subjects: []k8srbac.Subject{authorinoResources.GetSubjectForRoleBinding(foreignSA)}, + } + Expect(k8sClient.Create(ctx, foreignBinding)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + _ = k8sClient.Delete(ctx, foreignBinding) + }) + + Expect(k8sClient.Create(ctx, authorinoInstance)).To(Succeed()) + + // Once this instance's own namespace-qualified binding exists, the + // cleanup has run; the foreign legacy binding must still be present. + newBindingName := types.NamespacedName{Name: testAuthorinoNamespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName} + Eventually(func(g Gomega, ctx context.Context) { + g.Expect(k8sClient.Get(ctx, newBindingName, &k8srbac.ClusterRoleBinding{})).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + + Consistently(func(g Gomega, ctx context.Context) { + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: legacyName}, &k8srbac.ClusterRoleBinding{})).To(Succeed()) + }, "2s", "500ms").WithContext(ctx).Should(Succeed()) + }) + }) + Context("Authorino with part TLS enabled", func() { var authorinoInstance *api.Authorino diff --git a/pkg/reconcilers/authorino_reconciler.go b/pkg/reconcilers/authorino_reconciler.go index 0b11fa7a..a12b50c9 100644 --- a/pkg/reconcilers/authorino_reconciler.go +++ b/pkg/reconcilers/authorino_reconciler.go @@ -12,7 +12,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" k8smeta "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -143,9 +142,70 @@ func (r *AuthorinoReconciler) ReconcileAuthorinoPermissions(ctx context.Context, return err } + // Best-effort migration: remove ClusterRoleBindings created with the legacy + // name scheme (-). Those names omit the namespace, so + // instances sharing a CR name in different namespaces collided on a single + // cluster-scoped binding. This runs after the namespace-qualified bindings + // are reconciled, so this instance's own permissions are never dropped + // mid-reconcile. Note that when several instances shared a single legacy + // binding, deleting it here can briefly remove another instance's permissions + // until that instance reconciles and provisions its own namespace-qualified + // binding. + r.cleanupLegacyClusterRoleBindings(ctx, authorinoInstance) + return nil } +// cleanupLegacyClusterRoleBindings deletes the pre-namespace-qualified +// ClusterRoleBindings (named -) for this instance, if present. +// It is best-effort: a missing binding is not an error, and other failures are +// logged rather than surfaced so they don't block reconciliation. +// +// The legacy name is -, and crName is fully controlled by +// whoever creates the Authorino CR. Deleting purely by name would let a tenant +// name a CR so that the operator deletes a legacy binding owned by an instance +// in another namespace. To avoid that, each binding is read and only deleted +// once it is confirmed to belong to this instance: it must reference this +// instance's ServiceAccount as a subject. Bindings that cannot be confirmed are +// left in place. +func (r *AuthorinoReconciler) cleanupLegacyClusterRoleBindings(ctx context.Context, authorinoInstance *api.Authorino) { + logger, _ := logr.FromContext(ctx) + + sa := authorinoResources.GetAuthorinoServiceAccount(authorinoInstance.Namespace, authorinoInstance.Name, authorinoInstance.Labels) + ownSubject := authorinoResources.GetSubjectForRoleBinding(sa) + + for _, suffix := range []string{AuthorinoManagerClusterRoleBindingName, AuthorinoK8sAuthClusterRoleBindingName} { + legacyName := fmt.Sprintf("%s-%s", authorinoInstance.Name, suffix) + + binding := &k8srbac.ClusterRoleBinding{} + if err := r.Client.Get(ctx, client.ObjectKey{Name: legacyName}, binding); err != nil { + if !errors.IsNotFound(err) { + logger.Error(err, "failed to get legacy ClusterRoleBinding", "name", legacyName) + } + continue + } + + if !authorinoResources.SubjectIncluded(binding.Subjects, ownSubject) { + // The binding does not reference this instance's ServiceAccount, so + // it was created for a different Authorino instance (e.g. one sharing + // the CR name in another namespace). Leave it untouched. + logger.Info("skipping legacy ClusterRoleBinding not owned by this instance", "name", legacyName) + continue + } + + // Delete only the exact object that was validated: the resourceVersion and + // UID preconditions make the delete fail rather than remove a binding that + // changed between the Get and the Delete. + preconditions := client.Preconditions{ + UID: &binding.UID, + ResourceVersion: &binding.ResourceVersion, + } + if err := r.Client.Delete(ctx, binding, preconditions); err != nil && !errors.IsNotFound(err) { + logger.Error(err, "failed to delete legacy ClusterRoleBinding", "name", legacyName) + } + } +} + func (r *AuthorinoReconciler) reconcileManagerClusterRoleBinding(ctx context.Context, authorinoInstance *api.Authorino) error { clusterRoleKey := client.ObjectKey{Name: AuthorinoManagerClusterRoleName} if err := r.checkClusterRoleExists(ctx, clusterRoleKey, authorinoInstance); err != nil { @@ -156,13 +216,13 @@ func (r *AuthorinoReconciler) reconcileManagerClusterRoleBinding(ctx context.Con // if cluster scoped, ensure service account is in the binding if authorinoInstance.Spec.ClusterWide { - binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels) + binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Namespace, authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels) return r.reconcileClusterRoleBinding(ctx, binding, authorinoInstance) } // local namespace scope // if switching from cluster-wide to namespaced, delete the ClusterRoleBinding - binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels) + binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Namespace, authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels) TagObjectToDelete(binding) r.reconcileClusterRoleBinding(ctx, binding, authorinoInstance) @@ -194,7 +254,7 @@ func (r *AuthorinoReconciler) reconcileManagerAuthClusterRoleBinding(ctx context sa := authorinoResources.GetAuthorinoServiceAccount(authorinoInstance.Namespace, authorinoInstance.Name, authorinoInstance.Labels) - binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Name, AuthorinoK8sAuthClusterRoleBindingName, AuthorinoK8sAuthClusterRoleName, sa, authorinoInstance.Labels) + binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Namespace, authorinoInstance.Name, AuthorinoK8sAuthClusterRoleBindingName, AuthorinoK8sAuthClusterRoleName, sa, authorinoInstance.Labels) return r.reconcileClusterRoleBinding(ctx, binding, authorinoInstance) } @@ -517,31 +577,3 @@ func (r *AuthorinoReconciler) ReconcileAuthorinoServiceAccount(ctx context.Conte return nil } - -// remove SA from list of subjects of the clusterrolebinding -func (r *AuthorinoReconciler) UnboundAuthorinoServiceAccountFromClusterRole(ctx context.Context, roleBindingName string, sa *k8score.ServiceAccount) { - // TODO: should return error for error handling - logger, _ := logr.FromContext(ctx) - roleBinding := &k8srbac.ClusterRoleBinding{} - if err := r.Client.Get(ctx, types.NamespacedName{Name: roleBindingName}, roleBinding); err == nil { - staleSubject := authorinoResources.GetSubjectForRoleBinding(sa) - var subjects []k8srbac.Subject - for _, subject := range roleBinding.Subjects { - if subject.Kind != staleSubject.Kind || subject.Name != staleSubject.Name || subject.Namespace != staleSubject.Namespace { - subjects = append(subjects, subject) - } - } - - if len(subjects) == 0 { - if err = r.DeleteResource(ctx, roleBinding); err != nil { - logger.Error(err, "failed to delete authorino role binding", "roleBinding", roleBinding, "subject", staleSubject) - } - } else { - // FIXME: This is subject to race condition. The list of subjects may be outdated under concurrent updates - roleBinding.Subjects = subjects - if err = r.Client.Update(ctx, roleBinding); err != nil { - logger.Error(err, "failed to cleanup subject from authorino role binding", "roleBinding", roleBinding, "subject", staleSubject) - } - } - } -} diff --git a/pkg/reconcilers/authorino_reconciler_test.go b/pkg/reconcilers/authorino_reconciler_test.go index 21dd281c..4bf8763b 100644 --- a/pkg/reconcilers/authorino_reconciler_test.go +++ b/pkg/reconcilers/authorino_reconciler_test.go @@ -7,6 +7,8 @@ import ( appsv1 "k8s.io/api/apps/v1" k8score "k8s.io/api/core/v1" + k8srbac "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/pointer" @@ -16,6 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" api "github.com/kuadrant/authorino-operator/api/v1beta1" + authorinoResources "github.com/kuadrant/authorino-operator/pkg/resources" ) var namespace = "test-namespace" @@ -90,15 +93,15 @@ func TestBuildAuthorinoArgs(t *testing.T) { Spec: api.AuthorinoSpec{ Listener: api.Listener{ Tls: api.Tls{ - Enabled: pointer.Bool(false), - MinVersion: "1.2", + Enabled: pointer.Bool(false), + MinVersion: "1.2", CipherSuites: []string{"TLS_AES_128_GCM_SHA256"}, }, }, OIDCServer: api.OIDCServer{ Tls: api.Tls{ - Enabled: pointer.Bool(false), - MinVersion: "1.3", + Enabled: pointer.Bool(false), + MinVersion: "1.3", CipherSuites: []string{"TLS_AES_256_GCM_SHA384"}, }, }, @@ -120,16 +123,16 @@ func TestBuildAuthorinoArgs(t *testing.T) { Spec: api.AuthorinoSpec{ Listener: api.Listener{ Tls: api.Tls{ - Enabled: pointer.Bool(true), - MinVersion: "1.2", - MaxVersion: "1.3", + Enabled: pointer.Bool(true), + MinVersion: "1.2", + MaxVersion: "1.3", CipherSuites: []string{"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384"}, }, }, OIDCServer: api.OIDCServer{ Tls: api.Tls{ - Enabled: pointer.Bool(true), - MinVersion: "1.3", + Enabled: pointer.Bool(true), + MinVersion: "1.3", CipherSuites: []string{"TLS_CHACHA20_POLY1305_SHA256"}, }, }, @@ -386,3 +389,62 @@ func TestReconcileDeployment(t *testing.T) { } }) } + +func clusterRoleBindingSubject(namespace, crName string) k8srbac.Subject { + sa := authorinoResources.GetAuthorinoServiceAccount(namespace, crName, nil) + return authorinoResources.GetSubjectForRoleBinding(sa) +} + +func TestCleanupLegacyClusterRoleBindings(t *testing.T) { + const suffix = AuthorinoK8sAuthClusterRoleBindingName + + t.Run("deletes legacy binding owned by this instance", func(t *testing.T) { + instance := &api.Authorino{ + ObjectMeta: metav1.ObjectMeta{Name: "gateway", Namespace: "team-a"}, + } + legacyName := instance.Name + "-" + suffix + binding := &k8srbac.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: legacyName}, + RoleRef: k8srbac.RoleRef{APIGroup: k8srbac.GroupName, Kind: "ClusterRole", Name: AuthorinoK8sAuthClusterRoleName}, + Subjects: []k8srbac.Subject{clusterRoleBindingSubject("team-a", "gateway")}, + } + r, ctx := setupTestEnvironment(t, []client.Object{binding}) + + r.cleanupLegacyClusterRoleBindings(ctx, instance) + + err := r.Client.Get(ctx, client.ObjectKey{Name: legacyName}, &k8srbac.ClusterRoleBinding{}) + if !apierrors.IsNotFound(err) { + t.Fatalf("expected legacy binding %q to be deleted, got err=%v", legacyName, err) + } + }) + + t.Run("preserves colliding legacy binding owned by another instance", func(t *testing.T) { + // A legacy CR named "team-a.gateway" (in namespace "team") produced a + // binding named "team-a.gateway-", which is byte-identical to the + // new canonical name for namespace "team-a" / CR "gateway". Reconciling + // the "team-a.gateway" instance must not delete (or alter) a binding that + // belongs to the "team-a"/"gateway" instance. + instance := &api.Authorino{ + ObjectMeta: metav1.ObjectMeta{Name: "team-a.gateway", Namespace: "team"}, + } + legacyName := instance.Name + "-" + suffix // "team-a.gateway-" + + foreignSubject := clusterRoleBindingSubject("team-a", "gateway") + binding := &k8srbac.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: legacyName}, + RoleRef: k8srbac.RoleRef{APIGroup: k8srbac.GroupName, Kind: "ClusterRole", Name: AuthorinoK8sAuthClusterRoleName}, + Subjects: []k8srbac.Subject{foreignSubject}, + } + r, ctx := setupTestEnvironment(t, []client.Object{binding}) + + r.cleanupLegacyClusterRoleBindings(ctx, instance) + + got := &k8srbac.ClusterRoleBinding{} + if err := r.Client.Get(ctx, client.ObjectKey{Name: legacyName}, got); err != nil { + t.Fatalf("expected colliding binding %q to be preserved, got err=%v", legacyName, err) + } + if len(got.Subjects) != 1 || got.Subjects[0] != foreignSubject { + t.Errorf("expected legacy subject to be preserved, got %+v", got.Subjects) + } + }) +} diff --git a/pkg/resources/k8s_rbac.go b/pkg/resources/k8s_rbac.go index 00fc8cc2..8888ba2d 100644 --- a/pkg/resources/k8s_rbac.go +++ b/pkg/resources/k8s_rbac.go @@ -13,11 +13,11 @@ func GetAuthorinoServiceAccount(namespace, crName string, labels map[string]stri } } -func GetAuthorinoClusterRoleBinding(crName, clusterRoleBindingNameSuffix, clusterRoleName string, serviceAccount *k8score.ServiceAccount, labels map[string]string) *k8srbac.ClusterRoleBinding { +func GetAuthorinoClusterRoleBinding(namespace, crName, clusterRoleBindingNameSuffix, clusterRoleName string, serviceAccount *k8score.ServiceAccount, labels map[string]string) *k8srbac.ClusterRoleBinding { roleRef, roleSubject := getRoleRefAndSubject(clusterRoleName, "ClusterRole", serviceAccount) return &k8srbac.ClusterRoleBinding{ TypeMeta: k8smeta.TypeMeta{APIVersion: k8srbac.SchemeGroupVersion.String(), Kind: "ClusterRoleBinding"}, - ObjectMeta: k8smeta.ObjectMeta{Name: authorinoClusterRoleBindingName(crName, clusterRoleBindingNameSuffix), Labels: labels}, + ObjectMeta: k8smeta.ObjectMeta{Name: authorinoClusterRoleBindingName(namespace, crName, clusterRoleBindingNameSuffix), Labels: labels}, RoleRef: roleRef, Subjects: []k8srbac.Subject{roleSubject}, } @@ -56,7 +56,9 @@ func GetSubjectForRoleBinding(serviceAccount *k8score.ServiceAccount) k8srbac.Su } } -func subjectIncluded(subjects []k8srbac.Subject, subject k8srbac.Subject) bool { +// SubjectIncluded reports whether the given subject is present in the subjects slice, +// matching on Kind, Name and Namespace. +func SubjectIncluded(subjects []k8srbac.Subject, subject k8srbac.Subject) bool { for _, s := range subjects { if s.Kind == subject.Kind && s.Name == subject.Name && s.Namespace == subject.Namespace { return true @@ -89,25 +91,3 @@ func GetLeaderElectionRules() []k8srbac.PolicyRule { }, } } - -// MergeBindingSubject merges desired subject slice into the existing slice. -// -// The subject entries included in "existing" slice that are not included in the "desired" slice are preserved. -// -// It returns true if the existing slice was modified (i.e., at least one subject was added), -// and false otherwise. -func MergeBindingSubject(desired []k8srbac.Subject, existing *[]k8srbac.Subject) bool { - if existing == nil { - return false - } - - update := false - for idx := range desired { - if !subjectIncluded(*existing, desired[idx]) { - *existing = append(*existing, desired[idx]) - update = true - } - } - - return update -} diff --git a/pkg/resources/k8s_rbac_test.go b/pkg/resources/k8s_rbac_test.go index 9f80e339..67bda536 100644 --- a/pkg/resources/k8s_rbac_test.go +++ b/pkg/resources/k8s_rbac_test.go @@ -1,89 +1,52 @@ package resources import ( - "slices" + "strings" "testing" - k8srbac "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/util/validation" ) -func TestMergeBindingSubject(t *testing.T) { - subjectFoo := k8srbac.Subject{Kind: "Foo"} - subjectBar := k8srbac.Subject{Kind: "Bar"} +func TestAuthorinoClusterRoleBindingNameIsUnambiguous(t *testing.T) { + const suffix = "authorino-k8s-auth" - emptySlice := make([]k8srbac.Subject, 0) - var nilSlice []k8srbac.Subject + // Namespace "a-b"/name "c" and namespace "a"/name "b-c" must not collide: + // a plain "-" join would produce "a-b-c-" for both, causing the two + // cluster-scoped instances to share a single ClusterRoleBinding. + nameOne := authorinoClusterRoleBindingName("a-b", "c", suffix) + nameTwo := authorinoClusterRoleBindingName("a", "b-c", suffix) - type args struct { - existing *[]k8srbac.Subject - desired []k8srbac.Subject + if nameOne == nameTwo { + t.Errorf("expected distinct ClusterRoleBinding names, both resolved to %q", nameOne) } - tests := []struct { - name string - args args - wantUpdate bool - }{ - { - name: "nil pointer to slice", - args: args{ - existing: nil, - desired: []k8srbac.Subject{subjectFoo, subjectBar}, - }, - wantUpdate: false, - }, - { - name: "nil slice", - args: args{ - existing: &nilSlice, - desired: []k8srbac.Subject{subjectFoo, subjectBar}, - }, - wantUpdate: true, - }, - { - name: "empty slice", - args: args{ - existing: &emptySlice, - desired: []k8srbac.Subject{subjectFoo, subjectBar}, - }, - wantUpdate: true, - }, - { - name: "desired subjects not in existing", - args: args{ - existing: &[]k8srbac.Subject{subjectFoo}, - desired: []k8srbac.Subject{subjectBar}, - }, - wantUpdate: true, - }, - { - name: "same slices", - args: args{ - existing: &[]k8srbac.Subject{subjectFoo, subjectBar}, - desired: []k8srbac.Subject{subjectFoo, subjectBar}, - }, - wantUpdate: false, - }, +} + +func TestAuthorinoClusterRoleBindingNameRespectsMaxLength(t *testing.T) { + const suffix = "authorino-k8s-auth" + + longName := strings.Repeat("a", 253) + name := authorinoClusterRoleBindingName("my-namespace", longName, suffix) + + if len(name) > validation.DNS1123SubdomainMaxLength { + t.Errorf("expected name length <= %d, got %d (%q)", validation.DNS1123SubdomainMaxLength, len(name), name) } - for _, tt := range tests { - t.Run(tt.name, func(subT *testing.T) { - got := MergeBindingSubject(tt.args.desired, tt.args.existing) - if got != tt.wantUpdate { - subT.Errorf("MergeBindingSubject() got = %v, wantUpdate %v", got, tt.wantUpdate) - } + if !strings.HasPrefix(name, "my-namespace.") { + t.Errorf("expected name to preserve namespace prefix, got %q", name) + } + if !strings.HasSuffix(name, "-"+suffix) { + t.Errorf("expected name to preserve suffix, got %q", name) + } +} - if tt.args.existing == nil { - return - } +func TestAuthorinoClusterRoleBindingNameTruncationIsUnique(t *testing.T) { + const suffix = "authorino-k8s-auth" - if len(*tt.args.existing) < len(tt.args.desired) { - subT.Error("existing has less subjects than desired") - } + // Two long CR names sharing a common prefix must still yield distinct names. + prefix := strings.Repeat("a", 253) + nameOne := authorinoClusterRoleBindingName("ns", prefix+"one", suffix) + nameTwo := authorinoClusterRoleBindingName("ns", prefix+"two", suffix) - for idx := range tt.args.desired { - if !slices.Contains(*tt.args.existing, tt.args.desired[idx]) { - t.Errorf("MergeBindingSubject() desired subject not in existing: %v", tt.args.desired[idx]) - } - } - }) + if nameOne == nameTwo { + t.Errorf("expected distinct ClusterRoleBinding names for distinct CR names, both resolved to %q", nameOne) } } diff --git a/pkg/resources/k8s_util.go b/pkg/resources/k8s_util.go index 6c941881..a52dbf1a 100644 --- a/pkg/resources/k8s_util.go +++ b/pkg/resources/k8s_util.go @@ -1,9 +1,11 @@ package resources import ( + "crypto/sha256" "fmt" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation" ) func getObjectMeta(namespace, name string, labels map[string]string) v1.ObjectMeta { @@ -25,6 +27,60 @@ func authorinoRoleBindingName(crName, roleBindingNameSuffix string) string { return fmt.Sprintf("%s-%s", crName, roleBindingNameSuffix) } -func authorinoClusterRoleBindingName(crName, clusterRoleBindingNameSuffix string) string { - return fmt.Sprintf("%s-%s", crName, clusterRoleBindingNameSuffix) +// authorinoClusterRoleBindingName builds the name for a cluster-scoped +// ClusterRoleBinding. ClusterRoleBindings are not namespaced, so the CR +// namespace is included in the name to disambiguate Authorino instances that +// share the same CR name across different namespaces. +// +// The namespace and CR name are separated by a "." rather than a "-". A +// Kubernetes namespace is a DNS-1123 label and cannot contain a ".", so the +// segment before the first "." is always exactly the namespace. This keeps the +// name human-readable while staying unambiguous between any two instances using +// this scheme: e.g. namespace "a-b"/name "c" yields "a-b.c-" and +// namespace "a"/name "b-c" yields "a.b-c-", which are distinct. ("." is +// a valid character in a ClusterRoleBinding name, which is an RFC 1123 DNS +// subdomain.) +// +// Migration caveat: the legacy scheme was "-" (no namespace +// segment). A CR name is a DNS-1123 subdomain and may itself contain a ".", so a +// legacy name can coincide with a new name -- e.g. legacy CR "team-a.gateway" +// produced "team-a.gateway-", identical to the new name for namespace +// "team-a"/CR "gateway". Such a collision is only possible between a legacy and +// a new name (never between two new names), so it is confined to the migration +// window. The legacy-binding cleanup guards against acting on the wrong object +// by verifying the binding's subject before deleting it. +// +// The generated name is capped at the RFC 1123 DNS subdomain max length. When a +// long CR name would exceed that limit, the CR name portion is truncated and a +// deterministic hash of the CR name is appended, so distinct long names remain +// uniquely identifiable while the namespace and suffix are always preserved. +func authorinoClusterRoleBindingName(namespace, crName, clusterRoleBindingNameSuffix string) string { + name := fmt.Sprintf("%s.%s-%s", namespace, crName, clusterRoleBindingNameSuffix) + if len(name) <= validation.DNS1123SubdomainMaxLength { + return name + } + + // Deterministic hash of the CR name to keep truncated names unique: two long + // CR names can share a common prefix, so once truncated, uniqueness rests on + // the hash. The namespace is preserved verbatim as the prefix (and cannot + // contain a "."), so a collision is only possible within a single namespace + // -- hashing the namespace as well would add nothing. 64 bits (16 hex chars) + // makes an accidental collision negligible for any realistic set of names. + hash := fmt.Sprintf("%x", sha256.Sum256([]byte(crName)))[:16] + + // Room left for the CR name once the always-preserved parts (namespace, + // hash, suffix and their separators) are accounted for. The hash is + // appended directly onto the truncated CR name, so this fixed portion is + // exactly the final name minus the CR name. + available := validation.DNS1123SubdomainMaxLength - len(fmt.Sprintf("%s.%s-%s", namespace, hash, clusterRoleBindingNameSuffix)) + if available < 0 { + available = 0 + } + + truncatedName := crName + if len(truncatedName) > available { + truncatedName = truncatedName[:available] + } + + return fmt.Sprintf("%s.%s%s-%s", namespace, truncatedName, hash, clusterRoleBindingNameSuffix) }