From c9f9c45c0c4a9694a1b76e75a72ed52c5d0ddc59 Mon Sep 17 00:00:00 2001 From: Artem Bortnikov Date: Thu, 25 Apr 2024 16:14:50 +0300 Subject: [PATCH 1/2] update codebase Signed-off-by: Artem Bortnikov --- internal/controller/etcdcluster_controller.go | 38 ++--- internal/controller/factory/builders.go | 130 ++++-------------- internal/controller/factory/configMap.go | 10 +- internal/controller/factory/configmap_test.go | 10 +- internal/controller/factory/pdb.go | 8 +- internal/controller/factory/pdb_test.go | 10 +- internal/controller/factory/statefulset.go | 7 +- .../controller/factory/statefulset_test.go | 12 +- internal/controller/factory/suite_test.go | 9 +- internal/controller/factory/svc.go | 12 +- internal/controller/factory/svc_test.go | 37 +++-- 11 files changed, 94 insertions(+), 189 deletions(-) diff --git a/internal/controller/etcdcluster_controller.go b/internal/controller/etcdcluster_controller.go index f9860315..d3834494 100644 --- a/internal/controller/etcdcluster_controller.go +++ b/internal/controller/etcdcluster_controller.go @@ -22,6 +22,7 @@ import ( "fmt" policyv1 "k8s.io/api/policy/v1" + "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -32,7 +33,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" etcdaenixiov1alpha1 "github.com/aenix-io/etcd-operator/api/v1alpha1" @@ -127,45 +127,49 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // ensureClusterObjects creates or updates all objects owned by cluster CR func (r *EtcdClusterReconciler) ensureClusterObjects( ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster) error { - if err := factory.CreateOrUpdateClusterStateConfigMap(ctx, cluster, r.Client, r.Scheme); err != nil { + if err := factory.CreateOrUpdateClusterStateConfigMap(ctx, cluster, r.Client); err != nil { return err } - if err := factory.CreateOrUpdateHeadlessService(ctx, cluster, r.Client, r.Scheme); err != nil { + if err := factory.CreateOrUpdateHeadlessService(ctx, cluster, r.Client); err != nil { return err } - if err := factory.CreateOrUpdateStatefulSet(ctx, cluster, r.Client, r.Scheme); err != nil { + if err := factory.CreateOrUpdateStatefulSet(ctx, cluster, r.Client); err != nil { return err } - if err := factory.CreateOrUpdateClientService(ctx, cluster, r.Client, r.Scheme); err != nil { + if err := factory.CreateOrUpdateClientService(ctx, cluster, r.Client); err != nil { return err } - if err := factory.CreateOrUpdatePdb(ctx, cluster, r.Client, r.Scheme); err != nil { + if err := factory.CreateOrUpdatePdb(ctx, cluster, r.Client); err != nil { return err } - return nil } // updateStatusOnErr wraps error and updates EtcdCluster status func (r *EtcdClusterReconciler) updateStatusOnErr(ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, err error) (ctrl.Result, error) { - res, statusErr := r.updateStatus(ctx, cluster) + // The function 'updateStatusOnErr' will always return non-nil error. Hence, the ctrl.Result will always be ignored. + // Therefore, the ctrl.Result returned by 'updateStatus' function can be discarded. + // REF: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/reconcile@v0.17.3#Reconciler + _, statusErr := r.updateStatus(ctx, cluster) if statusErr != nil { - return res, goerrors.Join(statusErr, err) + return ctrl.Result{}, goerrors.Join(statusErr, err) } - return res, err + return ctrl.Result{}, err } // updateStatus updates EtcdCluster status and returns error and requeue in case status could not be updated due to conflict func (r *EtcdClusterReconciler) updateStatus(ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster) (ctrl.Result, error) { logger := log.FromContext(ctx) - if err := r.Status().Update(ctx, cluster); err != nil { - logger.Error(err, "unable to update cluster status") - if errors.IsConflict(err) { - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, err + err := r.Status().Update(ctx, cluster) + if err == nil { + return ctrl.Result{}, nil + } + if errors.IsConflict(err) { + logger.V(2).Info("conflict during cluster status update") + return ctrl.Result{Requeue: true}, nil } - return ctrl.Result{}, nil + logger.Error(err, "cannot update cluster status") + return ctrl.Result{}, err } // isStatefulSetReady gets managed StatefulSet and checks its readiness. diff --git a/internal/controller/factory/builders.go b/internal/controller/factory/builders.go index af18e07d..a57d4f90 100644 --- a/internal/controller/factory/builders.go +++ b/internal/controller/factory/builders.go @@ -20,123 +20,43 @@ import ( "context" "fmt" - appsv1 "k8s.io/api/apps/v1" - corev1 "k8s.io/api/core/v1" - v1 "k8s.io/api/policy/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/log" ) -func reconcileStatefulSet(ctx context.Context, rclient client.Client, crdName string, sts *appsv1.StatefulSet) error { - logger := log.FromContext(ctx) - logger.V(2).Info("statefulset reconciliation started") - - currentSts := &appsv1.StatefulSet{} - logger.V(2).Info("statefulset found", "sts_name", currentSts.Name) - - err := rclient.Get(ctx, types.NamespacedName{Namespace: sts.Namespace, Name: sts.Name}, currentSts) +func reconcileOwnedResource(ctx context.Context, c client.Client, resource client.Object) error { + gvk, err := apiutil.GVKForObject(resource, c.Scheme()) if err != nil { - if errors.IsNotFound(err) { - logger.V(2).Info("creating new statefulset", "sts_name", sts.Name, "crd_object", crdName) - return rclient.Create(ctx, sts) - } - return fmt.Errorf("cannot get existing statefulset: %s, for crd_object: %s, err: %w", sts.Name, crdName, err) + return err } - sts.Annotations = labels.Merge(currentSts.Annotations, sts.Annotations) - logger.V(2).Info("statefulset annotations merged", "sts_annotations", sts.Annotations) - sts.Status = currentSts.Status - return rclient.Update(ctx, sts) -} - -func reconcileConfigMap(ctx context.Context, rclient client.Client, crdName string, configMap *corev1.ConfigMap) error { - logger := log.FromContext(ctx) - logger.V(2).Info("configmap reconciliation started") - - currentConfigMap := &corev1.ConfigMap{} - logger.V(2).Info("configmap found", "cm_name", currentConfigMap.Name) - - err := rclient.Get(ctx, types.NamespacedName{Namespace: configMap.Namespace, Name: configMap.Name}, currentConfigMap) - if err != nil { - if errors.IsNotFound(err) { - logger.V(2).Info("creating new configMap", "cm_name", configMap.Name, "crd_object", crdName) - return rclient.Create(ctx, configMap) - } - return fmt.Errorf("cannot get existing configMap: %s, for crd_object: %s, err: %w", configMap.Name, crdName, err) + logger := log.FromContext(ctx).WithValues("group", gvk.GroupVersion().String(), "kind", gvk.Kind, "name", resource.GetName()) + logger.V(2).Info("reconciling owned resource") + + base := resource.DeepCopyObject().(client.Object) + err = c.Get(ctx, client.ObjectKeyFromObject(resource), base) + if err == nil { + logger.V(2).Info("updating owned resource") + resource.SetAnnotations(labels.Merge(base.GetAnnotations(), resource.GetAnnotations())) + resource.SetResourceVersion(base.GetResourceVersion()) + logger.V(2).Info("owned resource annotations merged", "annotations", resource.GetAnnotations()) + return c.Update(ctx, resource) } - configMap.Annotations = labels.Merge(currentConfigMap.Annotations, configMap.Annotations) - logger.V(2).Info("configmap annotations merged", "cm_annotations", configMap.Annotations) - return rclient.Update(ctx, configMap) -} - -func reconcileService(ctx context.Context, rclient client.Client, crdName string, svc *corev1.Service) error { - logger := log.FromContext(ctx) - logger.V(2).Info("service reconciliation started") - - if svc == nil { - return fmt.Errorf("service is nil for crd_object: %s", crdName) + if errors.IsNotFound(err) { + logger.V(2).Info("creating new owned resource") + return c.Create(ctx, resource) } - - currentSvc := &corev1.Service{} - logger.V(2).Info("service found", "svc_name", currentSvc.Name) - - err := rclient.Get(ctx, types.NamespacedName{Namespace: svc.Namespace, Name: svc.Name}, currentSvc) - if err != nil { - if errors.IsNotFound(err) { - logger.V(2).Info("creating new service", "svc_name", svc.Name, "crd_object", crdName) - return rclient.Create(ctx, svc) - } - return fmt.Errorf("cannot get existing service: %s, for crd_object: %s, err: %w", svc.Name, crdName, err) - } - svc.Annotations = labels.Merge(currentSvc.Annotations, svc.Annotations) - logger.V(2).Info("service annotations merged", "svc_annotations", svc.Annotations) - svc.Status = currentSvc.Status - return rclient.Update(ctx, svc) -} - -// deleteManagedPdb deletes cluster PDB if it exists. -// pdb parameter should have at least metadata.name and metadata.namespace fields filled. -func deleteManagedPdb(ctx context.Context, rclient client.Client, pdb *v1.PodDisruptionBudget) error { - logger := log.FromContext(ctx) - - currentPdb := &v1.PodDisruptionBudget{} - logger.V(2).Info("pdb found", "pdb_name", currentPdb.Name) - - err := rclient.Get(ctx, types.NamespacedName{Namespace: pdb.Namespace, Name: pdb.Name}, currentPdb) - if err != nil { - logger.V(2).Info("error getting cluster PDB", "error", err) - return client.IgnoreNotFound(err) - } - err = rclient.Delete(ctx, currentPdb) - if err != nil { - logger.Error(err, "error deleting cluster PDB", "name", pdb.Name) - return client.IgnoreNotFound(err) - } - - return nil + return fmt.Errorf("error getting owned resource: %w", err) } -func reconcilePdb(ctx context.Context, rclient client.Client, crdName string, pdb *v1.PodDisruptionBudget) error { - logger := log.FromContext(ctx) - logger.V(2).Info("pdb reconciliation started") - - currentPdb := &v1.PodDisruptionBudget{} - logger.V(2).Info("pdb found", "pdb_name", currentPdb.Name) - - err := rclient.Get(ctx, types.NamespacedName{Namespace: pdb.Namespace, Name: pdb.Name}, currentPdb) +func deleteOwnedResource(ctx context.Context, c client.Client, resource client.Object) error { + gvk, err := apiutil.GVKForObject(resource, c.Scheme()) if err != nil { - if errors.IsNotFound(err) { - logger.V(2).Info("creating new PDB", "pdb_name", pdb.Name, "crd_object", crdName) - return rclient.Create(ctx, pdb) - } - logger.V(2).Info("error getting cluster PDB", "error", err) - return fmt.Errorf("cannot get existing pdb resource: %s for crd_object: %s, err: %w", pdb.Name, crdName, err) + return err } - pdb.Annotations = labels.Merge(currentPdb.Annotations, pdb.Annotations) - logger.V(2).Info("pdb annotations merged", "pdb_annotations", pdb.Annotations) - pdb.ResourceVersion = currentPdb.ResourceVersion - pdb.Status = currentPdb.Status - return rclient.Update(ctx, pdb) + logger := log.FromContext(ctx).WithValues("group", gvk.GroupVersion().String(), "kind", gvk.Kind, "name", resource.GetName()) + logger.V(2).Info("deleting owned resource") + return client.IgnoreNotFound(c.Delete(ctx, resource)) } diff --git a/internal/controller/factory/configMap.go b/internal/controller/factory/configMap.go index 2271f79d..5c7bbc38 100644 --- a/internal/controller/factory/configMap.go +++ b/internal/controller/factory/configMap.go @@ -20,15 +20,12 @@ import ( "context" "fmt" + etcdaenixiov1alpha1 "github.com/aenix-io/etcd-operator/api/v1alpha1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" - - etcdaenixiov1alpha1 "github.com/aenix-io/etcd-operator/api/v1alpha1" ) func GetClusterStateConfigMapName(cluster *etcdaenixiov1alpha1.EtcdCluster) string { @@ -39,7 +36,6 @@ func CreateOrUpdateClusterStateConfigMap( ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, rclient client.Client, - rscheme *runtime.Scheme, ) error { initialCluster := "" clusterService := fmt.Sprintf("%s.%s.svc:2380", GetHeadlessServiceName(cluster), cluster.Namespace) @@ -73,11 +69,11 @@ func CreateOrUpdateClusterStateConfigMap( } logger.V(2).Info("configmap spec generated", "cm_name", configMap.Name, "cm_spec", configMap.Data) - if err := ctrl.SetControllerReference(cluster, configMap, rscheme); err != nil { + if err := ctrl.SetControllerReference(cluster, configMap, rclient.Scheme()); err != nil { return fmt.Errorf("cannot set controller reference: %w", err) } - return reconcileConfigMap(ctx, rclient, cluster.Name, configMap) + return reconcileOwnedResource(ctx, rclient, configMap) } // isEtcdClusterReady returns true if condition "Ready" has progressed diff --git a/internal/controller/factory/configmap_test.go b/internal/controller/factory/configmap_test.go index 06d3dcde..2b83f994 100644 --- a/internal/controller/factory/configmap_test.go +++ b/internal/controller/factory/configmap_test.go @@ -19,7 +19,6 @@ package factory import ( "github.com/google/uuid" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" @@ -88,7 +87,7 @@ var _ = Describe("CreateOrUpdateClusterStateConfigMap handlers", func() { It("should successfully ensure the configmap", func(ctx SpecContext) { var configMapUID types.UID By("processing new etcd cluster", func() { - Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Get(&configMap)).Should(Succeed()) Expect(configMap.Data["ETCD_INITIAL_CLUSTER_STATE"]).To(Equal("new")) configMapUID = configMap.GetUID() @@ -99,7 +98,7 @@ var _ = Describe("CreateOrUpdateClusterStateConfigMap handlers", func() { WithReason(string(etcdaenixiov1alpha1.EtcdCondTypeStatefulSetReady)). WithStatus(true). Complete()) - Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(&configMap)).Should(HaveField("ObjectMeta.UID", Equal(configMapUID))) Expect(configMap.Data["ETCD_INITIAL_CLUSTER_STATE"]).To(Equal("existing")) }) @@ -109,15 +108,14 @@ var _ = Describe("CreateOrUpdateClusterStateConfigMap handlers", func() { WithReason(string(etcdaenixiov1alpha1.EtcdCondTypeWaitingForFirstQuorum)). WithStatus(true). Complete()) - Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(&configMap)).Should(HaveField("ObjectMeta.UID", Equal(configMapUID))) Expect(configMap.Data["ETCD_INITIAL_CLUSTER_STATE"]).To(Equal("new")) }) }) It("should fail to create the configmap with invalid owner reference", func(ctx SpecContext) { - emptyScheme := runtime.NewScheme() - Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, k8sClient, emptyScheme)).NotTo(Succeed()) + Expect(CreateOrUpdateClusterStateConfigMap(ctx, &etcdcluster, clientWithEmptyScheme)).NotTo(Succeed()) }) }) }) diff --git a/internal/controller/factory/pdb.go b/internal/controller/factory/pdb.go index b7d449f8..efadbb93 100644 --- a/internal/controller/factory/pdb.go +++ b/internal/controller/factory/pdb.go @@ -23,7 +23,6 @@ import ( etcdaenixiov1alpha1 "github.com/aenix-io/etcd-operator/api/v1alpha1" v1 "k8s.io/api/policy/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" @@ -35,10 +34,9 @@ func CreateOrUpdatePdb( ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, rclient client.Client, - rscheme *runtime.Scheme, ) error { if cluster.Spec.PodDisruptionBudgetTemplate == nil { - return deleteManagedPdb(ctx, rclient, &v1.PodDisruptionBudget{ + return deleteOwnedResource(ctx, rclient, &v1.PodDisruptionBudget{ ObjectMeta: metav1.ObjectMeta{ Namespace: cluster.Namespace, Name: cluster.Name, @@ -67,9 +65,9 @@ func CreateOrUpdatePdb( logger.V(2).Info("pdb spec generated", "pdb_name", pdb.Name, "pdb_spec", pdb.Spec) - if err := ctrl.SetControllerReference(cluster, pdb, rscheme); err != nil { + if err := ctrl.SetControllerReference(cluster, pdb, rclient.Scheme()); err != nil { return fmt.Errorf("cannot set controller reference: %w", err) } - return reconcilePdb(ctx, rclient, cluster.Name, pdb) + return reconcileOwnedResource(ctx, rclient, pdb) } diff --git a/internal/controller/factory/pdb_test.go b/internal/controller/factory/pdb_test.go index 32405d28..436a11cf 100644 --- a/internal/controller/factory/pdb_test.go +++ b/internal/controller/factory/pdb_test.go @@ -87,7 +87,7 @@ var _ = Describe("CreateOrUpdatePdb handlers", func() { It("should create PDB with pre-filled data", func(ctx SpecContext) { etcdcluster.Spec.PodDisruptionBudgetTemplate.Spec.MinAvailable = ptr.To(intstr.FromInt32(int32(3))) - Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Get(&podDisruptionBudget)).Should(Succeed()) Expect(podDisruptionBudget.Spec.MinAvailable).NotTo(BeNil()) Expect(podDisruptionBudget.Spec.MinAvailable.IntValue()).To(Equal(3)) @@ -95,7 +95,7 @@ var _ = Describe("CreateOrUpdatePdb handlers", func() { }) It("should create PDB with empty data", func(ctx SpecContext) { - Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Get(&podDisruptionBudget)).Should(Succeed()) Expect(etcdcluster.Spec.PodDisruptionBudgetTemplate.Spec.MinAvailable).To(BeNil()) Expect(podDisruptionBudget.Spec.MinAvailable.IntValue()).To(Equal(2)) @@ -104,14 +104,14 @@ var _ = Describe("CreateOrUpdatePdb handlers", func() { It("should skip deletion of PDB if not filled and not exist", func(ctx SpecContext) { etcdcluster.Spec.PodDisruptionBudgetTemplate = nil - Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).NotTo(HaveOccurred()) + Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient)).NotTo(HaveOccurred()) }) It("should delete created PDB after updating CR", func(ctx SpecContext) { - Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Get(&podDisruptionBudget)).Should(Succeed()) etcdcluster.Spec.PodDisruptionBudgetTemplate = nil - Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).NotTo(HaveOccurred()) + Expect(CreateOrUpdatePdb(ctx, &etcdcluster, k8sClient)).NotTo(HaveOccurred()) err = Get(&podDisruptionBudget)() Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) diff --git a/internal/controller/factory/statefulset.go b/internal/controller/factory/statefulset.go index 2660f5a4..c29bf5c9 100644 --- a/internal/controller/factory/statefulset.go +++ b/internal/controller/factory/statefulset.go @@ -23,11 +23,9 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" etcdaenixiov1alpha1 "github.com/aenix-io/etcd-operator/api/v1alpha1" @@ -42,7 +40,6 @@ func CreateOrUpdateStatefulSet( ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, rclient client.Client, - rscheme *runtime.Scheme, ) error { podMetadata := metav1.ObjectMeta{ Labels: NewLabelsBuilder().WithName().WithInstance(cluster.Name).WithManagedBy(), @@ -107,11 +104,11 @@ func CreateOrUpdateStatefulSet( logger := log.FromContext(ctx) logger.V(2).Info("statefulset spec generated", "sts_name", statefulSet.Name, "sts_spec", statefulSet.Spec) - if err := ctrl.SetControllerReference(cluster, statefulSet, rscheme); err != nil { + if err = ctrl.SetControllerReference(cluster, statefulSet, rclient.Scheme()); err != nil { return fmt.Errorf("cannot set controller reference: %w", err) } - return reconcileStatefulSet(ctx, rclient, cluster.Name, statefulSet) + return reconcileOwnedResource(ctx, rclient, statefulSet) } func generateVolumes(cluster *etcdaenixiov1alpha1.EtcdCluster) []corev1.Volume { diff --git a/internal/controller/factory/statefulset_test.go b/internal/controller/factory/statefulset_test.go index fe0008de..a3aaf3e3 100644 --- a/internal/controller/factory/statefulset_test.go +++ b/internal/controller/factory/statefulset_test.go @@ -20,7 +20,6 @@ import ( "github.com/google/uuid" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/ptr" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" @@ -89,7 +88,7 @@ var _ = Describe("CreateOrUpdateStatefulSet handler", func() { }) It("should successfully ensure the statefulSet with empty spec", func(ctx SpecContext) { - Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(&statefulSet)).Should( HaveField("Spec.Replicas", Equal(etcdcluster.Spec.Replicas)), ) @@ -153,7 +152,7 @@ var _ = Describe("CreateOrUpdateStatefulSet handler", func() { ClientSecret: "client-secret", }, } - Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Get(&statefulSet)).Should(Succeed()) By("Checking the resources", func() { @@ -301,7 +300,7 @@ var _ = Describe("CreateOrUpdateStatefulSet handler", func() { }, }, } - Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Get(&statefulSet)).Should(Succeed()) By("Checking the updated startup probe", func() { @@ -361,7 +360,7 @@ var _ = Describe("CreateOrUpdateStatefulSet handler", func() { SizeLimit: ptr.To(size), }, } - Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Get(&statefulSet)).Should(Succeed()) By("Checking the emptyDir", func() { @@ -370,8 +369,7 @@ var _ = Describe("CreateOrUpdateStatefulSet handler", func() { }) It("should fail on creating the statefulset with invalid owner reference", func(ctx SpecContext) { - emptyScheme := runtime.NewScheme() - Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, k8sClient, emptyScheme)).NotTo(Succeed()) + Expect(CreateOrUpdateStatefulSet(ctx, &etcdcluster, clientWithEmptyScheme)).NotTo(Succeed()) }) }) diff --git a/internal/controller/factory/suite_test.go b/internal/controller/factory/suite_test.go index 0f472a3d..7260b791 100644 --- a/internal/controller/factory/suite_test.go +++ b/internal/controller/factory/suite_test.go @@ -24,12 +24,12 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" - + kruntime "k8s.io/apimachinery/pkg/runtime" "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" + . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -42,7 +42,7 @@ import ( // http://onsi.github.io/ginkgo/ to learn more about Ginkgo. var cfg *rest.Config -var k8sClient client.Client +var k8sClient, clientWithEmptyScheme client.Client var testEnv *envtest.Environment func TestFactories(t *testing.T) { @@ -80,6 +80,9 @@ var _ = BeforeSuite(func() { // +kubebuilder:scaffold:scheme + clientWithEmptyScheme, err = client.New(cfg, client.Options{Scheme: kruntime.NewScheme()}) + Expect(err).NotTo(HaveOccurred()) + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) diff --git a/internal/controller/factory/svc.go b/internal/controller/factory/svc.go index 499c0780..9d857dd6 100644 --- a/internal/controller/factory/svc.go +++ b/internal/controller/factory/svc.go @@ -22,11 +22,9 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" etcdaenixiov1alpha1 "github.com/aenix-io/etcd-operator/api/v1alpha1" @@ -53,7 +51,6 @@ func CreateOrUpdateHeadlessService( ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, rclient client.Client, - rscheme *runtime.Scheme, ) error { logger := log.FromContext(ctx) var err error @@ -87,18 +84,17 @@ func CreateOrUpdateHeadlessService( logger.V(2).Info("cluster service spec generated", "svc_name", svc.Name, "svc_spec", svc.Spec) - if err := ctrl.SetControllerReference(cluster, svc, rscheme); err != nil { + if err := ctrl.SetControllerReference(cluster, svc, rclient.Scheme()); err != nil { return fmt.Errorf("cannot set controller reference: %w", err) } - return reconcileService(ctx, rclient, cluster.Name, svc) + return reconcileOwnedResource(ctx, rclient, svc) } func CreateOrUpdateClientService( ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, rclient client.Client, - rscheme *runtime.Scheme, ) error { logger := log.FromContext(ctx) var err error @@ -130,9 +126,9 @@ func CreateOrUpdateClientService( logger.V(2).Info("client service spec generated", "svc_name", svc.Name, "svc_spec", svc.Spec) - if err := ctrl.SetControllerReference(cluster, &svc, rscheme); err != nil { + if err := ctrl.SetControllerReference(cluster, &svc, rclient.Scheme()); err != nil { return fmt.Errorf("cannot set controller reference: %w", err) } - return reconcileService(ctx, rclient, cluster.Name, &svc) + return reconcileOwnedResource(ctx, rclient, &svc) } diff --git a/internal/controller/factory/svc_test.go b/internal/controller/factory/svc_test.go index 3f8bd498..0e27752c 100644 --- a/internal/controller/factory/svc_test.go +++ b/internal/controller/factory/svc_test.go @@ -19,7 +19,6 @@ package factory import ( "github.com/google/uuid" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" @@ -99,7 +98,7 @@ var _ = Describe("CreateOrUpdateService handlers", func() { }) It("should successfully ensure headless service", func(ctx SpecContext) { - Expect(CreateOrUpdateHeadlessService(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateHeadlessService(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(&headlessService)).Should(SatisfyAll( HaveField("Spec.Type", Equal(corev1.ServiceTypeClusterIP)), HaveField("Spec.ClusterIP", Equal(corev1.ClusterIPNone)), @@ -107,8 +106,7 @@ var _ = Describe("CreateOrUpdateService handlers", func() { }) It("should successfully ensure headless service with custom metadata", func(ctx SpecContext) { - cluster := etcdcluster.DeepCopy() - cluster.Spec.HeadlessServiceTemplate = &etcdaenixiov1alpha1.EmbeddedMetadataResource{ + etcdcluster.Spec.HeadlessServiceTemplate = &etcdaenixiov1alpha1.EmbeddedMetadataResource{ EmbeddedObjectMetadata: etcdaenixiov1alpha1.EmbeddedObjectMetadata{ Name: "headless-name", Labels: map[string]string{"label": "value"}, @@ -116,23 +114,23 @@ var _ = Describe("CreateOrUpdateService handlers", func() { }, } svc := headlessService.DeepCopy() - svc.Name = cluster.Spec.HeadlessServiceTemplate.Name + svc.Name = etcdcluster.Spec.HeadlessServiceTemplate.Name - Expect(CreateOrUpdateHeadlessService(ctx, cluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateHeadlessService(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(svc)).Should(SatisfyAll( - HaveField("ObjectMeta.Name", Equal(cluster.Spec.HeadlessServiceTemplate.Name)), + HaveField("ObjectMeta.Name", Equal(etcdcluster.Spec.HeadlessServiceTemplate.Name)), HaveField("ObjectMeta.Labels", SatisfyAll( HaveKeyWithValue("label", "value"), HaveKeyWithValue("app.kubernetes.io/name", "etcd"), )), - HaveField("ObjectMeta.Annotations", Equal(cluster.Spec.HeadlessServiceTemplate.Annotations)), + HaveField("ObjectMeta.Annotations", Equal(etcdcluster.Spec.HeadlessServiceTemplate.Annotations)), )) // We need to manually cleanup here because we changed the name of the service Expect(k8sClient.Delete(ctx, svc)).Should(Succeed()) }) It("should successfully ensure client service", func(ctx SpecContext) { - Expect(CreateOrUpdateClientService(ctx, &etcdcluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateClientService(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(&clientService)).Should(SatisfyAll( HaveField("Spec.Type", Equal(corev1.ServiceTypeClusterIP)), HaveField("Spec.ClusterIP", Not(Equal(corev1.ClusterIPNone))), @@ -140,8 +138,7 @@ var _ = Describe("CreateOrUpdateService handlers", func() { }) It("should successfully ensure client service with custom metadata", func(ctx SpecContext) { - cluster := etcdcluster.DeepCopy() - cluster.Spec.ServiceTemplate = &etcdaenixiov1alpha1.EmbeddedService{ + etcdcluster.Spec.ServiceTemplate = &etcdaenixiov1alpha1.EmbeddedService{ EmbeddedObjectMetadata: etcdaenixiov1alpha1.EmbeddedObjectMetadata{ Name: "client-name", Labels: map[string]string{"label": "value"}, @@ -149,24 +146,23 @@ var _ = Describe("CreateOrUpdateService handlers", func() { }, } svc := clientService.DeepCopy() - svc.Name = cluster.Spec.ServiceTemplate.Name + svc.Name = etcdcluster.Spec.ServiceTemplate.Name - Expect(CreateOrUpdateClientService(ctx, cluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateClientService(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(svc)).Should(SatisfyAll( - HaveField("ObjectMeta.Name", Equal(cluster.Spec.ServiceTemplate.Name)), + HaveField("ObjectMeta.Name", Equal(etcdcluster.Spec.ServiceTemplate.Name)), HaveField("ObjectMeta.Labels", SatisfyAll( HaveKeyWithValue("label", "value"), HaveKeyWithValue("app.kubernetes.io/name", "etcd"), )), - HaveField("ObjectMeta.Annotations", Equal(cluster.Spec.ServiceTemplate.Annotations)), + HaveField("ObjectMeta.Annotations", Equal(etcdcluster.Spec.ServiceTemplate.Annotations)), )) // We need to manually cleanup here because we changed the name of the service Expect(k8sClient.Delete(ctx, svc)).Should(Succeed()) }) It("should successfully ensure client service with custom spec", func(ctx SpecContext) { - cluster := etcdcluster.DeepCopy() - cluster.Spec.ServiceTemplate = &etcdaenixiov1alpha1.EmbeddedService{ + etcdcluster.Spec.ServiceTemplate = &etcdaenixiov1alpha1.EmbeddedService{ Spec: corev1.ServiceSpec{ Type: corev1.ServiceTypeLoadBalancer, Ports: []corev1.ServicePort{ @@ -180,7 +176,7 @@ var _ = Describe("CreateOrUpdateService handlers", func() { }, } - Expect(CreateOrUpdateClientService(ctx, cluster, k8sClient, k8sClient.Scheme())).To(Succeed()) + Expect(CreateOrUpdateClientService(ctx, &etcdcluster, k8sClient)).To(Succeed()) Eventually(Object(&clientService)).Should(SatisfyAll( HaveField("Spec.Type", Equal(corev1.ServiceTypeLoadBalancer)), HaveField("Spec.LoadBalancerClass", Equal(ptr.To("someClass"))), @@ -196,9 +192,8 @@ var _ = Describe("CreateOrUpdateService handlers", func() { }) It("should fail on creating the client service with invalid owner reference", func(ctx SpecContext) { - emptyScheme := runtime.NewScheme() - Expect(CreateOrUpdateHeadlessService(ctx, &etcdcluster, k8sClient, emptyScheme)).NotTo(Succeed()) - Expect(CreateOrUpdateClientService(ctx, &etcdcluster, k8sClient, emptyScheme)).NotTo(Succeed()) + Expect(CreateOrUpdateHeadlessService(ctx, &etcdcluster, clientWithEmptyScheme)).NotTo(Succeed()) + Expect(CreateOrUpdateClientService(ctx, &etcdcluster, clientWithEmptyScheme)).NotTo(Succeed()) }) }) }) From 6cf51c5280682f2406dac9d8d2b5cf332f69f591 Mon Sep 17 00:00:00 2001 From: Artem Bortnikov Date: Thu, 25 Apr 2024 22:47:03 +0300 Subject: [PATCH 2/2] fix nilaway lint Signed-off-by: Artem Bortnikov --- internal/controller/factory/builders.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/controller/factory/builders.go b/internal/controller/factory/builders.go index a57d4f90..da1cc719 100644 --- a/internal/controller/factory/builders.go +++ b/internal/controller/factory/builders.go @@ -28,9 +28,12 @@ import ( ) func reconcileOwnedResource(ctx context.Context, c client.Client, resource client.Object) error { + if resource == nil { + return fmt.Errorf("resource cannot be nil") + } gvk, err := apiutil.GVKForObject(resource, c.Scheme()) if err != nil { - return err + return fmt.Errorf("failed to get GVK: %w", err) } logger := log.FromContext(ctx).WithValues("group", gvk.GroupVersion().String(), "kind", gvk.Kind, "name", resource.GetName()) logger.V(2).Info("reconciling owned resource")