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 <aneci@adobe.com>

---------

Signed-off-by: Adrian Aneci <aneci@adobe.com>

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
 }

applicationset/utils/clusterUtils_test.go

@@ -92,7 +92,12 @@ func TestValidateDestination(t *testing.T) {
 			Namespace: "default",
 		}
 
-		appCond := ValidateDestination(context.Background(), &dest, nil, fakeNamespace)
+		secret := createClusterSecret("my-secret", "minikube", "https://127.0.0.1:6443")
+		objects := []runtime.Object{}
+		objects = append(objects, secret)
+		kubeclientset := fake.NewSimpleClientset(objects...)
+
+		appCond := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
 		require.NoError(t, appCond)
 		assert.False(t, dest.IsServerInferred())
 	})

pkg/apis/application/v1alpha1/types.go

@@ -997,6 +997,8 @@ type ApplicationDestination struct {
 
 	// nolint:govet
 	isServerInferred bool `json:"-"`
+	// nolint:govet
+	isNameInferred bool `json:"-"`
 }
 
 // SetIsServerInferred sets the isServerInferred flag. This is used to allow comparison between two destinations where
@@ -3027,6 +3029,17 @@ func (dest ApplicationDestination) Equals(other ApplicationDestination) bool {
 		other.Server = ""
 		other.isServerInferred = false
 	}
+
+	if dest.isNameInferred {
+		dest.Name = ""
+		dest.isNameInferred = false
+	}
+
+	if other.isNameInferred {
+		other.Name = ""
+		other.isNameInferred = false
+	}
+
 	return reflect.DeepEqual(dest, other)
 }
 
@@ -3261,6 +3274,12 @@ func (d *ApplicationDestination) SetInferredServer(server string) {
 	d.Server = server
 }
 
+// SetInferredName sets the Name field of the destination. See IsNameInferred() for details.
+func (d *ApplicationDestination) SetInferredName(name string) {
+	d.isNameInferred = true
+	d.Name = name
+}
+
 // An ApplicationDestination has an 'inferred server' if the ApplicationDestination
 // contains a Name, but not a Server URL. In this case it is necessary to retrieve
 // the Server URL by looking up the cluster name.
@@ -3271,6 +3290,10 @@ func (d *ApplicationDestination) IsServerInferred() bool {
 	return d.isServerInferred
 }
 
+func (d *ApplicationDestination) IsNameInferred() bool {
+	return d.isNameInferred
+}
+
 // MarshalJSON marshals an application destination to JSON format
 func (d *ApplicationDestination) MarshalJSON() ([]byte, error) {
 	type Alias ApplicationDestination
@@ -3279,6 +3302,11 @@ func (d *ApplicationDestination) MarshalJSON() ([]byte, error) {
 		dest = dest.DeepCopy()
 		dest.Server = ""
 	}
+	if d.isNameInferred {
+		dest = dest.DeepCopy()
+		dest.Name = ""
+	}
+
 	return json.Marshal(&struct{ *Alias }{Alias: (*Alias)(dest)})
 }
 

server/application/application.go

@@ -369,7 +369,13 @@ func (s *Server) Create(ctx context.Context, q *application.ApplicationCreateReq
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "unable to check existing application details (%s): %v", appNs, err)
 	}
-	equalSpecs := reflect.DeepEqual(existing.Spec, a.Spec) &&
+
+	if err := argo.ValidateDestination(ctx, &existing.Spec.Destination, s.db); err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "application destination spec for %s is invalid: %s", existing.Name, err.Error())
+	}
+
+	equalSpecs := existing.Spec.Destination.Equals(a.Spec.Destination) &&
+		reflect.DeepEqual(existing.Spec, a.Spec) &&
 		reflect.DeepEqual(existing.Labels, a.Labels) &&
 		reflect.DeepEqual(existing.Annotations, a.Annotations) &&
 		reflect.DeepEqual(existing.Finalizers, a.Finalizers)

test/e2e/app_management_ns_test.go

@@ -347,7 +347,7 @@ func TestNamespacedAppCreationWithoutForceUpdate(t *testing.T) {
 		}).
 		When().
 		IgnoreErrors().
-		CreateApp().
+		CreateApp("--dest-server", KubernetesInternalAPIServerAddr).
 		Then().
 		Expect(Error("", "existing application spec is different, use upsert flag to force update"))
 }

test/e2e/app_management_test.go

@@ -450,7 +450,7 @@ func TestAppCreationWithoutForceUpdate(t *testing.T) {
 		}).
 		When().
 		IgnoreErrors().
-		CreateApp().
+		CreateApp("--dest-server", KubernetesInternalAPIServerAddr).
 		Then().
 		Expect(Error("", "existing application spec is different, use upsert flag to force update"))
 }

test/e2e/fixture/app/actions.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"slices"
 
 	log "github.com/sirupsen/logrus"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -225,7 +226,7 @@ func (a *Actions) prepareCreateAppArgs(args []string) []string {
 		"--repo", fixture.RepoURL(a.context.repoURLType),
 	}, args...)
 
-	if a.context.destName != "" {
+	if a.context.destName != "" && a.context.isDestServerInferred && !slices.Contains(args, "--dest-server") {
 		args = append(args, "--dest-name", a.context.destName)
 	} else {
 		args = append(args, "--dest-server", a.context.destServer)

test/e2e/fixture/app/context.go

@@ -14,7 +14,7 @@ import (
 	"github.com/argoproj/argo-cd/v2/util/settings"
 )
 
-// this implements the "given" part of given/when/then
+// Context implements the "given" part of given/when/then
 type Context struct {
 	t           *testing.T
 	path        string
@@ -26,6 +26,7 @@ type Context struct {
 	appNamespace           string
 	destServer             string
 	destName               string
+	isDestServerInferred     bool
 	env                    string
 	parameters             []string
 	namePrefix             string
@@ -63,12 +64,13 @@ func GivenWithNamespace(t *testing.T, namespace string) *Context {
 }
 
 func GivenWithSameState(t *testing.T) *Context {
-	// ARGOCE_E2E_DEFAULT_TIMEOUT can be used to override the default timeout
+	// ARGOCD_E2E_DEFAULT_TIMEOUT can be used to override the default timeout
 	// for any context.
 	timeout := env.ParseNumFromEnv("ARGOCD_E2E_DEFAULT_TIMEOUT", 20, 0, 180)
 	return &Context{
 		t:              t,
 		destServer:     v1alpha1.KubernetesInternalAPIServerAddr,
+		destName:       "in-cluster",
 		repoURLType:    fixture.RepoURLTypeFile,
 		name:           fixture.Name(),
 		timeout:        timeout,
@@ -257,11 +259,13 @@ func (c *Context) Timeout(timeout int) *Context {
 
 func (c *Context) DestServer(destServer string) *Context {
 	c.destServer = destServer
+	c.isDestServerInferred = false
 	return c
 }
 
 func (c *Context) DestName(destName string) *Context {
 	c.destName = destName
+	c.isDestServerInferred = true
 	return c
 }
 

util/argo/argo.go

@@ -484,16 +484,18 @@ func GetRefSources(ctx context.Context, sources argoappv1.ApplicationSources, pr
 	return refSources, nil
 }
 
-// ValidateDestination sets the 'Server' value of the ApplicationDestination, if it is not set.
+// ValidateDestination sets the 'Server' or the `Name` value of the ApplicationDestination, if it is not set.
 // NOTE: this function WILL write to the object pointed to by the 'dest' parameter.
-//
 // If an ApplicationDestination has a Name field, but has an empty Server (URL) field,
 // ValidationDestination will look up the cluster by name (to get the server URL), and
-// set the corresponding Server field value.
+// set the corresponding Server field value. Same goes for the opposite case.
 //
 // It also checks:
 // - If we used both name and server then we return an invalid spec error
 func ValidateDestination(ctx context.Context, dest *argoappv1.ApplicationDestination, db db.ArgoDB) 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, db, dest.Name)
@@ -504,9 +506,20 @@ func ValidateDestination(ctx context.Context, dest *argoappv1.ApplicationDestina
 				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 := getDestinationServerName(ctx, db, dest.Server)
+			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
 }
@@ -959,6 +972,22 @@ func getDestinationServer(ctx context.Context, db db.ArgoDB, clusterName string)
 	return servers[0], nil
 }
 
+func getDestinationServerName(ctx context.Context, db db.ArgoDB, server string) (string, error) {
+	if db == nil {
+		return "", fmt.Errorf("there are no clusters registered in the database")
+	}
+
+	cluster, err := db.GetCluster(ctx, server)
+	if err != nil {
+		return "", fmt.Errorf("error getting cluster name by server %q: %w", server, err)
+	}
+
+	if cluster.Name == "" {
+		return "", fmt.Errorf("there are no clusters with this URL: %s", server)
+	}
+	return cluster.Name, nil
+}
+
 func GetGlobalProjects(proj *argoappv1.AppProject, projLister applicationsv1.AppProjectLister, settingsManager *settings.SettingsManager) []*argoappv1.AppProject {
 	gps, err := settingsManager.GetGlobalProjectsSettings()
 	globalProjects := make([]*argoappv1.AppProject, 0)

util/argo/argo_test.go

@@ -702,7 +702,7 @@ func TestValidatePermissions(t *testing.T) {
 				SourceRepos: []string{"http://some/where/else"},
 			},
 		}
-		cluster := &argoappv1.Cluster{Server: "https://127.0.0.1:6443"}
+		cluster := &argoappv1.Cluster{Server: "https://127.0.0.1:6443", Name: "test"}
 		db := &dbmocks.ArgoDB{}
 		db.On("GetCluster", context.Background(), spec.Destination.Server).Return(cluster, nil)
 		conditions, err := ValidatePermissions(context.Background(), &spec, &proj, db)
@@ -735,7 +735,7 @@ func TestValidatePermissions(t *testing.T) {
 				SourceRepos: []string{"http://some/where"},
 			},
 		}
-		cluster := &argoappv1.Cluster{Server: "https://127.0.0.1:6443"}
+		cluster := &argoappv1.Cluster{Server: "https://127.0.0.1:6443", Name: "test"}
 		db := &dbmocks.ArgoDB{}
 		db.On("GetCluster", context.Background(), spec.Destination.Server).Return(cluster, nil)
 		conditions, err := ValidatePermissions(context.Background(), &spec, &proj, db)
@@ -773,7 +773,7 @@ func TestValidatePermissions(t *testing.T) {
 		conditions, err := ValidatePermissions(context.Background(), &spec, &proj, db)
 		require.NoError(t, err)
 		assert.Len(t, conditions, 1)
-		assert.Contains(t, conditions[0].Message, "has not been configured")
+		assert.Contains(t, conditions[0].Message, "unable to find destination server")
 	})
 
 	t.Run("Destination cluster name does not exist", func(t *testing.T) {
@@ -834,8 +834,10 @@ func TestValidatePermissions(t *testing.T) {
 		}
 		db := &dbmocks.ArgoDB{}
 		db.On("GetCluster", context.Background(), spec.Destination.Server).Return(nil, fmt.Errorf("Unknown error occurred"))
-		_, err := ValidatePermissions(context.Background(), &spec, &proj, db)
-		require.Error(t, err)
+		conditions, err := ValidatePermissions(context.Background(), &spec, &proj, db)
+		require.NoError(t, err)
+		assert.Len(t, conditions, 1)
+		assert.Contains(t, conditions[0].Message, "Unknown error occurred")
 	})
 
 	t.Run("Destination cluster name resolves to valid server", func(t *testing.T) {
@@ -934,7 +936,7 @@ func TestValidateDestination(t *testing.T) {
 		}
 
 		appCond := ValidateDestination(context.Background(), &dest, nil)
-		require.NoError(t, appCond)
+		require.Error(t, appCond)
 		assert.False(t, dest.IsServerInferred())
 	})
 
@@ -1422,7 +1424,7 @@ func TestValidatePermissionsMultipleSources(t *testing.T) {
 				SourceRepos: []string{"http://some/where/else"},
 			},
 		}
-		cluster := &argoappv1.Cluster{Server: "https://127.0.0.1:6443"}
+		cluster := &argoappv1.Cluster{Server: "https://127.0.0.1:6443", Name: "test"}
 		db := &dbmocks.ArgoDB{}
 		db.On("GetCluster", context.Background(), spec.Destination.Server).Return(cluster, nil)
 		conditions, err := ValidatePermissions(context.Background(), &spec, &proj, db)