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