From aae694f5394a0571b6e580b0aee5dd99f8bf7326 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Mon, 13 Apr 2026 12:18:18 +0200 Subject: [PATCH] chore: add review comments and clean up whitespace Document the upgrade gap for pre-existing Roles without the ClusterLabelName label, the single-owner assumption in SetControllerReference, and the race window between the Pre hook and the ObjectStore controller. Signed-off-by: Marco Nenciarini --- internal/cnpgi/operator/rbac/ensure.go | 7 +++++++ internal/cnpgi/operator/specs/ownership.go | 5 +++++ internal/cnpgi/restore/manager.go | 1 - internal/controller/objectstore_controller.go | 4 ++++ 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/cnpgi/operator/rbac/ensure.go b/internal/cnpgi/operator/rbac/ensure.go index 44bf706..b9d8c6a 100644 --- a/internal/cnpgi/operator/rbac/ensure.go +++ b/internal/cnpgi/operator/rbac/ensure.go @@ -42,6 +42,13 @@ import ( // 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. +// +// Note: the ObjectStore controller (EnsureRoleRules) can patch the +// same Role concurrently. Both paths use RetryOnConflict but compute +// desired rules from their own view of ObjectStores. If the Pre hook +// reads stale ObjectStore data from the informer cache, it may +// briefly revert a fresher update. This is self-healing: the next +// ObjectStore reconcile restores the correct state. func EnsureRole( ctx context.Context, c client.Client, diff --git a/internal/cnpgi/operator/specs/ownership.go b/internal/cnpgi/operator/specs/ownership.go index 7bc6c74..dded5d1 100644 --- a/internal/cnpgi/operator/specs/ownership.go +++ b/internal/cnpgi/operator/specs/ownership.go @@ -33,6 +33,11 @@ import ( // the operator does not know the CNPG API group at compile time // (it may be customized), while the Cluster object decoded from // the gRPC request carries the correct GVK in its TypeMeta. +// +// This function replaces all existing owner references rather than +// merging, so it assumes the controlled object has a single owner. +// This holds for plugin-managed Roles and RoleBindings, which are +// exclusively owned by one Cluster. func SetControllerReference(owner, controlled metav1.Object) error { ro, ok := owner.(runtime.Object) if !ok { diff --git a/internal/cnpgi/restore/manager.go b/internal/cnpgi/restore/manager.go index c14a02a..bcfe8c9 100644 --- a/internal/cnpgi/restore/manager.go +++ b/internal/cnpgi/restore/manager.go @@ -32,7 +32,6 @@ import ( "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/common" ) - // Start starts the sidecar informers and CNPG-i server func Start(ctx context.Context) error { setupLog := log.FromContext(ctx) diff --git a/internal/controller/objectstore_controller.go b/internal/controller/objectstore_controller.go index a79b7c9..e0a77dd 100644 --- a/internal/controller/objectstore_controller.go +++ b/internal/controller/objectstore_controller.go @@ -68,6 +68,10 @@ func (r *ObjectStoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) contextLogger.Info("ObjectStore reconciliation start") + // NOTE: Roles created before the introduction of ClusterLabelName + // are not discovered here. The Pre hook patches the label on every + // Cluster reconciliation, so unlabeled Roles are picked up after + // the next Cluster reconcile cycle. var roleList rbacv1.RoleList if err := r.List(ctx, &roleList, client.InNamespace(req.Namespace),