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 <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>

Author: gabriele-wolfox

internal/cnpgi/operator/rbac/ensure_test.go

@@ -21,18 +21,21 @@ package rbac_test
 
 import (
 	"context"
+	"fmt"
 
 	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"
+	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()

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)
 		}

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"))
+	})
 })

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{

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",
+		}))
+	})
+})

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")
+}