Skip to content

Commit 840fbc8

Browse files
committed
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 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>
1 parent 376e178 commit 840fbc8

File tree

16 files changed

+817
-269
lines changed

16 files changed

+817
-269
lines changed

config/rbac/role.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ rules:
4444
- postgresql.cnpg.io
4545
resources:
4646
- backups
47+
- clusters
4748
verbs:
4849
- get
4950
- list

internal/cmd/operator/main.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@ func NewCmd() *cobra.Command {
102102
_ = viper.BindPFlag("server-address", cmd.Flags().Lookup("server-address"))
103103

104104
_ = viper.BindEnv("sidecar-image", "SIDECAR_IMAGE")
105+
_ = viper.BindEnv("custom-cnpg-group", "CUSTOM_CNPG_GROUP")
106+
_ = viper.BindEnv("custom-cnpg-version", "CUSTOM_CNPG_VERSION")
105107

106108
return cmd
107109
}

internal/cmd/restore/main.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ func NewCmd() *cobra.Command {
5757
_ = viper.BindEnv("pod-name", "POD_NAME")
5858
_ = viper.BindEnv("pgdata", "PGDATA")
5959
_ = viper.BindEnv("spool-directory", "SPOOL_DIRECTORY")
60+
_ = viper.BindEnv("custom-cnpg-group", "CUSTOM_CNPG_GROUP")
61+
_ = viper.BindEnv("custom-cnpg-version", "CUSTOM_CNPG_VERSION")
6062

6163
return cmd
6264
}

internal/cnpgi/instance/manager.go

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,15 @@ import (
2828
"github.com/spf13/viper"
2929
corev1 "k8s.io/api/core/v1"
3030
"k8s.io/apimachinery/pkg/runtime"
31-
"k8s.io/apimachinery/pkg/runtime/schema"
3231
"k8s.io/apimachinery/pkg/types"
3332
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
3433
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
3534
ctrl "sigs.k8s.io/controller-runtime"
3635
"sigs.k8s.io/controller-runtime/pkg/client"
37-
"sigs.k8s.io/controller-runtime/pkg/scheme"
3836

3937
barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1"
4038
extendedclient "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/instance/internal/client"
39+
pluginscheme "github.com/cloudnative-pg/plugin-barman-cloud/internal/scheme"
4140
)
4241

4342
// Start starts the sidecar informers and CNPG-i server
@@ -127,26 +126,7 @@ func generateScheme(ctx context.Context) *runtime.Scheme {
127126

128127
utilruntime.Must(barmancloudv1.AddToScheme(result))
129128
utilruntime.Must(clientgoscheme.AddToScheme(result))
130-
131-
cnpgGroup := viper.GetString("custom-cnpg-group")
132-
cnpgVersion := viper.GetString("custom-cnpg-version")
133-
if len(cnpgGroup) == 0 {
134-
cnpgGroup = cnpgv1.SchemeGroupVersion.Group
135-
}
136-
if len(cnpgVersion) == 0 {
137-
cnpgVersion = cnpgv1.SchemeGroupVersion.Version
138-
}
139-
140-
// Proceed with custom registration of the CNPG scheme
141-
schemeGroupVersion := schema.GroupVersion{Group: cnpgGroup, Version: cnpgVersion}
142-
schemeBuilder := &scheme.Builder{GroupVersion: schemeGroupVersion}
143-
schemeBuilder.Register(&cnpgv1.Cluster{}, &cnpgv1.ClusterList{})
144-
schemeBuilder.Register(&cnpgv1.Backup{}, &cnpgv1.BackupList{})
145-
schemeBuilder.Register(&cnpgv1.ScheduledBackup{}, &cnpgv1.ScheduledBackupList{})
146-
utilruntime.Must(schemeBuilder.AddToScheme(result))
147-
148-
schemeLog := log.FromContext(ctx)
149-
schemeLog.Info("CNPG types registration", "schemeGroupVersion", schemeGroupVersion)
129+
pluginscheme.AddCNPGToScheme(ctx, result)
150130

151131
return result
152132
}

internal/cnpgi/operator/manager.go

Lines changed: 13 additions & 6 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"
@@ -38,25 +37,33 @@ import (
3837

3938
barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1"
4039
"github.com/cloudnative-pg/plugin-barman-cloud/internal/controller"
40+
pluginscheme "github.com/cloudnative-pg/plugin-barman-cloud/internal/scheme"
4141

4242
// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
4343
// to ensure that exec-entrypoint and run can make use of them.
4444
_ "k8s.io/client-go/plugin/pkg/client/auth"
4545
)
4646

47-
var scheme = runtime.NewScheme()
47+
// generateScheme creates a runtime.Scheme with all type definitions
48+
// needed by the operator. CNPG types are registered under a
49+
// configurable API group to support custom CNPG-based operators.
50+
func generateScheme(ctx context.Context) *runtime.Scheme {
51+
result := runtime.NewScheme()
4852

49-
func init() {
50-
utilruntime.Must(clientgoscheme.AddToScheme(scheme))
51-
utilruntime.Must(barmancloudv1.AddToScheme(scheme))
52-
utilruntime.Must(cnpgv1.AddToScheme(scheme))
53+
utilruntime.Must(clientgoscheme.AddToScheme(result))
54+
utilruntime.Must(barmancloudv1.AddToScheme(result))
55+
pluginscheme.AddCNPGToScheme(ctx, result)
5356
// +kubebuilder:scaffold:scheme
57+
58+
return result
5459
}
5560

5661
// Start starts the manager
5762
func Start(ctx context.Context) error {
5863
setupLog := log.FromContext(ctx)
5964

65+
scheme := generateScheme(ctx)
66+
6067
var tlsOpts []func(*tls.Config)
6168

6269
// if the enable-http2 flag is false (the default), http/2 should be disabled

internal/cnpgi/operator/ownership.go

Lines changed: 0 additions & 58 deletions
This file was deleted.
Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
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
23+
24+
import (
25+
"context"
26+
27+
cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
28+
"github.com/cloudnative-pg/machinery/pkg/log"
29+
rbacv1 "k8s.io/api/rbac/v1"
30+
"k8s.io/apimachinery/pkg/api/equality"
31+
apierrs "k8s.io/apimachinery/pkg/api/errors"
32+
"sigs.k8s.io/controller-runtime/pkg/client"
33+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
34+
35+
barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1"
36+
"github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/specs"
37+
)
38+
39+
// EnsureRole ensures the RBAC Role for the given Cluster matches
40+
// the desired state derived from the given ObjectStores. On creation,
41+
// the Cluster is set as the owner of the Role for garbage collection.
42+
//
43+
// This function is called from both the Pre hook (gRPC) and the
44+
// ObjectStore controller. To handle concurrent modifications
45+
// gracefully, AlreadyExists on Create and Conflict on Patch are
46+
// retried once rather than returned as errors.
47+
func EnsureRole(
48+
ctx context.Context,
49+
c client.Client,
50+
cluster *cnpgv1.Cluster,
51+
barmanObjects []barmancloudv1.ObjectStore,
52+
) error {
53+
newRole := specs.BuildRole(cluster, barmanObjects)
54+
55+
roleKey := client.ObjectKey{
56+
Namespace: newRole.Namespace,
57+
Name: newRole.Name,
58+
}
59+
60+
var role rbacv1.Role
61+
err := c.Get(ctx, roleKey, &role)
62+
63+
switch {
64+
case apierrs.IsNotFound(err):
65+
role, err := createRole(ctx, c, cluster, newRole)
66+
if err != nil {
67+
return err
68+
}
69+
if role == nil {
70+
// Created successfully, nothing else to do.
71+
return nil
72+
}
73+
// AlreadyExists: fall through to patch with the re-read role.
74+
return patchRoleRules(ctx, c, newRole.Rules, role)
75+
76+
case err != nil:
77+
return err
78+
79+
default:
80+
return patchRoleRules(ctx, c, newRole.Rules, &role)
81+
}
82+
}
83+
84+
// createRole attempts to create the Role. If another writer created
85+
// it concurrently (AlreadyExists), it re-reads and returns the
86+
// existing Role for the caller to patch. On success it returns nil.
87+
func createRole(
88+
ctx context.Context,
89+
c client.Client,
90+
cluster *cnpgv1.Cluster,
91+
newRole *rbacv1.Role,
92+
) (*rbacv1.Role, error) {
93+
contextLogger := log.FromContext(ctx)
94+
95+
if err := controllerutil.SetControllerReference(cluster, newRole, c.Scheme()); err != nil {
96+
return nil, err
97+
}
98+
99+
contextLogger.Info("Creating role",
100+
"name", newRole.Name, "namespace", newRole.Namespace)
101+
102+
createErr := c.Create(ctx, newRole)
103+
if createErr == nil {
104+
return nil, nil
105+
}
106+
if !apierrs.IsAlreadyExists(createErr) {
107+
return nil, createErr
108+
}
109+
110+
contextLogger.Info("Role was created concurrently, checking rules")
111+
112+
var role rbacv1.Role
113+
if err := c.Get(ctx, client.ObjectKeyFromObject(newRole), &role); err != nil {
114+
return nil, err
115+
}
116+
117+
return &role, nil
118+
}
119+
120+
// patchRoleRules patches the Role's rules if they differ from the
121+
// desired state. On Conflict (concurrent modification), it retries
122+
// once with a fresh read.
123+
func patchRoleRules(
124+
ctx context.Context,
125+
c client.Client,
126+
desiredRules []rbacv1.PolicyRule,
127+
role *rbacv1.Role,
128+
) error {
129+
if equality.Semantic.DeepEqual(desiredRules, role.Rules) {
130+
return nil
131+
}
132+
133+
contextLogger := log.FromContext(ctx)
134+
contextLogger.Info("Patching role",
135+
"name", role.Name, "namespace", role.Namespace, "rules", desiredRules)
136+
137+
oldRole := role.DeepCopy()
138+
role.Rules = desiredRules
139+
140+
patchErr := c.Patch(ctx, role, client.MergeFrom(oldRole))
141+
if patchErr == nil || !apierrs.IsConflict(patchErr) {
142+
return patchErr
143+
}
144+
145+
// Conflict: re-read and retry once.
146+
contextLogger.Info("Role was modified concurrently, retrying patch")
147+
if err := c.Get(ctx, client.ObjectKeyFromObject(role), role); err != nil {
148+
return err
149+
}
150+
if equality.Semantic.DeepEqual(desiredRules, role.Rules) {
151+
return nil
152+
}
153+
154+
oldRole = role.DeepCopy()
155+
role.Rules = desiredRules
156+
157+
return c.Patch(ctx, role, client.MergeFrom(oldRole))
158+
}

0 commit comments

Comments
 (0)