refactoring..

Author: PatrickLaabs

Makefile

@@ -231,7 +231,7 @@ CAPI_KIND_CLUSTER_NAME ?= capi-test
 # It is set by Prow GIT_TAG, a git-based tag of the form vYYYYMMDD-hash, e.g., v20210120-v0.3.10-308-gc61521971
 
 # Next release is: v0.3.2
-TAG ?= v0.3.2-preview.39
+TAG ?= v0.3.2-preview.47
 ARCH ?= $(shell go env GOARCH)
 ALL_ARCH = amd64 arm arm64
 

config/default/manager_image_patch.yaml

@@ -7,5 +7,5 @@ spec:
   template:
     spec:
       containers:
-      - image: ghcr.io/patricklaabs/cluster-api-addon-provider-cdk8s/cluster-api-cdk8s-controller:v0.3.2-preview.39
+      - image: ghcr.io/patricklaabs/cluster-api-addon-provider-cdk8s/cluster-api-cdk8s-controller:v0.3.2-preview.47
         name: manager

config/default/manager_image_patch.yaml-e

@@ -7,5 +7,5 @@ spec:
   template:
     spec:
       containers:
-      - image: ghcr.io/patricklaabs/cluster-api-addon-provider-cdk8s/cluster-api-cdk8s-controller:v0.3.2-preview.39
+      - image: ghcr.io/patricklaabs/cluster-api-addon-provider-cdk8s/cluster-api-cdk8s-controller:v0.3.2-preview.47
         name: manager

controllers/cdk8sappproxy/cdk8sappproxy_git_operator.go

@@ -12,9 +12,7 @@ import (
 	"path/filepath"
 )
 
-func (r *Reconciler) prepareSource(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, proxyNamespacedName types.NamespacedName, logger logr.Logger, operation string) (string, string, error) {
-	var appSourcePath string
-	var currentCommitHash string
+func (r *Reconciler) prepareSource(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, proxyNamespacedName types.NamespacedName, logger logr.Logger, operation string) (appSourcePath string, currentCommitHash string, err error) {
 	gitImpl := &gitoperator.GitImplementer{}
 	var buf bytes.Buffer
 	gitSpec := cdk8sAppProxy.Spec.GitRepository
@@ -52,17 +50,15 @@ func (r *Reconciler) prepareSource(ctx context.Context, cdk8sAppProxy *addonsv1a
 		}
 	default:
 		err := errors.New("no source specified")
-		if operation == OperationNormal { // Poller management and status updates for normal flow
-			//r.stopGitPoller(proxyNamespacedName, logger)
+		if operation == OperationNormal {
 			if cdk8sAppProxy != nil {
 				_ = r.updateStatusWithError(ctx, cdk8sAppProxy, addonsv1alpha1.EmptyGitRepositoryReason, "No source specified during "+operation, err, false)
 			}
 		} else if operation == OperationDeletion && cdk8sAppProxy != nil {
-			// For deletion, if source can't be determined, we might still want to remove finalizer.
 			_ = r.updateStatusWithError(ctx, cdk8sAppProxy, addonsv1alpha1.EmptyGitRepositoryReason, "No source specified, cannot determine resources to delete during "+operation, err, true)
 		}
 
-		return "", "", err
+		return appSourcePath, currentCommitHash, err
 	}
 
 	return appSourcePath, currentCommitHash, nil

controllers/cdk8sappproxy/cdk8sappproxy_reconciler.go

@@ -59,36 +59,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 
 func (r *Reconciler) reconcileDelete(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy) (ctrl.Result, error) {
 	logger := log.FromContext(ctx).WithValues("cdk8sappproxy", types.NamespacedName{Name: cdk8sAppProxy.Name, Namespace: cdk8sAppProxy.Namespace}, "reconcile_type", "delete")
-	logger.Info("Starting reconcileDelete")
 
 	proxyNamespacedName := types.NamespacedName{Name: cdk8sAppProxy.Name, Namespace: cdk8sAppProxy.Namespace}
 
-	// Stop any active git poller
-	//r.stopGitPoller(proxyNamespacedName, logger)
-
 	if !controllerutil.ContainsFinalizer(cdk8sAppProxy, Finalizer) {
 		logger.Info("Finalizer already removed, nothing to do.")
 
 		return ctrl.Result{}, nil
 	}
 
-	// Get a source path for deletion
-	appSourcePath, _, err := r.prepareSource(ctx, cdk8sAppProxy, proxyNamespacedName, logger, OperationDeletion)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// Get resources to delete
-	parsedResources, err := r.synthesizeAndParseResources(appSourcePath, logger)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// Delete resources from target clusters
-	if err := r.deleteResourcesFromClusters(ctx, cdk8sAppProxy, parsedResources, logger); err != nil {
-		return ctrl.Result{}, err
-	}
-
 	// Clean up watches and remove finalizer
 	if err := r.finalizeDeletion(ctx, cdk8sAppProxy, proxyNamespacedName, logger); err != nil {
 		return ctrl.Result{}, err
@@ -104,31 +83,51 @@ func (r *Reconciler) reconcileNormal(ctx context.Context, cdk8sAppProxy *addonsv
 
 	proxyNamespacedName := types.NamespacedName{Name: cdk8sAppProxy.Name, Namespace: cdk8sAppProxy.Namespace}
 
-	// Handle deletion trigger annotation
-	forceSynthAndApplyDueToDeletion, err := r.handleDeletionTriggerAnnotation(ctx, cdk8sAppProxy, logger)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	// Add finalizer if needed
 	if shouldRequeue, err := r.ensureFinalizer(ctx, cdk8sAppProxy, logger); err != nil || shouldRequeue {
 		return ctrl.Result{Requeue: shouldRequeue}, err
 	}
 
 	// Prepare a source path and get current commit hash
-	appSourcePath, currentCommitHash, err := r.prepareSource(ctx, cdk8sAppProxy, proxyNamespacedName, logger, OperationNormal)
+	appSourcePath, _, err := r.prepareSource(ctx, cdk8sAppProxy, proxyNamespacedName, logger, OperationNormal)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	parsedResources, err := r.synthesizeAndParseResources(appSourcePath, logger)
 	if err != nil {
+		logger.Error(err, "failed to synthesize and parse resources", "appSourcePath", appSourcePath)
+	}
+
+	if len(parsedResources) == 0 {
+		if err := r.handleNoResources(ctx, cdk8sAppProxy, logger); err != nil {
+			return ctrl.Result{}, err
+		}
+
+		return ctrl.Result{}, nil
+	}
+
+	// Determine if apply is needed
+	_, clusterList, err := r.applyNeeded(ctx, cdk8sAppProxy, parsedResources, logger)
+	if err != nil {
+		logger.Error(err, "Failed to determine if apply is needed")
+		//return ctrl.Result{}, err
+	}
+
+	_, err = r.applyResourcesToClusters(ctx, cdk8sAppProxy, parsedResources, clusterList, proxyNamespacedName, logger)
+	if err != nil {
+		logger.Error(err, "Failed to apply resources to clusters", "appSourcePath", appSourcePath)
+
 		return ctrl.Result{}, err
 	}
 
-	pollInterval := 1 * time.Minute
+	pollInterval := 5 * time.Minute
 	if cdk8sAppProxy.Spec.GitRepository.ReferencePollInterval != nil {
 		pollInterval = cdk8sAppProxy.Spec.GitRepository.ReferencePollInterval.Duration
 	}
 
 	repoUrl := cdk8sAppProxy.Spec.GitRepository.URL
 	branch := cdk8sAppProxy.Spec.GitRepository.Reference
-	tempDirPattern := "/tmp/cdk8s-git-clone-"
 	buf := &bytes.Buffer{}
 	polling := false
 
@@ -141,16 +140,50 @@ func (r *Reconciler) reconcileNormal(ctx context.Context, cdk8sAppProxy *addonsv
 			select {
 			case <-ticker.C:
 				logger.Info("Polling git repository for changes", "repoUrl", repoUrl, "branch", branch)
-				polling, err = gitImpl.Poll(repoUrl, branch, tempDirPattern, buf)
+				polling, err = gitImpl.Poll(repoUrl, branch, appSourcePath, buf)
 				if err != nil {
 					logger.Error(err, "Polling git repository", "repoUrl", repoUrl, "branch", branch)
 				}
 				if polling {
 					logger.Info("Detected changes in git repository, proceeding with reconciliation.")
-					appSourcePath, currentCommitHash, err = r.prepareSource(ctx, cdk8sAppProxy, proxyNamespacedName, logger, OperationNormal)
+					appSourcePath, _, err = r.prepareSource(ctx, cdk8sAppProxy, proxyNamespacedName, logger, OperationNormal)
 					if err != nil {
 						logger.Error(err, "Prepare source for reconciliation")
 					}
+
+					// Synthesize and parse resources
+					_, err := r.synthesizeAndParseResources(appSourcePath, logger)
+					if err != nil {
+						logger.Error(err, "failed to synthesize and parse resources", "appSourcePath", appSourcePath)
+					}
+
+					parsedResources, err := r.synthesizeAndParseResources(appSourcePath, logger)
+					if err != nil {
+						logger.Error(err, "failed to synthesize and parse resources", "appSourcePath", appSourcePath)
+					}
+
+					if len(parsedResources) == 0 {
+						if err := r.handleNoResources(ctx, cdk8sAppProxy, logger); err != nil {
+							logger.Error(err, "Failed to handle no resources case")
+							return
+						}
+
+						logger.Info("No valid Kubernetes resources parsed from manifest files, skipping apply.")
+					}
+
+					// Determine if apply is needed
+					_, clusterList, err := r.applyNeeded(ctx, cdk8sAppProxy, parsedResources, logger)
+					if err != nil {
+						logger.Error(err, "Failed to determine if apply is needed")
+						//return ctrl.Result{}, err
+					}
+
+					_, err = r.applyResourcesToClusters(ctx, cdk8sAppProxy, parsedResources, clusterList, proxyNamespacedName, logger)
+					if err != nil {
+						logger.Error(err, "Failed to apply resources to clusters", "appSourcePath", appSourcePath)
+
+						return
+					}
 				}
 			case <-ctx.Done():
 				logger.Info("Stopping git repository polling loop due to context cancellation.")
@@ -159,36 +192,7 @@ func (r *Reconciler) reconcileNormal(ctx context.Context, cdk8sAppProxy *addonsv
 		}
 	}()
 
-	// Synthesize and parse resources
-	parsedResources, err := r.synthesizeAndParseResources(appSourcePath, logger)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
-	if len(parsedResources) == 0 {
-		if err := r.handleNoResources(ctx, cdk8sAppProxy, logger); err != nil {
-			return ctrl.Result{}, err
-		}
-
-		return ctrl.Result{}, nil
-	}
-
-	// Determine if apply is needed
-	applyNeeded, clusterList, err := r.determineIfApplyNeeded(ctx, cdk8sAppProxy, parsedResources, currentCommitHash, forceSynthAndApplyDueToDeletion, logger)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
-	if !applyNeeded {
-		if err := r.handleSkipApply(ctx, cdk8sAppProxy, currentCommitHash, logger); err != nil {
-			return ctrl.Result{}, err
-		}
-
-		return ctrl.Result{}, nil
-	}
-
-	// Apply resources to clusters
-	return r.applyResourcesToClusters(ctx, cdk8sAppProxy, parsedResources, clusterList, currentCommitHash, proxyNamespacedName, logger)
+	return ctrl.Result{}, nil
 }
 
 func (r *Reconciler) handleDeletionTriggerAnnotation(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, logger logr.Logger) (bool, error) {
@@ -268,6 +272,18 @@ func (r *Reconciler) determineIfApplyNeeded(ctx context.Context, cdk8sAppProxy *
 	return true, clusterList, nil
 }
 
+func (r *Reconciler) applyNeeded(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, parsedResources []*unstructured.Unstructured, logger logr.Logger) (bool, clusterv1.ClusterList, error) {
+	var clusterList clusterv1.ClusterList
+
+	_, list, err := r.verifyResourcesOnClusters(ctx, cdk8sAppProxy, parsedResources, logger)
+	if err != nil {
+		return false, clusterList, err
+	}
+	clusterList = list
+
+	return true, clusterList, nil
+}
+
 func (r *Reconciler) verifyResourcesOnClusters(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, parsedResources []*unstructured.Unstructured, logger logr.Logger) (bool, clusterv1.ClusterList, error) {
 	var clusterList clusterv1.ClusterList
 	foundMissingResourcesOnAnyCluster := false
@@ -460,7 +476,7 @@ func (r *Reconciler) reestablishWatchesForExistingResources(ctx context.Context,
 }
 
 //nolint:unparam // ctrl.Result is required for controller-runtime reconciler pattern
-func (r *Reconciler) applyResourcesToClusters(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, parsedResources []*unstructured.Unstructured, clusterList clusterv1.ClusterList, currentCommitHash string, proxyNamespacedName types.NamespacedName, logger logr.Logger) (ctrl.Result, error) {
+func (r *Reconciler) applyResourcesToClusters(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, parsedResources []*unstructured.Unstructured, clusterList clusterv1.ClusterList, proxyNamespacedName types.NamespacedName, logger logr.Logger) (ctrl.Result, error) {
 	logger.Info("Proceeding with application of resources to target clusters.")
 
 	// Ensure clusterList is populated if needed
@@ -492,8 +508,8 @@ func (r *Reconciler) applyResourcesToClusters(ctx context.Context, cdk8sAppProxy
 		logger.Info("No parsed resources to apply, skipping application to clusters.")
 		cdk8sAppProxy.Status.ObservedGeneration = cdk8sAppProxy.Generation
 		conditions.MarkTrue(cdk8sAppProxy, addonsv1alpha1.DeploymentProgressingCondition)
-		if cdk8sAppProxy.Spec.GitRepository != nil && cdk8sAppProxy.Spec.GitRepository.URL != "" && currentCommitHash != "" {
-			cdk8sAppProxy.Status.LastProcessedGitHash = currentCommitHash
+		if cdk8sAppProxy.Spec.GitRepository != nil && cdk8sAppProxy.Spec.GitRepository.URL != "" {
+			//cdk8sAppProxy.Status.LastProcessedGitHash = currentCommitHash
 		}
 		if err := r.Status().Update(ctx, cdk8sAppProxy); err != nil {
 			logger.Error(err, "Failed to update status when no resources to apply")
@@ -557,8 +573,8 @@ func (r *Reconciler) applyResourcesToClusters(ctx context.Context, cdk8sAppProxy
 
 	// If we reach here, the overallSuccess is true.
 	if cdk8sAppProxy.Spec.GitRepository != nil && cdk8sAppProxy.Spec.GitRepository.URL != "" {
-		cdk8sAppProxy.Status.LastProcessedGitHash = currentCommitHash
-		logger.Info("Successfully updated LastProcessedGitHash in status after application", "hash", currentCommitHash)
+		//cdk8sAppProxy.Status.LastProcessedGitHash = currentCommitHash
+		//logger.Info("Successfully updated LastProcessedGitHash in status after application", "hash", currentCommitHash)
 	}
 
 	cdk8sAppProxy.Status.ObservedGeneration = cdk8sAppProxy.Generation

controllers/cdk8sappproxy/git/git.go

@@ -59,28 +59,30 @@ func (g *GitImplementer) Poll(repoUrl string, branch string, directory string, w
 
 	err = validateRepoUrl(repoUrl, writer)
 	if err != nil {
-		fmt.Fprintf(writer, "%s", addonsv1alpha1.GitHashFailureReason)
+		fmt.Fprintf(writer, "%s", addonsv1alpha1.InvalidGitRepositoryReason)
 
 		return detectedChanges, err
 	}
 	if directory == "" {
-		fmt.Fprintf(writer, "%s", addonsv1alpha1.GitHashFailureReason)
+		fmt.Fprintf(writer, "%s", addonsv1alpha1.EmptyGitRepositoryReason)
 
 		return detectedChanges, err
 	}
 
 	// get hash from local repo
 	localHash, err := localGitHash(directory, writer)
 	if err != nil {
-		fmt.Fprintf(writer, "%s", addonsv1alpha1.GitHashFailureReason)
+		//fmt.Fprintf(writer, "%s", addonsv1alpha1.GitHashFailureReason)
+		fmt.Fprintf(writer, "localGitHash error")
 
 		return detectedChanges, err
 	}
 
 	// Get Hash from remote repo
 	remoteHash, err := remoteGitHash(repoUrl, branch, writer)
 	if err != nil {
-		fmt.Fprintf(writer, "%s", addonsv1alpha1.GitHashFailureReason)
+		//fmt.Fprintf(writer, "%s", addonsv1alpha1.GitHashFailureReason)
+		fmt.Fprintf(writer, "remoteGitHash error")
 
 		return detectedChanges, err
 	}
@@ -118,17 +120,20 @@ func localGitHash(directory string, writer *bytes.Buffer) (hash string, err erro
 	hash = "0"
 	repo, err := git.PlainOpen(directory)
 	if err != nil {
-		return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		//return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		return hash, fmt.Errorf("failed to open local git repository: %v", err)
 	}
 
 	headRef, err := repo.Head()
 	if err != nil {
-		return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		//return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		return hash, fmt.Errorf("failed to get head for local git repo: %v", err)
 	}
 
 	hash = headRef.Hash().String()
 	if hash == "" {
-		return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		//return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		return hash, fmt.Errorf("failed to retrieve hash for local git repo")
 	}
 
 	return hash, err
@@ -149,7 +154,8 @@ func remoteGitHash(repoUrl string, branch string, writer *bytes.Buffer) (hash st
 
 	refs, err := repo.List(&git.ListOptions{})
 	if err != nil {
-		return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		//return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+		return hash, fmt.Errorf("failed to list remote refs: %v", err)
 	}
 
 	refName := plumbing.NewBranchReferenceName(branch)
@@ -159,5 +165,6 @@ func remoteGitHash(repoUrl string, branch string, writer *bytes.Buffer) (hash st
 		}
 	}
 
-	return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+	//return hash, fmt.Errorf("%s", addonsv1alpha1.GitHashFailureReason)
+	return hash, fmt.Errorf("failed to find remote ref for branch %s: %v", branch, refs)
 }