Fix akodeploymentconfig cluster selector didn't select the right clusters issue (#91)

* Fix akodeploymentconfig cluster selector didn't select the right clusters issue
* Remove duplicated add-on secrete deletion code 

Signed-off-by: Xudong Liu <xudongl@vmware.com>

Author: XudongLiuHarold

Makefile

@@ -13,7 +13,7 @@ PUBLISH?=publish
 BUILD_VERSION ?= $(shell git describe --always --match "v*" | sed 's/v//')
 
 # TKG Version
-TKG_VERSION ?= v1.6.0+vmware.11
+TKG_VERSION ?= v1.6.0+vmware.12
 
 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
 ifeq (,$(shell go env GOBIN))

api/v1alpha1/constants.go

@@ -37,7 +37,6 @@ const (
 
 	AVI_VERSION                                                  = "20.1.3"
 	AviClusterLabel                                              = "networking.tkg.tanzu.vmware.com/avi"
-	AviClusterSelectedLabel                                      = "networking.tkg.tanzu.vmware.com/avi-skip-default-adc"
 	AviClusterDeleteConfigLabel                                  = "networking.tkg.tanzu.vmware.com/avi-config-delete"
 	AviClusterSecretType                                         = "avi.cluster.x-k8s.io/secret"
 	AviNamespace                                                 = "avi-system"

api/v1alpha1/zz_generated.deepcopy.go

@@ -27,6 +27,8 @@ import (
 
 	akoov1alpha1 "github.com/vmware-samples/load-balancer-operator-for-kubernetes/api/v1alpha1"
 	akov1alpha1 "github.com/vmware/load-balancer-and-ingress-services-for-kubernetes/pkg/apis/ako/v1alpha1"
+
+	ako_operator "github.com/vmware-samples/load-balancer-operator-for-kubernetes/pkg/ako-operator"
 )
 
 func getAviCAFromADC(c client.Client, ctx context.Context,
@@ -291,7 +293,7 @@ func (r *AKODeploymentConfigReconciler) reconcileAviInfraSettingDelete(
 	log.Info("Start reconciling AVIInfraSetting Delete")
 
 	// Get the list of clusters managed by the AKODeploymentConfig
-	clusters, err := phases.ListAkoDeplymentConfigDeployedClusters(ctx, r.Client, adc)
+	clusters, err := ako_operator.ListAkoDeploymentConfigSelectClusters(ctx, r.Client, log, adc)
 	if err != nil {
 		log.Error(err, "Fail to list clusters deployed by current AKODeploymentConfig")
 		return res, err

controllers/akodeploymentconfig/akodeploymentconfig_controller_avi_phase.go

@@ -37,7 +37,6 @@ func (r *AKODeploymentConfigReconciler) reconcileClusters(
 
 	return phases.ReconcileClustersPhases(ctx, r.Client, log, obj,
 		[]phases.ReconcileClusterPhase{
-			r.applyClusterLabel,
 			r.addClusterFinalizer,
 			r.ClusterReconciler.ReconcileAddonSecret,
 		},
@@ -63,7 +62,6 @@ func (r *AKODeploymentConfigReconciler) reconcileClustersDelete(
 		// cluster is in normal state, remove the label and finalizer to
 		// stop managing it
 		[]phases.ReconcileClusterPhase{
-			r.removeClusterLabel,
 			r.removeClusterFinalizer,
 			r.ClusterReconciler.ReconcileAddonSecretDelete,
 		},
@@ -74,49 +72,6 @@ func (r *AKODeploymentConfigReconciler) reconcileClustersDelete(
 	)
 }
 
-// applyClusterLabel is a reconcileClusterPhase. It applies the AVI label to a
-// Cluster
-func (r *AKODeploymentConfigReconciler) applyClusterLabel(
-	_ context.Context,
-	log logr.Logger,
-	cluster *clusterv1.Cluster,
-	obj *akoov1alpha1.AKODeploymentConfig,
-) (ctrl.Result, error) {
-	if cluster.Labels == nil {
-		cluster.Labels = make(map[string]string)
-	}
-	if _, exists := cluster.Labels[akoov1alpha1.AviClusterLabel]; !exists {
-		log.Info("Adding label to cluster", "label", akoov1alpha1.AviClusterLabel)
-	} else {
-		log.Info("Label already applied to cluster", "label", akoov1alpha1.AviClusterLabel)
-	}
-	// When the cluster is selected by this AKODeploymentConfig, but its is not install-ako-for-all,
-	// this indicates that the cluster is managed by a customized ADC and the default ADC shall be skipped.
-	if obj.Name != akoov1alpha1.WorkloadClusterAkoDeploymentConfig {
-		cluster.Labels[akoov1alpha1.AviClusterSelectedLabel] = ""
-	}
-	// Always set avi label on managed cluster
-	cluster.Labels[akoov1alpha1.AviClusterLabel] = ""
-	return ctrl.Result{}, nil
-}
-
-// removeClusterLabel is a reconcileClusterPhase. It removes the AVI label from a
-// Cluster
-func (r *AKODeploymentConfigReconciler) removeClusterLabel(
-	_ context.Context,
-	log logr.Logger,
-	cluster *clusterv1.Cluster,
-	_ *akoov1alpha1.AKODeploymentConfig,
-) (ctrl.Result, error) {
-	if _, exists := cluster.Labels[akoov1alpha1.AviClusterLabel]; exists {
-		log.Info("Removing label from cluster", "label", akoov1alpha1.AviClusterLabel)
-	}
-	// Always deletes avi label on managed cluster
-	delete(cluster.Labels, akoov1alpha1.AviClusterLabel)
-	delete(cluster.Labels, akoov1alpha1.AviClusterSelectedLabel)
-	return ctrl.Result{}, nil
-}
-
 // addClusterFinalizer is a reconcileClusterPhase. It adds the AVI
 // finalizer to a Cluster.
 func (r *AKODeploymentConfigReconciler) addClusterFinalizer(

controllers/akodeploymentconfig/akodeploymentconfig_controller_cluster_phase.go

@@ -759,7 +759,7 @@ func intgTestAkoDeploymentConfigController() {
 						ensureClusterAviLabelMatchExpectation(client.ObjectKey{
 							Name:      cluster.Name,
 							Namespace: cluster.Namespace,
-						}, akoov1alpha1.AviClusterSelectedLabel, true)
+						}, akoov1alpha1.AviClusterLabel, true)
 					})
 
 					When("no longer selected by a customized ADC", func() {
@@ -775,11 +775,11 @@ func intgTestAkoDeploymentConfigController() {
 							}, "test", false)
 						})
 
-						It("should drop the skip-default-adc label (AviClusterSelectedLabel)", func() {
+						It("should drop the AviClusterLabel)", func() {
 							ensureClusterAviLabelMatchExpectation(client.ObjectKey{
 								Name:      cluster.Name,
 								Namespace: cluster.Namespace,
-							}, akoov1alpha1.AviClusterSelectedLabel, false)
+							}, akoov1alpha1.AviClusterLabel, false)
 						})
 					})
 				})

controllers/akodeploymentconfig/akodeploymentconfig_controller_intg_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/go-logr/logr"
 
 	"github.com/pkg/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util"
@@ -18,7 +17,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	akoov1alpha1 "github.com/vmware-samples/load-balancer-operator-for-kubernetes/api/v1alpha1"
-	handlers "github.com/vmware-samples/load-balancer-operator-for-kubernetes/pkg/handlers"
+	ako_operator "github.com/vmware-samples/load-balancer-operator-for-kubernetes/pkg/ako-operator"
 )
 
 // ReconcilePhase defines a function that reconciles one aspect of
@@ -67,7 +66,7 @@ func ReconcileClustersPhases(
 	res := ctrl.Result{}
 
 	// Get the list of clusters managed by the AKODeploymentConfig
-	clusters, err := ListAkoDeplymentConfigDeployedClusters(ctx, client, obj)
+	clusters, err := ako_operator.ListAkoDeploymentConfigSelectClusters(ctx, client, log, obj)
 	if err != nil {
 		log.Error(err, "Fail to list clusters deployed by current AKODeploymentConfig")
 		return res, err
@@ -92,6 +91,9 @@ func ReconcileClustersPhases(
 				cluster.GroupVersionKind(), cluster.Namespace+"/"+cluster.Name)
 		}
 
+		// update cluster avi label before run any phase functions
+		ako_operator.ApplyClusterLabel(log, &cluster, obj)
+
 		phases := normalPhases
 		if !cluster.GetDeletionTimestamp().IsZero() {
 			phases = deletePhases
@@ -127,40 +129,3 @@ func ReconcileClustersPhases(
 
 	return res, kerrors.NewAggregate(allErrs)
 }
-
-// ListAkoDeplymentConfigDeployedClusters list all clusters enabled current akodeploymentconfig
-func ListAkoDeplymentConfigDeployedClusters(ctx context.Context, kclient client.Client, obj *akoov1alpha1.AKODeploymentConfig) (*clusterv1.ClusterList, error) {
-	selector, err := metav1.LabelSelectorAsSelector(&obj.Spec.ClusterSelector)
-	if err != nil {
-		return nil, err
-	}
-	listOptions := []client.ListOption{
-		client.MatchingLabelsSelector{Selector: selector},
-	}
-	var clusters clusterv1.ClusterList
-	if err := kclient.List(ctx, &clusters, listOptions...); err != nil {
-		return nil, err
-	}
-
-	var newItems []clusterv1.Cluster
-	for _, c := range clusters.Items {
-		if !handlers.SkipCluster(&c) {
-			_, selected := c.Labels[akoov1alpha1.AviClusterSelectedLabel]
-			// when cluster selected by non-default AKODeploymentConfig,
-			// skip default select all AKODeploymentConfig
-			if selector.Empty() && selected {
-				continue
-			}
-			// management cluster can't be selected by other AKODeploymentConfig
-			// instead of management cluster AKODeploymentConfig
-			if c.Namespace == akoov1alpha1.TKGSystemNamespace &&
-				obj.Name != akoov1alpha1.ManagementClusterAkoDeploymentConfig {
-				continue
-			}
-			newItems = append(newItems, c)
-		}
-	}
-	clusters.Items = newItems
-
-	return &clusters, nil
-}

controllers/akodeploymentconfig/phases/phases.go

@@ -4,17 +4,22 @@
 package phases
 
 import (
+	ako_operator "github.com/vmware-samples/load-balancer-operator-for-kubernetes/pkg/ako-operator"
+
+	"github.com/go-logr/logr"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	akoov1alpha1 "github.com/vmware-samples/load-balancer-operator-for-kubernetes/api/v1alpha1"
 	"github.com/vmware-samples/load-balancer-operator-for-kubernetes/pkg/test/builder"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	ctrl "sigs.k8s.io/controller-runtime"
 )
 
 func ReconcilePhaseUnitTest() {
 	var (
 		err                 error
+		log                 logr.Logger
 		ctx                 *builder.IntegrationTestContext
 		akoDeploymentConfig *akoov1alpha1.AKODeploymentConfig
 	)
@@ -47,6 +52,7 @@ func ReconcilePhaseUnitTest() {
 			},
 		}
 		ctx = suite.NewIntegrationTestContext()
+		log = ctrl.Log.WithName("controllers").WithName("AKODeploymentConfig")
 	})
 
 	Context("Should be able to list all workload clusters", func() {
@@ -59,7 +65,7 @@ func ReconcilePhaseUnitTest() {
 					Name:      "test-cluster",
 					Namespace: "default",
 					Labels: map[string]string{
-						akoov1alpha1.AviClusterLabel: "",
+						akoov1alpha1.AviClusterLabel: "test-ako-deployment-config",
 						"test":                       "test",
 					},
 				},
@@ -75,7 +81,7 @@ func ReconcilePhaseUnitTest() {
 		})
 
 		It("list all selected workload clusters", func() {
-			clusterList, err := ListAkoDeplymentConfigDeployedClusters(ctx.Context, ctx.Client, akoDeploymentConfig)
+			clusterList, err := ako_operator.ListAkoDeploymentConfigSelectClusters(ctx.Context, ctx.Client, log, akoDeploymentConfig)
 			Expect(err).ShouldNot(HaveOccurred())
 			Expect(len(clusterList.Items)).To(Equal(1))
 			Expect(clusterList.Items[0].Name).To(Equal("test-cluster"))

controllers/akodeploymentconfig/phases/phases_unit_test.go

@@ -18,8 +18,6 @@ import (
 	akoov1alpha1 "github.com/vmware-samples/load-balancer-operator-for-kubernetes/api/v1alpha1"
 	"github.com/vmware-samples/load-balancer-operator-for-kubernetes/pkg/haprovider"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -76,6 +74,8 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		}
 	}()
 
+	log = log.WithValues("Cluster", cluster.Namespace+"/"+cluster.Name)
+
 	if ako_operator.IsHAProvider() {
 		log.Info("AVI is control plane HA provider")
 		r.Haprovider = haprovider.NewProvider(r.Client, r.Log)
@@ -88,89 +88,22 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		}
 	}
 
-	log = log.WithValues("Cluster", cluster.Namespace+"/"+cluster.Name)
-
-	if _, exist := cluster.Labels[akoov1alpha1.AviClusterLabel]; !exist {
-		log.Info("Cluster doesn't have AVI enabled, skip Cluster reconciling")
-		return res, nil
-	}
-
-	log.Info("Cluster has AVI enabled, start Cluster reconciling")
-	// Getting all akodeploymentconfigs
-	var akoDeploymentConfigs akoov1alpha1.AKODeploymentConfigList
-	if err := r.Client.List(ctx, &akoDeploymentConfigs); err != nil {
+	akoDeploymentConfig, err := ako_operator.GetAKODeploymentConfigForCluster(ctx, r.Client, log, cluster)
+	if err != nil {
+		log.Error(err, "failed to get cluster matched akodeploymentconfig")
 		return res, err
 	}
 
-	// Matches current cluster with all the akoDeploymentConfigs
-	clusterLabels := cluster.GetLabels()
-	matchedAkoDeploymentConfigs := make([]akoov1alpha1.AKODeploymentConfig, 0)
-	for _, akoDeploymentConfig := range akoDeploymentConfigs.Items {
-		if selector, err := metav1.LabelSelectorAsSelector(&akoDeploymentConfig.Spec.ClusterSelector); err != nil {
-			log.Error(err, "Failed to convert label sector to selector when matching ", "cluster", cluster.Name, " with ", akoDeploymentConfig.Name)
-		} else if selector.Matches(labels.Set(clusterLabels)) {
-			log.Info("Cluster ", cluster.Name, " is selected by", "Akodeploymentconfig", (akoDeploymentConfig.Namespace + "/" + akoDeploymentConfig.Name))
-			matchedAkoDeploymentConfigs = append(matchedAkoDeploymentConfigs, akoDeploymentConfig)
-		}
-	}
-
-	// When the cluster is not selected or only selected by the default ADC, a.k.a. install-ako-for-all
-	// We drop its skip-default-adc label, allowing cluster to use default ADC's config.
-	// It happens when a cluster is no longer selected by a customized ADC
-	if len(matchedAkoDeploymentConfigs) == 0 ||
-		(len(matchedAkoDeploymentConfigs) == 1 && matchedAkoDeploymentConfigs[0].Name == akoov1alpha1.WorkloadClusterAkoDeploymentConfig) {
-		delete(cluster.Labels, akoov1alpha1.AviClusterSelectedLabel)
-	}
-
-	// If the cluster is selected by some AKODeploymentConfig objects, skip removing the finalizer
-	if len(matchedAkoDeploymentConfigs) > 0 {
+	if akoDeploymentConfig == nil {
+		// Removing finalizer if current cluster can't be selected by any akoDeploymentConfig
+		log.Info("Not find cluster matched akodeploymentconfig, removing finalizer", "finalizer", akoov1alpha1.ClusterFinalizer)
+		ctrlutil.RemoveFinalizer(cluster, akoov1alpha1.ClusterFinalizer)
+		// Removing avi label after deleting all the resources
+		delete(cluster.Labels, akoov1alpha1.AviClusterLabel)
+		log.Info("Cluster doesn't have AVI enabled, skip Cluster reconciling")
 		return res, nil
 	}
 
-	// Removing finalizer if current cluster can't be selected by any akoDeploymentConfig
-	log.Info("Removing finalizer", "finalizer", akoov1alpha1.ClusterFinalizer)
-	ctrlutil.RemoveFinalizer(cluster, akoov1alpha1.ClusterFinalizer)
-
-	// Removing add on secret and its associated resources for a AKO
-	if _, err := r.deleteAddonSecret(ctx, log, cluster); err != nil {
-		log.Error(err, "Failed to remove secret", "secret", cluster.Name)
-		return res, err
-	}
-
-	// Removing avi label after deleting all the resources
-	delete(cluster.Labels, akoov1alpha1.AviClusterLabel)
-
+	log.Info("Cluster has AVI enabled")
 	return res, nil
 }
-
-// deleteAddonSecret delete cluster related add on secret
-func (r *ClusterReconciler) deleteAddonSecret(
-	ctx context.Context,
-	log logr.Logger,
-	cluster *clusterv1.Cluster,
-) (ctrl.Result, error) {
-	log.Info("Starts reconciling add on secret deletion")
-	res := ctrl.Result{}
-	secret := &corev1.Secret{}
-	if err := r.Get(ctx, client.ObjectKey{
-		Name:      r.akoAddonSecretName(cluster),
-		Namespace: cluster.Namespace,
-	}, secret); err != nil {
-		if apierrors.IsNotFound(err) {
-			log.V(3).Info("add on secret is already deleted")
-			return res, nil
-		}
-		log.Error(err, "Failed to get add on secret, requeue")
-		return res, err
-	}
-
-	if err := r.Delete(ctx, secret); err != nil {
-		log.Error(err, "Failed to delete add on secret, requeue")
-		return res, err
-	}
-	return res, nil
-}
-
-func (r *ClusterReconciler) akoAddonSecretName(cluster *clusterv1.Cluster) string {
-	return cluster.Name + "-load-balancer-and-ingress-service-addon"
-}