Skip to content

fix(rbac): reconcile Role when ObjectStore spec changes#823

Open
armru wants to merge 7 commits intomainfrom
dev/trigger-reconciliation
Open

fix(rbac): reconcile Role when ObjectStore spec changes#823
armru wants to merge 7 commits intomainfrom
dev/trigger-reconciliation

Conversation

@armru
Copy link
Copy Markdown
Member

@armru armru commented Mar 26, 2026

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 label set by the
Pre hook, inspects their rules to find which ObjectStores they
reference, fetches those ObjectStores, and patches the rules to
match the current specs.

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

@armru armru requested a review from a team as a code owner March 26, 2026 15:15
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. go Pull requests that update go code labels Mar 26, 2026
@armru armru force-pushed the dev/trigger-reconciliation branch 2 times, most recently from 064864b to bd2b992 Compare March 27, 2026 11:31
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Mar 27, 2026
@armru armru force-pushed the dev/trigger-reconciliation branch 2 times, most recently from 8fc4251 to 840fbc8 Compare March 27, 2026 15:46
@armru armru added this to the v0.12.0 milestone Apr 8, 2026
@mnencia mnencia force-pushed the dev/trigger-reconciliation branch from 840fbc8 to 3593c18 Compare April 9, 2026 12:43
@mnencia
Copy link
Copy Markdown
Member

mnencia commented Apr 9, 2026

I added a second commit that changes the ObjectStore controller to discover affected Roles directly instead of listing Clusters. The plugin should not need get/list/watch on Clusters.

The controller now lists Roles by a label (barmancloud.cnpg.io/cluster) set by the Pre hook, inspects their rules to find which ObjectStores they reference, then fetches those ObjectStores and rebuilds the rules. Conflict handling uses retry.RetryOnConflict to match the existing project pattern, and partial failures across Roles are aggregated with errors.Join instead of failing on the first one.

Pre-existing Roles without the label won't be found by the ObjectStore controller until the Pre hook adds it on the next Cluster reconciliation. Same staleness window as the current main branch.

PR description edited accordingly.

@mnencia mnencia force-pushed the dev/trigger-reconciliation branch 5 times, most recently from 1d638f9 to 134b45f Compare April 10, 2026 15:18
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 10, 2026
@mnencia mnencia force-pushed the dev/trigger-reconciliation branch 2 times, most recently from e89ba3a to 521822f Compare April 10, 2026 16:48
armru and others added 6 commits April 11, 2026 09:14
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 referencing
Clusters and update their Roles directly. Extract ensureRole into a
shared rbac.EnsureRole function used by both the Pre hook and the
ObjectStore controller.

Handle concurrent modifications between the Pre hook and ObjectStore
controller gracefully: AlreadyExists on Create and Conflict on Patch
are retried once to avoid propagating transient errors as gRPC failures
to CNPG.

Replace the custom setOwnerReference helper (ownership.go) with
controllerutil.SetControllerReference for both Role and RoleBinding.
The old helper read the GVK from the object's metadata and replaced
all owner references unconditionally. The new function reads the GVK
from the scheme and appends to existing owner references, refusing to
overwrite if another controller already owns the object. Both produce
identical results for our use case since the Role is always freshly
built. The GVK is now resolved from the scheme configured via
CUSTOM_CNPG_GROUP/CUSTOM_CNPG_VERSION, which must match the actual
CNPG API group (same requirement as the instance sidecar).

Add dynamic CNPG scheme registration (internal/scheme) to the operator,
instance, and restore managers, replacing hardcoded cnpgv1.AddToScheme
calls. Add RBAC permission for the plugin to list/watch Clusters.

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
The ObjectStore controller now lists Roles by a label
(barmancloud.cnpg.io/cluster) set by the Pre hook, inspects their
rules to find which ObjectStores they reference, then fetches those
ObjectStores and rebuilds the rules. This removes the clusters
get/list/watch permission. Conflict handling uses RetryOnConflict to
match the existing project pattern, and partial failures across Roles
are aggregated with errors.Join instead of failing on the first one.

Pre-existing Roles without the label won't be found by the ObjectStore
controller until the Pre hook adds it on the next Cluster
reconciliation. Same staleness window as the current main branch.

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
The operator does not know the CNPG API group at runtime (it is
not a sidecar injected by the CNPG operator, so CUSTOM_CNPG_GROUP
and CUSTOM_CNPG_VERSION are not available). Move SetControllerReference
to the specs package and read the GVK from the decoded Cluster's
TypeMeta rather than looking it up in the scheme.

Remove CNPG types from the operator's scheme and the env var bindings
from cmd/operator since they are no longer needed.

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Add an e2e test that verifies the ObjectStore controller updates the
RBAC Role when an ObjectStore's secret reference changes. The test
creates a Cluster with a MinIO ObjectStore, verifies the Role has the
cluster label and references the original secret, then updates the
ObjectStore to point to a new secret and waits for the Role to be
patched accordingly.

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
- Add 'namespace' structured field to the error log in Reconcile when a
role reconciliation fails
- Rename misleading local variable 'role' to 'roleBinding' in
ensureRoleBinding to match the actual type
- Add EnsureRole tests: transient Role creation error is propagated;
pre-existing unrelated labels are preserved after patch
- Add SetControllerReference test: returns an error when the owner does
not implement runtime.Object
- Add ObjectStoreReconciler tests: Role list failure and ObjectStore Get
transient error both surface through the reconcile return value
- Add scheme tests: AddCNPGToScheme with default and custom
group/version

Assisted-by: Claude Opus 4.6

Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Closes #840

The e2e kustomize overlay matches
docker.io/library/plugin-barman-cloud but the base kustomization at
kubernetes/kustomization.yaml matches the bare plugin-barman-cloud
name from kubernetes/deployment.yaml and replaces it with the GHCR
image. Since the overlay name never matches, the locally-built image
is never deployed and e2e tests always run against the main branch
code from GHCR.

Use the bare image name so the overlay rule overrides the base rule.

Broken since b7daaac (#89) changed the base kustomization's newName
from docker.io/library/plugin-barman-cloud to the GHCR image without
updating the e2e overlay.

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
@mnencia mnencia force-pushed the dev/trigger-reconciliation branch from 521822f to 1f876c2 Compare April 11, 2026 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update go code lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants