Refactor operation and requirement handling (#23)

* Refactor operation and requirement handling

- Removed the `ClearConditions` function and its associated test from the `operation` package.
- Introduced `OperationHelper` struct with methods for operation management, including condition clearing and ID generation.
- Added tests for `OperationHelper` methods, ensuring functionality for diffing app deployments and comparing job specifications.
- Deleted unused constants and introduced `RequirementHelper` for managing requirement conditions.
- Implemented tests for `RequirementHelper`, covering condition updates and cache miss checks.
- Added utility functions for generating random strings.
- Updated integration tests to remove dependencies on removed constants and ensure proper phase checks.
- Generated mock implementations for client interfaces to facilitate testing.

* Add tests for application teardown and provision cache fields

- Implemented `TestAppCacheFieldFromApplicationTeardown` to validate the extraction of cache fields from application teardown specifications.
- Implemented `TestAppCacheFieldFromApplicationProvision` to validate the extraction of cache fields from application provision specifications.
- Enhanced `CompareProvisionJobs` to utilize a helper function for comparing dependencies, ensuring consistent behavior.
- Added comprehensive test cases for comparing provision jobs, including scenarios with identical jobs, different dependencies, and empty dependencies.
- Introduced utility functions for comparing string slices and environment variables to streamline comparison logic.
- Updated existing tests to reflect changes in the comparison logic and ensure thorough coverage of edge cases.

Author: ninjadq

api/v1alpha1/appdeployment_types.go

@@ -21,6 +21,20 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const (
+	AppDeploymentOwnerKey = ".appDeployment.metadata.controller"
+
+	AppDeploymentFinalizerName = "finalizer.appdeployment.devinfra.goms.io"
+
+	// phase types
+	AppDeploymentPhaseEmpty     = ""
+	AppDeploymentPhasePending   = "Pending"
+	AppDeploymentPhaseDeploying = "Deploying"
+	AppDeploymentPhaseReady     = "Ready"
+	AppDeploymentPhaseDeleting  = "Deleting"
+	AppDeploymentPhaseDeleted   = "Deleted"
+)
+
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
 

api/v1alpha1/cache_types.go

@@ -20,6 +20,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const (
+	CacheOwnerKey = ".metadata.controller.cache"
+)
+
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
 

api/v1alpha1/operation_types.go

@@ -21,6 +21,19 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const (
+	OperationOwnerKey = ".operation.metadata.controller"
+
+	OperationFinalizerName         = "finalizer.operation.controller.azure.com"
+	OperationAcquiredAnnotationKey = "operation.controller.azure.com/acquired"
+
+	OperationPhaseEmpty       = ""
+	OperationPhaseReconciling = "Reconciling"
+	OperationPhaseReconciled  = "Reconciled"
+	OperationPhaseDeleting    = "Deleting"
+	OperationPhaseDeleted     = "Deleted"
+)
+
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
 

api/v1alpha1/requirement_types.go

@@ -20,6 +20,30 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const (
+	RequirementOwnerKey = ".requirement.metadata.controller"
+
+	RequirementFinalizerName = "finalizer.requirement.devinfra.goms.io"
+
+	RequirementConditionRequirementInitialized  = "RequirementInitialized"
+	RequirementConditionCacheResourceFound      = "CacheCRFound"
+	RequirementConditionCachedOperationAcquired = "CachedOpAcquired"
+	RequirementConditionOperationReady          = "OperationReady"
+
+	RequirementConditionReasonNoOperationAvailable = "NoOperationAvailable"
+	RequirementConditionReasonCacheCRNotFound      = "CacheCRNotFound"
+	RequirementConditionReasonCacheCRFound         = "CacheCRFound"
+	RequirementConditionReasonCacheHit             = "CacheHit"
+	RequirementConditionReasonCacheMiss            = "CacheMiss"
+
+	RequirementPhaseEmpty         = ""
+	RequirementPhaseCacheChecking = "CacheChecking"
+	RequirementPhaseOperating     = "Operating"
+	RequirementPhaseReady         = "Ready"
+	RequirementPhaseDeleted       = "Deleted"
+	RequirementPhaseDeleting      = "Deleting"
+)
+
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
 

internal/controller/appdeployment_controller.go

@@ -26,10 +26,11 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/log"
+	klog "sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/Azure/operation-cache-controller/api/v1alpha1"
-	apdutil "github.com/Azure/operation-cache-controller/internal/utils/controller/appdeployment"
+	"github.com/Azure/operation-cache-controller/internal/handler"
+	"github.com/Azure/operation-cache-controller/internal/log"
 	"github.com/Azure/operation-cache-controller/internal/utils/reconciler"
 )
 
@@ -57,23 +58,22 @@ type AppDeploymentReconciler struct {
 // For more details, check Reconcile and its Result here:
 // - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
 func (r *AppDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := log.FromContext(ctx).WithValues(apdutil.LogKeyAppDeploymentName, req.NamespacedName)
+	logger := klog.FromContext(ctx).WithValues(log.AppDeploymentJobName, req.NamespacedName)
 	appdeployment := &v1alpha1.AppDeployment{}
 	if err := r.Get(ctx, req.NamespacedName, appdeployment); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	adapter := NewAppDeploymentAdapter(ctx, appdeployment, logger, r.Client, r.recorder)
-	return r.ReconcileHandler(ctx, adapter)
+	return r.ReconcileHandler(ctx, handler.NewAppDeploymentHandler(ctx, appdeployment, logger, r.Client, r.recorder))
 }
-func (r *AppDeploymentReconciler) ReconcileHandler(ctx context.Context, adapter AppDeploymentAdapterInterface) (ctrl.Result, error) {
+func (r *AppDeploymentReconciler) ReconcileHandler(ctx context.Context, h handler.AppDeploymentHandlerInterface) (ctrl.Result, error) {
 	operations := []reconciler.ReconcileOperation{
-		adapter.EnsureApplicationValid,
-		adapter.EnsureFinalizer,
-		adapter.EnsureFinalizerDeleted,
-		adapter.EnsureDependenciesReady,
-		adapter.EnsureDeployingFinished,
-		adapter.EnsureTeardownFinished,
+		h.EnsureApplicationValid,
+		h.EnsureFinalizer,
+		h.EnsureFinalizerDeleted,
+		h.EnsureDependenciesReady,
+		h.EnsureDeployingFinished,
+		h.EnsureTeardownFinished,
 	}
 
 	for _, operation := range operations {
@@ -88,22 +88,21 @@ func (r *AppDeploymentReconciler) ReconcileHandler(ctx context.Context, adapter
 	return ctrl.Result{}, nil
 }
 
-var appDeploymentOwnerKey = ".appDeployment.metadata.controller"
+func appDeploymentIndexerFunc(rawObj client.Object) []string {
+	job := rawObj.(*batchv1.Job)
+	owner := metav1.GetControllerOf(job)
+	if owner == nil {
+		return nil
+	}
+	if owner.APIVersion != v1alpha1.GroupVersion.String() || owner.Kind != "AppDeployment" {
+		return nil
+	}
+	return []string{owner.Name}
+}
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *AppDeploymentReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &batchv1.Job{}, appDeploymentOwnerKey,
-		func(rawObj client.Object) []string {
-			job := rawObj.(*batchv1.Job)
-			owner := metav1.GetControllerOf(job)
-			if owner == nil {
-				return nil
-			}
-			if owner.APIVersion != v1alpha1.GroupVersion.String() || owner.Kind != "AppDeployment" {
-				return nil
-			}
-			return []string{owner.Name}
-		}); err != nil {
+	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &batchv1.Job{}, v1alpha1.AppDeploymentOwnerKey, appDeploymentIndexerFunc); err != nil {
 		return err
 	}
 

internal/controller/appdeployment_controller_test.go

@@ -33,8 +33,9 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/Azure/operation-cache-controller/api/v1alpha1"
-	ctrlmocks "github.com/Azure/operation-cache-controller/internal/controller/mocks"
-	mockpkg "github.com/Azure/operation-cache-controller/internal/mocks"
+	"github.com/Azure/operation-cache-controller/internal/handler"
+	hmocks "github.com/Azure/operation-cache-controller/internal/handler/mocks"
+	utilsmock "github.com/Azure/operation-cache-controller/internal/utils/mocks"
 	"github.com/Azure/operation-cache-controller/internal/utils/reconciler"
 )
 
@@ -87,9 +88,9 @@ var _ = Describe("AppDeployment Controller", func() {
 		const resourceName = "test-resource"
 		var (
 			mockRecorderCtrl *gomock.Controller
-			mockRecorder     *mockpkg.MockEventRecorder
+			mockRecorder     *utilsmock.MockEventRecorder
 			mockAdapterCtrl  *gomock.Controller
-			mockAdapter      *ctrlmocks.MockAppDeploymentAdapterInterface
+			mockAdapter      *hmocks.MockAppDeploymentHandlerInterface
 		)
 		ctx := context.Background()
 
@@ -121,9 +122,9 @@ var _ = Describe("AppDeployment Controller", func() {
 				Expect(k8sClient.Create(ctx, resource)).To(Succeed())
 			}
 			mockRecorderCtrl = gomock.NewController(GinkgoT())
-			mockRecorder = mockpkg.NewMockEventRecorder(mockRecorderCtrl)
+			mockRecorder = utilsmock.NewMockEventRecorder(mockRecorderCtrl)
 			mockAdapterCtrl = gomock.NewController(GinkgoT())
-			mockAdapter = ctrlmocks.NewMockAppDeploymentAdapterInterface(mockAdapterCtrl)
+			mockAdapter = hmocks.NewMockAppDeploymentHandlerInterface(mockAdapterCtrl)
 		})
 
 		AfterEach(func() {
@@ -142,7 +143,7 @@ var _ = Describe("AppDeployment Controller", func() {
 				Scheme:   k8sClient.Scheme(),
 				recorder: mockRecorder,
 			}
-			ctx = context.WithValue(ctx, appdeploymentAdapterContextKey{}, mockAdapter)
+			ctx = context.WithValue(ctx, handler.AppdeploymentHandlerContextKey{}, mockAdapter)
 
 			mockAdapter.EXPECT().EnsureApplicationValid(gomock.Any()).Return(reconciler.OperationResult{}, nil)
 			mockAdapter.EXPECT().EnsureFinalizer(gomock.Any()).Return(reconciler.OperationResult{}, nil)
@@ -165,7 +166,7 @@ var _ = Describe("AppDeployment Controller", func() {
 				Scheme:   k8sClient.Scheme(),
 				recorder: mockRecorder,
 			}
-			ctx = context.WithValue(ctx, appdeploymentAdapterContextKey{}, mockAdapter)
+			ctx = context.WithValue(ctx, handler.AppdeploymentHandlerContextKey{}, mockAdapter)
 
 			mockAdapter.EXPECT().EnsureApplicationValid(gomock.Any()).Return(reconciler.OperationResult{
 				CancelRequest: true,
@@ -184,7 +185,7 @@ var _ = Describe("AppDeployment Controller", func() {
 				Scheme:   k8sClient.Scheme(),
 				recorder: mockRecorder,
 			}
-			ctx = context.WithValue(ctx, appdeploymentAdapterContextKey{}, mockAdapter)
+			ctx = context.WithValue(ctx, handler.AppdeploymentHandlerContextKey{}, mockAdapter)
 
 			mockAdapter.EXPECT().EnsureApplicationValid(gomock.Any()).Return(reconciler.OperationResult{}, errors.NewServiceUnavailable("test error"))
 
@@ -194,4 +195,82 @@ var _ = Describe("AppDeployment Controller", func() {
 			Expect(errors.IsServiceUnavailable(err)).To(BeTrue(), "expected error is ServiceUnavailable")
 		})
 	})
+
+	Context("appDeploymentIndexerFunc tests", func() {
+		It("should return nil for Job without owner", func() {
+			job := &batchv1.Job{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "job-without-owner",
+					Namespace: "default",
+				},
+			}
+
+			result := appDeploymentIndexerFunc(job)
+			Expect(result).To(BeNil())
+		})
+
+		It("should return nil for Job with non-AppDeployment owner", func() {
+			job := &batchv1.Job{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "job-with-wrong-owner",
+					Namespace: "default",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "v1",
+							Kind:       "Pod",
+							Name:       "owner-pod",
+							UID:        "12345",
+							Controller: &[]bool{true}[0],
+						},
+					},
+				},
+			}
+
+			result := appDeploymentIndexerFunc(job)
+			Expect(result).To(BeNil())
+		})
+
+		It("should return owner name for Job with AppDeployment owner", func() {
+			ownerName := "test-appdeployment"
+			job := &batchv1.Job{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "job-with-appdeployment-owner",
+					Namespace: "default",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: v1alpha1.GroupVersion.String(),
+							Kind:       "AppDeployment",
+							Name:       ownerName,
+							UID:        "67890",
+							Controller: &[]bool{true}[0],
+						},
+					},
+				},
+			}
+
+			result := appDeploymentIndexerFunc(job)
+			Expect(result).To(Equal([]string{ownerName}))
+		})
+
+		It("should return nil for Job with AppDeployment owner reference that is not controller", func() {
+			job := &batchv1.Job{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "job-with-non-controller-appdeployment",
+					Namespace: "default",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: v1alpha1.GroupVersion.String(),
+							Kind:       "AppDeployment",
+							Name:       "test-appdeployment",
+							UID:        "13579",
+							Controller: nil, // Not a controller reference
+						},
+					},
+				},
+			}
+
+			result := appDeploymentIndexerFunc(job)
+			Expect(result).To(BeNil())
+		})
+	})
 })

internal/controller/cache_controller.go

@@ -29,6 +29,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/Azure/operation-cache-controller/api/v1alpha1"
+	"github.com/Azure/operation-cache-controller/internal/handler"
 	"github.com/Azure/operation-cache-controller/internal/utils/reconciler"
 )
 
@@ -65,18 +66,17 @@ func (r *CacheReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	adapter := NewCacheAdapter(ctx, cache, logger, r.Client, r.Scheme, r.recorder, ctrl.SetControllerReference)
-	return r.reconcileHandler(ctx, adapter)
+	return r.reconcileHandler(ctx, handler.NewCacheHandler(ctx, cache, logger, r.Client, r.Scheme, r.recorder, ctrl.SetControllerReference))
 }
 
-func (r *CacheReconciler) reconcileHandler(ctx context.Context, adapter CacheAdapterInterface) (ctrl.Result, error) {
+func (r *CacheReconciler) reconcileHandler(ctx context.Context, h handler.CacheHandlerInterface) (ctrl.Result, error) {
 	logger := log.FromContext(ctx)
 
 	operations := []reconciler.ReconcileOperation{
-		adapter.CheckCacheExpiry,
-		adapter.EnsureCacheInitialized,
-		adapter.CalculateKeepAliveCount,
-		adapter.AdjustCache,
+		h.CheckCacheExpiry,
+		h.EnsureCacheInitialized,
+		h.CalculateKeepAliveCount,
+		h.AdjustCache,
 	}
 
 	for _, operation := range operations {
@@ -94,8 +94,6 @@ func (r *CacheReconciler) reconcileHandler(ctx context.Context, adapter CacheAda
 	return ctrl.Result{RequeueAfter: defaultCacheCheckInterval}, nil
 }
 
-var cacheOwnerKey = ".metadata.controller.cache"
-
 func cacheOperationIndexerFunc(obj client.Object) []string {
 	// grab the operation object, extract the owner...
 	operation := obj.(*v1alpha1.Operation)
@@ -113,7 +111,7 @@ func cacheOperationIndexerFunc(obj client.Object) []string {
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *CacheReconciler) SetupWithManager(mgr ctrl.Manager) error { // +gocover:ignore:block init controller
-	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &v1alpha1.Operation{}, cacheOwnerKey, cacheOperationIndexerFunc); err != nil { // +gocover:ignore:block init controller
+	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &v1alpha1.Operation{}, v1alpha1.CacheOwnerKey, cacheOperationIndexerFunc); err != nil { // +gocover:ignore:block init controller
 		return err
 	}
 	// +gocover:ignore:block init controller

internal/controller/cache_controller_test.go

@@ -31,7 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/Azure/operation-cache-controller/api/v1alpha1"
-	"github.com/Azure/operation-cache-controller/internal/controller/mocks"
+	"github.com/Azure/operation-cache-controller/internal/handler/mocks"
 	"github.com/Azure/operation-cache-controller/internal/utils/reconciler"
 )
 
@@ -62,7 +62,7 @@ func TestReconcileHandler(t *testing.T) {
 			Scheme: scheme,
 		}
 		mockCacheAdapterCtrl := gomock.NewController(t)
-		cacheAdapter := mocks.NewMockCacheAdapterInterface(mockCacheAdapterCtrl)
+		cacheAdapter := mocks.NewMockCacheHandlerInterface(mockCacheAdapterCtrl)
 		cacheAdapter.EXPECT().CheckCacheExpiry(ctx).Return(reconciler.OperationResult{}, nil)
 		cacheAdapter.EXPECT().EnsureCacheInitialized(ctx).Return(reconciler.OperationResult{}, nil)
 		cacheAdapter.EXPECT().CalculateKeepAliveCount(ctx).Return(reconciler.OperationResult{}, nil)
@@ -83,7 +83,7 @@ func TestReconcileHandler(t *testing.T) {
 			Scheme: scheme,
 		}
 		mockCacheAdapterCtrl := gomock.NewController(t)
-		cacheAdapter := mocks.NewMockCacheAdapterInterface(mockCacheAdapterCtrl)
+		cacheAdapter := mocks.NewMockCacheHandlerInterface(mockCacheAdapterCtrl)
 		cacheAdapter.EXPECT().CheckCacheExpiry(ctx).Return(reconciler.OperationResult{CancelRequest: true}, nil)
 		res, err := cacheReconciler.reconcileHandler(ctx, cacheAdapter)
 		assert.NoError(t, err)
@@ -101,7 +101,7 @@ func TestReconcileHandler(t *testing.T) {
 			Scheme: scheme,
 		}
 		mockCacheAdapterCtrl := gomock.NewController(t)
-		cacheAdapter := mocks.NewMockCacheAdapterInterface(mockCacheAdapterCtrl)
+		cacheAdapter := mocks.NewMockCacheHandlerInterface(mockCacheAdapterCtrl)
 		cacheAdapter.EXPECT().CheckCacheExpiry(ctx).Return(reconciler.OperationResult{}, assert.AnError)
 		_, err := cacheReconciler.reconcileHandler(ctx, cacheAdapter)
 		assert.NotNil(t, err)