diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index 4c284150ec164..2a6856ecde7ac 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -36,6 +36,7 @@ import ( "github.com/argoproj/argo-cd/v2/applicationset/utils" appsetmetrics "github.com/argoproj/argo-cd/v2/applicationset/metrics" + argocommon "github.com/argoproj/argo-cd/v2/common" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake" dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks" @@ -48,9 +49,6 @@ func TestCreateOrUpdateInCluster(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, c := range []struct { // name is human-readable test name name string @@ -1091,9 +1089,6 @@ func TestRemoveFinalizerOnInvalidDestination_FinalizerTypes(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, c := range []struct { // name is human-readable test name name string @@ -1214,9 +1209,6 @@ func TestRemoveFinalizerOnInvalidDestination_DestinationTypes(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, c := range []struct { // name is human-readable test name name string @@ -1371,9 +1363,6 @@ func TestRemoveOwnerReferencesOnDeleteAppSet(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, c := range []struct { // name is human-readable test name name string @@ -1447,9 +1436,6 @@ func TestCreateApplications(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - testCases := []struct { name string appSet v1alpha1.ApplicationSet @@ -1653,8 +1639,6 @@ func TestDeleteInCluster(t *testing.T) { scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) for _, c := range []struct { // appSet is the application set on which the delete function is called @@ -1809,8 +1793,6 @@ func TestGetMinRequeueAfter(t *testing.T) { scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) client := fake.NewClientBuilder().WithScheme(scheme).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) @@ -1858,8 +1840,6 @@ func TestRequeueGeneratorFails(t *testing.T) { scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) appSet := v1alpha1.ApplicationSet{ ObjectMeta: metav1.ObjectMeta{ @@ -1913,18 +1893,6 @@ func TestValidateGeneratedApplications(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - - client := fake.NewClientBuilder().WithScheme(scheme).Build() - metrics := appsetmetrics.NewFakeAppsetMetrics(client) - - // 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"}, @@ -1945,6 +1913,9 @@ func TestValidateGeneratedApplications(t *testing.T) { }, } + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(myProject).Build() + metrics := appsetmetrics.NewFakeAppsetMetrics(client) + // Test a subset of the validations that 'validateGeneratedApplications' performs for _, cc := range []struct { name string @@ -2088,12 +2059,6 @@ 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) - argoObjs := []runtime.Object{myProject} for _, app := range cc.apps { argoObjs = append(argoObjs, &app) @@ -2104,7 +2069,7 @@ func TestValidateGeneratedApplications(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoCDNamespace: "namespace", ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, @@ -2150,8 +2115,6 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) { scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) project := v1alpha1.AppProject{ ObjectMeta: metav1.ObjectMeta{Name: "good-project", Namespace: "argocd"}, @@ -2189,18 +2152,10 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) { } kubeclientset := kubefake.NewSimpleClientset() - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{&project} client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet).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, @@ -2210,7 +2165,7 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Policy: v1alpha1.ApplicationsSyncPolicySync, @@ -2246,8 +2201,6 @@ func TestSetApplicationSetStatusCondition(t *testing.T) { scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) testCases := []struct { appset v1alpha1.ApplicationSet @@ -2394,7 +2347,6 @@ func TestSetApplicationSetStatusCondition(t *testing.T) { } kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} for _, testCase := range testCases { @@ -2409,7 +2361,7 @@ func TestSetApplicationSetStatusCondition(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -2428,8 +2380,6 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) defaultProject := v1alpha1.AppProject{ ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "argocd"}, @@ -2467,17 +2417,30 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp }, } - kubeclientset := kubefake.NewSimpleClientset() - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{&defaultProject} + 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).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, @@ -2487,7 +2450,7 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoCDNamespace: "argocd", ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, @@ -2593,8 +2556,6 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) defaultProject := v1alpha1.AppProject{ ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "argocd"}, @@ -2632,17 +2593,30 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp }, } - kubeclientset := kubefake.NewSimpleClientset() - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{&defaultProject} + 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).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, @@ -2652,7 +2626,7 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoCDNamespace: "argocd", ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, @@ -2728,7 +2702,7 @@ 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) { @@ -2736,7 +2710,7 @@ func TestDeletePerformedWithSyncPolicySync(t *testing.T) { apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, true) - assert.Empty(t, apps.Items) + assert.NotNil(t, apps.Items[0].DeletionTimestamp) } func TestDeletePerformedWithSyncPolicyCreateOnlyAndAllowPolicyOverrideFalse(t *testing.T) { @@ -2744,7 +2718,7 @@ func TestDeletePerformedWithSyncPolicyCreateOnlyAndAllowPolicyOverrideFalse(t *t apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, false) - assert.Empty(t, apps.Items) + assert.NotNil(t, apps.Items[0].DeletionTimestamp) } func TestPolicies(t *testing.T) { @@ -2752,21 +2726,12 @@ func TestPolicies(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - defaultProject := v1alpha1.AppProject{ 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) argoObjs := []runtime.Object{&defaultProject} for _, c := range []struct { @@ -2850,7 +2815,7 @@ func TestPolicies(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoCDNamespace: "argocd", ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, @@ -2923,11 +2888,8 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) { scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} for _, cc := range []struct { @@ -3012,7 +2974,7 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -3031,9 +2993,6 @@ func TestBuildAppDependencyList(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - client := fake.NewClientBuilder().WithScheme(scheme).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) @@ -3765,7 +3724,6 @@ func TestBuildAppDependencyList(t *testing.T) { } { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} r := ApplicationSetReconciler{ @@ -3773,7 +3731,7 @@ func TestBuildAppDependencyList(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -3791,9 +3749,6 @@ func TestBuildAppSyncMap(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - client := fake.NewClientBuilder().WithScheme(scheme).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) @@ -4437,7 +4392,6 @@ func TestBuildAppSyncMap(t *testing.T) { } { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} r := ApplicationSetReconciler{ @@ -4445,7 +4399,7 @@ func TestBuildAppSyncMap(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -4462,9 +4416,6 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, cc := range []struct { name string appSet v1alpha1.ApplicationSet @@ -5387,7 +5338,6 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { } { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build() @@ -5398,7 +5348,7 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -5422,9 +5372,6 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, cc := range []struct { name string appSet v1alpha1.ApplicationSet @@ -6141,7 +6088,6 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) { } { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build() @@ -6152,7 +6098,7 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -6176,9 +6122,6 @@ func TestUpdateResourceStatus(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, cc := range []struct { name string appSet v1alpha1.ApplicationSet @@ -6357,7 +6300,6 @@ func TestUpdateResourceStatus(t *testing.T) { } { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build() @@ -6368,7 +6310,7 @@ func TestUpdateResourceStatus(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -6424,8 +6366,6 @@ func TestResourceStatusAreOrdered(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) for _, cc := range []struct { name string appSet v1alpha1.ApplicationSet @@ -6449,7 +6389,6 @@ func TestResourceStatusAreOrdered(t *testing.T) { } { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) - argoDBMock := dbmocks.ArgoDB{} argoObjs := []runtime.Object{} client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build() @@ -6460,7 +6399,7 @@ func TestResourceStatusAreOrdered(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &argoDBMock, + ArgoDB: &dbmocks.ArgoDB{}, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, Metrics: metrics, @@ -6677,9 +6616,6 @@ func TestMigrateStatus(t *testing.T) { err := v1alpha1.AddToScheme(scheme) require.NoError(t, err) - err = v1alpha1.AddToScheme(scheme) - require.NoError(t, err) - for _, tc := range []struct { name string appset v1alpha1.ApplicationSet diff --git a/applicationset/utils/clusterUtils.go b/applicationset/utils/clusterUtils.go index 8c44dc1246be5..2645ff432cbb9 100644 --- a/applicationset/utils/clusterUtils.go +++ b/applicationset/utils/clusterUtils.go @@ -51,9 +51,12 @@ 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) } @@ -61,14 +64,25 @@ func ValidateDestination(ctx context.Context, dest *appv1.ApplicationDestination 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 } diff --git a/applicationset/utils/clusterUtils_test.go b/applicationset/utils/clusterUtils_test.go index 9e8694359b6bd..9cce629fa1291 100644 --- a/applicationset/utils/clusterUtils_test.go +++ b/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()) }) diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 5f7fca6ed3a1f..c656db1fe5d91 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/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)}) } diff --git a/server/application/application.go b/server/application/application.go index 99c22bc328779..954181b9b464e 100644 --- a/server/application/application.go +++ b/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) diff --git a/test/e2e/app_management_ns_test.go b/test/e2e/app_management_ns_test.go index d4717f57d0f7f..f202d7e3deae9 100644 --- a/test/e2e/app_management_ns_test.go +++ b/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")) } diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index bacbc10260f1b..303a6ed8eea9a 100644 --- a/test/e2e/app_management_test.go +++ b/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")) } diff --git a/test/e2e/fixture/app/actions.go b/test/e2e/fixture/app/actions.go index 7fc4489a620de..5df469b8242eb 100644 --- a/test/e2e/fixture/app/actions.go +++ b/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) diff --git a/test/e2e/fixture/app/context.go b/test/e2e/fixture/app/context.go index d1d0079a09f5e..a08bfeb1a2f19 100644 --- a/test/e2e/fixture/app/context.go +++ b/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 } diff --git a/util/argo/argo.go b/util/argo/argo.go index 4ecb88f27ad3d..7d01cd7b25ab4 100644 --- a/util/argo/argo.go +++ b/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) diff --git a/util/argo/argo_test.go b/util/argo/argo_test.go index b2c684c660c1f..accd5bdf4ceea 100644 --- a/util/argo/argo_test.go +++ b/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)