fix: Populate destination name when destination server is specified (a…

…rgoproj#21063) * Populate destination name when destination server is specified Signed-off-by: Adrian Aneci <[email protected]> * Lint nit Signed-off-by: Adrian Aneci <[email protected]> * Trigger CI Signed-off-by: Adrian Aneci <[email protected]> --------- Signed-off-by: Adrian Aneci <[email protected]>
crenshaw-dev · Dec 14, 2024 · 030a7be · 030a7be
1 parent 8dbddb1
commit 030a7be
Show file tree

Hide file tree

Showing 11 changed files with 169 additions and 87 deletions.
diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go
@@ -1894,12 +1894,6 @@ func TestValidateGeneratedApplications(t *testing.T) {
 	err := v1alpha1.AddToScheme(scheme)
 	require.NoError(t, err)
 
-	// Valid cluster
-	myCluster := v1alpha1.Cluster{
-		Server: "https://kubernetes.default.svc",
-		Name:   "my-cluster",
-	}
-
 	// Valid project
 	myProject := &v1alpha1.AppProject{
 		ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "namespace"},
@@ -2066,18 +2060,12 @@ func TestValidateGeneratedApplications(t *testing.T) {
 			objects := append([]runtime.Object{}, secret)
 			kubeclientset := kubefake.NewSimpleClientset(objects...)
 
-			argoDBMock := dbmocks.ArgoDB{}
-			argoDBMock.On("GetCluster", mock.Anything, "https://kubernetes.default.svc").Return(&myCluster, nil)
-			argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
-				myCluster,
-			}}, nil)
-
 			r := ApplicationSetReconciler{
 				Client:          client,
 				Scheme:          scheme,
 				Recorder:        record.NewFakeRecorder(1),
 				Generators:      map[string]generators.Generator{},
-				ArgoDB:          &argoDBMock,
+				ArgoDB:          &dbmocks.ArgoDB{},
 				ArgoCDNamespace: "namespace",
 				KubeClientset:   kubeclientset,
 				Metrics:         metrics,
@@ -2159,17 +2147,9 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) {
 	}
 
 	kubeclientset := kubefake.NewSimpleClientset()
-	argoDBMock := dbmocks.ArgoDB{}
 
 	client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &project).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build()
 	metrics := appsetmetrics.NewFakeAppsetMetrics(client)
-	goodCluster := v1alpha1.Cluster{Server: "https://good-cluster", Name: "good-cluster"}
-	badCluster := v1alpha1.Cluster{Server: "https://bad-cluster", Name: "bad-cluster"}
-	argoDBMock.On("GetCluster", mock.Anything, "https://good-cluster").Return(&goodCluster, nil)
-	argoDBMock.On("GetCluster", mock.Anything, "https://bad-cluster").Return(&badCluster, nil)
-	argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
-		goodCluster,
-	}}, nil)
 
 	r := ApplicationSetReconciler{
 		Client:   client,
@@ -2179,7 +2159,7 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) {
 		Generators: map[string]generators.Generator{
 			"List": generators.NewListGenerator(),
 		},
-		ArgoDB:          &argoDBMock,
+		ArgoDB:          &dbmocks.ArgoDB{},
 		KubeClientset:   kubeclientset,
 		Policy:          v1alpha1.ApplicationsSyncPolicySync,
 		ArgoCDNamespace: "argocd",
@@ -2363,7 +2343,6 @@ func TestSetApplicationSetStatusCondition(t *testing.T) {
 	}
 
 	kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-	argoDBMock := dbmocks.ArgoDB{}
 
 	for _, testCase := range testCases {
 		client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&testCase.appset).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).WithStatusSubresource(&testCase.appset).Build()
@@ -2377,7 +2356,7 @@ func TestSetApplicationSetStatusCondition(t *testing.T) {
 			Generators: map[string]generators.Generator{
 				"List": generators.NewListGenerator(),
 			},
-			ArgoDB:        &argoDBMock,
+			ArgoDB:        &dbmocks.ArgoDB{},
 			KubeClientset: kubeclientset,
 			Metrics:       metrics,
 		}
@@ -2433,16 +2412,28 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
 		},
 	}
 
-	kubeclientset := kubefake.NewSimpleClientset()
-	argoDBMock := dbmocks.ArgoDB{}
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-cluster",
+			Namespace: "argocd",
+			Labels: map[string]string{
+				argocommon.LabelKeySecretType: argocommon.LabelValueSecretTypeCluster,
+			},
+		},
+		Data: map[string][]byte{
+			// Since this test requires the cluster to be an invalid destination, we
+			// always return a cluster named 'my-cluster2' (different from app 'my-cluster', above)
+			"name":   []byte("good-cluster"),
+			"server": []byte("https://good-cluster"),
+			"config": []byte("{\"username\":\"foo\",\"password\":\"foo\"}"),
+		},
+	}
+
+	objects := append([]runtime.Object{}, secret)
+	kubeclientset := kubefake.NewSimpleClientset(objects...)
 
 	client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &defaultProject).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build()
 	metrics := appsetmetrics.NewFakeAppsetMetrics(client)
-	goodCluster := v1alpha1.Cluster{Server: "https://good-cluster", Name: "good-cluster"}
-	argoDBMock.On("GetCluster", mock.Anything, "https://good-cluster").Return(&goodCluster, nil)
-	argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
-		goodCluster,
-	}}, nil)
 
 	r := ApplicationSetReconciler{
 		Client:   client,
@@ -2452,7 +2443,7 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
 		Generators: map[string]generators.Generator{
 			"List": generators.NewListGenerator(),
 		},
-		ArgoDB:               &argoDBMock,
+		ArgoDB:               &dbmocks.ArgoDB{},
 		ArgoCDNamespace:      "argocd",
 		KubeClientset:        kubeclientset,
 		Policy:               v1alpha1.ApplicationsSyncPolicySync,
@@ -2595,16 +2586,28 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
 		},
 	}
 
-	kubeclientset := kubefake.NewSimpleClientset()
-	argoDBMock := dbmocks.ArgoDB{}
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-cluster",
+			Namespace: "argocd",
+			Labels: map[string]string{
+				argocommon.LabelKeySecretType: argocommon.LabelValueSecretTypeCluster,
+			},
+		},
+		Data: map[string][]byte{
+			// Since this test requires the cluster to be an invalid destination, we
+			// always return a cluster named 'my-cluster2' (different from app 'my-cluster', above)
+			"name":   []byte("good-cluster"),
+			"server": []byte("https://good-cluster"),
+			"config": []byte("{\"username\":\"foo\",\"password\":\"foo\"}"),
+		},
+	}
+
+	objects := append([]runtime.Object{}, secret)
+	kubeclientset := kubefake.NewSimpleClientset(objects...)
 
 	client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &defaultProject).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build()
 	metrics := appsetmetrics.NewFakeAppsetMetrics(client)
-	goodCluster := v1alpha1.Cluster{Server: "https://good-cluster", Name: "good-cluster"}
-	argoDBMock.On("GetCluster", mock.Anything, "https://good-cluster").Return(&goodCluster, nil)
-	argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
-		goodCluster,
-	}}, nil)
 
 	r := ApplicationSetReconciler{
 		Client:   client,
@@ -2614,7 +2617,7 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
 		Generators: map[string]generators.Generator{
 			"List": generators.NewListGenerator(),
 		},
-		ArgoDB:               &argoDBMock,
+		ArgoDB:               &dbmocks.ArgoDB{},
 		ArgoCDNamespace:      "argocd",
 		KubeClientset:        kubeclientset,
 		Policy:               v1alpha1.ApplicationsSyncPolicySync,
@@ -2689,23 +2692,23 @@ func TestDeletePerformedWithSyncPolicyCreateDelete(t *testing.T) {
 
 	apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, true)
 
-	assert.Empty(t, apps.Items)
+	assert.NotNil(t, apps.Items[0].DeletionTimestamp)
 }
 
 func TestDeletePerformedWithSyncPolicySync(t *testing.T) {
 	applicationsSyncPolicy := v1alpha1.ApplicationsSyncPolicySync
 
 	apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, true)
 
-	assert.Empty(t, apps.Items)
+	assert.NotNil(t, apps.Items[0].DeletionTimestamp)
 }
 
 func TestDeletePerformedWithSyncPolicyCreateOnlyAndAllowPolicyOverrideFalse(t *testing.T) {
 	applicationsSyncPolicy := v1alpha1.ApplicationsSyncPolicyCreateOnly
 
 	apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, false)
 
-	assert.Empty(t, apps.Items)
+	assert.NotNil(t, apps.Items[0].DeletionTimestamp)
 }
 
 func TestPolicies(t *testing.T) {
@@ -2717,14 +2720,8 @@ func TestPolicies(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "argocd"},
 		Spec:       v1alpha1.AppProjectSpec{SourceRepos: []string{"*"}, Destinations: []v1alpha1.ApplicationDestination{{Namespace: "*", Server: "https://kubernetes.default.svc"}}},
 	}
-	myCluster := v1alpha1.Cluster{
-		Server: "https://kubernetes.default.svc",
-		Name:   "my-cluster",
-	}
 
 	kubeclientset := kubefake.NewSimpleClientset()
-	argoDBMock := dbmocks.ArgoDB{}
-	argoDBMock.On("GetCluster", mock.Anything, "https://kubernetes.default.svc").Return(&myCluster, nil)
 
 	for _, c := range []struct {
 		name          string
@@ -2807,7 +2804,7 @@ func TestPolicies(t *testing.T) {
 				Generators: map[string]generators.Generator{
 					"List": generators.NewListGenerator(),
 				},
-				ArgoDB:          &argoDBMock,
+				ArgoDB:          &dbmocks.ArgoDB{},
 				ArgoCDNamespace: "argocd",
 				KubeClientset:   kubeclientset,
 				Policy:          policy,
@@ -2881,7 +2878,6 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) {
 	require.NoError(t, err)
 
 	kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-	argoDBMock := dbmocks.ArgoDB{}
 
 	for _, cc := range []struct {
 		name                string
@@ -2965,7 +2961,7 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) {
 				Generators: map[string]generators.Generator{
 					"List": generators.NewListGenerator(),
 				},
-				ArgoDB:        &argoDBMock,
+				ArgoDB:        &dbmocks.ArgoDB{},
 				KubeClientset: kubeclientset,
 				Metrics:       metrics,
 			}
@@ -3714,14 +3710,13 @@ func TestBuildAppDependencyList(t *testing.T) {
 	} {
 		t.Run(cc.name, func(t *testing.T) {
 			kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-			argoDBMock := dbmocks.ArgoDB{}
 
 			r := ApplicationSetReconciler{
 				Client:        client,
 				Scheme:        scheme,
 				Recorder:      record.NewFakeRecorder(1),
 				Generators:    map[string]generators.Generator{},
-				ArgoDB:        &argoDBMock,
+				ArgoDB:        &dbmocks.ArgoDB{},
 				KubeClientset: kubeclientset,
 				Metrics:       metrics,
 			}
@@ -4381,14 +4376,13 @@ func TestBuildAppSyncMap(t *testing.T) {
 	} {
 		t.Run(cc.name, func(t *testing.T) {
 			kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-			argoDBMock := dbmocks.ArgoDB{}
 
 			r := ApplicationSetReconciler{
 				Client:        client,
 				Scheme:        scheme,
 				Recorder:      record.NewFakeRecorder(1),
 				Generators:    map[string]generators.Generator{},
-				ArgoDB:        &argoDBMock,
+				ArgoDB:        &dbmocks.ArgoDB{},
 				KubeClientset: kubeclientset,
 				Metrics:       metrics,
 			}
@@ -5326,7 +5320,6 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 	} {
 		t.Run(cc.name, func(t *testing.T) {
 			kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-			argoDBMock := dbmocks.ArgoDB{}
 
 			client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build()
 			metrics := appsetmetrics.NewFakeAppsetMetrics(client)
@@ -5336,7 +5329,7 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 				Scheme:        scheme,
 				Recorder:      record.NewFakeRecorder(1),
 				Generators:    map[string]generators.Generator{},
-				ArgoDB:        &argoDBMock,
+				ArgoDB:        &dbmocks.ArgoDB{},
 				KubeClientset: kubeclientset,
 				Metrics:       metrics,
 			}
@@ -6075,7 +6068,6 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
 	} {
 		t.Run(cc.name, func(t *testing.T) {
 			kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-			argoDBMock := dbmocks.ArgoDB{}
 
 			client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build()
 			metrics := appsetmetrics.NewFakeAppsetMetrics(client)
@@ -6085,7 +6077,7 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
 				Scheme:        scheme,
 				Recorder:      record.NewFakeRecorder(1),
 				Generators:    map[string]generators.Generator{},
-				ArgoDB:        &argoDBMock,
+				ArgoDB:        &dbmocks.ArgoDB{},
 				KubeClientset: kubeclientset,
 				Metrics:       metrics,
 			}
@@ -6286,7 +6278,6 @@ func TestUpdateResourceStatus(t *testing.T) {
 	} {
 		t.Run(cc.name, func(t *testing.T) {
 			kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-			argoDBMock := dbmocks.ArgoDB{}
 
 			client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build()
 			metrics := appsetmetrics.NewFakeAppsetMetrics(client)
@@ -6296,7 +6287,7 @@ func TestUpdateResourceStatus(t *testing.T) {
 				Scheme:        scheme,
 				Recorder:      record.NewFakeRecorder(1),
 				Generators:    map[string]generators.Generator{},
-				ArgoDB:        &argoDBMock,
+				ArgoDB:        &dbmocks.ArgoDB{},
 				KubeClientset: kubeclientset,
 				Metrics:       metrics,
 			}
@@ -6376,7 +6367,6 @@ func TestResourceStatusAreOrdered(t *testing.T) {
 	} {
 		t.Run(cc.name, func(t *testing.T) {
 			kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
-			argoDBMock := dbmocks.ArgoDB{}
 
 			client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build()
 			metrics := appsetmetrics.NewFakeAppsetMetrics(client)
@@ -6386,7 +6376,7 @@ func TestResourceStatusAreOrdered(t *testing.T) {
 				Scheme:        scheme,
 				Recorder:      record.NewFakeRecorder(1),
 				Generators:    map[string]generators.Generator{},
-				ArgoDB:        &argoDBMock,
+				ArgoDB:        &dbmocks.ArgoDB{},
 				KubeClientset: kubeclientset,
 				Metrics:       metrics,
 			}

diff --git a/applicationset/utils/clusterUtils.go b/applicationset/utils/clusterUtils.go
@@ -51,24 +51,38 @@ const (
 // if we used destination name we infer the server url
 // if we used both name and server then we return an invalid spec error
 func ValidateDestination(ctx context.Context, dest *appv1.ApplicationDestination, clientset kubernetes.Interface, argoCDNamespace string) error {
+	if dest.IsServerInferred() && dest.IsNameInferred() {
+		return fmt.Errorf("application destination can't have both name and server inferred: %s %s", dest.Name, dest.Server)
+	}
 	if dest.Name != "" {
 		if dest.Server == "" {
-			server, err := getDestinationServer(ctx, dest.Name, clientset, argoCDNamespace)
+			server, err := getDestinationBy(ctx, dest.Name, clientset, argoCDNamespace, true)
 			if err != nil {
 				return fmt.Errorf("unable to find destination server: %w", err)
 			}
 			if server == "" {
 				return fmt.Errorf("application references destination cluster %s which does not exist", dest.Name)
 			}
 			dest.SetInferredServer(server)
-		} else if !dest.IsServerInferred() {
+		} else if !dest.IsServerInferred() && !dest.IsNameInferred() {
 			return fmt.Errorf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server)
 		}
+	} else if dest.Server != "" {
+		if dest.Name == "" {
+			serverName, err := getDestinationBy(ctx, dest.Server, clientset, argoCDNamespace, false)
+			if err != nil {
+				return fmt.Errorf("unable to find destination server: %w", err)
+			}
+			if serverName == "" {
+				return fmt.Errorf("application references destination cluster %s which does not exist", dest.Server)
+			}
+			dest.SetInferredName(serverName)
+		}
 	}
 	return nil
 }
 
-func getDestinationServer(ctx context.Context, clusterName string, clientset kubernetes.Interface, argoCDNamespace string) (string, error) {
+func getDestinationBy(ctx context.Context, cluster string, clientset kubernetes.Interface, argoCDNamespace string, byName bool) (string, error) {
 	// settingsMgr := settings.NewSettingsManager(context.TODO(), clientset, namespace)
 	// argoDB := db.NewDB(namespace, settingsMgr, clientset)
 	// clusterList, err := argoDB.ListClusters(ctx)
@@ -78,14 +92,17 @@ func getDestinationServer(ctx context.Context, clusterName string, clientset kub
 	}
 	var servers []string
 	for _, c := range clusterList.Items {
-		if c.Name == clusterName {
+		if byName && c.Name == cluster {
 			servers = append(servers, c.Server)
 		}
+		if !byName && c.Server == cluster {
+			servers = append(servers, c.Name)
+		}
 	}
 	if len(servers) > 1 {
 		return "", fmt.Errorf("there are %d clusters with the same name: %v", len(servers), servers)
 	} else if len(servers) == 0 {
-		return "", fmt.Errorf("there are no clusters with this name: %s", clusterName)
+		return "", fmt.Errorf("there are no clusters with this name: %s", cluster)
 	}
 	return servers[0], nil
 }