From fe445b154ff30780c39baa4d494a0d7bc618da10 Mon Sep 17 00:00:00 2001 From: Armando Ruocco Date: Thu, 26 Mar 2026 13:56:03 +0100 Subject: [PATCH 1/6] 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 --- config/rbac/role.yaml | 1 + internal/cmd/operator/main.go | 2 + internal/cnpgi/operator/manager.go | 19 +- internal/cnpgi/operator/ownership.go | 58 ---- internal/cnpgi/operator/rbac/ensure.go | 158 +++++++++ internal/cnpgi/operator/rbac/ensure_test.go | 175 ++++++++++ internal/cnpgi/operator/rbac/suite_test.go | 32 ++ internal/cnpgi/operator/reconciler.go | 58 +--- internal/cnpgi/restore/manager.go | 1 + internal/controller/objectstore_controller.go | 95 +++++- .../controller/objectstore_controller_test.go | 317 +++++++++++++++--- internal/controller/suite_test.go | 67 ---- internal/scheme/cnpg.go | 58 ++++ manifest.yaml | 1 + 14 files changed, 801 insertions(+), 241 deletions(-) delete mode 100644 internal/cnpgi/operator/ownership.go create mode 100644 internal/cnpgi/operator/rbac/ensure.go create mode 100644 internal/cnpgi/operator/rbac/ensure_test.go create mode 100644 internal/cnpgi/operator/rbac/suite_test.go create mode 100644 internal/scheme/cnpg.go diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 6d58decc..bf7eee3a 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -44,6 +44,7 @@ rules: - postgresql.cnpg.io resources: - backups + - clusters verbs: - get - list diff --git a/internal/cmd/operator/main.go b/internal/cmd/operator/main.go index 33570543..49f9ad9f 100644 --- a/internal/cmd/operator/main.go +++ b/internal/cmd/operator/main.go @@ -102,6 +102,8 @@ func NewCmd() *cobra.Command { _ = viper.BindPFlag("server-address", cmd.Flags().Lookup("server-address")) _ = viper.BindEnv("sidecar-image", "SIDECAR_IMAGE") + _ = viper.BindEnv("custom-cnpg-group", "CUSTOM_CNPG_GROUP") + _ = viper.BindEnv("custom-cnpg-version", "CUSTOM_CNPG_VERSION") return cmd } diff --git a/internal/cnpgi/operator/manager.go b/internal/cnpgi/operator/manager.go index 94dfa728..87cc8ee2 100644 --- a/internal/cnpgi/operator/manager.go +++ b/internal/cnpgi/operator/manager.go @@ -24,7 +24,6 @@ import ( "crypto/tls" // +kubebuilder:scaffold:imports - cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" "github.com/cloudnative-pg/machinery/pkg/log" "github.com/spf13/viper" "k8s.io/apimachinery/pkg/runtime" @@ -38,25 +37,33 @@ import ( barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" "github.com/cloudnative-pg/plugin-barman-cloud/internal/controller" + pluginscheme "github.com/cloudnative-pg/plugin-barman-cloud/internal/scheme" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" ) -var scheme = runtime.NewScheme() +// generateScheme creates a runtime.Scheme with all type definitions +// needed by the operator. CNPG types are registered under a +// configurable API group to support custom CNPG-based operators. +func generateScheme(ctx context.Context) *runtime.Scheme { + result := runtime.NewScheme() -func init() { - utilruntime.Must(clientgoscheme.AddToScheme(scheme)) - utilruntime.Must(barmancloudv1.AddToScheme(scheme)) - utilruntime.Must(cnpgv1.AddToScheme(scheme)) + utilruntime.Must(clientgoscheme.AddToScheme(result)) + utilruntime.Must(barmancloudv1.AddToScheme(result)) + pluginscheme.AddCNPGToScheme(ctx, result) // +kubebuilder:scaffold:scheme + + return result } // Start starts the manager func Start(ctx context.Context) error { setupLog := log.FromContext(ctx) + scheme := generateScheme(ctx) + var tlsOpts []func(*tls.Config) // if the enable-http2 flag is false (the default), http/2 should be disabled diff --git a/internal/cnpgi/operator/ownership.go b/internal/cnpgi/operator/ownership.go deleted file mode 100644 index e0aadcb0..00000000 --- a/internal/cnpgi/operator/ownership.go +++ /dev/null @@ -1,58 +0,0 @@ -/* -Copyright © contributors to CloudNativePG, established as -CloudNativePG a Series of LF Projects, LLC. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. - -SPDX-License-Identifier: Apache-2.0 -*/ - -package operator - -import ( - "fmt" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" -) - -// setOwnerReference explicitly set the owner reference between an -// owner object and a controller one. -// -// Important: this function won't use any registered scheme and will -// fail unless the metadata has been correctly set into the owner -// object. -func setOwnerReference(owner, controlled metav1.Object) error { - ro, ok := owner.(runtime.Object) - if !ok { - return fmt.Errorf("%T is not a runtime.Object, cannot call setOwnerReference", owner) - } - - if len(ro.DeepCopyObject().GetObjectKind().GroupVersionKind().Group) == 0 { - return fmt.Errorf("%T metadata have not been set, cannot call setOwnerReference", owner) - } - - controlled.SetOwnerReferences([]metav1.OwnerReference{ - { - APIVersion: ro.GetObjectKind().GroupVersionKind().GroupVersion().String(), - Kind: ro.GetObjectKind().GroupVersionKind().Kind, - Name: owner.GetName(), - UID: owner.GetUID(), - BlockOwnerDeletion: ptr.To(true), - Controller: ptr.To(true), - }, - }) - - return nil -} diff --git a/internal/cnpgi/operator/rbac/ensure.go b/internal/cnpgi/operator/rbac/ensure.go new file mode 100644 index 00000000..c289d1b3 --- /dev/null +++ b/internal/cnpgi/operator/rbac/ensure.go @@ -0,0 +1,158 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +// Package rbac contains utilities to reconcile RBAC resources +// for the barman-cloud plugin. +package rbac + +import ( + "context" + + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" + "github.com/cloudnative-pg/machinery/pkg/log" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" + apierrs "k8s.io/apimachinery/pkg/api/errors" + "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/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. +func EnsureRole( + ctx context.Context, + c client.Client, + cluster *cnpgv1.Cluster, + barmanObjects []barmancloudv1.ObjectStore, +) error { + newRole := specs.BuildRole(cluster, barmanObjects) + + roleKey := client.ObjectKey{ + Namespace: newRole.Namespace, + Name: newRole.Name, + } + + 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 + + default: + return patchRoleRules(ctx, c, newRole.Rules, &role) + } +} + +// 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( + ctx context.Context, + c client.Client, + cluster *cnpgv1.Cluster, + newRole *rbacv1.Role, +) (*rbacv1.Role, error) { + contextLogger := log.FromContext(ctx) + + if err := controllerutil.SetControllerReference(cluster, newRole, c.Scheme()); err != nil { + return nil, 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 + } + + return &role, nil +} + +// 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( + ctx context.Context, + c client.Client, + desiredRules []rbacv1.PolicyRule, + role *rbacv1.Role, +) error { + if equality.Semantic.DeepEqual(desiredRules, role.Rules) { + return nil + } + + contextLogger := log.FromContext(ctx) + contextLogger.Info("Patching role", + "name", role.Name, "namespace", role.Namespace, "rules", desiredRules) + + oldRole := role.DeepCopy() + role.Rules = desiredRules + + patchErr := c.Patch(ctx, role, client.MergeFrom(oldRole)) + if patchErr == nil || !apierrs.IsConflict(patchErr) { + return patchErr + } + + // 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 + } + + oldRole = role.DeepCopy() + role.Rules = desiredRules + + return c.Patch(ctx, role, client.MergeFrom(oldRole)) +} diff --git a/internal/cnpgi/operator/rbac/ensure_test.go b/internal/cnpgi/operator/rbac/ensure_test.go new file mode 100644 index 00000000..7d79d564 --- /dev/null +++ b/internal/cnpgi/operator/rbac/ensure_test.go @@ -0,0 +1,175 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package rbac_test + +import ( + "context" + + barmanapi "github.com/cloudnative-pg/barman-cloud/pkg/api" + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" + machineryapi "github.com/cloudnative-pg/machinery/pkg/api" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/rbac" +) + +func newScheme() *runtime.Scheme { + s := runtime.NewScheme() + _ = rbacv1.AddToScheme(s) + _ = cnpgv1.AddToScheme(s) + _ = barmancloudv1.AddToScheme(s) + return s +} + +func newCluster(name, namespace string) *cnpgv1.Cluster { + return &cnpgv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + } +} + +func newObjectStore(name, namespace, secretName string) barmancloudv1.ObjectStore { + return barmancloudv1.ObjectStore{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: barmancloudv1.ObjectStoreSpec{ + Configuration: barmanapi.BarmanObjectStoreConfiguration{ + DestinationPath: "s3://bucket/path", + BarmanCredentials: barmanapi.BarmanCredentials{ + AWS: &barmanapi.S3Credentials{ + AccessKeyIDReference: &machineryapi.SecretKeySelector{ + LocalObjectReference: machineryapi.LocalObjectReference{ + Name: secretName, + }, + Key: "ACCESS_KEY_ID", + }, + }, + }, + }, + }, + } +} + +var _ = Describe("EnsureRole", func() { + var ( + ctx context.Context + cluster *cnpgv1.Cluster + objects []barmancloudv1.ObjectStore + fakeClient client.Client + ) + + BeforeEach(func() { + ctx = context.Background() + cluster = newCluster("test-cluster", "default") + objects = []barmancloudv1.ObjectStore{ + newObjectStore("my-store", "default", "aws-creds"), + } + }) + + Context("when the Role does not exist", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + }) + + It("should create the Role with owner reference", func() { + err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) + Expect(err).NotTo(HaveOccurred()) + + var role rbacv1.Role + err = fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &role) + Expect(err).NotTo(HaveOccurred()) + Expect(role.Rules).To(HaveLen(3)) + + // Verify owner reference is set to the Cluster + Expect(role.OwnerReferences).To(HaveLen(1)) + Expect(role.OwnerReferences[0].Name).To(Equal("test-cluster")) + Expect(role.OwnerReferences[0].Kind).To(Equal("Cluster")) + }) + }) + + Context("when the Role exists with matching rules", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + Expect(rbac.EnsureRole(ctx, fakeClient, cluster, objects)).To(Succeed()) + }) + + It("should not patch the Role", func() { + var before rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &before)).To(Succeed()) + + err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) + Expect(err).NotTo(HaveOccurred()) + + var after rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &after)).To(Succeed()) + + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion)) + }) + }) + + Context("when the Role exists with different rules", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + oldObjects := []barmancloudv1.ObjectStore{ + newObjectStore("my-store", "default", "old-secret"), + } + Expect(rbac.EnsureRole(ctx, fakeClient, cluster, oldObjects)).To(Succeed()) + }) + + It("should patch the Role with new rules and preserve owner reference", func() { + err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) + Expect(err).NotTo(HaveOccurred()) + + var role rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &role)).To(Succeed()) + + secretsRule := role.Rules[2] + Expect(secretsRule.ResourceNames).To(ContainElement("aws-creds")) + Expect(secretsRule.ResourceNames).NotTo(ContainElement("old-secret")) + + // Owner reference must survive the patch + Expect(role.OwnerReferences).To(HaveLen(1)) + Expect(role.OwnerReferences[0].Name).To(Equal("test-cluster")) + }) + }) +}) diff --git a/internal/cnpgi/operator/rbac/suite_test.go b/internal/cnpgi/operator/rbac/suite_test.go new file mode 100644 index 00000000..42fedae8 --- /dev/null +++ b/internal/cnpgi/operator/rbac/suite_test.go @@ -0,0 +1,32 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package rbac_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestRBAC(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "RBAC Suite") +} diff --git a/internal/cnpgi/operator/reconciler.go b/internal/cnpgi/operator/reconciler.go index 9d64deb0..fa4b6fcd 100644 --- a/internal/cnpgi/operator/reconciler.go +++ b/internal/cnpgi/operator/reconciler.go @@ -28,12 +28,13 @@ import ( "github.com/cloudnative-pg/cnpg-i/pkg/reconciler" "github.com/cloudnative-pg/machinery/pkg/log" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" "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/operator/config" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/rbac" "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/specs" ) @@ -113,7 +114,7 @@ func (r ReconcilerImplementation) Pre( barmanObjects = append(barmanObjects, barmanObject) } - if err := r.ensureRole(ctx, &cluster, barmanObjects); err != nil { + if err := rbac.EnsureRole(ctx, r.Client, &cluster, barmanObjects); err != nil { return nil, err } @@ -137,57 +138,6 @@ func (r ReconcilerImplementation) Post( }, nil } -func (r ReconcilerImplementation) ensureRole( - ctx context.Context, - cluster *cnpgv1.Cluster, - barmanObjects []barmancloudv1.ObjectStore, -) error { - contextLogger := log.FromContext(ctx) - newRole := specs.BuildRole(cluster, barmanObjects) - - var role rbacv1.Role - if err := r.Client.Get(ctx, client.ObjectKey{ - Namespace: newRole.Namespace, - Name: newRole.Name, - }, &role); err != nil { - if !apierrs.IsNotFound(err) { - return err - } - - contextLogger.Info( - "Creating role", - "name", newRole.Name, - "namespace", newRole.Namespace, - ) - - if err := setOwnerReference(cluster, newRole); err != nil { - return err - } - - return r.Client.Create(ctx, newRole) - } - - if equality.Semantic.DeepEqual(newRole.Rules, role.Rules) { - // There's no need to hit the API server again - return nil - } - - contextLogger.Info( - "Patching role", - "name", newRole.Name, - "namespace", newRole.Namespace, - "rules", newRole.Rules, - ) - - oldRole := role.DeepCopy() - - // Apply to the role the new rules - role.Rules = newRole.Rules - - // Push it back to the API server - return r.Client.Patch(ctx, &role, client.MergeFrom(oldRole)) -} - func (r ReconcilerImplementation) ensureRoleBinding( ctx context.Context, cluster *cnpgv1.Cluster, @@ -213,7 +163,7 @@ func (r ReconcilerImplementation) createRoleBinding( cluster *cnpgv1.Cluster, ) error { roleBinding := specs.BuildRoleBinding(cluster) - if err := setOwnerReference(cluster, roleBinding); err != nil { + if err := controllerutil.SetControllerReference(cluster, roleBinding, r.Client.Scheme()); err != nil { return err } return r.Client.Create(ctx, roleBinding) diff --git a/internal/cnpgi/restore/manager.go b/internal/cnpgi/restore/manager.go index bcfe8c93..c14a02aa 100644 --- a/internal/cnpgi/restore/manager.go +++ b/internal/cnpgi/restore/manager.go @@ -32,6 +32,7 @@ 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 31fe83f7..937b1351 100644 --- a/internal/controller/objectstore_controller.go +++ b/internal/controller/objectstore_controller.go @@ -23,12 +23,18 @@ import ( "context" "fmt" + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" "github.com/cloudnative-pg/machinery/pkg/log" + apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/predicate" barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/config" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/rbac" ) // ObjectStoreReconciler reconciles a ObjectStore object. @@ -40,33 +46,96 @@ type ObjectStoreReconciler struct { // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=create;patch;update;get;list;watch // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=create;patch;update;get;list;watch // +kubebuilder:rbac:groups="",resources=secrets,verbs=create;list;get;watch;delete +// +kubebuilder:rbac:groups=postgresql.cnpg.io,resources=clusters,verbs=get;list;watch // +kubebuilder:rbac:groups=postgresql.cnpg.io,resources=clusters/finalizers,verbs=update // +kubebuilder:rbac:groups=postgresql.cnpg.io,resources=backups,verbs=get;list;watch // +kubebuilder:rbac:groups=barmancloud.cnpg.io,resources=objectstores,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=barmancloud.cnpg.io,resources=objectstores/status,verbs=get;update;patch // +kubebuilder:rbac:groups=barmancloud.cnpg.io,resources=objectstores/finalizers,verbs=update -// Reconcile is part of the main kubernetes reconciliation loop which aims to -// move the current state of the cluster closer to the desired state. -// TODO(user): Modify the Reconcile function to compare the state specified by -// the ObjectStore object against the actual cluster state, and then -// perform operations to make the cluster state reflect the state specified by -// the user. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.19.0/pkg/reconcile -func (r *ObjectStoreReconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) { - _ = log.FromContext(ctx) +// Reconcile ensures that the RBAC Role for each Cluster referencing +// this ObjectStore is up to date with the current ObjectStore spec. +func (r *ObjectStoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + contextLogger := log.FromContext(ctx).WithValues( + "objectStoreName", req.Name, + "namespace", req.Namespace, + ) + ctx = log.IntoContext(ctx, contextLogger) - // TODO(user): your logic here + contextLogger.Info("ObjectStore reconciliation start") + // List all Clusters in the same namespace + var clusterList cnpgv1.ClusterList + if err := r.List(ctx, &clusterList, client.InNamespace(req.Namespace)); err != nil { + return ctrl.Result{}, fmt.Errorf("while listing clusters: %w", err) + } + + // For each Cluster that references this ObjectStore, reconcile the Role + for i := range clusterList.Items { + cluster := &clusterList.Items[i] + + pluginConfiguration := config.NewFromCluster(cluster) + referredObjects := pluginConfiguration.GetReferredBarmanObjectsKey() + + if !referencesObjectStore(referredObjects, req.NamespacedName) { + continue + } + + contextLogger.Info("Reconciling RBAC for cluster", + "clusterName", cluster.Name) + + if err := r.reconcileRBACForCluster(ctx, cluster, referredObjects); err != nil { + return ctrl.Result{}, fmt.Errorf("while reconciling RBAC for cluster %s: %w", cluster.Name, err) + } + } + + contextLogger.Info("ObjectStore reconciliation completed") return ctrl.Result{}, nil } +// reconcileRBACForCluster ensures the Role for the given Cluster is +// up to date with the current ObjectStore specs. +func (r *ObjectStoreReconciler) reconcileRBACForCluster( + ctx context.Context, + cluster *cnpgv1.Cluster, + referredObjectKeys []client.ObjectKey, +) error { + contextLogger := log.FromContext(ctx) + barmanObjects := make([]barmancloudv1.ObjectStore, 0, len(referredObjectKeys)) + for _, key := range referredObjectKeys { + var barmanObject barmancloudv1.ObjectStore + if err := r.Get(ctx, key, &barmanObject); err != nil { + if apierrs.IsNotFound(err) { + contextLogger.Info("ObjectStore not found, skipping", + "objectStoreName", key.Name) + continue + } + return fmt.Errorf("while getting ObjectStore %s: %w", key, err) + } + barmanObjects = append(barmanObjects, barmanObject) + } + + return rbac.EnsureRole(ctx, r.Client, cluster, barmanObjects) +} + +// referencesObjectStore checks if the given ObjectStore is in the list +// of referred barman objects. +func referencesObjectStore( + referredObjects []client.ObjectKey, + objectStore client.ObjectKey, +) bool { + for _, ref := range referredObjects { + if ref.Name == objectStore.Name && ref.Namespace == objectStore.Namespace { + return true + } + } + return false +} + // SetupWithManager sets up the controller with the Manager. func (r *ObjectStoreReconciler) SetupWithManager(mgr ctrl.Manager) error { err := ctrl.NewControllerManagedBy(mgr). - For(&barmancloudv1.ObjectStore{}). + For(&barmancloudv1.ObjectStore{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). Complete(r) if err != nil { return fmt.Errorf("unable to create controller: %w", err) diff --git a/internal/controller/objectstore_controller_test.go b/internal/controller/objectstore_controller_test.go index 6c163d38..fd9d0810 100644 --- a/internal/controller/objectstore_controller_test.go +++ b/internal/controller/objectstore_controller_test.go @@ -22,70 +22,301 @@ package controller import ( "context" + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" barmanapi "github.com/cloudnative-pg/barman-cloud/pkg/api" - "k8s.io/apimachinery/pkg/api/errors" + machineryapi "github.com/cloudnative-pg/machinery/pkg/api" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata" ) -var _ = Describe("ObjectStore Controller", func() { - Context("When reconciling a resource", func() { - const resourceName = "test-resource" +func newFakeScheme() *runtime.Scheme { + s := runtime.NewScheme() + _ = rbacv1.AddToScheme(s) + _ = cnpgv1.AddToScheme(s) + _ = barmancloudv1.AddToScheme(s) + return s +} - ctx := context.Background() - - typeNamespacedName := types.NamespacedName{ - Name: resourceName, - Namespace: "default", // TODO(user):Modify as needed - } - objectstore := &barmancloudv1.ObjectStore{} - - BeforeEach(func() { - By("creating the custom resource for the Kind ObjectStore") - err := k8sClient.Get(ctx, typeNamespacedName, objectstore) - if err != nil && errors.IsNotFound(err) { - resource := &barmancloudv1.ObjectStore{ - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, - Namespace: "default", +func newTestCluster(name, namespace, objectStoreName string) *cnpgv1.Cluster { + return &cnpgv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: cnpgv1.ClusterSpec{ + Plugins: []cnpgv1.PluginConfiguration{ + { + Name: metadata.PluginName, + Parameters: map[string]string{ + "barmanObjectName": objectStoreName, }, - Spec: barmancloudv1.ObjectStoreSpec{ - Configuration: barmanapi.BarmanObjectStoreConfiguration{DestinationPath: "/tmp"}, + }, + }, + }, + } +} + +func newTestObjectStore(name, namespace, secretName string) *barmancloudv1.ObjectStore { + return &barmancloudv1.ObjectStore{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: barmancloudv1.ObjectStoreSpec{ + Configuration: barmanapi.BarmanObjectStoreConfiguration{ + DestinationPath: "s3://bucket/path", + BarmanCredentials: barmanapi.BarmanCredentials{ + AWS: &barmanapi.S3Credentials{ + AccessKeyIDReference: &machineryapi.SecretKeySelector{ + LocalObjectReference: machineryapi.LocalObjectReference{ + Name: secretName, + }, + Key: "ACCESS_KEY_ID", + }, }, - // TODO(user): Specify other spec details if needed. - } - Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + }, + }, + }, + } +} + +var _ = Describe("referencesObjectStore", func() { + It("should return true when ObjectStore is in the list", func() { + refs := []client.ObjectKey{ + {Name: "store-a", Namespace: "default"}, + {Name: "store-b", Namespace: "default"}, + } + Expect(referencesObjectStore(refs, client.ObjectKey{ + Name: "store-b", Namespace: "default", + })).To(BeTrue()) + }) + + It("should return false when ObjectStore is not in the list", func() { + refs := []client.ObjectKey{ + {Name: "store-a", Namespace: "default"}, + } + Expect(referencesObjectStore(refs, client.ObjectKey{ + Name: "store-b", Namespace: "default", + })).To(BeFalse()) + }) + + It("should return false when namespace differs", func() { + refs := []client.ObjectKey{ + {Name: "store-a", Namespace: "ns1"}, + } + Expect(referencesObjectStore(refs, client.ObjectKey{ + Name: "store-a", Namespace: "ns2", + })).To(BeFalse()) + }) + + It("should return false for empty list", func() { + Expect(referencesObjectStore(nil, client.ObjectKey{ + Name: "store-a", Namespace: "default", + })).To(BeFalse()) + }) +}) + +var _ = Describe("ObjectStoreReconciler", func() { + var ( + ctx context.Context + scheme *runtime.Scheme + ) + + BeforeEach(func() { + ctx = context.Background() + scheme = newFakeScheme() + }) + + Describe("Reconcile", func() { + It("should create a Role for a Cluster that references the ObjectStore", func() { + objectStore := newTestObjectStore("my-store", "default", "aws-creds") + cluster := newTestCluster("my-cluster", "default", "my-store") + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objectStore, cluster). + Build() + + reconciler := &ObjectStoreReconciler{ + Client: fakeClient, + Scheme: scheme, } + + result, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "my-store", + Namespace: "default", + }, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + + var role rbacv1.Role + err = fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "my-cluster-barman-cloud", + }, &role) + Expect(err).NotTo(HaveOccurred()) + Expect(role.Rules).To(HaveLen(3)) + + // Verify the secrets rule contains the expected secret + secretsRule := role.Rules[2] + Expect(secretsRule.ResourceNames).To(ContainElement("aws-creds")) + + // Verify owner reference is set to the Cluster + Expect(role.OwnerReferences).To(HaveLen(1)) + Expect(role.OwnerReferences[0].Name).To(Equal("my-cluster")) + Expect(role.OwnerReferences[0].Kind).To(Equal("Cluster")) }) - AfterEach(func() { - // TODO(user): Cleanup logic after each test, like removing the resource instance. - resource := &barmancloudv1.ObjectStore{} - err := k8sClient.Get(ctx, typeNamespacedName, resource) + It("should skip Clusters that don't reference the ObjectStore", func() { + objectStore := newTestObjectStore("my-store", "default", "aws-creds") + cluster := newTestCluster("my-cluster", "default", "other-store") + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objectStore, cluster). + Build() + + reconciler := &ObjectStoreReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + result, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "my-store", + Namespace: "default", + }, + }) Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) - By("Cleanup the specific resource instance ObjectStore") - Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + // No Role should have been created + var role rbacv1.Role + err = fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "my-cluster-barman-cloud", + }, &role) + Expect(err).To(HaveOccurred()) }) - It("should successfully reconcile the resource", func() { - By("Reconciling the created resource") - controllerReconciler := &ObjectStoreReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), + + It("should succeed with no Clusters in the namespace", func() { + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + Build() + + reconciler := &ObjectStoreReconciler{ + Client: fakeClient, + Scheme: scheme, } - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, + result, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "my-store", + Namespace: "default", + }, }) Expect(err).NotTo(HaveOccurred()) - // TODO(user): Add more specific assertions depending on your controller's reconciliation logic. - // Example: If you expect a certain status condition after reconciliation, verify it here. + Expect(result).To(Equal(reconcile.Result{})) + }) + }) + + Describe("reconcileRBACForCluster", func() { + It("should skip deleted ObjectStores and still reconcile the Role", func() { + // Cluster references two ObjectStores, but one is deleted + cluster := newTestCluster("my-cluster", "default", "store-a") + existingStore := newTestObjectStore("store-a", "default", "aws-creds") + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(existingStore). + Build() + + reconciler := &ObjectStoreReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + // Pass two keys, but "store-b" doesn't exist + err := reconciler.reconcileRBACForCluster(ctx, cluster, []client.ObjectKey{ + {Name: "store-a", Namespace: "default"}, + {Name: "store-b", Namespace: "default"}, + }) + Expect(err).NotTo(HaveOccurred()) + + // Role should be created with only store-a's secrets + var role rbacv1.Role + err = fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "my-cluster-barman-cloud", + }, &role) + Expect(err).NotTo(HaveOccurred()) + Expect(role.Rules).To(HaveLen(3)) + + // ObjectStore rule should only reference store-a + objectStoreRule := role.Rules[0] + Expect(objectStoreRule.ResourceNames).To(ContainElement("store-a")) + Expect(objectStoreRule.ResourceNames).NotTo(ContainElement("store-b")) + + // Verify owner reference is set + Expect(role.OwnerReferences).To(HaveLen(1)) + Expect(role.OwnerReferences[0].Name).To(Equal("my-cluster")) + }) + + It("should update Role when ObjectStore credentials change", func() { + cluster := newTestCluster("my-cluster", "default", "my-store") + oldStore := newTestObjectStore("my-store", "default", "old-secret") + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(oldStore). + Build() + + reconciler := &ObjectStoreReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + // First reconcile - creates Role with old-secret + err := reconciler.reconcileRBACForCluster(ctx, cluster, []client.ObjectKey{ + {Name: "my-store", Namespace: "default"}, + }) + Expect(err).NotTo(HaveOccurred()) + + // Update the ObjectStore with new credentials + var currentStore barmancloudv1.ObjectStore + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Name: "my-store", Namespace: "default", + }, ¤tStore)).To(Succeed()) + currentStore.Spec.Configuration.BarmanCredentials.AWS.AccessKeyIDReference.LocalObjectReference.Name = "new-secret" + Expect(fakeClient.Update(ctx, ¤tStore)).To(Succeed()) + + // Second reconcile - should patch Role with new-secret + err = reconciler.reconcileRBACForCluster(ctx, cluster, []client.ObjectKey{ + {Name: "my-store", Namespace: "default"}, + }) + Expect(err).NotTo(HaveOccurred()) + + var role rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "my-cluster-barman-cloud", + }, &role)).To(Succeed()) + + secretsRule := role.Rules[2] + Expect(secretsRule.ResourceNames).To(ContainElement("new-secret")) + Expect(secretsRule.ResourceNames).NotTo(ContainElement("old-secret")) }) }) }) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 18a4029a..711c56ad 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -20,81 +20,14 @@ SPDX-License-Identifier: Apache-2.0 package controller import ( - "context" - "fmt" - "path/filepath" - "runtime" "testing" - // +kubebuilder:scaffold:imports - "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/rest" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/envtest" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - - barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) -// These tests use Ginkgo (BDD-style Go testing framework). Refer to -// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. - -var ( - cfg *rest.Config - k8sClient client.Client - testEnv *envtest.Environment - ctx context.Context - cancel context.CancelFunc -) - func TestControllers(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Controller Suite") } - -var _ = BeforeSuite(func() { - logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) - - ctx, cancel = context.WithCancel(context.TODO()) - - By("bootstrapping test environment") - testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, - ErrorIfCRDPathMissing: true, - - // The BinaryAssetsDirectory is only required if you want to run the tests directly - // without call the makefile target test. If not informed it will look for the - // default path defined in controller-runtime which is /usr/local/kubebuilder/. - // Note that you must have the required binaries setup under the bin directory to perform - // the tests directly. When we run make test it will be setup and used automatically. - BinaryAssetsDirectory: filepath.Join("..", "..", "bin", "k8s", - fmt.Sprintf("1.31.0-%s-%s", runtime.GOOS, runtime.GOARCH)), - } - - var err error - // cfg is defined in this file globally. - cfg, err = testEnv.Start() - Expect(err).NotTo(HaveOccurred()) - Expect(cfg).NotTo(BeNil()) - - err = barmancloudv1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - - // +kubebuilder:scaffold:scheme - - k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) - Expect(err).NotTo(HaveOccurred()) - Expect(k8sClient).NotTo(BeNil()) -}) - -var _ = AfterSuite(func() { - By("tearing down the test environment") - cancel() - err := testEnv.Stop() - Expect(err).NotTo(HaveOccurred()) -}) diff --git a/internal/scheme/cnpg.go b/internal/scheme/cnpg.go new file mode 100644 index 00000000..5ebd4b3e --- /dev/null +++ b/internal/scheme/cnpg.go @@ -0,0 +1,58 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +// Package scheme provides utilities for building runtime schemes +// with support for custom CNPG API groups. +package scheme + +import ( + "context" + + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" + "github.com/cloudnative-pg/machinery/pkg/log" + "github.com/spf13/viper" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + crscheme "sigs.k8s.io/controller-runtime/pkg/scheme" +) + +// AddCNPGToScheme registers CNPG types into the given scheme using +// the API group configured via CUSTOM_CNPG_GROUP/CUSTOM_CNPG_VERSION +// environment variables, defaulting to postgresql.cnpg.io/v1. +// This allows the plugin to work with any CNPG-based operator. +func AddCNPGToScheme(ctx context.Context, s *runtime.Scheme) { + cnpgGroup := viper.GetString("custom-cnpg-group") + cnpgVersion := viper.GetString("custom-cnpg-version") + if len(cnpgGroup) == 0 { + cnpgGroup = cnpgv1.SchemeGroupVersion.Group + } + if len(cnpgVersion) == 0 { + cnpgVersion = cnpgv1.SchemeGroupVersion.Version + } + + schemeGroupVersion := schema.GroupVersion{Group: cnpgGroup, Version: cnpgVersion} + schemeBuilder := &crscheme.Builder{GroupVersion: schemeGroupVersion} + schemeBuilder.Register(&cnpgv1.Cluster{}, &cnpgv1.ClusterList{}) + schemeBuilder.Register(&cnpgv1.Backup{}, &cnpgv1.BackupList{}) + schemeBuilder.Register(&cnpgv1.ScheduledBackup{}, &cnpgv1.ScheduledBackupList{}) + utilruntime.Must(schemeBuilder.AddToScheme(s)) + + log.FromContext(ctx).Info("CNPG types registration", "schemeGroupVersion", schemeGroupVersion) +} diff --git a/manifest.yaml b/manifest.yaml index 60b755e6..9e9b70a7 100644 --- a/manifest.yaml +++ b/manifest.yaml @@ -870,6 +870,7 @@ rules: - postgresql.cnpg.io resources: - backups + - clusters verbs: - get - list From c7ddf03d043977d4b7d61a86af95d8ecb8ebe72d Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Thu, 9 Apr 2026 19:10:05 +0200 Subject: [PATCH 2/6] fix: discover affected Roles by label instead of listing Clusters 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 --- config/rbac/role.yaml | 1 - internal/cnpgi/metadata/constants.go | 4 + internal/cnpgi/operator/rbac/ensure.go | 167 +++++----- internal/cnpgi/operator/rbac/ensure_test.go | 139 +++++++- internal/cnpgi/operator/specs/role.go | 46 ++- internal/cnpgi/operator/specs/role_test.go | 210 +++++++++++++ internal/controller/objectstore_controller.go | 86 +++-- .../controller/objectstore_controller_test.go | 297 ++++++++++-------- manifest.yaml | 1 - 9 files changed, 678 insertions(+), 273 deletions(-) create mode 100644 internal/cnpgi/operator/specs/role_test.go diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index bf7eee3a..6d58decc 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -44,7 +44,6 @@ rules: - postgresql.cnpg.io resources: - backups - - clusters verbs: - get - list diff --git a/internal/cnpgi/metadata/constants.go b/internal/cnpgi/metadata/constants.go index dad413a5..aed103bc 100644 --- a/internal/cnpgi/metadata/constants.go +++ b/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. diff --git a/internal/cnpgi/operator/rbac/ensure.go b/internal/cnpgi/operator/rbac/ensure.go index c289d1b3..c92821c9 100644 --- a/internal/cnpgi/operator/rbac/ensure.go +++ b/internal/cnpgi/operator/rbac/ensure.go @@ -29,10 +29,12 @@ 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" ) @@ -40,10 +42,9 @@ import ( // 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, @@ -51,108 +52,126 @@ func EnsureRole( 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 } diff --git a/internal/cnpgi/operator/rbac/ensure_test.go b/internal/cnpgi/operator/rbac/ensure_test.go index 7d79d564..380c2f6c 100644 --- a/internal/cnpgi/operator/rbac/ensure_test.go +++ b/internal/cnpgi/operator/rbac/ensure_test.go @@ -30,18 +30,20 @@ import ( rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" 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/rbac" ) func newScheme() *runtime.Scheme { s := runtime.NewScheme() - _ = rbacv1.AddToScheme(s) - _ = cnpgv1.AddToScheme(s) - _ = barmancloudv1.AddToScheme(s) + utilruntime.Must(rbacv1.AddToScheme(s)) + utilruntime.Must(cnpgv1.AddToScheme(s)) + utilruntime.Must(barmancloudv1.AddToScheme(s)) return s } @@ -99,7 +101,7 @@ var _ = Describe("EnsureRole", func() { fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() }) - It("should create the Role with owner reference", func() { + It("should create the Role with owner reference and label", func() { err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) Expect(err).NotTo(HaveOccurred()) @@ -111,10 +113,11 @@ var _ = Describe("EnsureRole", func() { Expect(err).NotTo(HaveOccurred()) Expect(role.Rules).To(HaveLen(3)) - // Verify owner reference is set to the Cluster Expect(role.OwnerReferences).To(HaveLen(1)) Expect(role.OwnerReferences[0].Name).To(Equal("test-cluster")) Expect(role.OwnerReferences[0].Kind).To(Equal("Cluster")) + + Expect(role.Labels).To(HaveKeyWithValue(metadata.ClusterLabelName, "test-cluster")) }) }) @@ -167,9 +170,133 @@ var _ = Describe("EnsureRole", func() { Expect(secretsRule.ResourceNames).To(ContainElement("aws-creds")) Expect(secretsRule.ResourceNames).NotTo(ContainElement("old-secret")) - // Owner reference must survive the patch Expect(role.OwnerReferences).To(HaveLen(1)) Expect(role.OwnerReferences[0].Name).To(Equal("test-cluster")) }) }) + + Context("when the Role exists without the cluster label (upgrade scenario)", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + + // Create a Role without the label (simulates pre-upgrade state) + unlabeledRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-barman-cloud", + Namespace: "default", + }, + Rules: []rbacv1.PolicyRule{}, + } + Expect(fakeClient.Create(ctx, unlabeledRole)).To(Succeed()) + }) + + It("should add the label and update rules", func() { + err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) + Expect(err).NotTo(HaveOccurred()) + + var role rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &role)).To(Succeed()) + + Expect(role.Labels).To(HaveKeyWithValue(metadata.ClusterLabelName, "test-cluster")) + Expect(role.Rules).To(HaveLen(3)) + }) + }) +}) + +var _ = Describe("EnsureRoleRules", func() { + var ( + ctx context.Context + fakeClient client.Client + objects []barmancloudv1.ObjectStore + ) + + BeforeEach(func() { + ctx = context.Background() + objects = []barmancloudv1.ObjectStore{ + newObjectStore("my-store", "default", "aws-creds"), + } + }) + + Context("when the Role exists", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + + // Seed a labeled Role with old rules + cluster := newCluster("test-cluster", "default") + oldObjects := []barmancloudv1.ObjectStore{ + newObjectStore("my-store", "default", "old-secret"), + } + Expect(rbac.EnsureRole(ctx, fakeClient, cluster, oldObjects)).To(Succeed()) + }) + + It("should patch the rules", func() { + roleKey := client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + } + err := rbac.EnsureRoleRules(ctx, fakeClient, roleKey, objects) + Expect(err).NotTo(HaveOccurred()) + + var role rbacv1.Role + Expect(fakeClient.Get(ctx, roleKey, &role)).To(Succeed()) + + secretsRule := role.Rules[2] + Expect(secretsRule.ResourceNames).To(ContainElement("aws-creds")) + Expect(secretsRule.ResourceNames).NotTo(ContainElement("old-secret")) + }) + + It("should not patch when rules already match", func() { + // Seed with the same objects so rules match + cluster := newCluster("test-cluster", "default") + Expect(rbac.EnsureRole(ctx, fakeClient, cluster, objects)).To(Succeed()) + + roleKey := client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + } + + var before rbacv1.Role + Expect(fakeClient.Get(ctx, roleKey, &before)).To(Succeed()) + + Expect(rbac.EnsureRoleRules(ctx, fakeClient, roleKey, objects)).To(Succeed()) + + var after rbacv1.Role + Expect(fakeClient.Get(ctx, roleKey, &after)).To(Succeed()) + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion)) + }) + + It("should not modify labels", func() { + roleKey := client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + } + + var before rbacv1.Role + Expect(fakeClient.Get(ctx, roleKey, &before)).To(Succeed()) + + Expect(rbac.EnsureRoleRules(ctx, fakeClient, roleKey, objects)).To(Succeed()) + + var after rbacv1.Role + Expect(fakeClient.Get(ctx, roleKey, &after)).To(Succeed()) + Expect(after.Labels).To(Equal(before.Labels)) + }) + }) + + Context("when the Role does not exist", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + }) + + It("should return nil", func() { + roleKey := client.ObjectKey{ + Namespace: "default", + Name: "nonexistent-barman-cloud", + } + err := rbac.EnsureRoleRules(ctx, fakeClient, roleKey, objects) + Expect(err).NotTo(HaveOccurred()) + }) + }) }) diff --git a/internal/cnpgi/operator/specs/role.go b/internal/cnpgi/operator/specs/role.go index 0c5fa705..0972f473 100644 --- a/internal/cnpgi/operator/specs/role.go +++ b/internal/cnpgi/operator/specs/role.go @@ -21,6 +21,7 @@ package specs import ( "fmt" + "slices" cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" "github.com/cloudnative-pg/machinery/pkg/stringset" @@ -28,6 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata" ) // BuildRole builds the Role object for this cluster @@ -35,15 +37,20 @@ func BuildRole( cluster *cnpgv1.Cluster, barmanObjects []barmancloudv1.ObjectStore, ) *rbacv1.Role { - role := &rbacv1.Role{ + return &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ Namespace: cluster.Namespace, Name: GetRBACName(cluster.Name), + Labels: map[string]string{ + metadata.ClusterLabelName: cluster.Name, + }, }, - - Rules: []rbacv1.PolicyRule{}, + Rules: BuildRoleRules(barmanObjects), } +} +// BuildRoleRules builds the RBAC PolicyRules for the given ObjectStores. +func BuildRoleRules(barmanObjects []barmancloudv1.ObjectStore) []rbacv1.PolicyRule { secretsSet := stringset.New() barmanObjectsSet := stringset.New() @@ -54,11 +61,10 @@ func BuildRole( } } - role.Rules = append( - role.Rules, - rbacv1.PolicyRule{ + return []rbacv1.PolicyRule{ + { APIGroups: []string{ - "barmancloud.cnpg.io", + barmancloudv1.GroupVersion.Group, }, Verbs: []string{ "get", @@ -70,9 +76,9 @@ func BuildRole( }, ResourceNames: barmanObjectsSet.ToSortedList(), }, - rbacv1.PolicyRule{ + { APIGroups: []string{ - "barmancloud.cnpg.io", + barmancloudv1.GroupVersion.Group, }, Verbs: []string{ "update", @@ -82,7 +88,7 @@ func BuildRole( }, ResourceNames: barmanObjectsSet.ToSortedList(), }, - rbacv1.PolicyRule{ + { APIGroups: []string{ "", }, @@ -96,9 +102,25 @@ func BuildRole( }, ResourceNames: secretsSet.ToSortedList(), }, - ) + } +} + +// ObjectStoreNamesFromRole extracts the ObjectStore names referenced +// by a plugin-managed Role. It finds the objectstores rule +// semantically (by APIGroup and Resource, not by index) and returns +// a copy of its ResourceNames. Returns nil if no matching rule is +// found. +func ObjectStoreNamesFromRole(role *rbacv1.Role) []string { + for _, rule := range role.Rules { + if len(rule.APIGroups) == 1 && + rule.APIGroups[0] == barmancloudv1.GroupVersion.Group && + len(rule.Resources) == 1 && + rule.Resources[0] == "objectstores" { + return slices.Clone(rule.ResourceNames) + } + } - return role + return nil } // BuildRoleBinding builds the role binding object for this cluster diff --git a/internal/cnpgi/operator/specs/role_test.go b/internal/cnpgi/operator/specs/role_test.go new file mode 100644 index 00000000..bb5c9a13 --- /dev/null +++ b/internal/cnpgi/operator/specs/role_test.go @@ -0,0 +1,210 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package specs + +import ( + barmanapi "github.com/cloudnative-pg/barman-cloud/pkg/api" + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" + machineryapi "github.com/cloudnative-pg/machinery/pkg/api" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata" +) + +func newTestObjectStore(name, secretName string) barmancloudv1.ObjectStore { + return barmancloudv1.ObjectStore{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + }, + Spec: barmancloudv1.ObjectStoreSpec{ + Configuration: barmanapi.BarmanObjectStoreConfiguration{ + DestinationPath: "s3://bucket/path", + BarmanCredentials: barmanapi.BarmanCredentials{ + AWS: &barmanapi.S3Credentials{ + AccessKeyIDReference: &machineryapi.SecretKeySelector{ + LocalObjectReference: machineryapi.LocalObjectReference{ + Name: secretName, + }, + Key: "ACCESS_KEY_ID", + }, + }, + }, + }, + }, + } +} + +var _ = Describe("BuildRoleRules", func() { + It("should produce 3 rules with correct ResourceNames", func() { + objects := []barmancloudv1.ObjectStore{ + newTestObjectStore("store-a", "secret-a"), + newTestObjectStore("store-b", "secret-b"), + } + rules := BuildRoleRules(objects) + Expect(rules).To(HaveLen(3)) + + Expect(rules[0].APIGroups).To(Equal([]string{barmancloudv1.GroupVersion.Group})) + Expect(rules[0].Resources).To(Equal([]string{"objectstores"})) + Expect(rules[0].ResourceNames).To(ConsistOf("store-a", "store-b")) + + Expect(rules[1].APIGroups).To(Equal([]string{barmancloudv1.GroupVersion.Group})) + Expect(rules[1].Resources).To(Equal([]string{"objectstores/status"})) + Expect(rules[1].ResourceNames).To(ConsistOf("store-a", "store-b")) + + Expect(rules[2].APIGroups).To(Equal([]string{""})) + Expect(rules[2].Resources).To(Equal([]string{"secrets"})) + Expect(rules[2].ResourceNames).To(ConsistOf("secret-a", "secret-b")) + }) + + It("should produce rules with empty ResourceNames for empty input", func() { + rules := BuildRoleRules(nil) + Expect(rules).To(HaveLen(3)) + Expect(rules[0].ResourceNames).To(BeEmpty()) + Expect(rules[0].ResourceNames).NotTo(BeNil()) + Expect(rules[1].ResourceNames).To(BeEmpty()) + Expect(rules[2].ResourceNames).To(BeEmpty()) + }) + + It("should deduplicate secret names across ObjectStores", func() { + objects := []barmancloudv1.ObjectStore{ + newTestObjectStore("store-a", "shared-secret"), + newTestObjectStore("store-b", "shared-secret"), + } + rules := BuildRoleRules(objects) + Expect(rules[2].ResourceNames).To(Equal([]string{"shared-secret"})) + }) +}) + +var _ = Describe("BuildRole", func() { + It("should set the cluster label", func() { + cluster := &cnpgv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + Namespace: "default", + }, + } + role := BuildRole(cluster, nil) + Expect(role.Labels).To(HaveKeyWithValue(metadata.ClusterLabelName, "my-cluster")) + Expect(role.Name).To(Equal("my-cluster-barman-cloud")) + Expect(role.Namespace).To(Equal("default")) + }) +}) + +var _ = Describe("BuildRoleRules / ObjectStoreNamesFromRole round-trip", func() { + It("should recover the same ObjectStore names from built rules", func() { + objects := []barmancloudv1.ObjectStore{ + newTestObjectStore("store-a", "secret-a"), + newTestObjectStore("store-b", "secret-b"), + } + rules := BuildRoleRules(objects) + role := &rbacv1.Role{Rules: rules} + names := ObjectStoreNamesFromRole(role) + Expect(names).To(ConsistOf("store-a", "store-b")) + }) + + It("should recover empty names from rules built with no ObjectStores", func() { + rules := BuildRoleRules(nil) + role := &rbacv1.Role{Rules: rules} + names := ObjectStoreNamesFromRole(role) + Expect(names).To(BeEmpty()) + }) +}) + +var _ = Describe("ObjectStoreNamesFromRole", func() { + It("should extract ObjectStore names from a well-formed Role", func() { + role := &rbacv1.Role{ + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{barmancloudv1.GroupVersion.Group}, + Resources: []string{"objectstores"}, + ResourceNames: []string{"store-a", "store-b"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"secrets"}, + ResourceNames: []string{"secret-a"}, + }, + }, + } + Expect(ObjectStoreNamesFromRole(role)).To(Equal([]string{"store-a", "store-b"})) + }) + + It("should return nil for a Role with no matching rule", func() { + role := &rbacv1.Role{ + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"secrets"}, + ResourceNames: []string{"secret-a"}, + }, + }, + } + Expect(ObjectStoreNamesFromRole(role)).To(BeNil()) + }) + + It("should return nil for a Role with empty rules", func() { + role := &rbacv1.Role{} + Expect(ObjectStoreNamesFromRole(role)).To(BeNil()) + }) + + It("should not match a rule with a different APIGroup", func() { + role := &rbacv1.Role{ + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{"other.io"}, + Resources: []string{"objectstores"}, + ResourceNames: []string{"store-a"}, + }, + }, + } + Expect(ObjectStoreNamesFromRole(role)).To(BeNil()) + }) + + It("should not match a rule with multiple APIGroups", func() { + role := &rbacv1.Role{ + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{barmancloudv1.GroupVersion.Group, "other.io"}, + Resources: []string{"objectstores"}, + ResourceNames: []string{"store-a"}, + }, + }, + } + Expect(ObjectStoreNamesFromRole(role)).To(BeNil()) + }) + + It("should not match a rule for objectstores/status", func() { + role := &rbacv1.Role{ + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{barmancloudv1.GroupVersion.Group}, + Resources: []string{"objectstores/status"}, + ResourceNames: []string{"store-a"}, + }, + }, + } + Expect(ObjectStoreNamesFromRole(role)).To(BeNil()) + }) +}) diff --git a/internal/controller/objectstore_controller.go b/internal/controller/objectstore_controller.go index 937b1351..a79b7c95 100644 --- a/internal/controller/objectstore_controller.go +++ b/internal/controller/objectstore_controller.go @@ -21,10 +21,12 @@ package controller import ( "context" + "errors" "fmt" + "slices" - cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" "github.com/cloudnative-pg/machinery/pkg/log" + rbacv1 "k8s.io/api/rbac/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -33,8 +35,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" - "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/config" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata" "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/rbac" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/specs" ) // ObjectStoreReconciler reconciles a ObjectStore object. @@ -46,15 +49,16 @@ type ObjectStoreReconciler struct { // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=create;patch;update;get;list;watch // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=create;patch;update;get;list;watch // +kubebuilder:rbac:groups="",resources=secrets,verbs=create;list;get;watch;delete -// +kubebuilder:rbac:groups=postgresql.cnpg.io,resources=clusters,verbs=get;list;watch -// +kubebuilder:rbac:groups=postgresql.cnpg.io,resources=clusters/finalizers,verbs=update // +kubebuilder:rbac:groups=postgresql.cnpg.io,resources=backups,verbs=get;list;watch +// +kubebuilder:rbac:groups=postgresql.cnpg.io,resources=clusters/finalizers,verbs=update // +kubebuilder:rbac:groups=barmancloud.cnpg.io,resources=objectstores,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=barmancloud.cnpg.io,resources=objectstores/status,verbs=get;update;patch // +kubebuilder:rbac:groups=barmancloud.cnpg.io,resources=objectstores/finalizers,verbs=update // Reconcile ensures that the RBAC Role for each Cluster referencing // this ObjectStore is up to date with the current ObjectStore spec. +// It discovers affected Roles by listing plugin-managed Roles and +// inspecting their rules, without needing access to Cluster objects. func (r *ObjectStoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { contextLogger := log.FromContext(ctx).WithValues( "objectStoreName", req.Name, @@ -64,72 +68,64 @@ func (r *ObjectStoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) contextLogger.Info("ObjectStore reconciliation start") - // List all Clusters in the same namespace - var clusterList cnpgv1.ClusterList - if err := r.List(ctx, &clusterList, client.InNamespace(req.Namespace)); err != nil { - return ctrl.Result{}, fmt.Errorf("while listing clusters: %w", err) + var roleList rbacv1.RoleList + if err := r.List(ctx, &roleList, + client.InNamespace(req.Namespace), + client.HasLabels{metadata.ClusterLabelName}, + ); err != nil { + return ctrl.Result{}, fmt.Errorf("while listing roles: %w", err) } - // For each Cluster that references this ObjectStore, reconcile the Role - for i := range clusterList.Items { - cluster := &clusterList.Items[i] + var errs []error + for i := range roleList.Items { + role := &roleList.Items[i] - pluginConfiguration := config.NewFromCluster(cluster) - referredObjects := pluginConfiguration.GetReferredBarmanObjectsKey() - - if !referencesObjectStore(referredObjects, req.NamespacedName) { + objectStoreNames := specs.ObjectStoreNamesFromRole(role) + if !slices.Contains(objectStoreNames, req.Name) { continue } - contextLogger.Info("Reconciling RBAC for cluster", - "clusterName", cluster.Name) + contextLogger.Info("Reconciling RBAC for role", + "roleName", role.Name) - if err := r.reconcileRBACForCluster(ctx, cluster, referredObjects); err != nil { - return ctrl.Result{}, fmt.Errorf("while reconciling RBAC for cluster %s: %w", cluster.Name, err) + if err := r.reconcileRoleRules(ctx, role, objectStoreNames); err != nil { + contextLogger.Error(err, "Failed to reconcile RBAC for role", + "roleName", role.Name) + errs = append(errs, fmt.Errorf("while reconciling role %s: %w", role.Name, err)) } } contextLogger.Info("ObjectStore reconciliation completed") - return ctrl.Result{}, nil + return ctrl.Result{}, errors.Join(errs...) } -// reconcileRBACForCluster ensures the Role for the given Cluster is -// up to date with the current ObjectStore specs. -func (r *ObjectStoreReconciler) reconcileRBACForCluster( +// reconcileRoleRules fetches the ObjectStores referenced by the +// Role and patches its rules to match the current specs. +func (r *ObjectStoreReconciler) reconcileRoleRules( ctx context.Context, - cluster *cnpgv1.Cluster, - referredObjectKeys []client.ObjectKey, + role *rbacv1.Role, + objectStoreNames []string, ) error { contextLogger := log.FromContext(ctx) - barmanObjects := make([]barmancloudv1.ObjectStore, 0, len(referredObjectKeys)) - for _, key := range referredObjectKeys { + barmanObjects := make([]barmancloudv1.ObjectStore, 0, len(objectStoreNames)) + + for _, name := range objectStoreNames { var barmanObject barmancloudv1.ObjectStore - if err := r.Get(ctx, key, &barmanObject); err != nil { + if err := r.Get(ctx, client.ObjectKey{ + Namespace: role.Namespace, + Name: name, + }, &barmanObject); err != nil { if apierrs.IsNotFound(err) { contextLogger.Info("ObjectStore not found, skipping", - "objectStoreName", key.Name) + "objectStoreName", name) continue } - return fmt.Errorf("while getting ObjectStore %s: %w", key, err) + return fmt.Errorf("while getting ObjectStore %s: %w", name, err) } barmanObjects = append(barmanObjects, barmanObject) } - return rbac.EnsureRole(ctx, r.Client, cluster, barmanObjects) -} - -// referencesObjectStore checks if the given ObjectStore is in the list -// of referred barman objects. -func referencesObjectStore( - referredObjects []client.ObjectKey, - objectStore client.ObjectKey, -) bool { - for _, ref := range referredObjects { - if ref.Name == objectStore.Name && ref.Namespace == objectStore.Namespace { - return true - } - } - return false + return rbac.EnsureRoleRules(ctx, r.Client, client.ObjectKeyFromObject(role), barmanObjects) } // SetupWithManager sets up the controller with the Manager. diff --git a/internal/controller/objectstore_controller_test.go b/internal/controller/objectstore_controller_test.go index fd9d0810..6779415d 100644 --- a/internal/controller/objectstore_controller_test.go +++ b/internal/controller/objectstore_controller_test.go @@ -22,7 +22,6 @@ package controller import ( "context" - cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" barmanapi "github.com/cloudnative-pg/barman-cloud/pkg/api" machineryapi "github.com/cloudnative-pg/machinery/pkg/api" . "github.com/onsi/ginkgo/v2" @@ -30,6 +29,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -37,35 +37,16 @@ import ( 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" ) func newFakeScheme() *runtime.Scheme { s := runtime.NewScheme() - _ = rbacv1.AddToScheme(s) - _ = cnpgv1.AddToScheme(s) - _ = barmancloudv1.AddToScheme(s) + utilruntime.Must(rbacv1.AddToScheme(s)) + utilruntime.Must(barmancloudv1.AddToScheme(s)) return s } -func newTestCluster(name, namespace, objectStoreName string) *cnpgv1.Cluster { - return &cnpgv1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - }, - Spec: cnpgv1.ClusterSpec{ - Plugins: []cnpgv1.PluginConfiguration{ - { - Name: metadata.PluginName, - Parameters: map[string]string{ - "barmanObjectName": objectStoreName, - }, - }, - }, - }, - } -} - func newTestObjectStore(name, namespace, secretName string) *barmancloudv1.ObjectStore { return &barmancloudv1.ObjectStore{ ObjectMeta: metav1.ObjectMeta{ @@ -90,46 +71,23 @@ func newTestObjectStore(name, namespace, secretName string) *barmancloudv1.Objec } } -var _ = Describe("referencesObjectStore", func() { - It("should return true when ObjectStore is in the list", func() { - refs := []client.ObjectKey{ - {Name: "store-a", Namespace: "default"}, - {Name: "store-b", Namespace: "default"}, - } - Expect(referencesObjectStore(refs, client.ObjectKey{ - Name: "store-b", Namespace: "default", - })).To(BeTrue()) - }) - - It("should return false when ObjectStore is not in the list", func() { - refs := []client.ObjectKey{ - {Name: "store-a", Namespace: "default"}, - } - Expect(referencesObjectStore(refs, client.ObjectKey{ - Name: "store-b", Namespace: "default", - })).To(BeFalse()) - }) - - It("should return false when namespace differs", func() { - refs := []client.ObjectKey{ - {Name: "store-a", Namespace: "ns1"}, - } - Expect(referencesObjectStore(refs, client.ObjectKey{ - Name: "store-a", Namespace: "ns2", - })).To(BeFalse()) - }) - - It("should return false for empty list", func() { - Expect(referencesObjectStore(nil, client.ObjectKey{ - Name: "store-a", Namespace: "default", - })).To(BeFalse()) - }) -}) +func newLabeledRole(clusterName, namespace string, objectStores []barmancloudv1.ObjectStore) *rbacv1.Role { + return &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: specs.GetRBACName(clusterName), + Namespace: namespace, + Labels: map[string]string{ + metadata.ClusterLabelName: clusterName, + }, + }, + Rules: specs.BuildRoleRules(objectStores), + } +} var _ = Describe("ObjectStoreReconciler", func() { var ( - ctx context.Context - scheme *runtime.Scheme + ctx context.Context + scheme *runtime.Scheme ) BeforeEach(func() { @@ -138,13 +96,16 @@ var _ = Describe("ObjectStoreReconciler", func() { }) Describe("Reconcile", func() { - It("should create a Role for a Cluster that references the ObjectStore", func() { - objectStore := newTestObjectStore("my-store", "default", "aws-creds") - cluster := newTestCluster("my-cluster", "default", "my-store") + It("should update Role rules when ObjectStore credentials change", func() { + oldStore := newTestObjectStore("my-store", "default", "old-secret") + role := newLabeledRole("my-cluster", "default", []barmancloudv1.ObjectStore{*oldStore}) + + // Update the ObjectStore with new credentials + newStore := newTestObjectStore("my-store", "default", "new-secret") fakeClient := fake.NewClientBuilder(). WithScheme(scheme). - WithObjects(objectStore, cluster). + WithObjects(role, newStore). Build() reconciler := &ObjectStoreReconciler{ @@ -161,31 +122,24 @@ var _ = Describe("ObjectStoreReconciler", func() { Expect(err).NotTo(HaveOccurred()) Expect(result).To(Equal(reconcile.Result{})) - var role rbacv1.Role - err = fakeClient.Get(ctx, client.ObjectKey{ + var updatedRole rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ Namespace: "default", Name: "my-cluster-barman-cloud", - }, &role) - Expect(err).NotTo(HaveOccurred()) - Expect(role.Rules).To(HaveLen(3)) + }, &updatedRole)).To(Succeed()) - // Verify the secrets rule contains the expected secret - secretsRule := role.Rules[2] - Expect(secretsRule.ResourceNames).To(ContainElement("aws-creds")) - - // Verify owner reference is set to the Cluster - Expect(role.OwnerReferences).To(HaveLen(1)) - Expect(role.OwnerReferences[0].Name).To(Equal("my-cluster")) - Expect(role.OwnerReferences[0].Kind).To(Equal("Cluster")) + secretsRule := updatedRole.Rules[2] + Expect(secretsRule.ResourceNames).To(ContainElement("new-secret")) + Expect(secretsRule.ResourceNames).NotTo(ContainElement("old-secret")) }) - It("should skip Clusters that don't reference the ObjectStore", func() { - objectStore := newTestObjectStore("my-store", "default", "aws-creds") - cluster := newTestCluster("my-cluster", "default", "other-store") + It("should skip Roles that don't reference the ObjectStore", func() { + otherStore := newTestObjectStore("other-store", "default", "other-creds") + role := newLabeledRole("my-cluster", "default", []barmancloudv1.ObjectStore{*otherStore}) fakeClient := fake.NewClientBuilder(). WithScheme(scheme). - WithObjects(objectStore, cluster). + WithObjects(role). Build() reconciler := &ObjectStoreReconciler{ @@ -193,25 +147,31 @@ var _ = Describe("ObjectStoreReconciler", func() { Scheme: scheme, } + var before rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "my-cluster-barman-cloud", + }, &before)).To(Succeed()) + result, err := reconciler.Reconcile(ctx, reconcile.Request{ NamespacedName: types.NamespacedName{ - Name: "my-store", + Name: "unrelated-store", Namespace: "default", }, }) Expect(err).NotTo(HaveOccurred()) Expect(result).To(Equal(reconcile.Result{})) - // No Role should have been created - var role rbacv1.Role - err = fakeClient.Get(ctx, client.ObjectKey{ + var after rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ Namespace: "default", Name: "my-cluster-barman-cloud", - }, &role) - Expect(err).To(HaveOccurred()) + }, &after)).To(Succeed()) + + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion)) }) - It("should succeed with no Clusters in the namespace", func() { + It("should succeed with no labeled Roles in the namespace", func() { fakeClient := fake.NewClientBuilder(). WithScheme(scheme). Build() @@ -230,17 +190,16 @@ var _ = Describe("ObjectStoreReconciler", func() { Expect(err).NotTo(HaveOccurred()) Expect(result).To(Equal(reconcile.Result{})) }) - }) - Describe("reconcileRBACForCluster", func() { - It("should skip deleted ObjectStores and still reconcile the Role", func() { - // Cluster references two ObjectStores, but one is deleted - cluster := newTestCluster("my-cluster", "default", "store-a") - existingStore := newTestObjectStore("store-a", "default", "aws-creds") + It("should handle deleted ObjectStores gracefully", func() { + storeA := newTestObjectStore("store-a", "default", "secret-a") + storeB := newTestObjectStore("store-b", "default", "secret-b") + role := newLabeledRole("my-cluster", "default", []barmancloudv1.ObjectStore{*storeA, *storeB}) + // Only store-a exists; store-b was deleted fakeClient := fake.NewClientBuilder(). WithScheme(scheme). - WithObjects(existingStore). + WithObjects(role, storeA). Build() reconciler := &ObjectStoreReconciler{ @@ -248,39 +207,40 @@ var _ = Describe("ObjectStoreReconciler", func() { Scheme: scheme, } - // Pass two keys, but "store-b" doesn't exist - err := reconciler.reconcileRBACForCluster(ctx, cluster, []client.ObjectKey{ - {Name: "store-a", Namespace: "default"}, - {Name: "store-b", Namespace: "default"}, + result, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "store-b", + Namespace: "default", + }, }) Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) - // Role should be created with only store-a's secrets - var role rbacv1.Role - err = fakeClient.Get(ctx, client.ObjectKey{ + var updatedRole rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ Namespace: "default", Name: "my-cluster-barman-cloud", - }, &role) - Expect(err).NotTo(HaveOccurred()) - Expect(role.Rules).To(HaveLen(3)) + }, &updatedRole)).To(Succeed()) - // ObjectStore rule should only reference store-a - objectStoreRule := role.Rules[0] + objectStoreRule := updatedRole.Rules[0] Expect(objectStoreRule.ResourceNames).To(ContainElement("store-a")) Expect(objectStoreRule.ResourceNames).NotTo(ContainElement("store-b")) - - // Verify owner reference is set - Expect(role.OwnerReferences).To(HaveLen(1)) - Expect(role.OwnerReferences[0].Name).To(Equal("my-cluster")) }) - It("should update Role when ObjectStore credentials change", func() { - cluster := newTestCluster("my-cluster", "default", "my-store") - oldStore := newTestObjectStore("my-store", "default", "old-secret") + It("should not panic on a Role with empty rules", func() { + emptyRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-barman-cloud", + Namespace: "default", + Labels: map[string]string{ + metadata.ClusterLabelName: "empty", + }, + }, + } fakeClient := fake.NewClientBuilder(). WithScheme(scheme). - WithObjects(oldStore). + WithObjects(emptyRole). Build() reconciler := &ObjectStoreReconciler{ @@ -288,35 +248,104 @@ var _ = Describe("ObjectStoreReconciler", func() { Scheme: scheme, } - // First reconcile - creates Role with old-secret - err := reconciler.reconcileRBACForCluster(ctx, cluster, []client.ObjectKey{ - {Name: "my-store", Namespace: "default"}, + result, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "my-store", + Namespace: "default", + }, }) Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + }) - // Update the ObjectStore with new credentials - var currentStore barmancloudv1.ObjectStore - Expect(fakeClient.Get(ctx, client.ObjectKey{ - Name: "my-store", Namespace: "default", - }, ¤tStore)).To(Succeed()) - currentStore.Spec.Configuration.BarmanCredentials.AWS.AccessKeyIDReference.LocalObjectReference.Name = "new-secret" - Expect(fakeClient.Update(ctx, ¤tStore)).To(Succeed()) - - // Second reconcile - should patch Role with new-secret - err = reconciler.reconcileRBACForCluster(ctx, cluster, []client.ObjectKey{ - {Name: "my-store", Namespace: "default"}, + It("should produce empty ResourceNames when all ObjectStores are deleted", func() { + store := newTestObjectStore("my-store", "default", "aws-creds") + role := newLabeledRole("my-cluster", "default", []barmancloudv1.ObjectStore{*store}) + + // Don't add the ObjectStore to the fake client (simulates deletion) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(role). + Build() + + reconciler := &ObjectStoreReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + result, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "my-store", + Namespace: "default", + }, }) Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) - var role rbacv1.Role + var updatedRole rbacv1.Role Expect(fakeClient.Get(ctx, client.ObjectKey{ Namespace: "default", Name: "my-cluster-barman-cloud", - }, &role)).To(Succeed()) + }, &updatedRole)).To(Succeed()) - secretsRule := role.Rules[2] - Expect(secretsRule.ResourceNames).To(ContainElement("new-secret")) - Expect(secretsRule.ResourceNames).NotTo(ContainElement("old-secret")) + // All rules should have empty ResourceNames + Expect(updatedRole.Rules[0].ResourceNames).To(BeEmpty()) + Expect(updatedRole.Rules[1].ResourceNames).To(BeEmpty()) + Expect(updatedRole.Rules[2].ResourceNames).To(BeEmpty()) + }) + + It("should reconcile multiple Roles referencing the same ObjectStore", func() { + store := newTestObjectStore("shared-store", "default", "new-secret") + oldStore := barmancloudv1.ObjectStore{ + ObjectMeta: metav1.ObjectMeta{Name: "shared-store", Namespace: "default"}, + Spec: barmancloudv1.ObjectStoreSpec{ + Configuration: barmanapi.BarmanObjectStoreConfiguration{ + DestinationPath: "s3://bucket/path", + BarmanCredentials: barmanapi.BarmanCredentials{ + AWS: &barmanapi.S3Credentials{ + AccessKeyIDReference: &machineryapi.SecretKeySelector{ + LocalObjectReference: machineryapi.LocalObjectReference{Name: "old-secret"}, + Key: "ACCESS_KEY_ID", + }, + }, + }, + }, + }, + } + + role1 := newLabeledRole("cluster-1", "default", []barmancloudv1.ObjectStore{oldStore}) + role2 := newLabeledRole("cluster-2", "default", []barmancloudv1.ObjectStore{oldStore}) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(role1, role2, store). + Build() + + reconciler := &ObjectStoreReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + result, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "shared-store", + Namespace: "default", + }, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + + for _, clusterName := range []string{"cluster-1", "cluster-2"} { + var updatedRole rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: specs.GetRBACName(clusterName), + }, &updatedRole)).To(Succeed()) + + secretsRule := updatedRole.Rules[2] + Expect(secretsRule.ResourceNames).To(ContainElement("new-secret")) + Expect(secretsRule.ResourceNames).NotTo(ContainElement("old-secret")) + } }) }) }) diff --git a/manifest.yaml b/manifest.yaml index 9e9b70a7..60b755e6 100644 --- a/manifest.yaml +++ b/manifest.yaml @@ -870,7 +870,6 @@ rules: - postgresql.cnpg.io resources: - backups - - clusters verbs: - get - list From 4dfd20b2efd3c30e7555a66e08e81f0a6760a787 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Fri, 10 Apr 2026 10:25:54 +0200 Subject: [PATCH 3/6] fix: read owner GVK from object metadata instead of scheme 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 --- internal/cmd/operator/main.go | 2 - internal/cnpgi/operator/manager.go | 17 +--- internal/cnpgi/operator/rbac/doc.go | 22 +++++ internal/cnpgi/operator/rbac/ensure.go | 5 +- internal/cnpgi/operator/rbac/ensure_test.go | 4 + internal/cnpgi/operator/reconciler.go | 3 +- internal/cnpgi/operator/specs/ownership.go | 59 ++++++++++++ .../cnpgi/operator/specs/ownership_test.go | 90 +++++++++++++++++++ internal/scheme/cnpg.go | 2 - internal/scheme/doc.go | 22 +++++ 10 files changed, 203 insertions(+), 23 deletions(-) create mode 100644 internal/cnpgi/operator/rbac/doc.go create mode 100644 internal/cnpgi/operator/specs/ownership.go create mode 100644 internal/cnpgi/operator/specs/ownership_test.go create mode 100644 internal/scheme/doc.go diff --git a/internal/cmd/operator/main.go b/internal/cmd/operator/main.go index 49f9ad9f..33570543 100644 --- a/internal/cmd/operator/main.go +++ b/internal/cmd/operator/main.go @@ -102,8 +102,6 @@ func NewCmd() *cobra.Command { _ = viper.BindPFlag("server-address", cmd.Flags().Lookup("server-address")) _ = viper.BindEnv("sidecar-image", "SIDECAR_IMAGE") - _ = viper.BindEnv("custom-cnpg-group", "CUSTOM_CNPG_GROUP") - _ = viper.BindEnv("custom-cnpg-version", "CUSTOM_CNPG_VERSION") return cmd } diff --git a/internal/cnpgi/operator/manager.go b/internal/cnpgi/operator/manager.go index 87cc8ee2..53db11e8 100644 --- a/internal/cnpgi/operator/manager.go +++ b/internal/cnpgi/operator/manager.go @@ -37,33 +37,24 @@ import ( barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" "github.com/cloudnative-pg/plugin-barman-cloud/internal/controller" - pluginscheme "github.com/cloudnative-pg/plugin-barman-cloud/internal/scheme" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" ) -// generateScheme creates a runtime.Scheme with all type definitions -// needed by the operator. CNPG types are registered under a -// configurable API group to support custom CNPG-based operators. -func generateScheme(ctx context.Context) *runtime.Scheme { - result := runtime.NewScheme() +var scheme = runtime.NewScheme() - utilruntime.Must(clientgoscheme.AddToScheme(result)) - utilruntime.Must(barmancloudv1.AddToScheme(result)) - pluginscheme.AddCNPGToScheme(ctx, result) +func init() { + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(barmancloudv1.AddToScheme(scheme)) // +kubebuilder:scaffold:scheme - - return result } // Start starts the manager func Start(ctx context.Context) error { setupLog := log.FromContext(ctx) - scheme := generateScheme(ctx) - var tlsOpts []func(*tls.Config) // if the enable-http2 flag is false (the default), http/2 should be disabled diff --git a/internal/cnpgi/operator/rbac/doc.go b/internal/cnpgi/operator/rbac/doc.go new file mode 100644 index 00000000..802e058b --- /dev/null +++ b/internal/cnpgi/operator/rbac/doc.go @@ -0,0 +1,22 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +// Package rbac contains utilities to reconcile RBAC resources +// for the barman-cloud plugin. +package rbac diff --git a/internal/cnpgi/operator/rbac/ensure.go b/internal/cnpgi/operator/rbac/ensure.go index c92821c9..44bf706a 100644 --- a/internal/cnpgi/operator/rbac/ensure.go +++ b/internal/cnpgi/operator/rbac/ensure.go @@ -17,8 +17,6 @@ limitations under the License. SPDX-License-Identifier: Apache-2.0 */ -// Package rbac contains utilities to reconcile RBAC resources -// for the barman-cloud plugin. package rbac import ( @@ -31,7 +29,6 @@ import ( 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" @@ -106,7 +103,7 @@ func ensureRoleExists( return err } - if err := controllerutil.SetControllerReference(cluster, newRole, c.Scheme()); err != nil { + if err := specs.SetControllerReference(cluster, newRole); err != nil { return err } diff --git a/internal/cnpgi/operator/rbac/ensure_test.go b/internal/cnpgi/operator/rbac/ensure_test.go index 380c2f6c..23f364e9 100644 --- a/internal/cnpgi/operator/rbac/ensure_test.go +++ b/internal/cnpgi/operator/rbac/ensure_test.go @@ -49,6 +49,10 @@ func newScheme() *runtime.Scheme { func newCluster(name, namespace string) *cnpgv1.Cluster { return &cnpgv1.Cluster{ + TypeMeta: metav1.TypeMeta{ + APIVersion: cnpgv1.SchemeGroupVersion.String(), + Kind: cnpgv1.ClusterKind, + }, ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, diff --git a/internal/cnpgi/operator/reconciler.go b/internal/cnpgi/operator/reconciler.go index fa4b6fcd..87647ebc 100644 --- a/internal/cnpgi/operator/reconciler.go +++ b/internal/cnpgi/operator/reconciler.go @@ -30,7 +30,6 @@ import ( rbacv1 "k8s.io/api/rbac/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "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/operator/config" @@ -163,7 +162,7 @@ func (r ReconcilerImplementation) createRoleBinding( cluster *cnpgv1.Cluster, ) error { roleBinding := specs.BuildRoleBinding(cluster) - if err := controllerutil.SetControllerReference(cluster, roleBinding, r.Client.Scheme()); err != nil { + if err := specs.SetControllerReference(cluster, roleBinding); err != nil { return err } return r.Client.Create(ctx, roleBinding) diff --git a/internal/cnpgi/operator/specs/ownership.go b/internal/cnpgi/operator/specs/ownership.go new file mode 100644 index 00000000..7bc6c747 --- /dev/null +++ b/internal/cnpgi/operator/specs/ownership.go @@ -0,0 +1,59 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package specs + +import ( + "fmt" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" +) + +// SetControllerReference sets an owner reference on controlled +// pointing to owner, reading the GVK from the owner object's +// metadata rather than from a scheme. This is necessary because +// 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. +func SetControllerReference(owner, controlled metav1.Object) error { + ro, ok := owner.(runtime.Object) + if !ok { + return fmt.Errorf("%T is not a runtime.Object, cannot call SetControllerReference", owner) + } + + gvk := ro.GetObjectKind().GroupVersionKind() + if gvk.Kind == "" { + return fmt.Errorf("%T has no GVK set in its metadata, cannot call SetControllerReference", owner) + } + + controlled.SetOwnerReferences([]metav1.OwnerReference{ + { + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, + Name: owner.GetName(), + UID: owner.GetUID(), + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(true), + }, + }) + + return nil +} diff --git a/internal/cnpgi/operator/specs/ownership_test.go b/internal/cnpgi/operator/specs/ownership_test.go new file mode 100644 index 00000000..35a0673d --- /dev/null +++ b/internal/cnpgi/operator/specs/ownership_test.go @@ -0,0 +1,90 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package specs + +import ( + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +var _ = Describe("SetControllerReference", func() { + It("should set the owner reference from the owner's TypeMeta", func() { + owner := &cnpgv1.Cluster{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "postgresql.cnpg.io/v1", + Kind: "Cluster", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + Namespace: "default", + UID: types.UID("test-uid"), + }, + } + controlled := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-role", + Namespace: "default", + }, + } + + Expect(SetControllerReference(owner, controlled)).To(Succeed()) + Expect(controlled.OwnerReferences).To(HaveLen(1)) + Expect(controlled.OwnerReferences[0].APIVersion).To(Equal("postgresql.cnpg.io/v1")) + Expect(controlled.OwnerReferences[0].Kind).To(Equal("Cluster")) + Expect(controlled.OwnerReferences[0].Name).To(Equal("my-cluster")) + Expect(controlled.OwnerReferences[0].UID).To(Equal(types.UID("test-uid"))) + Expect(*controlled.OwnerReferences[0].Controller).To(BeTrue()) + Expect(*controlled.OwnerReferences[0].BlockOwnerDeletion).To(BeTrue()) + }) + + It("should work with a custom CNPG API group", func() { + owner := &cnpgv1.Cluster{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "mycompany.io/v1", + Kind: "Cluster", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + UID: types.UID("test-uid"), + }, + } + controlled := &rbacv1.Role{} + + Expect(SetControllerReference(owner, controlled)).To(Succeed()) + Expect(controlled.OwnerReferences[0].APIVersion).To(Equal("mycompany.io/v1")) + }) + + It("should fail when the owner has no GVK set", func() { + owner := &cnpgv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + }, + } + controlled := &rbacv1.Role{} + + err := SetControllerReference(owner, controlled) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("has no GVK set")) + }) +}) diff --git a/internal/scheme/cnpg.go b/internal/scheme/cnpg.go index 5ebd4b3e..a012e9dc 100644 --- a/internal/scheme/cnpg.go +++ b/internal/scheme/cnpg.go @@ -17,8 +17,6 @@ limitations under the License. SPDX-License-Identifier: Apache-2.0 */ -// Package scheme provides utilities for building runtime schemes -// with support for custom CNPG API groups. package scheme import ( diff --git a/internal/scheme/doc.go b/internal/scheme/doc.go new file mode 100644 index 00000000..71285235 --- /dev/null +++ b/internal/scheme/doc.go @@ -0,0 +1,22 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +// Package scheme provides utilities for building runtime schemes +// with support for custom CNPG API groups. +package scheme From 450353275fd5278af7b0a43e74a77ec4a99b7a49 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Fri, 10 Apr 2026 16:57:39 +0200 Subject: [PATCH 4/6] test: add e2e test for credential rotation 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 --- test/e2e/e2e_suite_test.go | 1 + .../credentialrotation/credential_rotation.go | 171 ++++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100644 test/e2e/internal/tests/credentialrotation/credential_rotation.go diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 64fae91a..bbf7b901 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -37,6 +37,7 @@ import ( "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/kustomize" _ "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/tests/backup" + _ "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/tests/credentialrotation" _ "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/tests/replicacluster" . "github.com/onsi/ginkgo/v2" diff --git a/test/e2e/internal/tests/credentialrotation/credential_rotation.go b/test/e2e/internal/tests/credentialrotation/credential_rotation.go new file mode 100644 index 00000000..f5302591 --- /dev/null +++ b/test/e2e/internal/tests/credentialrotation/credential_rotation.go @@ -0,0 +1,171 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package credentialrotation + +import ( + "time" + + cloudnativepgv1 "github.com/cloudnative-pg/api/pkg/api/v1" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/specs" + internalClient "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/client" + internalCluster "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/cluster" + nmsp "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/namespace" + "github.com/cloudnative-pg/plugin-barman-cloud/test/e2e/internal/objectstore" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +const ( + clusterName = "source" + objectStoreName = "source" + oldSecretName = "minio" + newSecretName = "minio-rotated" +) + +var _ = Describe("Credential rotation", func() { + var namespace *corev1.Namespace + var cl client.Client + + BeforeEach(func(ctx SpecContext) { + var err error + cl, _, err = internalClient.NewClient() + Expect(err).NotTo(HaveOccurred()) + namespace, err = nmsp.CreateUniqueNamespace(ctx, cl, "cred-rotation") + Expect(err).NotTo(HaveOccurred()) + }) + + AfterEach(func(ctx SpecContext) { + Expect(cl.Delete(ctx, namespace)).To(Succeed()) + }) + + It("should update the Role when the ObjectStore secret reference changes", func(ctx SpecContext) { + By("starting the ObjectStore deployment") + resources := objectstore.NewMinioObjectStoreResources(namespace.Name, oldSecretName) + Expect(resources.Create(ctx, cl)).To(Succeed()) + + By("creating the ObjectStore") + store := objectstore.NewMinioObjectStore(namespace.Name, objectStoreName, oldSecretName) + Expect(cl.Create(ctx, store)).To(Succeed()) + + By("creating the Cluster") + cluster := newCluster(namespace.Name) + Expect(cl.Create(ctx, cluster)).To(Succeed()) + + By("waiting for the Cluster to be ready") + Eventually(func(g Gomega) { + g.Expect(cl.Get(ctx, types.NamespacedName{ + Name: cluster.Name, + Namespace: cluster.Namespace, + }, cluster)).To(Succeed()) + g.Expect(internalCluster.IsReady(*cluster)).To(BeTrue()) + }).WithTimeout(10 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) + + roleKey := types.NamespacedName{ + Name: specs.GetRBACName(clusterName), + Namespace: namespace.Name, + } + + By("verifying the Role has the cluster label and references the original secret") + var role rbacv1.Role + Expect(cl.Get(ctx, roleKey, &role)).To(Succeed()) + Expect(role.Labels).To(HaveKeyWithValue(metadata.ClusterLabelName, clusterName)) + Expect(secretNamesFromRole(&role)).To(ContainElement(oldSecretName)) + + By("creating a new secret with the same credentials") + newSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: newSecretName, + Namespace: namespace.Name, + }, + Data: map[string][]byte{ + "ACCESS_KEY_ID": []byte("minio"), + "ACCESS_SECRET_KEY": []byte("minio123"), + }, + } + Expect(cl.Create(ctx, newSecret)).To(Succeed()) + + By("updating the ObjectStore to reference the new secret") + Expect(cl.Get(ctx, types.NamespacedName{ + Name: objectStoreName, + Namespace: namespace.Name, + }, store)).To(Succeed()) + store.Spec.Configuration.BarmanCredentials.AWS.AccessKeyIDReference.Name = newSecretName + store.Spec.Configuration.BarmanCredentials.AWS.SecretAccessKeyReference.Name = newSecretName + Expect(cl.Update(ctx, store)).To(Succeed()) + + By("waiting for the Role to reference the new secret") + Eventually(func(g Gomega) { + g.Expect(cl.Get(ctx, roleKey, &role)).To(Succeed()) + g.Expect(secretNamesFromRole(&role)).To(ContainElement(newSecretName)) + g.Expect(secretNamesFromRole(&role)).NotTo(ContainElement(oldSecretName)) + }).WithTimeout(3 * time.Minute).WithPolling(5 * time.Second).Should(Succeed()) + }) +}) + +func newCluster(namespace string) *cloudnativepgv1.Cluster { + return &cloudnativepgv1.Cluster{ + TypeMeta: metav1.TypeMeta{ + Kind: "Cluster", + APIVersion: "postgresql.cnpg.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: namespace, + }, + Spec: cloudnativepgv1.ClusterSpec{ + Instances: 1, + ImagePullPolicy: corev1.PullAlways, + Plugins: []cloudnativepgv1.PluginConfiguration{ + { + Name: "barman-cloud.cloudnative-pg.io", + Parameters: map[string]string{ + "barmanObjectName": objectStoreName, + }, + IsWALArchiver: ptr.To(true), + }, + }, + StorageConfiguration: cloudnativepgv1.StorageConfiguration{ + Size: "1Gi", + }, + }, + } +} + +func secretNamesFromRole(role *rbacv1.Role) []string { + for _, rule := range role.Rules { + if len(rule.APIGroups) == 1 && + rule.APIGroups[0] == "" && + len(rule.Resources) == 1 && + rule.Resources[0] == "secrets" { + return rule.ResourceNames + } + } + + return nil +} From 61350c794c3a57a124259fb03c22aeea8a69621a Mon Sep 17 00:00:00 2001 From: Gabriele Quaresima Date: Fri, 10 Apr 2026 17:15:26 +0200 Subject: [PATCH 5/6] chore: add unit tests, improve log and code readability (#839) - 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 Signed-off-by: Marco Nenciarini Co-authored-by: Marco Nenciarini --- internal/cnpgi/operator/rbac/ensure_test.go | 53 ++++++++ internal/cnpgi/operator/reconciler.go | 4 +- .../cnpgi/operator/specs/ownership_test.go | 10 ++ .../controller/objectstore_controller_test.go | 51 ++++++++ internal/scheme/cnpg_test.go | 116 ++++++++++++++++++ internal/scheme/suite_test.go | 32 +++++ 6 files changed, 264 insertions(+), 2 deletions(-) create mode 100644 internal/scheme/cnpg_test.go create mode 100644 internal/scheme/suite_test.go diff --git a/internal/cnpgi/operator/rbac/ensure_test.go b/internal/cnpgi/operator/rbac/ensure_test.go index 23f364e9..56a78242 100644 --- a/internal/cnpgi/operator/rbac/ensure_test.go +++ b/internal/cnpgi/operator/rbac/ensure_test.go @@ -21,6 +21,7 @@ package rbac_test import ( "context" + "fmt" barmanapi "github.com/cloudnative-pg/barman-cloud/pkg/api" cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" @@ -28,11 +29,13 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" rbacv1 "k8s.io/api/rbac/v1" + apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata" @@ -179,6 +182,56 @@ var _ = Describe("EnsureRole", func() { }) }) + Context("when Role creation fails with a transient error", func() { + BeforeEach(func() { + internalErr := apierrs.NewInternalError(fmt.Errorf("etcd timeout")) + fakeClient = fake.NewClientBuilder(). + WithScheme(newScheme()). + WithInterceptorFuncs(interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + return internalErr + }, + }). + Build() + }) + + It("should propagate the error", func() { + err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) + Expect(err).To(HaveOccurred()) + Expect(apierrs.IsInternalError(err)).To(BeTrue()) + }) + }) + + Context("when the Role has pre-existing unrelated labels", func() { + BeforeEach(func() { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() + existing := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-barman-cloud", + Namespace: "default", + Labels: map[string]string{ + "app.kubernetes.io/managed-by": "helm", + }, + }, + } + Expect(fakeClient.Create(ctx, existing)).To(Succeed()) + }) + + It("should preserve unrelated labels while adding the cluster label", func() { + err := rbac.EnsureRole(ctx, fakeClient, cluster, objects) + Expect(err).NotTo(HaveOccurred()) + + var role rbacv1.Role + Expect(fakeClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-cluster-barman-cloud", + }, &role)).To(Succeed()) + + Expect(role.Labels).To(HaveKeyWithValue("app.kubernetes.io/managed-by", "helm")) + Expect(role.Labels).To(HaveKeyWithValue(metadata.ClusterLabelName, "test-cluster")) + }) + }) + Context("when the Role exists without the cluster label (upgrade scenario)", func() { BeforeEach(func() { fakeClient = fake.NewClientBuilder().WithScheme(newScheme()).Build() diff --git a/internal/cnpgi/operator/reconciler.go b/internal/cnpgi/operator/reconciler.go index 87647ebc..5ac61cd5 100644 --- a/internal/cnpgi/operator/reconciler.go +++ b/internal/cnpgi/operator/reconciler.go @@ -141,11 +141,11 @@ func (r ReconcilerImplementation) ensureRoleBinding( ctx context.Context, cluster *cnpgv1.Cluster, ) error { - var role rbacv1.RoleBinding + var roleBinding rbacv1.RoleBinding if err := r.Client.Get(ctx, client.ObjectKey{ Namespace: cluster.Namespace, Name: specs.GetRBACName(cluster.Name), - }, &role); err != nil { + }, &roleBinding); err != nil { if apierrs.IsNotFound(err) { return r.createRoleBinding(ctx, cluster) } diff --git a/internal/cnpgi/operator/specs/ownership_test.go b/internal/cnpgi/operator/specs/ownership_test.go index 35a0673d..9be7a5d5 100644 --- a/internal/cnpgi/operator/specs/ownership_test.go +++ b/internal/cnpgi/operator/specs/ownership_test.go @@ -87,4 +87,14 @@ var _ = Describe("SetControllerReference", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("has no GVK set")) }) + + It("should fail when the owner does not implement runtime.Object", func() { + // metav1.ObjectMeta satisfies metav1.Object but not runtime.Object. + owner := &metav1.ObjectMeta{Name: "my-cluster"} + controlled := &rbacv1.Role{} + + err := SetControllerReference(owner, controlled) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("is not a runtime.Object")) + }) }) diff --git a/internal/controller/objectstore_controller_test.go b/internal/controller/objectstore_controller_test.go index 6779415d..a8b896eb 100644 --- a/internal/controller/objectstore_controller_test.go +++ b/internal/controller/objectstore_controller_test.go @@ -21,18 +21,21 @@ package controller import ( "context" + "fmt" barmanapi "github.com/cloudnative-pg/barman-cloud/pkg/api" machineryapi "github.com/cloudnative-pg/machinery/pkg/api" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" rbacv1 "k8s.io/api/rbac/v1" + apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" @@ -294,6 +297,54 @@ var _ = Describe("ObjectStoreReconciler", func() { Expect(updatedRole.Rules[2].ResourceNames).To(BeEmpty()) }) + It("should return an error when listing Roles fails", func() { + internalErr := apierrs.NewInternalError(fmt.Errorf("etcd timeout")) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*rbacv1.RoleList); ok { + return internalErr + } + return c.List(ctx, list, opts...) + }, + }). + Build() + + reconciler := &ObjectStoreReconciler{Client: fakeClient, Scheme: scheme} + _, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: "my-store", Namespace: "default"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("while listing roles")) + }) + + It("should return an error when fetching an ObjectStore fails with a transient error", func() { + store := newTestObjectStore("my-store", "default", "aws-creds") + role := newLabeledRole("my-cluster", "default", []barmancloudv1.ObjectStore{*store}) + + internalErr := apierrs.NewInternalError(fmt.Errorf("etcd timeout")) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(role). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*barmancloudv1.ObjectStore); ok { + return internalErr + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + reconciler := &ObjectStoreReconciler{Client: fakeClient, Scheme: scheme} + _, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: "my-store", Namespace: "default"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("while reconciling role")) + }) + It("should reconcile multiple Roles referencing the same ObjectStore", func() { store := newTestObjectStore("shared-store", "default", "new-secret") oldStore := barmancloudv1.ObjectStore{ diff --git a/internal/scheme/cnpg_test.go b/internal/scheme/cnpg_test.go new file mode 100644 index 00000000..67d21519 --- /dev/null +++ b/internal/scheme/cnpg_test.go @@ -0,0 +1,116 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package scheme + +import ( + "context" + + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/spf13/viper" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +var _ = Describe("AddCNPGToScheme", func() { + var s *runtime.Scheme + + BeforeEach(func() { + s = runtime.NewScheme() + }) + + AfterEach(func() { + viper.Reset() + }) + + It("should register CNPG types under the default group and version", func() { + AddCNPGToScheme(context.Background(), s) + + gvks, _, err := s.ObjectKinds(&cnpgv1.Cluster{}) + Expect(err).NotTo(HaveOccurred()) + Expect(gvks).To(ContainElement(schema.GroupVersionKind{ + Group: cnpgv1.SchemeGroupVersion.Group, + Version: cnpgv1.SchemeGroupVersion.Version, + Kind: "Cluster", + })) + }) + + It("should register Backup and ScheduledBackup under the default group", func() { + AddCNPGToScheme(context.Background(), s) + + gvks, _, err := s.ObjectKinds(&cnpgv1.Backup{}) + Expect(err).NotTo(HaveOccurred()) + Expect(gvks).To(ContainElement(HaveField("Group", cnpgv1.SchemeGroupVersion.Group))) + + gvks, _, err = s.ObjectKinds(&cnpgv1.ScheduledBackup{}) + Expect(err).NotTo(HaveOccurred()) + Expect(gvks).To(ContainElement(HaveField("Group", cnpgv1.SchemeGroupVersion.Group))) + }) + + It("should register CNPG types under a custom group", func() { + viper.Set("custom-cnpg-group", "mycompany.io") + + AddCNPGToScheme(context.Background(), s) + + gvks, _, err := s.ObjectKinds(&cnpgv1.Cluster{}) + Expect(err).NotTo(HaveOccurred()) + Expect(gvks).To(ContainElement(schema.GroupVersionKind{ + Group: "mycompany.io", + Version: cnpgv1.SchemeGroupVersion.Version, + Kind: "Cluster", + })) + // The default group must not be registered + Expect(s.Recognizes(schema.GroupVersionKind{ + Group: cnpgv1.SchemeGroupVersion.Group, + Version: cnpgv1.SchemeGroupVersion.Version, + Kind: "Cluster", + })).To(BeFalse()) + }) + + It("should register CNPG types under a custom version", func() { + viper.Set("custom-cnpg-version", "v2") + + AddCNPGToScheme(context.Background(), s) + + gvks, _, err := s.ObjectKinds(&cnpgv1.Cluster{}) + Expect(err).NotTo(HaveOccurred()) + Expect(gvks).To(ContainElement(schema.GroupVersionKind{ + Group: cnpgv1.SchemeGroupVersion.Group, + Version: "v2", + Kind: "Cluster", + })) + }) + + It("should register CNPG types under both a custom group and custom version", func() { + viper.Set("custom-cnpg-group", "mycompany.io") + viper.Set("custom-cnpg-version", "v2") + + AddCNPGToScheme(context.Background(), s) + + gvks, _, err := s.ObjectKinds(&cnpgv1.Cluster{}) + Expect(err).NotTo(HaveOccurred()) + Expect(gvks).To(ContainElement(schema.GroupVersionKind{ + Group: "mycompany.io", + Version: "v2", + Kind: "Cluster", + })) + }) +}) diff --git a/internal/scheme/suite_test.go b/internal/scheme/suite_test.go new file mode 100644 index 00000000..289f44c7 --- /dev/null +++ b/internal/scheme/suite_test.go @@ -0,0 +1,32 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package scheme + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestScheme(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Scheme Suite") +} From aae694f5394a0571b6e580b0aee5dd99f8bf7326 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Mon, 13 Apr 2026 12:18:18 +0200 Subject: [PATCH 6/6] 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 44bf706a..b9d8c6a8 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 7bc6c747..dded5d1a 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 c14a02aa..bcfe8c93 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 a79b7c95..e0a77dd5 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),