Refactor, add tests

Signed-off-by: Shakti <[email protected]>

cmd/rollouts-controller/main.go

@@ -2,7 +2,6 @@ package main
 
 import (
 	"fmt"
-	"github.com/argoproj/argo-rollouts/rollout/trafficrouting/istio"
 	"os"
 	"time"
 
@@ -132,8 +131,12 @@ func newCommand() *cobra.Command {
 			// a single namespace (i.e. rollouts-controller --namespace foo).
 			clusterDynamicInformerFactory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(dynamicClient, resyncDuration, metav1.NamespaceAll, instanceIDTweakListFunc)
 			// 3. We finally need an istio dynamic informer factory which does not use a tweakListFunc.
-			istioPrimaryCluster := istio.NewPrimaryCluster(kubeClient, dynamicClient, namespace)
-			istioDynamicInformerFactory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(istioPrimaryCluster.GetDynamicClient(), resyncDuration, namespace, nil)
+			//istioPrimaryCluster := istio.NewPrimaryCluster(kubeClient, dynamicClient, namespace)
+			_, istioPrimaryDynamicClient := istioutil.GetPrimaryClusterDynamicClient(kubeClient, namespace)
+			if istioPrimaryDynamicClient == nil {
+				istioPrimaryDynamicClient = dynamicClient
+			}
+			istioDynamicInformerFactory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(istioPrimaryDynamicClient, resyncDuration, namespace, nil)
 
 			k8sRequestProvider := &metrics.K8sRequestsCountProvider{}
 			kubeclientmetrics.AddMetricsTransportWrapper(config, k8sRequestProvider.IncKubernetesRequest)
@@ -154,7 +157,7 @@ func newCommand() *cobra.Command {
 				tolerantinformer.NewTolerantAnalysisRunInformer(dynamicInformerFactory),
 				tolerantinformer.NewTolerantAnalysisTemplateInformer(dynamicInformerFactory),
 				tolerantinformer.NewTolerantClusterAnalysisTemplateInformer(clusterDynamicInformerFactory),
-				istioPrimaryCluster,
+				istioPrimaryDynamicClient,
 				istioDynamicInformerFactory.ForResource(istioutil.GetIstioVirtualServiceGVR()).Informer(),
 				istioDynamicInformerFactory.ForResource(istioutil.GetIstioDestinationRuleGVR()).Informer(),
 				resyncDuration,
@@ -173,7 +176,7 @@ func newCommand() *cobra.Command {
 			jobInformerFactory.Start(stopCh)
 
 			// Check if Istio installed on cluster before starting dynamicInformerFactory
-			if istioutil.DoesIstioExist(istioPrimaryCluster.GetDynamicClient(), namespace) {
+			if istioutil.DoesIstioExist(istioPrimaryDynamicClient, namespace) {
 				istioDynamicInformerFactory.Start(stopCh)
 			}
 

controller/controller.go

@@ -2,7 +2,6 @@ package controller
 
 import (
 	"fmt"
-	"github.com/argoproj/argo-rollouts/rollout/trafficrouting/istio"
 	"time"
 
 	"github.com/argoproj/argo-rollouts/utils/queue"
@@ -109,7 +108,7 @@ func NewManager(
 	analysisRunInformer informers.AnalysisRunInformer,
 	analysisTemplateInformer informers.AnalysisTemplateInformer,
 	clusterAnalysisTemplateInformer informers.ClusterAnalysisTemplateInformer,
-	istioPrimaryCluster istio.PrimaryCluster,
+	istioPrimaryDynamicClient dynamic.Interface,
 	istioVirtualServiceInformer cache.SharedIndexInformer,
 	istioDestinationRuleInformer cache.SharedIndexInformer,
 	resyncPeriod time.Duration,
@@ -155,7 +154,7 @@ func NewManager(
 		AnalysisRunInformer:             analysisRunInformer,
 		AnalysisTemplateInformer:        analysisTemplateInformer,
 		ClusterAnalysisTemplateInformer: clusterAnalysisTemplateInformer,
-		IstioPrimaryCluster: istioPrimaryCluster,
+		IstioPrimaryDynamicClient:       istioPrimaryDynamicClient,
 		IstioVirtualServiceInformer:     istioVirtualServiceInformer,
 		IstioDestinationRuleInformer:    istioDestinationRuleInformer,
 		ReplicaSetInformer:              replicaSetInformer,

rollout/controller.go

@@ -96,7 +96,7 @@ type ControllerConfig struct {
 	ServicesInformer                coreinformers.ServiceInformer
 	IngressInformer                 extensionsinformers.IngressInformer
 	RolloutsInformer                informers.RolloutInformer
-	IstioPrimaryCluster istio.PrimaryCluster
+	IstioPrimaryDynamicClient       dynamic.Interface
 	IstioVirtualServiceInformer     cache.SharedIndexInformer
 	IstioDestinationRuleInformer    cache.SharedIndexInformer
 	ResyncPeriod                    time.Duration
@@ -203,9 +203,8 @@ func NewController(cfg ControllerConfig) *Controller {
 	}
 
 	controller.IstioController = istio.NewIstioController(istio.IstioControllerConfig{
-		PrimaryCluster: cfg.IstioPrimaryCluster,
 		ArgoprojClientSet:       cfg.ArgoProjClientset,
-		DynamicClientSet:        cfg.IstioPrimaryCluster.GetDynamicClient(),
+		DynamicClientSet:        cfg.IstioPrimaryDynamicClient,
 		EnqueueRollout:          controller.enqueueRollout,
 		RolloutsInformer:        cfg.RolloutsInformer,
 		VirtualServiceInformer:  cfg.IstioVirtualServiceInformer,

rollout/controller_test.go

@@ -511,6 +511,7 @@ func (f *fixture) newController(resync resyncFunc) (*Controller, informers.Share
 		ServicesInformer:                k8sI.Core().V1().Services(),
 		IngressInformer:                 k8sI.Extensions().V1beta1().Ingresses(),
 		RolloutsInformer:                i.Argoproj().V1alpha1().Rollouts(),
+		IstioPrimaryDynamicClient:       dynamicClient,
 		IstioVirtualServiceInformer:     istioVirtualServiceInformer,
 		IstioDestinationRuleInformer:    istioDestinationRuleInformer,
 		ResyncPeriod:                    resync(),

rollout/trafficrouting.go

@@ -34,9 +34,9 @@ func (c *Controller) NewTrafficRoutingReconciler(roCtx *rolloutContext) (Traffic
 	}
 	if rollout.Spec.Strategy.Canary.TrafficRouting.Istio != nil {
 		if c.IstioController.VirtualServiceInformer.HasSynced() {
-			return istio.NewReconciler(rollout, c.IstioController.IstioControllerConfig.PrimaryCluster.GetDynamicClient(), c.recorder, c.IstioController.VirtualServiceLister, c.IstioController.DestinationRuleLister), nil
+			return istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, c.IstioController.VirtualServiceLister, c.IstioController.DestinationRuleLister), nil
 		} else {
-			return istio.NewReconciler(rollout, c.IstioController.IstioControllerConfig.PrimaryCluster.GetDynamicClient(), c.recorder, nil, nil), nil
+			return istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, nil, nil), nil
 		}
 	}
 	if rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil {

rollout/trafficrouting/istio/controller.go

@@ -41,7 +41,6 @@ const (
 )
 
 type IstioControllerConfig struct {
-	PrimaryCluster
 	ArgoprojClientSet       roclientset.Interface
 	DynamicClientSet        dynamic.Interface
 	EnqueueRollout          func(ro interface{})

rollout/trafficrouting/istio/multicluster.go → utils/istio/multicluster.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+
 	log "github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -18,61 +19,30 @@ const (
 	PrimaryClusterSecretLabel = "istio.argoproj.io/primary-cluster"
 )
 
-type PrimaryCluster interface {
-	GetKubeClient() kubernetes.Interface
-	GetDynamicClient() dynamic.Interface
-}
-
-type primaryCluster struct {
-	namespace string
-	secret *corev1.Secret
-	kubeClient kubernetes.Interface
-	dynamicClient dynamic.Interface
-}
-
-func NewPrimaryCluster(kubeClient kubernetes.Interface, dynamicClient dynamic.Interface, namespace string) PrimaryCluster {
-	pc := &primaryCluster{namespace: namespace, kubeClient: kubeClient, dynamicClient: dynamicClient}
-
+func GetPrimaryClusterDynamicClient(kubeClient kubernetes.Interface, namespace string) (string, dynamic.Interface) {
 	primaryClusterSecret := getPrimaryClusterSecret(kubeClient, namespace)
 	if primaryClusterSecret != nil {
-		pc.secret = primaryClusterSecret
-		clientConfig, err := getKubeClientConfig(primaryClusterSecret)
+		clusterId, clientConfig, err := getKubeClientConfig(primaryClusterSecret)
 		if err != nil {
-			// TODO log the error
-			return pc
+			return clusterId, nil
 		}
 
 		config, err := clientConfig.ClientConfig()
 		if err != nil {
-			// TODO log the error
-			return pc
-		}
-
-		kubeClient, err := kubernetes.NewForConfig(config)
-		if err != nil {
-			// TODO log the error
-			return pc
+			log.Errorf("Error fetching primary ClientConfig: %v", err)
+			return clusterId, nil
 		}
 
 		dynamicClient, err := dynamic.NewForConfig(config)
 		if err != nil {
-			// TODO log the error
-			return pc
+			log.Errorf("Error building dynamic client from config: %v", err)
+			return clusterId, nil
 		}
 
-		pc.kubeClient = kubeClient
-		pc.dynamicClient = dynamicClient
+		return clusterId, dynamicClient
 	}
 
-	return pc
-}
-
-func (pc *primaryCluster) GetKubeClient() kubernetes.Interface {
-	return pc.kubeClient
-}
-
-func (pc *primaryCluster) GetDynamicClient() dynamic.Interface {
-	return pc.dynamicClient
+	return "", nil
 }
 
 func getPrimaryClusterSecret(kubeClient kubernetes.Interface, namespace string) *corev1.Secret {
@@ -93,17 +63,17 @@ func getPrimaryClusterSecret(kubeClient kubernetes.Interface, namespace string)
 	return nil
 }
 
-func getKubeClientConfig(secret *corev1.Secret) (clientcmd.ClientConfig, error) {
+func getKubeClientConfig(secret *corev1.Secret) (string, clientcmd.ClientConfig, error) {
 	for clusterId, kubeConfig := range secret.Data {
 		primaryClusterConfig, err := buildKubeClientConfig(kubeConfig)
 		if err != nil {
-			// TODO log error
-			continue
+			log.Errorf("Error building kubeconfig for primary cluster %s: %v", clusterId, err)
+			return clusterId, nil, err
 		}
 		log.Infof("Istio primary/config cluster is %s", clusterId)
-		return primaryClusterConfig, err
+		return clusterId, primaryClusterConfig, err
 	}
-	return nil, nil
+	return "", nil, nil
 }
 
 func buildKubeClientConfig(kubeConfig []byte) (clientcmd.ClientConfig, error) {

utils/istio/multicluster_test.go

@@ -0,0 +1,88 @@
+package istio
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/fake"
+)
+
+func TestGetPrimaryClusterDynamicClient(t *testing.T) {
+	testCases := []struct {
+		name              string
+		namespace         string
+		existingSecrets   []*v1.Secret
+		expectedClusterId string
+	}{
+		{
+			"TestNoPrimaryClusterSecret",
+			metav1.NamespaceAll,
+			nil,
+			"",
+		},
+		{
+			"TestPrimaryClusterSingleSecret",
+			metav1.NamespaceAll,
+			[]*v1.Secret{
+				makeSecret("secret0", "namespace0", "primary0", []byte("kubeconfig0-0")),
+			},
+			"primary0",
+		},
+		{
+			"TestPrimaryClusterMultipleSecrets",
+			metav1.NamespaceAll,
+			[]*v1.Secret{
+				makeSecret("secret0", "namespace0", "primary0", []byte("kubeconfig0-0")),
+				makeSecret("secret1", "namespace1", "primary1", []byte("kubeconfig1-1")),
+			},
+			"primary0",
+		},
+		{
+			"TestPrimaryClusterNoSecretInNamespaceForNamespacedController",
+			"argo-rollout-ns",
+			[]*v1.Secret{
+				makeSecret("secret0", "namespace0", "primary0", []byte("kubeconfig0-0")),
+			},
+			"",
+		},
+		{
+			"TestPrimaryClusterSingleSecretInNamespaceForNamespacedController",
+			"argo-rollout-ns",
+			[]*v1.Secret{
+				makeSecret("secret0", "argo-rollout-ns", "primary0", []byte("kubeconfig0-0")),
+			},
+			"primary0",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			var existingObjs []runtime.Object
+			for _, s := range tc.existingSecrets {
+				existingObjs = append(existingObjs, s)
+			}
+
+			client := fake.NewSimpleClientset(existingObjs...)
+			clusterId, _ := GetPrimaryClusterDynamicClient(client, tc.namespace)
+			assert.Equal(t, tc.expectedClusterId, clusterId)
+		})
+	}
+}
+
+func makeSecret(secret, namespace, clusterID string, kubeconfig []byte) *v1.Secret {
+	return &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secret,
+			Namespace: namespace,
+			Labels: map[string]string{
+				PrimaryClusterSecretLabel: "true",
+			},
+		},
+		Data: map[string][]byte{
+			clusterID: kubeconfig,
+		},
+	}
+}