From 176b55de781f827c39e83a345e620f63562169b0 Mon Sep 17 00:00:00 2001 From: Gabriele Quaresima Date: Fri, 10 Apr 2026 16:16:03 +0200 Subject: [PATCH] chore: add unit tests, improve log and code readability - 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 Co-authored-by: Claude Sonnet 4.6 Signed-off-by: Gabriele Quaresima --- internal/cnpgi/operator/rbac/ensure_test.go | 53 ++++++++ internal/cnpgi/operator/reconciler.go | 4 +- .../cnpgi/operator/specs/ownership_test.go | 10 ++ internal/controller/objectstore_controller.go | 2 +- .../controller/objectstore_controller_test.go | 51 ++++++++ internal/scheme/cnpg_test.go | 116 ++++++++++++++++++ internal/scheme/suite_test.go | 32 +++++ 7 files changed, 265 insertions(+), 3 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 23f364e..56a7824 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 87647eb..5ac61cd 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 35a0673..9be7a5d 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.go b/internal/controller/objectstore_controller.go index a79b7c9..0bab577 100644 --- a/internal/controller/objectstore_controller.go +++ b/internal/controller/objectstore_controller.go @@ -90,7 +90,7 @@ func (r *ObjectStoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) if err := r.reconcileRoleRules(ctx, role, objectStoreNames); err != nil { contextLogger.Error(err, "Failed to reconcile RBAC for role", - "roleName", role.Name) + "roleName", role.Name, "namespace", role.Namespace) errs = append(errs, fmt.Errorf("while reconciling role %s: %w", role.Name, err)) } } diff --git a/internal/controller/objectstore_controller_test.go b/internal/controller/objectstore_controller_test.go index 6779415..a8b896e 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 0000000..67d2151 --- /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 0000000..289f44c --- /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") +}