doing some refactoring and code cleanup

Author: PatrickLaabs

Makefile

@@ -230,7 +230,8 @@ 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
 
-TAG ?= v0.3.0-alpha
+# Next release is: v0.3.1-alpha
+TAG ?= v0.3.1-preview
 ARCH ?= $(shell go env GOARCH)
 ALL_ARCH = amd64 arm arm64
 

README.md

@@ -8,10 +8,6 @@
 
 # Cluster API Add-on Provider for Cdk8s
 
-### 👋 Welcome! Here are some links to help you get started:
-
-- [Project design outline](https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20220712-cluster-api-addon-orchestration.md)
-
 ## ✨ What is Cluster API Add-on Provider for Cdk8s?
 
 Cluster API Add-on Provider for Cdk8s extends Cluster API by managing the installation, configuration, upgrade, and deletion of cluster add-ons using cdk8s applications. This provider is based on the [Cluster API Add-on Orchestration Proposal](https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20220712-cluster-api-addon-orchestration.md), a larger effort to bring orchestration for add-ons to CAPI by using existing package management tools.
@@ -47,14 +43,30 @@ apiVersion: addons.cluster.x-k8s.io/v1alpha1
 kind: Cdk8sAppProxy
 metadata:
   name: cdk8s-sample-app
-  namespace: caapc-system
+  namespace: default
 spec:
   gitRepository:
-    url: https://github.com/PatrickLaabs/cdk8s-sample-deployment
-    # reference: main # Optional: specify a branch, tag, or commit
-    # path: "" # Optional: specify a path within the repository if cdk8s app is not at root
-  clusterSelector:
-    matchLabels: {}
+    url: "https://github.com/PatrickLaabs/cluster-api-addon-provider-cdk8s/examples/cdk8s-sample-deployment"
+    reference: "main"
+    path: "."
+    # authSecretRef:
+    #   name: git-credentials
+  clusterSelector: {}
+    # matchLabels:
+      # environment: development
+# ---
+# apiVersion: v1
+# kind: Secret
+# metadata:
+#   name: git-credentials
+#   namespace: default
+# type: Opaque
+# data:
+#   # Base64 encoded username
+#   username: <base64-encoded-username>
+#   # Base64 encoded password/token
+#   password: <base64-encoded-password-or-token>
+
 ```
 
 ### Cdk8sAppProxySpec Fields

api/v1alpha1/cdk8sappproxy_types.go

@@ -56,10 +56,6 @@ type Cdk8sAppProxySpec struct {
 	// +kubebuilder:validation:Optional
 	GitRepository *GitRepositorySpec `json:"gitRepository,omitempty"`
 
-	// Values is a string containing the values to be passed to the cdk8s app
-	// +kubebuilder:validation:Optional
-	Values string `json:"values,omitempty"`
-
 	// ClusterSelector selects the clusters to deploy the cdk8s app to.
 	// +kubebuilder:validation:Required
 	ClusterSelector metav1.LabelSelector `json:"clusterSelector"`

config/crd/bases/addons.cluster.x-k8s.io_cdk8sappproxies.yaml

@@ -140,10 +140,6 @@ spec:
                   LocalPath is the local filesystem path to the cdk8s app.
                   One of LocalPath or GitRepository must be specified.
                 type: string
-              values:
-                description: Values is a string containing the values to be passed
-                  to the cdk8s app
-                type: string
             required:
             - clusterSelector
             type: object

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-amd64:v0.3.0-alpha
+      - image: ghcr.io/patricklaabs/cluster-api-addon-provider-cdk8s/cluster-api-cdk8s-controller:v0.3.1-preview
         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.2.11-preview
+      - image: ghcr.io/patricklaabs/cluster-api-addon-provider-cdk8s/cluster-api-cdk8s-controller:v0.3.1-preview
         name: manager

config/default/manager_pull_policy.yaml

@@ -8,4 +8,4 @@ spec:
     spec:
       containers:
       - name: manager
-        imagePullPolicy: Always
+        imagePullPolicy: IfNotPresent

config/default/manager_pull_policy.yaml-e

@@ -8,4 +8,4 @@ spec:
     spec:
       containers:
       - name: manager
-        imagePullPolicy: IfNotPresent
+        imagePullPolicy: Always

controllers/cdk8sappproxy/cdk8sappproxy_controller.go

@@ -186,57 +186,46 @@ func (r *Reconciler) parseManifestFiles(manifestFiles []string, logger logr.Logg
 	var parsedResources []*unstructured.Unstructured
 
 	for _, manifestFile := range manifestFiles {
-		resources, err := r.parseManifestFile(manifestFile, logger, operation)
-		if err != nil {
-			return nil, err
-		}
-		parsedResources = append(parsedResources, resources...)
-	}
+		logger.Info("Processing manifest file", "file", manifestFile, "operation", operation)
 
-	logger.Info("Total resources parsed", "count", len(parsedResources), "operation", operation)
+		fileContent, readErr := os.ReadFile(manifestFile)
+		if readErr != nil {
+			logger.Error(readErr, "Failed to read manifest file", "file", manifestFile, "operation", operation)
 
-	return parsedResources, nil
-}
-
-func (r *Reconciler) parseManifestFile(manifestFile string, logger logr.Logger, operation string) ([]*unstructured.Unstructured, error) {
-	logger.Info("Processing manifest file", "file", manifestFile, "operation", operation)
-
-	fileContent, readErr := os.ReadFile(manifestFile)
-	if readErr != nil {
-		logger.Error(readErr, "Failed to read manifest file", "file", manifestFile, "operation", operation)
+			return nil, readErr
+		}
 
-		return nil, readErr
-	}
+		yamlDecoder := k8syaml.NewYAMLOrJSONDecoder(bytes.NewReader(fileContent), 100)
 
-	var parsedResources []*unstructured.Unstructured
-	yamlDecoder := k8syaml.NewYAMLOrJSONDecoder(bytes.NewReader(fileContent), 100)
+		for {
+			var rawObj runtime.RawExtension
+			if err := yamlDecoder.Decode(&rawObj); err != nil {
+				if err.Error() == "EOF" {
+					break
+				}
+				logger.Error(err, "Failed to decode YAML from manifest file", "file", manifestFile, "operation", operation)
 
-	for {
-		var rawObj runtime.RawExtension
-		if err := yamlDecoder.Decode(&rawObj); err != nil {
-			if err.Error() == "EOF" {
-				break
+				return nil, err
 			}
-			logger.Error(err, "Failed to decode YAML from manifest file", "file", manifestFile, "operation", operation)
 
-			return nil, err
-		}
+			if rawObj.Raw == nil {
+				continue
+			}
 
-		if rawObj.Raw == nil {
-			continue
-		}
+			u := &unstructured.Unstructured{}
+			if _, _, err := unstructured.UnstructuredJSONScheme.Decode(rawObj.Raw, nil, u); err != nil {
+				logger.Error(err, "Failed to decode RawExtension to Unstructured", "file", manifestFile, "operation", operation)
 
-		u := &unstructured.Unstructured{}
-		if _, _, err := unstructured.UnstructuredJSONScheme.Decode(rawObj.Raw, nil, u); err != nil {
-			logger.Error(err, "Failed to decode RawExtension to Unstructured", "file", manifestFile, "operation", operation)
+				return nil, err
+			}
 
-			return nil, err
+			parsedResources = append(parsedResources, u)
+			logger.Info("Parsed resource", "GVK", u.GroupVersionKind().String(), "Name", u.GetName(), "Namespace", u.GetNamespace(), "operation", operation)
 		}
-
-		parsedResources = append(parsedResources, u)
-		logger.Info("Parsed resource", "GVK", u.GroupVersionKind().String(), "Name", u.GetName(), "Namespace", u.GetNamespace(), "operation", operation)
 	}
 
+	logger.Info("Total resources parsed", "count", len(parsedResources), "operation", operation)
+
 	return parsedResources, nil
 }
 
@@ -258,6 +247,39 @@ func (r *Reconciler) deleteResourcesFromClusters(ctx context.Context, cdk8sAppPr
 	return nil
 }
 
+func (r *Reconciler) deleteResourcesFromSingleCluster(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, cluster clusterv1.Cluster, parsedResources []*unstructured.Unstructured, logger logr.Logger) error {
+	clusterLogger := logger.WithValues("targetCluster", cluster.Name)
+	clusterLogger.Info("Deleting resources from cluster")
+
+	dynamicClient, err := r.getDynamicClientForCluster(ctx, cdk8sAppProxy.Namespace, cluster.Name)
+	if err != nil {
+		clusterLogger.Error(err, "Failed to get dynamic client for cluster during deletion, skipping this cluster")
+
+		return err
+	}
+
+	clusterLogger.Info("Successfully created dynamic client for cluster deletion")
+
+	for _, resource := range parsedResources {
+		gvr := resource.GroupVersionKind().GroupVersion().WithResource(getPluralFromKind(resource.GetKind()))
+		clusterLogger.Info("Deleting resource from cluster", "GVK", resource.GroupVersionKind().String(), "Name", resource.GetName(), "Namespace", resource.GetNamespace())
+
+		err := dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).Delete(ctx, resource.GetName(), metav1.DeleteOptions{})
+
+		switch {
+		case err != nil && !apierrors.IsNotFound(err):
+			clusterLogger.Error(err, "Failed to delete resource from cluster", "resourceName", resource.GetName())
+			// Log but continue with other resources
+		case apierrors.IsNotFound(err):
+			clusterLogger.Info("Resource already deleted from cluster", "resourceName", resource.GetName())
+		case err == nil:
+			clusterLogger.Info("Successfully deleted resource from cluster", "resourceName", resource.GetName())
+		}
+	}
+
+	return nil
+}
+
 func (r *Reconciler) getTargetClustersForDeletion(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, logger logr.Logger) (*clusterv1.ClusterList, error) {
 	clusterList := &clusterv1.ClusterList{}
 	selector, err := metav1.LabelSelectorAsSelector(&cdk8sAppProxy.Spec.ClusterSelector)
@@ -283,83 +305,77 @@ func (r *Reconciler) getTargetClustersForDeletion(ctx context.Context, cdk8sAppP
 	return clusterList, nil
 }
 
-func (r *Reconciler) deleteResourcesFromSingleCluster(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, cluster clusterv1.Cluster, parsedResources []*unstructured.Unstructured, logger logr.Logger) error {
-	clusterLogger := logger.WithValues("targetCluster", cluster.Name)
-	clusterLogger.Info("Deleting resources from cluster")
-
-	dynamicClient, err := r.getDynamicClientForCluster(ctx, cdk8sAppProxy.Namespace, cluster.Name)
-	if err != nil {
-		clusterLogger.Error(err, "Failed to get dynamic client for cluster during deletion, skipping this cluster")
+// func (r *Reconciler) deleteResourcesFromSingleCluster(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, cluster clusterv1.Cluster, parsedResources []*unstructured.Unstructured, logger logr.Logger) error {
+// 	clusterLogger := logger.WithValues("targetCluster", cluster.Name)
+// 	clusterLogger.Info("Deleting resources from cluster")
 
-		return err
-	}
+// 	dynamicClient, err := r.getDynamicClientForCluster(ctx, cdk8sAppProxy.Namespace, cluster.Name)
+// 	if err != nil {
+// 		clusterLogger.Error(err, "Failed to get dynamic client for cluster during deletion, skipping this cluster")
 
-	clusterLogger.Info("Successfully created dynamic client for cluster deletion")
+// 		return err
+// 	}
 
-	for _, resource := range parsedResources {
-		if err := r.deleteResourceFromCluster(ctx, dynamicClient, resource, clusterLogger); err != nil {
-			// Log but continue with other resources
-			clusterLogger.Error(err, "Failed to delete resource from cluster", "resourceName", resource.GetName())
-		}
-	}
+// 	clusterLogger.Info("Successfully created dynamic client for cluster deletion")
 
-	return nil
-}
+// 	for _, resource := range parsedResources {
+// 		if err := r.deleteResourceFromCluster(ctx, dynamicClient, resource, clusterLogger); err != nil {
+// 			// Log but continue with other resources
+// 			clusterLogger.Error(err, "Failed to delete resource from cluster", "resourceName", resource.GetName())
+// 		}
+// 	}
 
-func (r *Reconciler) deleteResourceFromCluster(ctx context.Context, dynamicClient dynamic.Interface, resource *unstructured.Unstructured, logger logr.Logger) error {
-	gvr := resource.GroupVersionKind().GroupVersion().WithResource(getPluralFromKind(resource.GetKind()))
-	logger.Info("Deleting resource from cluster", "GVK", resource.GroupVersionKind().String(), "Name", resource.GetName(), "Namespace", resource.GetNamespace())
+// 	return nil
+// }
 
-	err := dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).Delete(ctx, resource.GetName(), metav1.DeleteOptions{})
+// func (r *Reconciler) deleteResourceFromCluster(ctx context.Context, dynamicClient dynamic.Interface, resource *unstructured.Unstructured, logger logr.Logger) error {
+// 	gvr := resource.GroupVersionKind().GroupVersion().WithResource(getPluralFromKind(resource.GetKind()))
+// 	logger.Info("Deleting resource from cluster", "GVK", resource.GroupVersionKind().String(), "Name", resource.GetName(), "Namespace", resource.GetNamespace())
 
-	switch {
-	case err != nil && !apierrors.IsNotFound(err):
-		logger.Error(err, "Failed to delete resource from cluster", "resourceName", resource.GetName())
+// 	err := dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).Delete(ctx, resource.GetName(), metav1.DeleteOptions{})
 
-		return err
-	case apierrors.IsNotFound(err):
-		logger.Info("Resource already deleted from cluster", "resourceName", resource.GetName())
-	case err == nil:
-		logger.Info("Successfully deleted resource from cluster", "resourceName", resource.GetName())
-	}
+// 	switch {
+// 	case err != nil && !apierrors.IsNotFound(err):
+// 		logger.Error(err, "Failed to delete resource from cluster", "resourceName", resource.GetName())
 
-	return nil
-}
+// 		return err
+// 	case apierrors.IsNotFound(err):
+// 		logger.Info("Resource already deleted from cluster", "resourceName", resource.GetName())
+// 	case err == nil:
+// 		logger.Info("Successfully deleted resource from cluster", "resourceName", resource.GetName())
+// 	}
 
-func (r *Reconciler) finalizeDeletion(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, proxyNamespacedName types.NamespacedName, logger logr.Logger) (ctrl.Result, error) {
-	// Cancel any active watches for this Cdk8sAppProxy
-	r.cancelActiveWatches(proxyNamespacedName, logger)
+// 	return nil
+// }
 
-	// Remove finalizer
-	return r.removeFinalizer(ctx, cdk8sAppProxy, logger)
-}
+func (r *Reconciler) finalizeDeletion(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, proxyNamespacedName types.NamespacedName, logger logr.Logger) error {
+	logger.Info("Starting finalization process")
 
-func (r *Reconciler) cancelActiveWatches(proxyNamespacedName types.NamespacedName, logger logr.Logger) {
+	// Cancel any active watches for this Cdk8sAppProxy
 	if watchesForProxy, ok := r.ActiveWatches[proxyNamespacedName]; ok {
 		logger.Info("Cancelling active watches for Cdk8sAppProxy before deletion", "count", len(watchesForProxy))
 		for watchKey, cancelFunc := range watchesForProxy {
 			logger.Info("Cancelling watch", "watchKey", watchKey)
 			cancelFunc() // Stop the goroutine and its associated Kubernetes watch
 		}
-		// After all, watches for this proxy are canceled, remove its entry from the main map
+		// After all watches for this proxy are canceled, remove its entry from the main map
 		delete(r.ActiveWatches, proxyNamespacedName)
 		logger.Info("Removed Cdk8sAppProxy entry from ActiveWatches map")
 	} else {
-		logger.Info("No active watches found for this Cdk8sAppProxy to cancel.")
+		logger.Info("No active watches found for this Cdk8sAppProxy to cancel")
 	}
-}
 
-func (r *Reconciler) removeFinalizer(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, logger logr.Logger) (ctrl.Result, error) {
+	// Remove finalizer
 	logger.Info("Finished deletion logic, removing finalizer")
 	controllerutil.RemoveFinalizer(cdk8sAppProxy, Finalizer)
 	if err := r.Update(ctx, cdk8sAppProxy); err != nil {
 		logger.Error(err, "Failed to remove finalizer")
 
-		return ctrl.Result{}, err
+		return err
 	}
 	logger.Info("Finalizer removed successfully")
 
-	return ctrl.Result{}, nil
+	return nil
 }
 
 func (r *Reconciler) getDynamicClientForCluster(ctx context.Context, secretNamespace, clusterName string) (dynamic.Interface, error) {

controllers/cdk8sappproxy/cdk8sappproxy_reconciler.go

@@ -103,7 +103,11 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cdk8sAppProxy *addonsv
 	}
 
 	// Clean up watches and remove finalizer
-	return r.finalizeDeletion(ctx, cdk8sAppProxy, proxyNamespacedName, logger)
+	if err := r.finalizeDeletion(ctx, cdk8sAppProxy, proxyNamespacedName, logger); err != nil {
+		return ctrl.Result{}, err
+	}
+
+	return ctrl.Result{}, nil
 }
 
 func (r *Reconciler) reconcileNormal(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy) (ctrl.Result, error) {