diff --git a/internal/cnpgi/operator/rbac/ensure.go b/internal/cnpgi/operator/rbac/ensure.go index 1913ee7..00ff711 100644 --- a/internal/cnpgi/operator/rbac/ensure.go +++ b/internal/cnpgi/operator/rbac/ensure.go @@ -21,6 +21,7 @@ package rbac import ( "context" + "fmt" cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" "github.com/cloudnative-pg/machinery/pkg/log" @@ -87,51 +88,141 @@ func EnsureRoleRules( return err } -// EnsureRoleBinding ensures the RoleBinding for the given Cluster matches -// the desired state. +// EnsureRoleBinding ensures the RoleBinding for the given Cluster +// is present and carries the recommended labels. // -// This function is called from the Pre hook (gRPC). It creates the RoleBinding -// if it does not exist, otherwise it patches Subjects and labels to match -// the desired state. +// This function is called from the Pre hook (gRPC). It creates the +// RoleBinding if it does not exist, then reconciles labels and +// Subjects: +// - Labels are written per-key. Keys the plugin manages overwrite +// existing values; unrelated keys (anything outside the desired +// set) are left alone. +// - Subjects are additive. The plugin guarantees its own Subject +// is bound, but never removes Subjects added by other actors — +// a Subject is a grant of access, and silently revoking access +// someone else granted is the wrong default. +// +// RoleRef is immutable in Kubernetes. If the existing RoleBinding +// points to a different Role, the plugin fails loudly so the +// operator notices and recreates the object. func EnsureRoleBinding(ctx context.Context, c client.Client, cluster *cnpgv1.Cluster) error { - contextLogger := log.FromContext(ctx) - desiredRoleBinding := specs.BuildRoleBinding(cluster) if err := specs.SetControllerReference(cluster, desiredRoleBinding); err != nil { return err } - roleBinding := &rbacv1.RoleBinding{} - - if err := c.Get(ctx, client.ObjectKey{ - Namespace: cluster.Namespace, - Name: specs.GetRBACName(cluster.Name), - }, roleBinding); err != nil { - if apierrs.IsNotFound(err) { - contextLogger.Info("Creating RoleBinding", "name", desiredRoleBinding.Name, - "namespace", desiredRoleBinding.Namespace) - return c.Create(ctx, desiredRoleBinding) - } + roleBinding, err := getOrCreateRoleBinding(ctx, c, desiredRoleBinding) + if err != nil || roleBinding == nil { + // Either an error, or we just created the object with the + // desired state — nothing to patch. return err } - if !roleBindingNeedsUpdate(roleBinding, desiredRoleBinding) { - return nil + return reconcileRoleBinding(ctx, c, roleBinding, desiredRoleBinding) +} + +// getOrCreateRoleBinding returns the existing RoleBinding when it +// is already present on the API server, or nil after a successful +// Create when the just-created object already carries the desired +// state (so the caller can skip the patch path). +// +// On a stale-informer-cache race during plugin pod startup, where +// Get returns NotFound but Create returns AlreadyExists, the +// function re-Gets to return the racing winner — the caller then +// falls through to reconciliation against that real object. +func getOrCreateRoleBinding( + ctx context.Context, + c client.Client, + desired *rbacv1.RoleBinding, +) (*rbacv1.RoleBinding, error) { + contextLogger := log.FromContext(ctx) + + rb := &rbacv1.RoleBinding{} + err := c.Get(ctx, client.ObjectKeyFromObject(desired), rb) + if err == nil { + return rb, nil + } + if !apierrs.IsNotFound(err) { + return nil, err } - contextLogger.Info("Patching role binding", - "name", roleBinding.Name, "namespace", roleBinding.Namespace) - - oldRoleBinding := roleBinding.DeepCopy() - if roleBinding.Labels == nil { - roleBinding.Labels = make(map[string]string, len(desiredRoleBinding.Labels)) + createErr := c.Create(ctx, desired) + switch { + case createErr == nil: + contextLogger.Info("Created RoleBinding", + "name", desired.Name, "namespace", desired.Namespace) + // Just-created with the desired state — caller skips patch. + return nil, nil + case apierrs.IsAlreadyExists(createErr): + contextLogger.Debug( + "RoleBinding already exists, likely a stale informer cache; re-fetching", + "name", desired.Name, "namespace", desired.Namespace) + // Re-Get to return the racing winner so the caller can + // reconcile against the real existing object. + fetched := &rbacv1.RoleBinding{} + if err := c.Get(ctx, client.ObjectKeyFromObject(desired), fetched); err != nil { + return nil, err + } + return fetched, nil + default: + return nil, createErr } - for k, v := range desiredRoleBinding.Labels { - roleBinding.Labels[k] = v - } - roleBinding.Subjects = desiredRoleBinding.Subjects +} - return c.Patch(ctx, roleBinding, client.MergeFrom(oldRoleBinding)) +// reconcileRoleBinding brings an existing RoleBinding's labels and +// Subjects into alignment with desired. It re-Gets the object on +// conflict-retry so each attempt observes fresh server state, and +// uses optimistic locking so a competing writer's patch is rejected +// with 409 instead of silently last-write-winning. +// +// On the first attempt the function uses the existing object passed +// in by the caller, avoiding a second Get on the steady-state path. +func reconcileRoleBinding( + ctx context.Context, + c client.Client, + existing, desired *rbacv1.RoleBinding, +) error { + contextLogger := log.FromContext(ctx) + first := true + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + var roleBinding *rbacv1.RoleBinding + if first { + roleBinding = existing + first = false + } else { + roleBinding = &rbacv1.RoleBinding{} + if err := c.Get(ctx, client.ObjectKeyFromObject(desired), roleBinding); err != nil { + return err + } + } + + // RoleRef is immutable in Kubernetes; we cannot patch it. + // Divergence at the canonical name is corruption regardless + // of who wrote the existing object — fail loudly so the + // operator notices and deletes the RoleBinding, and the + // next Pre call recreates it correctly. + if !equality.Semantic.DeepEqual(roleBinding.RoleRef, desired.RoleRef) { + return fmt.Errorf( + "RoleBinding %s/%s has divergent immutable RoleRef "+ + "(existing=%+v, desired=%+v); delete the RoleBinding to allow recreation", + roleBinding.Namespace, roleBinding.Name, + roleBinding.RoleRef, desired.RoleRef) + } + + if !roleBindingNeedsUpdate(roleBinding, desired) { + return nil + } + + contextLogger.Info("Patching role binding", + "name", roleBinding.Name, "namespace", roleBinding.Namespace) + + oldRoleBinding := roleBinding.DeepCopy() + roleBinding.Labels = mergeLabels(roleBinding.Labels, desired.Labels) + roleBinding.Subjects = mergeSubjects(roleBinding.Subjects, desired.Subjects) + + return c.Patch(ctx, roleBinding, + client.MergeFromWithOptions(oldRoleBinding, client.MergeFromWithOptimisticLock{})) + }) } // ensureRoleExists creates the Role if it does not exist. Returns @@ -199,22 +290,32 @@ func patchRole( oldRole := role.DeepCopy() role.Rules = desiredRules + role.Labels = mergeLabels(role.Labels, desiredLabels) - if desiredLabels != nil { - if role.Labels == nil { - role.Labels = make(map[string]string, len(desiredLabels)) - } - for k, v := range desiredLabels { - role.Labels[k] = v - } - } - - return c.Patch(ctx, &role, client.MergeFrom(oldRole)) + return c.Patch(ctx, &role, + client.MergeFromWithOptions(oldRole, client.MergeFromWithOptimisticLock{})) }) } -// labelsNeedUpdate returns true if any key in desired is missing -// or has a different value in existing. +// mergeLabels writes the desired labels onto existing per-key. +// Keys in desired overwrite the existing value; keys not in desired +// (any unrelated label a user may have set) are left alone. +func mergeLabels(existing, desired map[string]string) map[string]string { + if len(desired) == 0 { + return existing + } + if existing == nil { + existing = make(map[string]string, len(desired)) + } + for k, v := range desired { + existing[k] = v + } + return existing +} + +// labelsNeedUpdate returns true if a Patch is required to bring +// existing labels into the state mergeLabels would produce, i.e. +// any desired key is missing or has a different value in existing. func labelsNeedUpdate(existing, desired map[string]string) bool { for k, v := range desired { if existing[k] != v { @@ -224,11 +325,44 @@ func labelsNeedUpdate(existing, desired map[string]string) bool { return false } -// roleBindingNeedsUpdate returns true if the existing RoleBinding's -// Subjects differ from the desired or if labels need update. +// containsSubject reports whether subjects contains an element that +// is semantically equal to subject. +func containsSubject(subjects []rbacv1.Subject, subject rbacv1.Subject) bool { + for _, s := range subjects { + if equality.Semantic.DeepEqual(s, subject) { + return true + } + } + return false +} + +// mergeSubjects appends desired Subjects that are not already +// present in existing. +// +// This is intentionally asymmetric to mergeLabels: labels are +// metadata, so replacing stale plugin-set values is safe. A +// Subject is a grant of access, so removing a Subject silently +// revokes permissions an external operator chose to grant. The +// plugin only requires that ITS Subject is present, not that it +// is exclusive. +func mergeSubjects(existing, desired []rbacv1.Subject) []rbacv1.Subject { + for _, d := range desired { + if !containsSubject(existing, d) { + existing = append(existing, d) + } + } + return existing +} + +// roleBindingNeedsUpdate returns true if a Patch is required to +// bring existing into alignment with desired — any desired Subject +// missing (see mergeSubjects), or any desired label key missing +// or holding a stale value (see mergeLabels). func roleBindingNeedsUpdate(existing, desired *rbacv1.RoleBinding) bool { - if !equality.Semantic.DeepEqual(existing.Subjects, desired.Subjects) { - return true + for _, s := range desired.Subjects { + if !containsSubject(existing.Subjects, s) { + return true + } } if labelsNeedUpdate(existing.Labels, desired.Labels) { diff --git a/internal/cnpgi/operator/rbac/ensure_test.go b/internal/cnpgi/operator/rbac/ensure_test.go index e3d7885..7744444 100644 --- a/internal/cnpgi/operator/rbac/ensure_test.go +++ b/internal/cnpgi/operator/rbac/ensure_test.go @@ -52,6 +52,33 @@ func expectRequiredLabels(labels map[string]string, clusterName string) { ExpectWithOffset(1, labels).To(HaveKeyWithValue(utils.KubernetesAppVersionLabelName, metadata.Data.Version)) } +// newPatchCountingClient returns a fake client plus a pointer to a +// counter incremented on every Patch call. Useful for asserting +// that a "no-op" reconcile path issues no Patch — more reliable +// than comparing ResourceVersion across reads, which depends on +// fake-client RV-bumping semantics that are not part of the +// controller-runtime contract. +func newPatchCountingClient(initObjs ...client.Object) (client.Client, *int) { + count := 0 + c := fake.NewClientBuilder(). + WithScheme(newScheme()). + WithObjects(initObjs...). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func( + ctx context.Context, + client client.WithWatch, + obj client.Object, + patch client.Patch, + opts ...client.PatchOption, + ) error { + count++ + return client.Patch(ctx, obj, patch, opts...) + }, + }). + Build() + return c, &count +} + func newScheme() *runtime.Scheme { s := runtime.NewScheme() utilruntime.Must(rbacv1.AddToScheme(s)) @@ -139,28 +166,18 @@ var _ = Describe("EnsureRole", func() { }) Context("when the Role exists with matching rules", func() { + var patchCount *int + BeforeEach(func() { - fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + fakeClient, patchCount = newPatchCountingClient() Expect(rbac.EnsureRole(ctx, fakeClient, cluster, objects)).To(Succeed()) + *patchCount = 0 }) It("should not patch the Role", func() { - var before rbacv1.Role - Expect(fakeClient.Get(ctx, client.ObjectKey{ - Namespace: "default", - Name: "test-cluster-barman-cloud", - }, &before)).To(Succeed()) - err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) Expect(err).NotTo(HaveOccurred()) - - var after rbacv1.Role - Expect(fakeClient.Get(ctx, client.ObjectKey{ - Namespace: "default", - Name: "test-cluster-barman-cloud", - }, &after)).To(Succeed()) - - Expect(after.ResourceVersion).To(Equal(before.ResourceVersion)) + Expect(*patchCount).To(BeZero()) }) }) @@ -242,6 +259,38 @@ var _ = Describe("EnsureRole", func() { }) }) + Context("when the Role exists with a stale label value", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + existing := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-barman-cloud", + Namespace: "default", + Labels: map[string]string{ + // Stale value as if written by an older plugin + // version. + utils.KubernetesAppVersionLabelName: "0.0.0-stale", + }, + }, + } + Expect(fakeClient.Create(ctx, existing)).To(Succeed()) + }) + + It("should overwrite the stale value with the current plugin's value", func() { + err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) + Expect(err).NotTo(HaveOccurred()) + + var role rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &role)).To(Succeed()) + + Expect(role.Labels).To(HaveKeyWithValue( + utils.KubernetesAppVersionLabelName, metadata.Data.Version)) + }) + }) + Context("when the Role exists without the cluster label (upgrade scenario)", func() { BeforeEach(func() { fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() @@ -316,32 +365,22 @@ var _ = Describe("EnsureRoleBinding", func() { }) Context("when the RoleBinding exists with matching state", func() { + var patchCount *int + BeforeEach(func() { - fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + fakeClient, patchCount = newPatchCountingClient() Expect(rbac.EnsureRoleBinding(ctx, fakeClient, cluster)).To(Succeed()) + *patchCount = 0 }) It("should not patch the RoleBinding", func() { - var before rbacv1.RoleBinding - Expect(fakeClient.Get(ctx, client.ObjectKey{ - Namespace: "default", - Name: "test-cluster-barman-cloud", - }, &before)).To(Succeed()) - err := rbac.EnsureRoleBinding(ctx, fakeClient, cluster) Expect(err).NotTo(HaveOccurred()) - - var after rbacv1.RoleBinding - Expect(fakeClient.Get(ctx, client.ObjectKey{ - Namespace: "default", - Name: "test-cluster-barman-cloud", - }, &after)).To(Succeed()) - - Expect(after.ResourceVersion).To(Equal(before.ResourceVersion)) + Expect(*patchCount).To(BeZero()) }) }) - Context("when the RoleBinding exists with different subjects", func() { + Context("when the RoleBinding exists with extra user-added subjects", func() { BeforeEach(func() { fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() existing := &rbacv1.RoleBinding{ @@ -352,7 +391,7 @@ var _ = Describe("EnsureRoleBinding", func() { Subjects: []rbacv1.Subject{ { Kind: "ServiceAccount", - Name: "old-sa", + Name: "user-debug-sa", APIGroup: "", }, }, @@ -365,7 +404,7 @@ var _ = Describe("EnsureRoleBinding", func() { Expect(fakeClient.Create(ctx, existing)).To(Succeed()) }) - It("should patch the subjects to match the desired state", func() { + It("should add the plugin's subject without removing user-added ones", func() { err := rbac.EnsureRoleBinding(ctx, fakeClient, cluster) Expect(err).NotTo(HaveOccurred()) @@ -375,8 +414,62 @@ var _ = Describe("EnsureRoleBinding", func() { Name: "test-cluster-barman-cloud", }, &rb)).To(Succeed()) - Expect(rb.Subjects).To(HaveLen(1)) - Expect(rb.Subjects[0].Name).To(Equal("test-cluster")) + // Additive policy: the user-added subject must remain. + Expect(rb.Subjects).To(ContainElement(rbacv1.Subject{ + Kind: "ServiceAccount", + Name: "user-debug-sa", + APIGroup: "", + })) + // The plugin's required subject must be present. + Expect(rb.Subjects).To(ContainElement(rbacv1.Subject{ + Kind: "ServiceAccount", + Name: "test-cluster", + Namespace: "default", + APIGroup: "", + })) + }) + }) + + Context("when the RoleBinding exists with a stale label value", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + existing := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-barman-cloud", + Namespace: "default", + Labels: map[string]string{ + utils.KubernetesAppVersionLabelName: "0.0.0-stale", + }, + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: "test-cluster", + Namespace: "default", + APIGroup: "", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "test-cluster-barman-cloud", + }, + } + Expect(fakeClient.Create(ctx, existing)).To(Succeed()) + }) + + It("should overwrite the stale value with the current plugin's value", func() { + err := rbac.EnsureRoleBinding(ctx, fakeClient, cluster) + Expect(err).NotTo(HaveOccurred()) + + var rb rbacv1.RoleBinding + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &rb)).To(Succeed()) + + Expect(rb.Labels).To(HaveKeyWithValue( + utils.KubernetesAppVersionLabelName, metadata.Data.Version)) }) }) @@ -423,6 +516,108 @@ var _ = Describe("EnsureRoleBinding", func() { }) }) + Context("when the RoleBinding has a divergent RoleRef", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + existing := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-barman-cloud", + Namespace: "default", + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: "test-cluster", + Namespace: "default", + APIGroup: "", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "wrong-role", + }, + } + Expect(fakeClient.Create(ctx, existing)).To(Succeed()) + }) + + It("should return a descriptive error since RoleRef is immutable", func() { + err := rbac.EnsureRoleBinding(ctx, fakeClient, cluster) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("RoleRef")) + Expect(err.Error()).To(ContainSubstring("wrong-role")) + }) + }) + + Context("when an AlreadyExists race happens during a stale-cache create (plugin pod startup)", func() { + var preExisting *rbacv1.RoleBinding + + BeforeEach(func() { + preExisting = &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-barman-cloud", + Namespace: "default", + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: "test-cluster", + Namespace: "default", + APIGroup: "", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "test-cluster-barman-cloud", + }, + } + + // First Get returns NotFound (simulates cold informer + // cache after plugin pod restart). Subsequent Gets + // fall through to real fake-client behavior. + gets := 0 + fakeClient = fake.NewClientBuilder(). + WithScheme(newScheme()). + WithObjects(preExisting). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func( + ctx context.Context, + c client.WithWatch, + key client.ObjectKey, + obj client.Object, + opts ...client.GetOption, + ) error { + gets++ + if gets == 1 { + return apierrs.NewNotFound( + rbacv1.Resource("rolebindings"), key.Name) + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + }) + + It("should tolerate the AlreadyExists and reconcile from the existing object", func() { + err := rbac.EnsureRoleBinding(ctx, fakeClient, cluster) + Expect(err).NotTo(HaveOccurred()) + + var rb rbacv1.RoleBinding + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &rb)).To(Succeed()) + + Expect(rb.Subjects).To(ContainElement(rbacv1.Subject{ + Kind: "ServiceAccount", + Name: "test-cluster", + Namespace: "default", + APIGroup: "", + })) + }) + }) + Context("when the RoleBinding exists without labels (upgrade scenario)", func() { BeforeEach(func() { fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() @@ -506,23 +701,21 @@ var _ = Describe("EnsureRoleRules", func() { }) It("should not patch when rules already match", func() { - // Seed with the same objects so rules match + // Replace the seeded client with a counting one, + // then re-seed via EnsureRole so the desired rules + // are already in place when EnsureRoleRules runs. + var patchCount *int + fakeClient, patchCount = newPatchCountingClient() cluster := newCluster("test-cluster", "default") Expect(rbac.EnsureRole(ctx, fakeClient, cluster, objects)).To(Succeed()) + *patchCount = 0 roleKey := client.ObjectKey{ Namespace: "default", Name: "test-cluster-barman-cloud", } - - var before rbacv1.Role - Expect(fakeClient.Get(ctx, roleKey, &before)).To(Succeed()) - Expect(rbac.EnsureRoleRules(ctx, fakeClient, roleKey, objects)).To(Succeed()) - - var after rbacv1.Role - Expect(fakeClient.Get(ctx, roleKey, &after)).To(Succeed()) - Expect(after.ResourceVersion).To(Equal(before.ResourceVersion)) + Expect(*patchCount).To(BeZero()) }) It("should not modify labels", func() {