Refactor NFSPVC Logic

With this pr, several files where seperated and refactored in order to improve readability and general code quality
Also removed usage of deprecated kustomize functionality

api/v1alpha1/nfspvc_webhook.go

@@ -34,9 +34,8 @@ import (
 var c client.Client
 
 const (
-	UpdateNfsPvcError      = "forbidden: NFSPVC spec is immutable after creation"
-	PVCAlreadyExists       = "a PVC of this name already exists in the namespace. Please rename your NFSPVC"
-	InvalidAccessModeError = "forbidden: only the following AccessModes are permitted"
+	pvcAlreadyExists       = "a PVC of this name already exists in the namespace. Please rename your NFSPVC"
+	invalidAccessModeError = "forbidden: only the following AccessModes are permitted"
 )
 
 var supportedAccessModes = sets.New(
@@ -67,11 +66,11 @@ func (r *NfsPvc) ValidateCreate() (admission.Warnings, error) {
 	nfspvclog.Info("validate create", "name", r.Name)
 
 	if r.doesPVCExist(c) {
-		return admission.Warnings{PVCAlreadyExists}, fmt.Errorf(PVCAlreadyExists)
+		return admission.Warnings{pvcAlreadyExists}, fmt.Errorf(pvcAlreadyExists)
 	}
 
 	if !r.validateAccessMode(r.Spec.AccessModes) {
-		return admission.Warnings{InvalidAccessModeError}, fmt.Errorf(InvalidAccessModeError+": %v", supportedAccessModes)
+		return admission.Warnings{invalidAccessModeError}, fmt.Errorf(invalidAccessModeError+": %v", supportedAccessModes)
 	}
 
 	return nil, nil

cmd/main.go

@@ -35,7 +35,7 @@ import (
 
 	nfspvcv1alpha1 "github.com/dana-team/nfspvc-operator/api/v1alpha1"
 	"github.com/dana-team/nfspvc-operator/internal/controller"
-	utils "github.com/dana-team/nfspvc-operator/internal/controller/utils"
+	"github.com/dana-team/nfspvc-operator/internal/controller/utils"
 	//+kubebuilder:scaffold:imports
 )
 

config/default/kustomization.yaml

@@ -14,53 +14,36 @@ namePrefix: nfspvc-operator-
 #  pairs:
 #    someName: someValue
 
+# [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
+# crd/kustomization.yaml
+# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'. 'WEBHOOK' components are required.
 resources:
 - ../crd
 - ../rbac
 - ../manager
-# [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
-# crd/kustomization.yaml
 - ../webhook
-# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'. 'WEBHOOK' components are required.
 - ../certmanager
 # [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.
 #- ../prometheus
 
-patchesStrategicMerge:
 # Protect the /metrics endpoint by putting it behind auth.
 # If you want your controller-manager to expose the /metrics
 # endpoint w/o any authn/z, please comment the following line.
-- manager_auth_proxy_patch.yaml
 
 
 
 # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
 # crd/kustomization.yaml
-- manager_webhook_patch.yaml
-
+patches:
+  - path: manager_auth_proxy_patch.yaml
+  - path: manager_webhook_patch.yaml
+  - path: webhookcainjection_patch.yaml
 # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'.
 # Uncomment 'CERTMANAGER' sections in crd/kustomization.yaml to enable the CA injection in the admission webhooks.
 # 'CERTMANAGER' needs to be enabled to use ca injection
-- webhookcainjection_patch.yaml
 
 # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER' prefix.
 # Uncomment the following replacements to add the cert-manager CA injection annotations
-replacements:
- - source: # Add cert-manager annotation to ValidatingWebhookConfiguration, MutatingWebhookConfiguration and CRDs
-     kind: Certificate
-     group: cert-manager.io
-     version: v1
-     name: serving-cert # this name should match the one in certificate.yaml
-     fieldPath: .metadata.namespace # namespace of the certificate CR
-   targets:
-     - select:
-         kind: ValidatingWebhookConfiguration
-       fieldPaths:
-         - .metadata.annotations.[cert-manager.io/inject-ca-from]
-       options:
-         delimiter: '/'
-         index: 0
-         create: true
      #- select:
      #    kind: MutatingWebhookConfiguration
      #  fieldPaths:
@@ -69,29 +52,6 @@ replacements:
      #    delimiter: '/'
      #    index: 0
      #    create: true
-     - select:
-         kind: CustomResourceDefinition
-       fieldPaths:
-         - .metadata.annotations.[cert-manager.io/inject-ca-from]
-       options:
-         delimiter: '/'
-         index: 0
-         create: true
- - source:
-     kind: Certificate
-     group: cert-manager.io
-     version: v1
-     name: serving-cert # this name should match the one in certificate.yaml
-     fieldPath: .metadata.name
-   targets:
-     - select:
-         kind: ValidatingWebhookConfiguration
-       fieldPaths:
-         - .metadata.annotations.[cert-manager.io/inject-ca-from]
-       options:
-         delimiter: '/'
-         index: 1
-         create: true
      #- select:
      #    kind: MutatingWebhookConfiguration
      #  fieldPaths:
@@ -100,45 +60,84 @@ replacements:
      #    delimiter: '/'
      #    index: 1
      #    create: true
-     - select:
-         kind: CustomResourceDefinition
-       fieldPaths:
-         - .metadata.annotations.[cert-manager.io/inject-ca-from]
-       options:
-         delimiter: '/'
-         index: 1
-         create: true
- - source: # Add cert-manager annotation to the webhook Service
-     kind: Service
-     version: v1
-     name: webhook-service
-     fieldPath: .metadata.name # namespace of the service
-   targets:
-     - select:
-         kind: Certificate
-         group: cert-manager.io
-         version: v1
-       fieldPaths:
-         - .spec.dnsNames.0
-         - .spec.dnsNames.1
-       options:
-         delimiter: '.'
-         index: 0
-         create: true
- - source:
-     kind: Service
-     version: v1
-     name: webhook-service
-     fieldPath: .metadata.namespace # namespace of the service
-   targets:
-     - select:
-         kind: Certificate
-         group: cert-manager.io
-         version: v1
-       fieldPaths:
-         - .spec.dnsNames.0
-         - .spec.dnsNames.1
-       options:
-         delimiter: '.'
-         index: 1
-         create: true
+replacements:
+- source:
+    fieldPath: .metadata.namespace
+    group: cert-manager.io
+    kind: Certificate
+    name: serving-cert
+    version: v1
+  targets:
+  - fieldPaths:
+    - .metadata.annotations.[cert-manager.io/inject-ca-from]
+    options:
+      create: true
+      delimiter: /
+    select:
+      kind: ValidatingWebhookConfiguration
+  - fieldPaths:
+    - .metadata.annotations.[cert-manager.io/inject-ca-from]
+    options:
+      create: true
+      delimiter: /
+    select:
+      kind: CustomResourceDefinition
+- source:
+    fieldPath: .metadata.name
+    group: cert-manager.io
+    kind: Certificate
+    name: serving-cert
+    version: v1
+  targets:
+  - fieldPaths:
+    - .metadata.annotations.[cert-manager.io/inject-ca-from]
+    options:
+      create: true
+      delimiter: /
+      index: 1
+    select:
+      kind: ValidatingWebhookConfiguration
+  - fieldPaths:
+    - .metadata.annotations.[cert-manager.io/inject-ca-from]
+    options:
+      create: true
+      delimiter: /
+      index: 1
+    select:
+      kind: CustomResourceDefinition
+- source:
+    fieldPath: .metadata.name
+    kind: Service
+    name: webhook-service
+    version: v1
+  targets:
+  - fieldPaths:
+    - .spec.dnsNames.0
+    - .spec.dnsNames.1
+    options:
+      create: true
+      delimiter: .
+    select:
+      group: cert-manager.io
+      kind: Certificate
+      version: v1
+- source:
+    fieldPath: .metadata.namespace
+    kind: Service
+    name: webhook-service
+    version: v1
+  targets:
+  - fieldPaths:
+    - .spec.dnsNames.0
+    - .spec.dnsNames.1
+    options:
+      create: true
+      delimiter: .
+      index: 1
+    select:
+      group: cert-manager.io
+      kind: Certificate
+      version: v1
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+

config/manager/kustomization.yaml

@@ -5,4 +5,4 @@ kind: Kustomization
 images:
 - name: controller
   newName: controller
-  newTag: latest
+  newTag: latest

internal/controller/finalizer/finalizer.go

@@ -0,0 +1,31 @@
+package finalizer
+
+import (
+	"context"
+
+	"github.com/dana-team/nfspvc-operator/internal/controller/utils"
+
+	danaiov1alpha1 "github.com/dana-team/nfspvc-operator/api/v1alpha1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+// Remove removes a finalizer from the nfspvc object.
+func Remove(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client) error {
+	controllerutil.RemoveFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer)
+	if err := k8sClient.Update(ctx, &nfspvc); err != nil {
+		return err
+	}
+	return nil
+}
+
+// Ensure adds a finalizer to the nfspvc object if one does not exist.
+func Ensure(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client) error {
+	if !controllerutil.ContainsFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer) {
+		controllerutil.AddFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer)
+		if err := k8sClient.Update(ctx, &nfspvc); err != nil {
+			return err
+		}
+	}
+	return nil
+}

internal/controller/nfspvc_controller.go

@@ -18,16 +18,17 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
 	danaiov1alpha1 "github.com/dana-team/nfspvc-operator/api/v1alpha1"
-	finalizerutils "github.com/dana-team/nfspvc-operator/internal/controller/utils/finalizer"
-	statusutils "github.com/dana-team/nfspvc-operator/internal/controller/utils/status"
-	syncutils "github.com/dana-team/nfspvc-operator/internal/controller/utils/sync"
+	"github.com/dana-team/nfspvc-operator/internal/controller/finalizer"
+	"github.com/dana-team/nfspvc-operator/internal/controller/resources"
+	"github.com/dana-team/nfspvc-operator/internal/controller/status"
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -68,34 +69,35 @@ func (r *NfsPvcReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func (r *NfsPvcReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	logger := log.FromContext(ctx).WithValues("NfsPvc", req.Name, "NfsPvcNamespace", req.Namespace)
 	logger.Info("Starting Reconcile")
-
 	nfspvc := danaiov1alpha1.NfsPvc{}
 	if err := r.Client.Get(ctx, req.NamespacedName, &nfspvc); err != nil {
-		if errors.IsNotFound(err) {
-			logger.Info(fmt.Sprintf("Didn't find NfsPvc: %s, from the namespace: %s", nfspvc.Name, nfspvc.Namespace))
+		if apierrors.IsNotFound(err) {
+			logger.Info("Didn't find NfsPvc")
 			return ctrl.Result{}, nil
 		}
 		return ctrl.Result{}, fmt.Errorf("failed to get NfsPvc: %s", err.Error())
 	}
-
-	err, deleted := finalizerutils.HandleResourceDeletion(ctx, nfspvc, logger, r.Client)
-	if err != nil {
-		if finalizerutils.IsFailedCleanUp(err) {
-			// this means the error is of type *FailedCleanUpError.
-			logger.Info(fmt.Sprintf("failed to handle NfsPvc deletion: %s, so trying again in a few seconds", err.Error()))
-			return ctrl.Result{RequeueAfter: time.Second * RequeueIntervalSeconds}, nil
+	if nfspvc.ObjectMeta.DeletionTimestamp != nil {
+		deleted, err := resources.HandleDelete(ctx, nfspvc, r.Client)
+		if err != nil {
+			if errors.Is(err, resources.FailedCleanupError) {
+				logger.Info(fmt.Sprintf("failed to handle NfsPvc deletion: %s, so trying again in a few seconds", err.Error()))
+				return ctrl.Result{RequeueAfter: time.Second * RequeueIntervalSeconds}, nil
+			}
+			return ctrl.Result{}, fmt.Errorf("failed to handle NfsPvc deletion: %s", err.Error())
+		}
+		if deleted {
+			if err := finalizer.Remove(ctx, nfspvc, r.Client); err != nil {
+				return ctrl.Result{}, err
+			}
+			return ctrl.Result{}, nil
 		}
-		return ctrl.Result{}, fmt.Errorf("failed to handle NfsPvc deletion: %s", err.Error())
-	}
-	if deleted {
-		return ctrl.Result{}, nil
 	}
-	if err := finalizerutils.EnsureFinalizer(ctx, nfspvc, r.Client, logger); err != nil {
+
+	if err := finalizer.Ensure(ctx, nfspvc, r.Client); err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to ensure finalizer in NfsPvc: %s", err.Error())
 	}
-
-	// now sync the objects to the nfspvc object.
-	if err := SyncNfsPvc(ctx, nfspvc, logger, r.Client); err != nil {
+	if err := r.Update(ctx, nfspvc); err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to sync NfsPvc: %s", err.Error())
 	}
 
@@ -132,16 +134,15 @@ func (r *NfsPvcReconciler) enqueueRequestsFromPersistentVolumeClaim(ctx context.
 	return requests
 }
 
-func SyncNfsPvc(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, log logr.Logger, k8sClient client.Client) error {
+// Update handles any update to an NFSPVC.
+func (r *NfsPvcReconciler) Update(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc) error {
 	if nfspvc.ObjectMeta.DeletionTimestamp == nil {
-		if err := syncutils.CreateOrUpdateStorageObjects(ctx, nfspvc, log, k8sClient); err != nil {
+		if err := resources.HandleStorageObjectState(ctx, nfspvc, r.Client); err != nil {
 			return err
 		}
 	}
-
-	if err := statusutils.SyncNfsPvcStatus(ctx, nfspvc, log, k8sClient); err != nil {
+	if err := status.Update(ctx, nfspvc, r.Client); err != nil {
 		return err
 	}
-
 	return nil
 }