fix(rbac): reconcile Role when ObjectStore spec changes

When an ObjectStore's credentials change (e.g., secret rename), the
RBAC Role granting the Cluster's ServiceAccount access to those secrets
was not updated because nothing triggered a Cluster reconciliation.

Implement the ObjectStore controller's Reconcile to detect affected
Roles and update their rules directly, without needing access to
Cluster objects. The controller lists Roles by a well-known label
(barmancloud.cnpg.io/cluster), inspects their rules to find which
ObjectStores they reference, fetches those ObjectStores, and patches
the rules to match the current specs.

Extract BuildRoleRules into a shared function and add
ObjectStoreNamesFromRole for semantic rule inspection. Add
EnsureRoleRules for the ObjectStore controller path (rules-only patch,
no owner references). Refactor patchRole to use retry.RetryOnConflict
(matching the existing project pattern) and support atomic label +
rules patching.

Replace the custom setOwnerReference helper (ownership.go) with
controllerutil.SetControllerReference. Add dynamic CNPG scheme
registration (internal/scheme) to the operator, instance, and restore
managers.

Remove clusters get/list/watch RBAC permission (the plugin no longer
lists Cluster objects). Retain clusters/finalizers update permission
required by OwnerReferencesPermissionEnforcement admission controller
(see #465).

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>

Author: mnencia

config/rbac/role.yaml

@@ -44,7 +44,6 @@ rules:
   - postgresql.cnpg.io
   resources:
   - backups
-  - clusters
   verbs:
   - get
   - list

internal/cnpgi/metadata/constants.go

@@ -26,6 +26,10 @@ import "github.com/cloudnative-pg/cnpg-i/pkg/identity"
 const PluginName = "barman-cloud.cloudnative-pg.io"
 
 const (
+	// ClusterLabelName is the label applied to RBAC resources created
+	// by this plugin. Its value is the name of the owning Cluster.
+	ClusterLabelName = "barmancloud.cnpg.io/cluster"
+
 	// CheckEmptyWalArchiveFile is the name of the file in the PGDATA that,
 	// if present, requires the WAL archiver to check that the backup object
 	// store is empty.

internal/cnpgi/operator/rbac/ensure.go

@@ -29,130 +29,149 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1"
+	"github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata"
 	"github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/specs"
 )
 
 // EnsureRole ensures the RBAC Role for the given Cluster matches
 // the desired state derived from the given ObjectStores. On creation,
 // the Cluster is set as the owner of the Role for garbage collection.
 //
-// This function is called from both the Pre hook (gRPC) and the
-// ObjectStore controller. To handle concurrent modifications
-// gracefully, AlreadyExists on Create and Conflict on Patch are
-// retried once rather than returned as errors.
+// This function is called from the Pre hook (gRPC). It creates the
+// Role if it does not exist, then patches rules and labels to match
+// the desired state.
 func EnsureRole(
 	ctx context.Context,
 	c client.Client,
 	cluster *cnpgv1.Cluster,
 	barmanObjects []barmancloudv1.ObjectStore,
 ) error {
 	newRole := specs.BuildRole(cluster, barmanObjects)
+	roleKey := client.ObjectKeyFromObject(newRole)
 
-	roleKey := client.ObjectKey{
-		Namespace: newRole.Namespace,
-		Name:      newRole.Name,
+	if err := ensureRoleExists(ctx, c, cluster, newRole); err != nil {
+		return err
 	}
 
-	var role rbacv1.Role
-	err := c.Get(ctx, roleKey, &role)
-
-	switch {
-	case apierrs.IsNotFound(err):
-		role, err := createRole(ctx, c, cluster, newRole)
-		if err != nil {
-			return err
-		}
-		if role == nil {
-			// Created successfully, nothing else to do.
-			return nil
-		}
-		// AlreadyExists: fall through to patch with the re-read role.
-		return patchRoleRules(ctx, c, newRole.Rules, role)
-
-	case err != nil:
-		return err
+	return patchRole(ctx, c, roleKey, newRole.Rules, map[string]string{
+		metadata.ClusterLabelName: cluster.Name,
+	})
+}
 
-	default:
-		return patchRoleRules(ctx, c, newRole.Rules, &role)
+// EnsureRoleRules updates the rules of an existing Role to match
+// the desired state derived from the given ObjectStores. Unlike
+// EnsureRole, this function does not create Roles or set owner
+// references — it only patches rules on Roles that already exist.
+// It is intended for the ObjectStore controller path where no
+// Cluster object is available. Returns nil if the Role does not
+// exist (the Pre hook has not created it yet).
+func EnsureRoleRules(
+	ctx context.Context,
+	c client.Client,
+	roleKey client.ObjectKey,
+	barmanObjects []barmancloudv1.ObjectStore,
+) error {
+	err := patchRole(ctx, c, roleKey, specs.BuildRoleRules(barmanObjects), nil)
+	if apierrs.IsNotFound(err) {
+		log.FromContext(ctx).Debug("Role not found, skipping rule update",
+			"name", roleKey.Name, "namespace", roleKey.Namespace)
+		return nil
 	}
+
+	return err
 }
 
-// createRole attempts to create the Role. If another writer created
-// it concurrently (AlreadyExists), it re-reads and returns the
-// existing Role for the caller to patch. On success it returns nil.
-func createRole(
+// ensureRoleExists creates the Role if it does not exist. Returns
+// nil on success and nil on AlreadyExists (another writer created
+// it concurrently). The caller always follows up with patchRole.
+func ensureRoleExists(
 	ctx context.Context,
 	c client.Client,
 	cluster *cnpgv1.Cluster,
 	newRole *rbacv1.Role,
-) (*rbacv1.Role, error) {
+) error {
 	contextLogger := log.FromContext(ctx)
 
+	var existing rbacv1.Role
+	err := c.Get(ctx, client.ObjectKeyFromObject(newRole), &existing)
+	if err == nil {
+		return nil
+	}
+	if !apierrs.IsNotFound(err) {
+		return err
+	}
+
 	if err := controllerutil.SetControllerReference(cluster, newRole, c.Scheme()); err != nil {
-		return nil, err
+		return err
 	}
 
 	contextLogger.Info("Creating role",
 		"name", newRole.Name, "namespace", newRole.Namespace)
 
 	createErr := c.Create(ctx, newRole)
-	if createErr == nil {
-		return nil, nil
-	}
-	if !apierrs.IsAlreadyExists(createErr) {
-		return nil, createErr
-	}
-
-	contextLogger.Info("Role was created concurrently, checking rules")
-
-	var role rbacv1.Role
-	if err := c.Get(ctx, client.ObjectKeyFromObject(newRole), &role); err != nil {
-		return nil, err
+	if createErr == nil || apierrs.IsAlreadyExists(createErr) {
+		return nil
 	}
 
-	return &role, nil
+	return createErr
 }
 
-// patchRoleRules patches the Role's rules if they differ from the
-// desired state. On Conflict (concurrent modification), it retries
-// once with a fresh read.
-func patchRoleRules(
+// patchRole patches the Role's rules and optionally its labels to
+// match the desired state. When desiredLabels is nil, labels are
+// not modified. Uses retry.RetryOnConflict for concurrent
+// modification handling.
+func patchRole(
 	ctx context.Context,
 	c client.Client,
+	roleKey client.ObjectKey,
 	desiredRules []rbacv1.PolicyRule,
-	role *rbacv1.Role,
+	desiredLabels map[string]string,
 ) error {
-	if equality.Semantic.DeepEqual(desiredRules, role.Rules) {
-		return nil
-	}
+	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		var role rbacv1.Role
+		if err := c.Get(ctx, roleKey, &role); err != nil {
+			return err
+		}
 
-	contextLogger := log.FromContext(ctx)
-	contextLogger.Info("Patching role",
-		"name", role.Name, "namespace", role.Namespace, "rules", desiredRules)
+		rulesMatch := equality.Semantic.DeepEqual(desiredRules, role.Rules)
+		labelsMatch := desiredLabels == nil || !labelsNeedUpdate(role.Labels, desiredLabels)
+
+		if rulesMatch && labelsMatch {
+			return nil
+		}
 
-	oldRole := role.DeepCopy()
-	role.Rules = desiredRules
+		contextLogger := log.FromContext(ctx)
+		contextLogger.Info("Patching role",
+			"name", role.Name, "namespace", role.Namespace)
 
-	patchErr := c.Patch(ctx, role, client.MergeFrom(oldRole))
-	if patchErr == nil || !apierrs.IsConflict(patchErr) {
-		return patchErr
-	}
+		oldRole := role.DeepCopy()
+		role.Rules = desiredRules
 
-	// Conflict: re-read and retry once.
-	contextLogger.Info("Role was modified concurrently, retrying patch")
-	if err := c.Get(ctx, client.ObjectKeyFromObject(role), role); err != nil {
-		return err
-	}
-	if equality.Semantic.DeepEqual(desiredRules, role.Rules) {
-		return nil
-	}
+		if desiredLabels != nil {
+			if role.Labels == nil {
+				role.Labels = make(map[string]string, len(desiredLabels))
+			}
+			for k, v := range desiredLabels {
+				role.Labels[k] = v
+			}
+		}
 
-	oldRole = role.DeepCopy()
-	role.Rules = desiredRules
+		return c.Patch(ctx, &role, client.MergeFrom(oldRole))
+	})
+}
 
-	return c.Patch(ctx, role, client.MergeFrom(oldRole))
+// labelsNeedUpdate returns true if any key in desired 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 {
+			return true
+		}
+	}
+	return false
 }