Skip to content

Commit 8971a39

Browse files
armrumnenciagabriele-wolfox
authored
fix(rbac): reconcile Role when ObjectStore spec changes (#823)
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. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com> Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com> Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com> Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com> Co-authored-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
1 parent b1f373d commit 8971a39

File tree

20 files changed

+1804
-210
lines changed

20 files changed

+1804
-210
lines changed

internal/cnpgi/metadata/constants.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ import "github.com/cloudnative-pg/cnpg-i/pkg/identity"
2626
const PluginName = "barman-cloud.cloudnative-pg.io"
2727

2828
const (
29+
// ClusterLabelName is the label applied to RBAC resources created
30+
// by this plugin. Its value is the name of the owning Cluster.
31+
ClusterLabelName = "barmancloud.cnpg.io/cluster"
32+
2933
// CheckEmptyWalArchiveFile is the name of the file in the PGDATA that,
3034
// if present, requires the WAL archiver to check that the backup object
3135
// store is empty.

internal/cnpgi/operator/manager.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
"crypto/tls"
2525

2626
// +kubebuilder:scaffold:imports
27-
cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
2827
"github.com/cloudnative-pg/machinery/pkg/log"
2928
"github.com/spf13/viper"
3029
"k8s.io/apimachinery/pkg/runtime"
@@ -49,7 +48,6 @@ var scheme = runtime.NewScheme()
4948
func init() {
5049
utilruntime.Must(clientgoscheme.AddToScheme(scheme))
5150
utilruntime.Must(barmancloudv1.AddToScheme(scheme))
52-
utilruntime.Must(cnpgv1.AddToScheme(scheme))
5351
// +kubebuilder:scaffold:scheme
5452
}
5553

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/*
2+
Copyright © contributors to CloudNativePG, established as
3+
CloudNativePG a Series of LF Projects, LLC.
4+
5+
Licensed under the Apache License, Version 2.0 (the "License");
6+
you may not use this file except in compliance with the License.
7+
You may obtain a copy of the License at
8+
9+
http://www.apache.org/licenses/LICENSE-2.0
10+
11+
Unless required by applicable law or agreed to in writing, software
12+
distributed under the License is distributed on an "AS IS" BASIS,
13+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
See the License for the specific language governing permissions and
15+
limitations under the License.
16+
17+
SPDX-License-Identifier: Apache-2.0
18+
*/
19+
20+
// Package rbac contains utilities to reconcile RBAC resources
21+
// for the barman-cloud plugin.
22+
package rbac
Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
/*
2+
Copyright © contributors to CloudNativePG, established as
3+
CloudNativePG a Series of LF Projects, LLC.
4+
5+
Licensed under the Apache License, Version 2.0 (the "License");
6+
you may not use this file except in compliance with the License.
7+
You may obtain a copy of the License at
8+
9+
http://www.apache.org/licenses/LICENSE-2.0
10+
11+
Unless required by applicable law or agreed to in writing, software
12+
distributed under the License is distributed on an "AS IS" BASIS,
13+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
See the License for the specific language governing permissions and
15+
limitations under the License.
16+
17+
SPDX-License-Identifier: Apache-2.0
18+
*/
19+
20+
package rbac
21+
22+
import (
23+
"context"
24+
25+
cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
26+
"github.com/cloudnative-pg/machinery/pkg/log"
27+
rbacv1 "k8s.io/api/rbac/v1"
28+
"k8s.io/apimachinery/pkg/api/equality"
29+
apierrs "k8s.io/apimachinery/pkg/api/errors"
30+
"k8s.io/client-go/util/retry"
31+
"sigs.k8s.io/controller-runtime/pkg/client"
32+
33+
barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1"
34+
"github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata"
35+
"github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/specs"
36+
)
37+
38+
// EnsureRole ensures the RBAC Role for the given Cluster matches
39+
// the desired state derived from the given ObjectStores. On creation,
40+
// the Cluster is set as the owner of the Role for garbage collection.
41+
//
42+
// This function is called from the Pre hook (gRPC). It creates the
43+
// Role if it does not exist, then patches rules and labels to match
44+
// the desired state.
45+
//
46+
// Note: the ObjectStore controller (EnsureRoleRules) can patch the
47+
// same Role concurrently. Both paths use RetryOnConflict but compute
48+
// desired rules from their own view of ObjectStores. If the Pre hook
49+
// reads stale ObjectStore data from the informer cache, it may
50+
// briefly revert a fresher update. This is self-healing: the next
51+
// ObjectStore reconcile restores the correct state.
52+
func EnsureRole(
53+
ctx context.Context,
54+
c client.Client,
55+
cluster *cnpgv1.Cluster,
56+
barmanObjects []barmancloudv1.ObjectStore,
57+
) error {
58+
newRole := specs.BuildRole(cluster, barmanObjects)
59+
roleKey := client.ObjectKeyFromObject(newRole)
60+
61+
if err := ensureRoleExists(ctx, c, cluster, newRole); err != nil {
62+
return err
63+
}
64+
65+
return patchRole(ctx, c, roleKey, newRole.Rules, map[string]string{
66+
metadata.ClusterLabelName: cluster.Name,
67+
})
68+
}
69+
70+
// EnsureRoleRules updates the rules of an existing Role to match
71+
// the desired state derived from the given ObjectStores. Unlike
72+
// EnsureRole, this function does not create Roles or set owner
73+
// references — it only patches rules on Roles that already exist.
74+
// It is intended for the ObjectStore controller path where no
75+
// Cluster object is available. Returns nil if the Role does not
76+
// exist (the Pre hook has not created it yet).
77+
func EnsureRoleRules(
78+
ctx context.Context,
79+
c client.Client,
80+
roleKey client.ObjectKey,
81+
barmanObjects []barmancloudv1.ObjectStore,
82+
) error {
83+
err := patchRole(ctx, c, roleKey, specs.BuildRoleRules(barmanObjects), nil)
84+
if apierrs.IsNotFound(err) {
85+
log.FromContext(ctx).Debug("Role not found, skipping rule update",
86+
"name", roleKey.Name, "namespace", roleKey.Namespace)
87+
return nil
88+
}
89+
90+
return err
91+
}
92+
93+
// ensureRoleExists creates the Role if it does not exist. Returns
94+
// nil on success and nil on AlreadyExists (another writer created
95+
// it concurrently). The caller always follows up with patchRole.
96+
func ensureRoleExists(
97+
ctx context.Context,
98+
c client.Client,
99+
cluster *cnpgv1.Cluster,
100+
newRole *rbacv1.Role,
101+
) error {
102+
contextLogger := log.FromContext(ctx)
103+
104+
var existing rbacv1.Role
105+
err := c.Get(ctx, client.ObjectKeyFromObject(newRole), &existing)
106+
if err == nil {
107+
return nil
108+
}
109+
if !apierrs.IsNotFound(err) {
110+
return err
111+
}
112+
113+
if err := specs.SetControllerReference(cluster, newRole); err != nil {
114+
return err
115+
}
116+
117+
contextLogger.Info("Creating role",
118+
"name", newRole.Name, "namespace", newRole.Namespace)
119+
120+
createErr := c.Create(ctx, newRole)
121+
if createErr == nil || apierrs.IsAlreadyExists(createErr) {
122+
return nil
123+
}
124+
125+
return createErr
126+
}
127+
128+
// patchRole patches the Role's rules and optionally its labels to
129+
// match the desired state. When desiredLabels is nil, labels are
130+
// not modified. Uses retry.RetryOnConflict for concurrent
131+
// modification handling.
132+
func patchRole(
133+
ctx context.Context,
134+
c client.Client,
135+
roleKey client.ObjectKey,
136+
desiredRules []rbacv1.PolicyRule,
137+
desiredLabels map[string]string,
138+
) error {
139+
return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
140+
var role rbacv1.Role
141+
if err := c.Get(ctx, roleKey, &role); err != nil {
142+
return err
143+
}
144+
145+
rulesMatch := equality.Semantic.DeepEqual(desiredRules, role.Rules)
146+
labelsMatch := desiredLabels == nil || !labelsNeedUpdate(role.Labels, desiredLabels)
147+
148+
if rulesMatch && labelsMatch {
149+
return nil
150+
}
151+
152+
contextLogger := log.FromContext(ctx)
153+
contextLogger.Info("Patching role",
154+
"name", role.Name, "namespace", role.Namespace)
155+
156+
oldRole := role.DeepCopy()
157+
role.Rules = desiredRules
158+
159+
if desiredLabels != nil {
160+
if role.Labels == nil {
161+
role.Labels = make(map[string]string, len(desiredLabels))
162+
}
163+
for k, v := range desiredLabels {
164+
role.Labels[k] = v
165+
}
166+
}
167+
168+
return c.Patch(ctx, &role, client.MergeFrom(oldRole))
169+
})
170+
}
171+
172+
// labelsNeedUpdate returns true if any key in desired is missing
173+
// or has a different value in existing.
174+
func labelsNeedUpdate(existing, desired map[string]string) bool {
175+
for k, v := range desired {
176+
if existing[k] != v {
177+
return true
178+
}
179+
}
180+
return false
181+
}

0 commit comments

Comments
 (0)