refactor: introduce 'byClusterName' secret index to speedup cluster s…

…erver URL lookup (argoproj#8133) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
cyrilix · Jan 30, 2022 · a444804 · a444804
1 parent 47c89ee
commit a444804
Show file tree

Hide file tree

Showing 7 changed files with 135 additions and 39 deletions.
diff --git a/util/argo/argo.go b/util/argo/argo.go
@@ -607,16 +607,10 @@ func GetPermittedRepos(proj *argoappv1.AppProject, repos []*argoappv1.Repository
 }
 
 func getDestinationServer(ctx context.Context, db db.ArgoDB, clusterName string) (string, error) {
-	clusterList, err := db.ListClusters(ctx)
+	servers, err := db.GetClusterServersByName(ctx, clusterName)
 	if err != nil {
 		return "", err
 	}
-	var servers []string
-	for _, c := range clusterList.Items {
-		if c.Name == clusterName {
-			servers = append(servers, c.Server)
-		}
-	}
 	if len(servers) > 1 {
 		return "", fmt.Errorf("there are %d clusters with the same name: %v", len(servers), servers)
 	} else if len(servers) == 0 {

diff --git a/util/argo/argo_test.go b/util/argo/argo_test.go
@@ -680,14 +680,7 @@ func TestValidateDestination(t *testing.T) {
 		}
 
 		db := &dbmocks.ArgoDB{}
-		db.On("ListClusters", context.Background()).Return(&argoappv1.ClusterList{
-			Items: []argoappv1.Cluster{
-				{
-					Name:   "minikube",
-					Server: "https://127.0.0.1:6443",
-				},
-			},
-		}, nil)
+		db.On("GetClusterServersByName", context.Background(), "minikube").Return([]string{"https://127.0.0.1:6443"}, nil)
 
 		appCond := ValidateDestination(context.Background(), &dest, db)
 		assert.Nil(t, appCond)
@@ -707,13 +700,13 @@ func TestValidateDestination(t *testing.T) {
 		assert.False(t, dest.IsServerInferred())
 	})
 
-	t.Run("List clusters fails", func(t *testing.T) {
+	t.Run("GetClusterServersByName fails", func(t *testing.T) {
 		dest := argoappv1.ApplicationDestination{
 			Name: "minikube",
 		}
 
 		db := &dbmocks.ArgoDB{}
-		db.On("ListClusters", context.Background()).Return(nil, fmt.Errorf("an error occurred"))
+		db.On("GetClusterServersByName", context.Background(), mock.Anything).Return(nil, fmt.Errorf("an error occurred"))
 
 		err := ValidateDestination(context.Background(), &dest, db)
 		assert.Equal(t, "unable to find destination server: an error occurred", err.Error())
@@ -726,14 +719,7 @@ func TestValidateDestination(t *testing.T) {
 		}
 
 		db := &dbmocks.ArgoDB{}
-		db.On("ListClusters", context.Background()).Return(&argoappv1.ClusterList{
-			Items: []argoappv1.Cluster{
-				{
-					Name:   "dind",
-					Server: "https://127.0.0.1:6443",
-				},
-			},
-		}, nil)
+		db.On("GetClusterServersByName", context.Background(), "minikube").Return(nil, nil)
 
 		err := ValidateDestination(context.Background(), &dest, db)
 		assert.Equal(t, "unable to find destination server: there are no clusters with this name: minikube", err.Error())
@@ -746,18 +732,7 @@ func TestValidateDestination(t *testing.T) {
 		}
 
 		db := &dbmocks.ArgoDB{}
-		db.On("ListClusters", context.Background()).Return(&argoappv1.ClusterList{
-			Items: []argoappv1.Cluster{
-				{
-					Name:   "dind",
-					Server: "https://127.0.0.1:2443",
-				},
-				{
-					Name:   "dind",
-					Server: "https://127.0.0.1:8443",
-				},
-			},
-		}, nil)
+		db.On("GetClusterServersByName", context.Background(), "dind").Return([]string{"https://127.0.0.1:2443", "https://127.0.0.1:8443"}, nil)
 
 		err := ValidateDestination(context.Background(), &dest, db)
 		assert.Equal(t, "unable to find destination server: there are 2 clusters with the same name: [https://127.0.0.1:2443 https://127.0.0.1:8443]", err.Error())

diff --git a/util/db/cluster.go b/util/db/cluster.go
@@ -232,6 +232,34 @@ func (db *db) GetProjectClusters(ctx context.Context, project string) ([]*appv1.
 	return res, nil
 }
 
+func (db *db) GetClusterServersByName(ctx context.Context, name string) ([]string, error) {
+	informer, err := db.settingsMgr.GetSecretsInformer()
+	if err != nil {
+		return nil, err
+	}
+
+	// if local cluster name is not overridden and specified name is local cluster name, return local cluster server
+	localClusterSecrets, err := informer.GetIndexer().ByIndex(settings.ByClusterURLIndexer, appv1.KubernetesInternalAPIServerAddr)
+	if err != nil {
+		return nil, err
+	}
+
+	if len(localClusterSecrets) == 0 && db.getLocalCluster().Name == name {
+		return []string{appv1.KubernetesInternalAPIServerAddr}, nil
+	}
+
+	secrets, err := informer.GetIndexer().ByIndex(settings.ByClusterNameIndexer, name)
+	if err != nil {
+		return nil, err
+	}
+	var res []string
+	for i := range secrets {
+		s := secrets[i].(*apiv1.Secret)
+		res = append(res, strings.TrimRight(string(s.Data["server"]), "/"))
+	}
+	return res, nil
+}
+
 // UpdateCluster updates a cluster
 func (db *db) UpdateCluster(ctx context.Context, c *appv1.Cluster) (*appv1.Cluster, error) {
 	clusterSecret, err := db.getClusterSecret(c.Server)

diff --git a/util/db/db.go b/util/db/db.go
@@ -27,8 +27,10 @@ type ArgoDB interface {
 		handleAddEvent func(cluster *appv1.Cluster),
 		handleModEvent func(oldCluster *appv1.Cluster, newCluster *appv1.Cluster),
 		handleDeleteEvent func(clusterServer string)) error
-	// GetCluster get returns a cluster by given server url
+	// GetCluster returns a cluster by given server url
 	GetCluster(ctx context.Context, server string) (*appv1.Cluster, error)
+	// GetClusterServersByName returns a cluster server urls by given cluster name
+	GetClusterServersByName(ctx context.Context, name string) ([]string, error)
 	// GetProjectClusters return project scoped clusters by given project name
 	GetProjectClusters(ctx context.Context, project string) ([]*appv1.Cluster, error)
 	// UpdateCluster updates a cluster

diff --git a/util/db/db_test.go b/util/db/db_test.go
@@ -693,3 +693,59 @@ func TestHelmRepositorySecretsTrim(t *testing.T) {
 		assert.Equal(t, tt.expectedSecret, tt.retrievedSecret)
 	}
 }
+
+func TestGetClusterServersByName(t *testing.T) {
+	clientset := getClientset(nil, &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-cluster-secret",
+			Namespace: testNamespace,
+			Labels: map[string]string{
+				common.LabelKeySecretType: common.LabelValueSecretTypeCluster,
+			},
+			Annotations: map[string]string{
+				common.AnnotationKeyManagedBy: common.AnnotationValueManagedByArgoCD,
+			},
+		},
+		Data: map[string][]byte{
+			"name":   []byte("my-cluster-name"),
+			"server": []byte("https://my-cluster-server"),
+			"config": []byte("{}"),
+		},
+	})
+	db := NewDB(testNamespace, settings.NewSettingsManager(context.Background(), clientset, testNamespace), clientset)
+	servers, err := db.GetClusterServersByName(context.Background(), "my-cluster-name")
+	assert.NoError(t, err)
+	assert.ElementsMatch(t, []string{"https://my-cluster-server"}, servers)
+}
+
+func TestGetClusterServersByName_InClusterNotConfigured(t *testing.T) {
+	clientset := getClientset(nil)
+	db := NewDB(testNamespace, settings.NewSettingsManager(context.Background(), clientset, testNamespace), clientset)
+	servers, err := db.GetClusterServersByName(context.Background(), "in-cluster")
+	assert.NoError(t, err)
+	assert.ElementsMatch(t, []string{v1alpha1.KubernetesInternalAPIServerAddr}, servers)
+}
+
+func TestGetClusterServersByName_InClusterConfigured(t *testing.T) {
+	clientset := getClientset(nil, &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-cluster-secret",
+			Namespace: testNamespace,
+			Labels: map[string]string{
+				common.LabelKeySecretType: common.LabelValueSecretTypeCluster,
+			},
+			Annotations: map[string]string{
+				common.AnnotationKeyManagedBy: common.AnnotationValueManagedByArgoCD,
+			},
+		},
+		Data: map[string][]byte{
+			"name":   []byte("in-cluster-renamed"),
+			"server": []byte(v1alpha1.KubernetesInternalAPIServerAddr),
+			"config": []byte("{}"),
+		},
+	})
+	db := NewDB(testNamespace, settings.NewSettingsManager(context.Background(), clientset, testNamespace), clientset)
+	servers, err := db.GetClusterServersByName(context.Background(), "in-cluster-renamed")
+	assert.NoError(t, err)
+	assert.ElementsMatch(t, []string{v1alpha1.KubernetesInternalAPIServerAddr}, servers)
+}
diff --git a/util/db/mocks/ArgoDB.go b/util/db/mocks/ArgoDB.go
diff --git a/util/settings/settings.go b/util/settings/settings.go
@@ -163,6 +163,23 @@ var (
 		}
 		return nil, nil
 	}
+	ByClusterNameIndexer     = "byClusterName"
+	byClusterNameIndexerFunc = func(obj interface{}) ([]string, error) {
+		s, ok := obj.(*apiv1.Secret)
+		if !ok {
+			return nil, nil
+		}
+		if s.Labels == nil || s.Labels[common.LabelKeySecretType] != common.LabelValueSecretTypeCluster {
+			return nil, nil
+		}
+		if s.Data == nil {
+			return nil, nil
+		}
+		if name, ok := s.Data["name"]; ok {
+			return []string{string(name)}, nil
+		}
+		return nil, nil
+	}
 	ByProjectClusterIndexer = "byProjectCluster"
 	ByProjectRepoIndexer    = "byProjectRepo"
 	byProjectIndexerFunc    = func(secretType string) func(obj interface{}) ([]string, error) {
@@ -1044,6 +1061,7 @@ func (mgr *SettingsManager) initialize(ctx context.Context) error {
 	indexers := cache.Indexers{
 		cache.NamespaceIndex:    cache.MetaNamespaceIndexFunc,
 		ByClusterURLIndexer:     byClusterURLIndexerFunc,
+		ByClusterNameIndexer:    byClusterNameIndexerFunc,
 		ByProjectClusterIndexer: byProjectIndexerFunc(common.LabelValueSecretTypeCluster),
 		ByProjectRepoIndexer:    byProjectIndexerFunc(common.LabelValueSecretTypeRepository),
 	}