diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index 1993279e2385b..0ca06dd880ec3 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -496,7 +496,7 @@ func (r *ApplicationSetReconciler) validateGeneratedApplications(ctx context.Con return nil, err } - if err := utils.ValidateDestination(ctx, &app.Spec.Destination, r.KubeClientset, r.ArgoCDNamespace); err != nil { + if _, err := argoutil.GetDestinationCluster(ctx, app.Spec.Destination, r.ArgoDB); err != nil { errorsByIndex[i] = fmt.Errorf("application destination spec is invalid: %s", err.Error()) continue } @@ -783,21 +783,19 @@ func (r *ApplicationSetReconciler) removeFinalizerOnInvalidDestination(ctx conte var validDestination bool // Detect if the destination is invalid (name doesn't correspond to a matching cluster) - if err := utils.ValidateDestination(ctx, &app.Spec.Destination, r.KubeClientset, r.ArgoCDNamespace); err != nil { + if destCluster, err := argoutil.GetDestinationCluster(ctx, app.Spec.Destination, r.ArgoDB); err != nil { appLog.Warnf("The destination cluster for %s couldn't be found: %v", app.Name, err) validDestination = false } else { // Detect if the destination's server field does not match an existing cluster - matchingCluster := false for _, cluster := range clusterList.Items { - // Server fields must match. Note that ValidateDestination ensures that the server field is set, if applicable. - if app.Spec.Destination.Server != cluster.Server { + if destCluster.Server != cluster.Server { continue } // The name must match, if it is not empty - if app.Spec.Destination.Name != "" && cluster.Name != app.Spec.Destination.Name { + if destCluster.Name != "" && cluster.Name != destCluster.Name { continue } diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index c9e7a4f42db19..da7b74baa3580 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -4,9 +4,7 @@ import ( "context" "encoding/json" "fmt" - "reflect" "strconv" - "strings" "testing" "time" @@ -33,14 +31,13 @@ import ( "github.com/argoproj/argo-cd/v2/applicationset/generators" "github.com/argoproj/argo-cd/v2/applicationset/generators/mocks" - "github.com/argoproj/argo-cd/v2/applicationset/utils" - appsetmetrics "github.com/argoproj/argo-cd/v2/applicationset/metrics" + "github.com/argoproj/argo-cd/v2/applicationset/utils" argocommon "github.com/argoproj/argo-cd/v2/common" - "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" - dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks" - "github.com/argoproj/argo-cd/v2/pkg/apis/application" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/v2/util/db" + "github.com/argoproj/argo-cd/v2/util/settings" ) func TestCreateOrUpdateInCluster(t *testing.T) { @@ -1167,16 +1164,17 @@ func TestRemoveFinalizerOnInvalidDestination_FinalizerTypes(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset(objects...) metrics := appsetmetrics.NewFakeAppsetMetrics(client) + settingsMgr := settings.NewSettingsManager(context.TODO(), kubeclientset, "namespace") + argoDB := db.NewDB("namespace", settingsMgr, kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(10), KubeClientset: kubeclientset, Metrics: metrics, + ArgoDB: argoDB, } - // settingsMgr := settings.NewSettingsManager(context.TODO(), kubeclientset, "namespace") - // argoDB := db.NewDB("namespace", settingsMgr, r.KubeClientset) - // clusterList, err := argoDB.ListClusters(context.Background()) clusterList, err := utils.ListClusters(context.Background(), kubeclientset, "namespace") require.NoError(t, err) @@ -1305,7 +1303,7 @@ func TestRemoveFinalizerOnInvalidDestination_DestinationTypes(t *testing.T) { secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "my-secret", - Namespace: "namespace", + Namespace: "argocd", Labels: map[string]string{ argocommon.LabelKeySecretType: argocommon.LabelValueSecretTypeCluster, }, @@ -1323,17 +1321,19 @@ func TestRemoveFinalizerOnInvalidDestination_DestinationTypes(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset(objects...) metrics := appsetmetrics.NewFakeAppsetMetrics(client) + settingsMgr := settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd") + argoDB := db.NewDB("argocd", settingsMgr, kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(10), KubeClientset: kubeclientset, Metrics: metrics, + ArgoDB: argoDB, } - // settingsMgr := settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd") - // argoDB := db.NewDB("argocd", settingsMgr, r.KubeClientset) // clusterList, err := argoDB.ListClusters(context.Background()) - clusterList, err := utils.ListClusters(context.Background(), kubeclientset, "namespace") + clusterList, err := utils.ListClusters(context.Background(), kubeclientset, "argocd") require.NoError(t, err) appLog := log.WithFields(log.Fields{"app": app.Name, "appSet": ""}) @@ -1968,7 +1968,7 @@ func TestValidateGeneratedApplications(t *testing.T) { }, }, }, - expectedErrors: []string{"application destination can't have both name and server defined"}, + expectedErrors: []string{"application destination spec is invalid: application destination can't have both name and server defined: my-cluster my-server"}, validationErrors: map[int]error{0: fmt.Errorf("application destination spec is invalid: application destination can't have both name and server defined: my-cluster my-server")}, }, { @@ -2037,8 +2037,8 @@ func TestValidateGeneratedApplications(t *testing.T) { }, }, }, - expectedErrors: []string{"there are no clusters with this name: nonexistent-cluster"}, - validationErrors: map[int]error{0: fmt.Errorf("application destination spec is invalid: unable to find destination server: there are no clusters with this name: nonexistent-cluster")}, + expectedErrors: []string{"application destination spec is invalid: there are no clusters with this name: nonexistent-cluster"}, + validationErrors: map[int]error{0: fmt.Errorf("application destination spec is invalid: there are no clusters with this name: nonexistent-cluster")}, }, } { t.Run(cc.name, func(t *testing.T) { @@ -2060,12 +2060,14 @@ func TestValidateGeneratedApplications(t *testing.T) { objects := append([]runtime.Object{}, secret) kubeclientset := kubefake.NewSimpleClientset(objects...) + argodb := db.NewDB("namespace", settings.NewSettingsManager(context.TODO(), kubeclientset, "namespace"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, ArgoCDNamespace: "namespace", KubeClientset: kubeclientset, Metrics: metrics, @@ -2082,25 +2084,8 @@ func TestValidateGeneratedApplications(t *testing.T) { if len(errorMessages) == 0 { assert.Empty(t, cc.expectedErrors, "Expected errors but none were seen") } else { - // An error was returned: it should be expected - matched := false - for _, expectedErr := range cc.expectedErrors { - foundMatch := strings.Contains(strings.Join(errorMessages, ";"), expectedErr) - assert.True(t, foundMatch, "Unble to locate expected error: %s", cc.expectedErrors) - matched = matched || foundMatch - } - assert.True(t, matched, "An unexpected error occurrred: %v", err) - // validation message was returned: it should be expected - matched = false - foundMatch := reflect.DeepEqual(validationErrors, cc.validationErrors) - var message string - for _, v := range validationErrors { - message = v.Error() - break - } - assert.True(t, foundMatch, "Unble to locate validation message: %s", message) - matched = matched || foundMatch - assert.True(t, matched, "An unexpected error occurrred: %v", err) + assert.Equal(t, cc.expectedErrors, errorMessages) + assert.Equal(t, cc.validationErrors, validationErrors) } }) } @@ -2151,6 +2136,8 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &project).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, @@ -2159,7 +2146,7 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Policy: v1alpha1.ApplicationsSyncPolicySync, ArgoCDNamespace: "argocd", @@ -2348,6 +2335,8 @@ func TestSetApplicationSetStatusCondition(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&testCase.appset).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).WithStatusSubresource(&testCase.appset).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, @@ -2356,7 +2345,7 @@ func TestSetApplicationSetStatusCondition(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } @@ -2435,6 +2424,8 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &defaultProject).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, @@ -2443,7 +2434,7 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, ArgoCDNamespace: "argocd", KubeClientset: kubeclientset, Policy: v1alpha1.ApplicationsSyncPolicySync, @@ -2460,7 +2451,7 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp // Verify that on validation error, no error is returned, but the object is requeued resCreate, err := r.Reconcile(context.Background(), req) - require.NoError(t, err) + require.NoErrorf(t, err, "Reconcile failed with error: %v", err) assert.Equal(t, time.Duration(0), resCreate.RequeueAfter) var app v1alpha1.Application @@ -2609,6 +2600,8 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &defaultProject).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, @@ -2617,7 +2610,7 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, ArgoCDNamespace: "argocd", KubeClientset: kubeclientset, Policy: v1alpha1.ApplicationsSyncPolicySync, @@ -2796,6 +2789,8 @@ func TestPolicies(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &defaultProject).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, @@ -2804,7 +2799,7 @@ func TestPolicies(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, ArgoCDNamespace: "argocd", KubeClientset: kubeclientset, Policy: policy, @@ -2953,6 +2948,8 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, @@ -2961,7 +2958,7 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) { Generators: map[string]generators.Generator{ "List": generators.NewListGenerator(), }, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } @@ -3711,12 +3708,14 @@ func TestBuildAppDependencyList(t *testing.T) { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } @@ -4377,12 +4376,14 @@ func TestBuildAppSyncMap(t *testing.T) { t.Run(cc.name, func(t *testing.T) { kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } @@ -5324,12 +5325,14 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } @@ -6072,12 +6075,14 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } @@ -6282,12 +6287,14 @@ func TestUpdateResourceStatus(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } @@ -6371,12 +6378,14 @@ func TestResourceStatusAreOrdered(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) + argodb := db.NewDB("argocd", settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd"), kubeclientset) + r := ApplicationSetReconciler{ Client: client, Scheme: scheme, Recorder: record.NewFakeRecorder(1), Generators: map[string]generators.Generator{}, - ArgoDB: &dbmocks.ArgoDB{}, + ArgoDB: argodb, KubeClientset: kubeclientset, Metrics: metrics, } diff --git a/applicationset/utils/clusterUtils.go b/applicationset/utils/clusterUtils.go index 2645ff432cbb9..7747aa46ec653 100644 --- a/applicationset/utils/clusterUtils.go +++ b/applicationset/utils/clusterUtils.go @@ -47,66 +47,6 @@ const ( ArgoCDSecretTypeCluster = "cluster" ) -// ValidateDestination checks: -// 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 := 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() && !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 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) - clusterList, err := ListClusters(ctx, clientset, argoCDNamespace) - if err != nil { - return "", err - } - var servers []string - for _, c := range clusterList.Items { - 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", cluster) - } - return servers[0], nil -} - func ListClusters(ctx context.Context, clientset kubernetes.Interface, namespace string) (*appv1.ClusterList, error) { clusterSecretsList, err := clientset.CoreV1().Secrets(namespace).List(ctx, metav1.ListOptions{LabelSelector: common.LabelKeySecretType + "=" + common.LabelValueSecretTypeCluster}) diff --git a/applicationset/utils/clusterUtils_test.go b/applicationset/utils/clusterUtils_test.go index 3161f8ac82864..a247f46aa8c45 100644 --- a/applicationset/utils/clusterUtils_test.go +++ b/applicationset/utils/clusterUtils_test.go @@ -1,17 +1,12 @@ package utils import ( - "context" - "fmt" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/fake" - kubetesting "k8s.io/client-go/testing" argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" ) @@ -64,117 +59,3 @@ func Test_secretToCluster_NoConfig(t *testing.T) { Server: "http://mycluster", }, *cluster) } - -func createClusterSecret(secretName string, clusterName string, clusterServer string) *corev1.Secret { - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: fakeNamespace, - Labels: map[string]string{ - ArgoCDSecretTypeLabel: ArgoCDSecretTypeCluster, - }, - }, - Data: map[string][]byte{ - "name": []byte(clusterName), - "server": []byte(clusterServer), - "config": []byte("{\"username\":\"foo\",\"password\":\"foo\"}"), - }, - } - - return secret -} - -// From util/argo/argo_test.go -// (ported to use kubeclientset) -func TestValidateDestination(t *testing.T) { - t.Run("Validate destination with server url", func(t *testing.T) { - dest := argoappv1.ApplicationDestination{ - Server: "https://127.0.0.1:6443", - Namespace: "default", - } - - 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()) - }) - - t.Run("Validate destination with server name", func(t *testing.T) { - dest := argoappv1.ApplicationDestination{ - Name: "minikube", - } - - 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.Equal(t, "https://127.0.0.1:6443", dest.Server) - assert.True(t, dest.IsServerInferred()) - }) - - t.Run("Error when having both server url and name", func(t *testing.T) { - dest := argoappv1.ApplicationDestination{ - Server: "https://127.0.0.1:6443", - Name: "minikube", - Namespace: "default", - } - - err := ValidateDestination(context.Background(), &dest, nil, fakeNamespace) - assert.Equal(t, "application destination can't have both name and server defined: minikube https://127.0.0.1:6443", err.Error()) - assert.False(t, dest.IsServerInferred()) - }) - - t.Run("List clusters fails", func(t *testing.T) { - dest := argoappv1.ApplicationDestination{ - Name: "minikube", - } - kubeclientset := fake.NewSimpleClientset() - - kubeclientset.PrependReactor("list", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, fmt.Errorf("an error occurred") - }) - - err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace) - assert.Equal(t, "unable to find destination server: an error occurred", err.Error()) - assert.False(t, dest.IsServerInferred()) - }) - - t.Run("Destination cluster does not exist", func(t *testing.T) { - dest := argoappv1.ApplicationDestination{ - Name: "minikube", - } - - secret := createClusterSecret("dind", "dind", "https://127.0.0.1:6443") - objects := []runtime.Object{} - objects = append(objects, secret) - kubeclientset := fake.NewSimpleClientset(objects...) - - err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace) - assert.Equal(t, "unable to find destination server: there are no clusters with this name: minikube", err.Error()) - assert.False(t, dest.IsServerInferred()) - }) - - t.Run("Validate too many clusters with the same name", func(t *testing.T) { - dest := argoappv1.ApplicationDestination{ - Name: "dind", - } - - secret := createClusterSecret("dind", "dind", "https://127.0.0.1:2443") - secret2 := createClusterSecret("dind2", "dind", "https://127.0.0.1:8443") - - objects := []runtime.Object{} - objects = append(objects, secret, secret2) - kubeclientset := fake.NewSimpleClientset(objects...) - - err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace) - 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()) - assert.False(t, dest.IsServerInferred()) - }) -} diff --git a/cmd/argocd/commands/admin/cluster.go b/cmd/argocd/commands/admin/cluster.go index 130b9a750c60f..8e4b71d4ed541 100644 --- a/cmd/argocd/commands/admin/cluster.go +++ b/cmd/argocd/commands/admin/cluster.go @@ -123,13 +123,6 @@ func loadClusters(ctx context.Context, kubeClient *kubernetes.Clientset, appClie } apps := appItems.Items - for i, app := range apps { - err := argo.ValidateDestination(ctx, &app.Spec.Destination, argoDB) - if err != nil { - return nil, err - } - apps[i] = app - } clusters := make([]ClusterWithInfo, len(clustersList.Items)) batchSize := 10 @@ -154,7 +147,11 @@ func loadClusters(ctx context.Context, kubeClient *kubernetes.Clientset, appClie } nsSet := map[string]bool{} for _, app := range apps { - if app.Spec.Destination.Server == cluster.Server { + destCluster, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, argoDB) + if err != nil { + return fmt.Errorf("error validating application destination: %w", err) + } + if destCluster.Server == cluster.Server { nsSet[app.Spec.Destination.Namespace] = true } } @@ -281,18 +278,16 @@ func runClusterNamespacesCommand(ctx context.Context, clientConfig clientcmd.Cli return fmt.Errorf("error listing application: %w", err) } apps := appItems.Items - for i, app := range apps { - if err := argo.ValidateDestination(ctx, &app.Spec.Destination, argoDB); err != nil { - return fmt.Errorf("error validating application destination: %w", err) - } - apps[i] = app - } - clusters := map[string][]string{} for _, cluster := range clustersList.Items { nsSet := map[string]bool{} for _, app := range apps { - if app.Spec.Destination.Server != cluster.Server { + destCluster, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, argoDB) + if err != nil { + return fmt.Errorf("error validating application destination: %w", err) + } + + if destCluster.Server != cluster.Server { continue } // Use namespaces of actually deployed resources, since some application use dummy target namespace diff --git a/controller/appcontroller.go b/controller/appcontroller.go index ac5b43014bcbf..5084b34cd9458 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -463,7 +463,7 @@ func (ctrl *ApplicationController) handleObjectUpdated(managedByApp map[string]b // setAppManagedResources will build a list of ResourceDiff based on the provided comparisonResult // and persist app resources related data in the cache. Will return the persisted ApplicationTree. -func (ctrl *ApplicationController) setAppManagedResources(a *appv1.Application, comparisonResult *comparisonResult) (*appv1.ApplicationTree, error) { +func (ctrl *ApplicationController) setAppManagedResources(destCluster *appv1.Cluster, a *appv1.Application, comparisonResult *comparisonResult) (*appv1.ApplicationTree, error) { ts := stats.NewTimingStats() defer func() { logCtx := getAppLog(a) @@ -478,7 +478,7 @@ func (ctrl *ApplicationController) setAppManagedResources(a *appv1.Application, if err != nil { return nil, fmt.Errorf("error getting managed resources: %w", err) } - tree, err := ctrl.getResourceTree(a, managedResources) + tree, err := ctrl.getResourceTree(destCluster, a, managedResources) ts.AddCheckpoint("get_resource_tree_ms") if err != nil { return nil, fmt.Errorf("error getting resource tree: %w", err) @@ -520,7 +520,7 @@ func isKnownOrphanedResourceExclusion(key kube.ResourceKey, proj *appv1.AppProje return false } -func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managedResources []*appv1.ResourceDiff) (*appv1.ApplicationTree, error) { +func (ctrl *ApplicationController) getResourceTree(destCluster *appv1.Cluster, a *appv1.Application, managedResources []*appv1.ResourceDiff) (*appv1.ApplicationTree, error) { ts := stats.NewTimingStats() defer func() { logCtx := getAppLog(a) @@ -577,7 +577,7 @@ func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managed } } err = ctrl.stateCache.IterateHierarchyV2(a.Spec.Destination.Server, managedResourcesKeys, func(child appv1.ResourceNode, appName string) bool { - permitted, _ := proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination, func(project string) ([]*appv1.Cluster, error) { + permitted, _ := proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, destCluster, func(project string) ([]*appv1.Cluster, error) { clusters, err := ctrl.db.GetProjectClusters(context.TODO(), project) if err != nil { return nil, fmt.Errorf("failed to get project clusters: %w", err) @@ -601,7 +601,7 @@ func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managed orphanedNodesKeys = append(orphanedNodesKeys, k) } } - err = ctrl.stateCache.IterateHierarchyV2(a.Spec.Destination.Server, orphanedNodesKeys, func(child appv1.ResourceNode, appName string) bool { + err = ctrl.stateCache.IterateHierarchyV2(destCluster.Server, orphanedNodesKeys, func(child appv1.ResourceNode, appName string) bool { belongToAnotherApp := false if appName != "" { appKey := ctrl.toAppKey(appName) @@ -614,7 +614,7 @@ func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managed return false } - permitted, _ := proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination, func(project string) ([]*appv1.Cluster, error) { + permitted, _ := proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, destCluster, func(project string) ([]*appv1.Cluster, error) { return ctrl.db.GetProjectClusters(context.TODO(), project) }) @@ -1111,14 +1111,14 @@ func (ctrl *ApplicationController) shouldBeDeleted(app *appv1.Application, obj * !resourceutil.HasAnnotationOption(obj, helm.ResourcePolicyAnnotation, helm.ResourcePolicyKeep) } -func (ctrl *ApplicationController) getPermittedAppLiveObjects(app *appv1.Application, proj *appv1.AppProject, projectClusters func(project string) ([]*appv1.Cluster, error)) (map[kube.ResourceKey]*unstructured.Unstructured, error) { +func (ctrl *ApplicationController) getPermittedAppLiveObjects(destCluster *appv1.Cluster, app *appv1.Application, proj *appv1.AppProject, projectClusters func(project string) ([]*appv1.Cluster, error)) (map[kube.ResourceKey]*unstructured.Unstructured, error) { objsMap, err := ctrl.stateCache.GetManagedLiveObjs(app, []*unstructured.Unstructured{}) if err != nil { return nil, err } // Don't delete live resources which are not permitted in the app project for k, v := range objsMap { - permitted, err := proj.IsLiveResourcePermitted(v, app.Spec.Destination.Server, app.Spec.Destination.Name, projectClusters) + permitted, err := proj.IsLiveResourcePermitted(v, destCluster, projectClusters) if err != nil { return nil, err } @@ -1130,23 +1130,6 @@ func (ctrl *ApplicationController) getPermittedAppLiveObjects(app *appv1.Applica return objsMap, nil } -func (ctrl *ApplicationController) isValidDestination(app *appv1.Application) (bool, *appv1.Cluster) { - logCtx := getAppLog(app) - // Validate the cluster using the Application destination's `name` field, if applicable, - // and set the Server field, if needed. - if err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); err != nil { - logCtx.Warnf("Unable to validate destination of the Application being deleted: %v", err) - return false, nil - } - - cluster, err := ctrl.db.GetCluster(context.Background(), app.Spec.Destination.Server) - if err != nil { - logCtx.Warnf("Unable to locate cluster URL for Application being deleted: %v", err) - return false, nil - } - return true, cluster -} - func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Application, projectClusters func(project string) ([]*appv1.Cluster, error)) error { logCtx := getAppLog(app) // Get refreshed application info, since informer app copy might be stale @@ -1161,8 +1144,12 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic if err != nil { return err } - - isValid, cluster := ctrl.isValidDestination(app) + isValid := true + destCluster, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, ctrl.db) + if err != nil { + logCtx.Warnf("Unable to get destination cluster: %v", err) + isValid = false + } if !isValid { app.UnSetCascadedDeletion() app.UnSetPostDeleteFinalizer() @@ -1172,7 +1159,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic logCtx.Infof("Resource entries removed from undefined cluster") return nil } - clusterRESTConfig, err := cluster.RESTConfig() + clusterRESTConfig, err := destCluster.RESTConfig() if err != nil { return err } @@ -1184,7 +1171,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic logCtx.Infof("Deleting resources") // ApplicationDestination points to a valid cluster, so we may clean up the live objects objs := make([]*unstructured.Unstructured, 0) - objsMap, err := ctrl.getPermittedAppLiveObjects(app, proj, projectClusters) + objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) if err != nil { return err } @@ -1221,7 +1208,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic return err } - objsMap, err = ctrl.getPermittedAppLiveObjects(app, proj, projectClusters) + objsMap, err = ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) if err != nil { return err } @@ -1241,7 +1228,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic } if app.HasPostDeleteFinalizer() { - objsMap, err := ctrl.getPermittedAppLiveObjects(app, proj, projectClusters) + objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) if err != nil { return err } @@ -1258,7 +1245,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic } if app.HasPostDeleteFinalizer("cleanup") { - objsMap, err := ctrl.getPermittedAppLiveObjects(app, proj, projectClusters) + objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) if err != nil { return err } @@ -1399,7 +1386,7 @@ func (ctrl *ApplicationController) processRequestedAppOperation(app *appv1.Appli } ts.AddCheckpoint("initial_operation_stage_ms") - if err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); err != nil { + if _, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, ctrl.db); err != nil { state.Phase = synccommon.OperationFailed state.Message = err.Error() } else { @@ -1629,8 +1616,13 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo if err := ctrl.cache.GetAppManagedResources(app.InstanceName(ctrl.namespace), &managedResources); err != nil { logCtx.Warnf("Failed to get cached managed resources for tree reconciliation, fall back to full reconciliation") } else { + destCluster, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, ctrl.db) + if err != nil { + logCtx.Errorf("Failed to get destination cluster: %v", err) + return + } var tree *appv1.ApplicationTree - if tree, err = ctrl.getResourceTree(app, managedResources); err == nil { + if tree, err = ctrl.getResourceTree(destCluster, app, managedResources); err == nil { app.Status.Summary = tree.GetSummary(app) if err := ctrl.cache.SetAppResourcesTree(app.InstanceName(ctrl.namespace), tree); err != nil { logCtx.Errorf("Failed to cache resources tree: %v", err) @@ -1712,7 +1704,12 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo ctrl.normalizeApplication(origApp, app) ts.AddCheckpoint("normalize_application_ms") - tree, err := ctrl.setAppManagedResources(app, compareResult) + destCluster, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, ctrl.db) + if err != nil { + logCtx.Errorf("Failed to get destination cluster: %v", err) + return + } + tree, err := ctrl.setAppManagedResources(destCluster, app, compareResult) ts.AddCheckpoint("set_app_managed_resources_ms") if err != nil { logCtx.Errorf("Failed to cache app resources: %v", err) @@ -1823,7 +1820,7 @@ func (ctrl *ApplicationController) needRefreshAppStatus(app *appv1.Application, reason = fmt.Sprintf("comparison expired, requesting hard refresh. reconciledAt: %v, expiry: %v", reconciledAtStr, statusHardRefreshTimeout) refreshType = appv1.RefreshTypeHard } - } else if !app.Spec.Destination.Equals(app.Status.Sync.ComparedTo.Destination) { + } else if !reflect.DeepEqual(app.Spec.Destination, app.Status.Sync.ComparedTo.Destination) { reason = "spec.destination differs" } else if app.HasChangedManagedNamespaceMetadata() { reason = "spec.syncPolicy.managedNamespaceMetadata differs" @@ -2242,13 +2239,8 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar if _, err := ctrl.getAppProj(app); err != nil { ctrl.setAppCondition(app, ctrl.projectErrorToCondition(err, app)) } else { - // This call to 'ValidateDestination' ensures that the .spec.destination field of all Applications - // returned by the informer/lister will have server field set (if not already set) based on the name. - // (or, if not found, an error app condition) - - // If the server field is not set, set it based on the cluster name; if the cluster name can't be found, - // log an error as an App Condition. - if err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); err != nil { + // If the cluster can't be found, log an error as an App Condition. + if _, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, ctrl.db); err != nil { ctrl.setAppCondition(app, appv1.ApplicationCondition{Type: appv1.ApplicationConditionInvalidSpecError, Message: err.Error()}) } } diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 6adb5cc02d2fa..8a385288dd660 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -1283,7 +1283,7 @@ func TestGetResourceTree_HasOrphanedResources(t *testing.T) { kube.NewResourceKey("apps", "Deployment", "default", "deploy2"): {ResourceNode: orphanedDeploy2}, }, }, nil) - tree, err := ctrl.getResourceTree(app, []*v1alpha1.ResourceDiff{{ + tree, err := ctrl.getResourceTree(&v1alpha1.Cluster{Server: "https://localhost:6443", Name: "fake-cluster"}, app, []*v1alpha1.ResourceDiff{{ Namespace: "default", Name: "nginx-deployment", Kind: "Deployment", diff --git a/controller/cache/cache.go b/controller/cache/cache.go index b17afbca5234b..673485c85779d 100644 --- a/controller/cache/cache.go +++ b/controller/cache/cache.go @@ -719,11 +719,12 @@ func (c *liveStateCache) isClusterHasApps(apps []interface{}, cluster *appv1.Clu if !ok { continue } - err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, c.db) + destCluster, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, c.db) if err != nil { + log.Warnf("Failed to get destination cluster: %v", err) continue } - if app.Spec.Destination.Server == cluster.Server { + if destCluster.Server == cluster.Server { return true } } diff --git a/controller/clusterinfoupdater.go b/controller/clusterinfoupdater.go index 655ff6a59b759..ff3a7eafb3881 100644 --- a/controller/clusterinfoupdater.go +++ b/controller/clusterinfoupdater.go @@ -132,10 +132,11 @@ func (c *clusterInfoUpdater) getUpdatedClusterInfo(ctx context.Context, apps []* continue } } - if err := argo.ValidateDestination(ctx, &a.Spec.Destination, c.db); err != nil { + destCluster, err := argo.GetDestinationCluster(ctx, a.Spec.Destination, c.db) + if err != nil { continue } - if a.Spec.Destination.Server == cluster.Server { + if destCluster.Server == cluster.Server { appCount += 1 } } diff --git a/controller/state.go b/controller/state.go index 8f84be1f65348..6aad8e8465da5 100644 --- a/controller/state.go +++ b/controller/state.go @@ -575,9 +575,14 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 logCtx.Debugf("Retrieved live manifests") + destCluster, err := argo.GetDestinationCluster(context.TODO(), app.Spec.Destination, m.db) + if err != nil { + msg := fmt.Sprintf("Failed to get destination cluster %q: %s", app.Spec.Destination.Server, err.Error()) + conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: msg, LastTransitionTime: &now}) + } // filter out all resources which are not permitted in the application project for k, v := range liveObjByKey { - permitted, err := project.IsLiveResourcePermitted(v, app.Spec.Destination.Server, app.Spec.Destination.Name, func(project string) ([]*v1alpha1.Cluster, error) { + permitted, err := project.IsLiveResourcePermitted(v, destCluster, func(project string) ([]*v1alpha1.Cluster, error) { clusters, err := m.db.GetProjectClusters(context.TODO(), project) if err != nil { return nil, fmt.Errorf("failed to get clusters for project %q: %w", project, err) @@ -956,20 +961,6 @@ func specEqualsCompareTo(spec v1alpha1.ApplicationSpec, comparedTo v1alpha1.Comp // Make a copy to be sure we don't mutate the original. specCopy := spec.DeepCopy() currentSpec := specCopy.BuildComparedToStatus() - - // The spec might have been augmented to include both server and name, so change it to match the comparedTo before - // comparing. - if comparedTo.Destination.Server == "" { - currentSpec.Destination.Server = "" - } - if comparedTo.Destination.Name == "" { - currentSpec.Destination.Name = "" - } - - // Set IsServerInferred to false on both, because that field is not important for comparison. - comparedTo.Destination.SetIsServerInferred(false) - currentSpec.Destination.SetIsServerInferred(false) - return reflect.DeepEqual(comparedTo, currentSpec) } diff --git a/controller/state_test.go b/controller/state_test.go index 2efc51718f9ef..04f373aa6fd6f 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -810,7 +810,7 @@ func TestSetManagedResourcesWithOrphanedResources(t *testing.T) { }, }, nil) - tree, err := ctrl.setAppManagedResources(app, &comparisonResult{managedResources: make([]managedResource, 0)}) + tree, err := ctrl.setAppManagedResources(&v1alpha1.Cluster{Server: "test", Name: "test"}, app, &comparisonResult{managedResources: make([]managedResource, 0)}) require.NoError(t, err) assert.Len(t, tree.OrphanedNodes, 1) @@ -839,7 +839,7 @@ func TestSetManagedResourcesWithResourcesOfAnotherApp(t *testing.T) { }, }, nil) - tree, err := ctrl.setAppManagedResources(app1, &comparisonResult{managedResources: make([]managedResource, 0)}) + tree, err := ctrl.setAppManagedResources(&argoappv1.Cluster{Server: "test", Name: "test"}, app1, &comparisonResult{managedResources: make([]managedResource, 0)}) require.NoError(t, err) assert.Empty(t, tree.OrphanedNodes) @@ -893,7 +893,7 @@ func TestSetManagedResourcesKnownOrphanedResourceExceptions(t *testing.T) { }, }, nil) - tree, err := ctrl.setAppManagedResources(app, &comparisonResult{managedResources: make([]managedResource, 0)}) + tree, err := ctrl.setAppManagedResources(&argoappv1.Cluster{Server: "test", Name: "test"}, app, &comparisonResult{managedResources: make([]managedResource, 0)}) require.NoError(t, err) assert.Len(t, tree.OrphanedNodes, 1) @@ -1568,10 +1568,6 @@ func TestUseDiffCache(t *testing.T) { t.Fatalf("error merging app: %s", err) } } - if app.Spec.Destination.Name != "" && app.Spec.Destination.Server != "" { - // Simulate the controller's process for populating both of these fields. - app.Spec.Destination.SetInferredServer(app.Spec.Destination.Server) - } return app } @@ -1737,44 +1733,6 @@ func TestUseDiffCache(t *testing.T) { expectedUseCache: false, serverSideDiff: false, }, - { - // There are code paths that modify the ApplicationSpec and augment the destination field with both the - // destination server and name. Since both fields are populated in the app spec but not in the comparedTo, - // we need to make sure we correctly compare the fields and don't miss the cache. - testName: "will return true if the app spec destination contains both server and name, but otherwise matches comparedTo", - noCache: false, - manifestInfos: manifestInfos("rev1"), - sources: sources(), - app: app("httpbin", "rev1", false, &argoappv1.Application{ - Spec: argoappv1.ApplicationSpec{ - Destination: argoappv1.ApplicationDestination{ - Server: "https://kubernetes.default.svc", - Name: "httpbin", - Namespace: "httpbin", - }, - }, - Status: argoappv1.ApplicationStatus{ - Resources: []argoappv1.ResourceStatus{}, - Sync: argoappv1.SyncStatus{ - Status: argoappv1.SyncStatusCodeSynced, - ComparedTo: argoappv1.ComparedTo{ - Destination: argoappv1.ApplicationDestination{ - Server: "https://kubernetes.default.svc", - Namespace: "httpbin", - }, - }, - Revision: "rev1", - }, - ReconciledAt: &metav1.Time{ - Time: time.Now().Add(-time.Hour), - }, - }, - }), - manifestRevisions: []string{"rev1"}, - statusRefreshTimeout: time.Hour * 24, - expectedUseCache: true, - serverSideDiff: true, - }, } for _, tc := range cases { diff --git a/controller/sync.go b/controller/sync.go index dcbb768a79be8..f9414e8390bdc 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -221,21 +221,21 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha return } - clst, err := m.db.GetCluster(context.Background(), app.Spec.Destination.Server) + cluster, err := m.db.GetCluster(context.Background(), app.Spec.Destination.Server) if err != nil { state.Phase = common.OperationError state.Message = err.Error() return } - rawConfig, err := clst.RawRestConfig() + rawConfig, err := cluster.RawRestConfig() if err != nil { state.Phase = common.OperationError state.Message = err.Error() return } - clusterRESTConfig, err := clst.RESTConfig() + clusterRESTConfig, err := cluster.RESTConfig() if err != nil { state.Phase = common.OperationError state.Message = err.Error() @@ -285,7 +285,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha prunePropagationPolicy = v1.DeletePropagationOrphan } - openAPISchema, err := m.getOpenAPISchema(clst.Server) + openAPISchema, err := m.getOpenAPISchema(cluster.Server) if err != nil { state.Phase = common.OperationError state.Message = fmt.Sprintf("failed to load openAPISchema: %v", err) @@ -349,7 +349,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha return fmt.Errorf("resource %s:%s is not permitted in project %s", un.GroupVersionKind().Group, un.GroupVersionKind().Kind, proj.Name) } if res.Namespaced { - permitted, err := proj.IsDestinationPermitted(v1alpha1.ApplicationDestination{Namespace: un.GetNamespace(), Server: app.Spec.Destination.Server, Name: app.Spec.Destination.Name}, func(project string) ([]*v1alpha1.Cluster, error) { + permitted, err := proj.IsDestinationPermitted(cluster, un.GetNamespace(), func(project string) ([]*v1alpha1.Cluster, error) { return m.db.GetProjectClusters(context.TODO(), project) }) if err != nil { diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index 436e578548e3d..f480bcac5092e 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -400,16 +400,16 @@ func (proj AppProject) IsGroupKindPermitted(gk schema.GroupKind, namespaced bool } // IsLiveResourcePermitted returns whether a live resource found in the cluster is permitted by an AppProject -func (proj AppProject) IsLiveResourcePermitted(un *unstructured.Unstructured, server string, name string, projectClusters func(project string) ([]*Cluster, error)) (bool, error) { - return proj.IsResourcePermitted(un.GroupVersionKind().GroupKind(), un.GetNamespace(), ApplicationDestination{Server: server, Name: name}, projectClusters) +func (proj AppProject) IsLiveResourcePermitted(un *unstructured.Unstructured, destCluster *Cluster, projectClusters func(project string) ([]*Cluster, error)) (bool, error) { + return proj.IsResourcePermitted(un.GroupVersionKind().GroupKind(), un.GetNamespace(), destCluster, projectClusters) } -func (proj AppProject) IsResourcePermitted(groupKind schema.GroupKind, namespace string, dest ApplicationDestination, projectClusters func(project string) ([]*Cluster, error)) (bool, error) { +func (proj AppProject) IsResourcePermitted(groupKind schema.GroupKind, namespace string, destCluster *Cluster, projectClusters func(project string) ([]*Cluster, error)) (bool, error) { if !proj.IsGroupKindPermitted(groupKind, namespace != "") { return false, nil } if namespace != "" { - return proj.IsDestinationPermitted(ApplicationDestination{Server: dest.Server, Name: dest.Name, Namespace: namespace}, projectClusters) + return proj.IsDestinationPermitted(destCluster, namespace, projectClusters) } return true, nil } @@ -461,7 +461,11 @@ func (proj AppProject) IsSourcePermitted(src ApplicationSource) bool { } // IsDestinationPermitted validates if the provided application's destination is one of the allowed destinations for the project -func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination, projectClusters func(project string) ([]*Cluster, error)) (bool, error) { +func (proj AppProject) IsDestinationPermitted(destCluster *Cluster, destNamespace string, projectClusters func(project string) ([]*Cluster, error)) (bool, error) { + if destCluster == nil { + return false, nil + } + dst := ApplicationDestination{Server: destCluster.Server, Name: destCluster.Name, Namespace: destNamespace} destinationMatched := proj.isDestinationMatched(dst) if destinationMatched && proj.Spec.PermitOnlyProjectScopedClusters { clusters, err := projectClusters(proj.Name) diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 452914457056a..878ff88a524e2 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -1002,17 +1002,6 @@ type ApplicationDestination struct { Namespace string `json:"namespace,omitempty" protobuf:"bytes,2,opt,name=namespace"` // Name is an alternate way of specifying the target cluster by its symbolic name. This must be set if Server is not set. Name string `json:"name,omitempty" protobuf:"bytes,3,opt,name=name"` - - // 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 -// one server is inferred and the other is not. -func (d *ApplicationDestination) SetIsServerInferred(inferred bool) { - d.isServerInferred = inferred } type ResourceHealthLocation string @@ -3043,33 +3032,6 @@ func (source *ApplicationSource) ExplicitType() (*ApplicationSourceType, error) return &appType, nil } -// Equals compares two instances of ApplicationDestination and returns true if instances are equal. -func (dest ApplicationDestination) Equals(other ApplicationDestination) bool { - // ignore destination cluster name and isServerInferred fields during comparison - // since server URL is inferred from cluster name - if dest.isServerInferred { - dest.Server = "" - dest.isServerInferred = false - } - - if other.isServerInferred { - 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) -} - // GetProject returns the application's project. This is preferred over spec.Project which may be empty func (spec ApplicationSpec) GetProject() string { if spec.Project == "" { @@ -3323,44 +3285,10 @@ func (r ResourceDiff) TargetObject() (*unstructured.Unstructured, error) { return UnmarshalToUnstructured(r.TargetState) } -// SetInferredServer sets the Server field of the destination. See IsServerInferred() for details. -func (d *ApplicationDestination) SetInferredServer(server string) { - d.isServerInferred = true - 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. -// -// As of this writing, looking up the cluster name, and setting the URL, is -// performed by 'utils.ValidateDestination(...)', which then calls SetInferredServer. -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 dest := d - if d.isServerInferred { - dest = dest.DeepCopy() - dest.Server = "" - } - if d.isNameInferred { - dest = dest.DeepCopy() - dest.Name = "" - } return json.Marshal(&struct{ *Alias }{Alias: (*Alias)(dest)}) } diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 1d29219d05d9e..34ce0fd7d5941 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -236,7 +236,11 @@ func TestAppProject_IsDestinationPermitted(t *testing.T) { Destinations: data.projDest, }, } - permitted, _ := proj.IsDestinationPermitted(data.appDest, func(project string) ([]*Cluster, error) { + destCluster := &Cluster{ + Server: data.appDest.Server, + Name: data.appDest.Name, + } + permitted, _ := proj.IsDestinationPermitted(destCluster, data.appDest.Namespace, func(project string) ([]*Cluster, error) { return []*Cluster{}, nil }) assert.Equal(t, data.isPermitted, permitted) @@ -403,7 +407,11 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { Destinations: data.projDest, }, } - permitted, _ := proj.IsDestinationPermitted(data.appDest, func(project string) ([]*Cluster, error) { + destCluster := &Cluster{ + Server: data.appDest.Server, + Name: data.appDest.Name, + } + permitted, _ := proj.IsDestinationPermitted(destCluster, data.appDest.Namespace, func(project string) ([]*Cluster, error) { return []*Cluster{}, nil }) assert.Equalf(t, data.isPermitted, permitted, "appDest mismatch for %+v with project destinations %+v", data.appDest, data.projDest) @@ -475,8 +483,11 @@ func TestAppProject_IsDestinationPermitted_PermitOnlyProjectScopedClusters(t *te Destinations: data.projDest, }, } - - permitted, _ := proj.IsDestinationPermitted(data.appDest, func(_ string) ([]*Cluster, error) { + destCluster := &Cluster{ + Server: data.appDest.Server, + Name: data.appDest.Name, + } + permitted, _ := proj.IsDestinationPermitted(destCluster, data.appDest.Namespace, func(_ string) ([]*Cluster, error) { return data.clusters, nil }) assert.Equal(t, data.isPermitted, permitted) @@ -490,8 +501,7 @@ func TestAppProject_IsDestinationPermitted_PermitOnlyProjectScopedClusters(t *te }}, }, } - - _, err := proj.IsDestinationPermitted(ApplicationDestination{Server: "https://my-cluster.123.com", Namespace: "default"}, func(_ string) ([]*Cluster, error) { + _, err := proj.IsDestinationPermitted(&Cluster{Server: "https://my-cluster.123.com"}, "default", func(_ string) ([]*Cluster, error) { return nil, errors.New("some error") }) assert.ErrorContains(t, err, "could not retrieve project clusters") @@ -1084,32 +1094,6 @@ func TestAppSource_GetNamespaceOrDefault(t *testing.T) { } } -func TestAppDestinationEquality(t *testing.T) { - left := &ApplicationDestination{ - Server: "https://kubernetes.default.svc", - Namespace: "default", - } - right := left.DeepCopy() - assert.True(t, left.Equals(*right)) - right.Namespace = "kube-system" - assert.False(t, left.Equals(*right)) -} - -func TestAppDestinationEquality_InferredServerURL(t *testing.T) { - left := ApplicationDestination{ - Name: "in-cluster", - Namespace: "default", - } - right := ApplicationDestination{ - Name: "in-cluster", - Server: "https://kubernetes.default.svc", - Namespace: "default", - isServerInferred: true, - } - assert.True(t, left.Equals(right)) - assert.True(t, right.Equals(left)) -} - func TestAppProjectSpec_DestinationClusters(t *testing.T) { tests := []struct { name string diff --git a/server/application/application.go b/server/application/application.go index 9fee8369795b1..e898995683825 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -370,11 +370,11 @@ func (s *Server) Create(ctx context.Context, q *application.ApplicationCreateReq return nil, status.Errorf(codes.Internal, "unable to check existing application details (%s): %v", appNs, err) } - if err := argo.ValidateDestination(ctx, &existing.Spec.Destination, s.db); err != nil { + if _, err := argo.GetDestinationCluster(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) && + equalSpecs := reflect.DeepEqual(existing.Spec.Destination, a.Spec.Destination) && reflect.DeepEqual(existing.Spec, a.Spec) && reflect.DeepEqual(existing.Labels, a.Labels) && reflect.DeepEqual(existing.Annotations, a.Annotations) && @@ -1269,7 +1269,7 @@ func (s *Server) validateAndNormalizeApp(ctx context.Context, app *appv1.Applica } } - if err := argo.ValidateDestination(ctx, &app.Spec.Destination, s.db); err != nil { + if _, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, s.db); err != nil { return status.Errorf(codes.InvalidArgument, "application destination spec for %s is invalid: %s", app.Name, err.Error()) } @@ -1300,14 +1300,11 @@ func (s *Server) validateAndNormalizeApp(ctx context.Context, app *appv1.Applica } func (s *Server) getApplicationClusterConfig(ctx context.Context, a *appv1.Application) (*rest.Config, error) { - if err := argo.ValidateDestination(ctx, &a.Spec.Destination, s.db); err != nil { - return nil, fmt.Errorf("error validating destination: %w", err) - } - clst, err := s.db.GetCluster(ctx, a.Spec.Destination.Server) + cluster, err := argo.GetDestinationCluster(ctx, a.Spec.Destination, s.db) if err != nil { - return nil, fmt.Errorf("error getting cluster: %w", err) + return nil, fmt.Errorf("error validating destination: %w", err) } - config, err := clst.RESTConfig() + config, err := cluster.RESTConfig() if err != nil { return nil, fmt.Errorf("error getting cluster REST config: %w", err) } @@ -2162,7 +2159,8 @@ func (s *Server) getObjectsForDeepLinks(ctx context.Context, app *appv1.Applicat return s.db.GetProjectClusters(ctx, project) } - if err := argo.ValidateDestination(ctx, &app.Spec.Destination, s.db); err != nil { + destCluster, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, s.db) + if err != nil { log.WithFields(map[string]interface{}{ "application": app.GetName(), "ns": app.GetNamespace(), @@ -2171,7 +2169,7 @@ func (s *Server) getObjectsForDeepLinks(ctx context.Context, app *appv1.Applicat return nil, nil, nil } - permitted, err := proj.IsDestinationPermitted(app.Spec.Destination, getProjectClusters) + permitted, err := proj.IsDestinationPermitted(destCluster, app.Spec.Destination.Namespace, getProjectClusters) if err != nil { return nil, nil, err } @@ -2471,6 +2469,11 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA return nil, err } + destCluster, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, s.db) + if err != nil { + return nil, err + } + // First, make sure all the returned resources are permitted, for each operation. // Also perform create with dry-runs for all create-operation resources. // This is performed separately to reduce the risk of only some of the resources being successfully created later. @@ -2478,7 +2481,7 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA // the dry-run for relevant apply/delete operation would have to be invoked as well. for _, impactedResource := range newObjects { newObj := impactedResource.UnstructuredObj - err := s.verifyResourcePermitted(app, proj, newObj) + err := s.verifyResourcePermitted(destCluster, proj, newObj) if err != nil { return nil, err } @@ -2571,8 +2574,8 @@ func (s *Server) patchResource(ctx context.Context, config *rest.Config, liveObj return &application.ApplicationResponse{}, nil } -func (s *Server) verifyResourcePermitted(app *appv1.Application, proj *appv1.AppProject, obj *unstructured.Unstructured) error { - permitted, err := proj.IsResourcePermitted(schema.GroupKind{Group: obj.GroupVersionKind().Group, Kind: obj.GroupVersionKind().Kind}, obj.GetNamespace(), app.Spec.Destination, func(project string) ([]*appv1.Cluster, error) { +func (s *Server) verifyResourcePermitted(destCluster *appv1.Cluster, proj *appv1.AppProject, obj *unstructured.Unstructured) error { + permitted, err := proj.IsResourcePermitted(schema.GroupKind{Group: obj.GroupVersionKind().Group, Kind: obj.GroupVersionKind().Kind}, obj.GetNamespace(), destCluster, func(project string) ([]*appv1.Cluster, error) { clusters, err := s.db.GetProjectClusters(context.TODO(), project) if err != nil { return nil, fmt.Errorf("failed to get project clusters: %w", err) @@ -2583,7 +2586,7 @@ func (s *Server) verifyResourcePermitted(app *appv1.Application, proj *appv1.App return fmt.Errorf("error checking resource permissions: %w", err) } if !permitted { - return fmt.Errorf("application %s is not permitted to manage %s/%s/%s in %s", app.RBACName(s.ns), obj.GroupVersionKind().Group, obj.GroupVersionKind().Kind, obj.GetName(), obj.GetNamespace()) + return fmt.Errorf("application is not permitted to manage %s/%s/%s in %s", obj.GroupVersionKind().Group, obj.GroupVersionKind().Kind, obj.GetName(), obj.GetNamespace()) } return nil diff --git a/server/application/application_test.go b/server/application/application_test.go index 79395916d8397..54b98ea30be2a 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1505,7 +1505,6 @@ func TestCreateAppWithDestName(t *testing.T) { app, err := appServer.Create(context.Background(), &createReq) require.NoError(t, err) assert.NotNil(t, app) - assert.Equal(t, "https://cluster-api.example.com", app.Spec.Destination.Server) } // TestCreateAppWithOperation tests that an application created with an operation is created with the operation removed. diff --git a/server/application/terminal.go b/server/application/terminal.go index 46884ce2ffa5e..4661a8645c74f 100644 --- a/server/application/terminal.go +++ b/server/application/terminal.go @@ -63,14 +63,11 @@ func NewHandler(appLister applisters.ApplicationLister, namespace string, enable } func (s *terminalHandler) getApplicationClusterRawConfig(ctx context.Context, a *appv1.Application) (*rest.Config, error) { - if err := argo.ValidateDestination(ctx, &a.Spec.Destination, s.db); err != nil { - return nil, err - } - clst, err := s.db.GetCluster(ctx, a.Spec.Destination.Server) + destCluster, err := argo.GetDestinationCluster(ctx, a.Spec.Destination, s.db) if err != nil { return nil, err } - rawConfig, err := clst.RawRestConfig() + rawConfig, err := destCluster.RawRestConfig() if err != nil { return nil, err } diff --git a/server/extension/extension.go b/server/extension/extension.go index d28d3aac5dc9f..c26f11d95d8c6 100644 --- a/server/extension/extension.go +++ b/server/extension/extension.go @@ -343,6 +343,7 @@ type Manager struct { settings SettingsGetter application ApplicationGetter project ProjectGetter + cluster argo.ClusterGetter rbac RbacEnforcer registry ExtensionRegistry metricsReg ExtensionMetricsRegistry @@ -362,13 +363,14 @@ type ExtensionMetricsRegistry interface { } // NewManager will initialize a new manager. -func NewManager(log *log.Entry, namespace string, sg SettingsGetter, ag ApplicationGetter, pg ProjectGetter, rbac RbacEnforcer, ug UserGetter) *Manager { +func NewManager(log *log.Entry, namespace string, sg SettingsGetter, ag ApplicationGetter, pg ProjectGetter, cg argo.ClusterGetter, rbac RbacEnforcer, ug UserGetter) *Manager { return &Manager{ log: log, namespace: namespace, settings: sg, application: ag, project: pg, + cluster: cg, rbac: rbac, userGetter: ug, } @@ -685,7 +687,11 @@ func (m *Manager) authorize(ctx context.Context, rr *RequestResources, extName s if proj == nil { return nil, fmt.Errorf("invalid project provided in the %q header", HeaderArgoCDProjectName) } - permitted, err := proj.IsDestinationPermitted(app.Spec.Destination, m.project.GetClusters) + destCluster, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, m.cluster) + if err != nil { + return nil, fmt.Errorf("error getting destination cluster: %w", err) + } + permitted, err := proj.IsDestinationPermitted(destCluster, app.Spec.Destination.Namespace, m.project.GetClusters) if err != nil { return nil, fmt.Errorf("error validating project destinations: %w", err) } diff --git a/server/extension/extension_test.go b/server/extension/extension_test.go index 3a92fcfeffb8f..003add27ee122 100644 --- a/server/extension/extension_test.go +++ b/server/extension/extension_test.go @@ -21,6 +21,7 @@ import ( "github.com/argoproj/argo-cd/v2/server/extension" "github.com/argoproj/argo-cd/v2/server/extension/mocks" "github.com/argoproj/argo-cd/v2/server/rbacpolicy" + dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks" "github.com/argoproj/argo-cd/v2/util/settings" ) @@ -136,7 +137,7 @@ func TestRegisterExtensions(t *testing.T) { logger, _ := test.NewNullLogger() logEntry := logger.WithContext(context.Background()) - m := extension.NewManager(logEntry, "", settMock, nil, nil, nil, nil) + m := extension.NewManager(logEntry, "", settMock, nil, nil, nil, nil, nil) return &fixture{ settingsGetterMock: settMock, @@ -251,9 +252,13 @@ func TestCallExtension(t *testing.T) { metricsMock := &mocks.ExtensionMetricsRegistry{} userMock := &mocks.UserGetter{} + dbMock := &dbmocks.ArgoDB{} + dbMock.On("GetClusterServersByName", mock.Anything, mock.Anything).Return([]string{"cluster1"}, nil) + dbMock.On("GetCluster", mock.Anything, mock.Anything).Return(&v1alpha1.Cluster{Server: "some-url", Name: "cluster1"}, nil) + logger, _ := test.NewNullLogger() logEntry := logger.WithContext(context.Background()) - m := extension.NewManager(logEntry, defaultServerNamespace, settMock, appMock, projMock, rbacMock, userMock) + m := extension.NewManager(logEntry, defaultServerNamespace, settMock, appMock, projMock, dbMock, rbacMock, userMock) m.AddMetricsRegistry(metricsMock) mux := http.NewServeMux() @@ -392,8 +397,7 @@ func TestCallExtension(t *testing.T) { f := setup() backendResponse := "some data" backendEndpoint := "some-backend" - clusterName := "clusterName" - clusterURL := "clusterURL" + clusterURL := "some-url" backendSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { for k, v := range r.Header { w.Header().Add(k, strings.Join(v, ",")) @@ -407,7 +411,7 @@ func TestCallExtension(t *testing.T) { ts := startTestServer(t, f) defer ts.Close() r := newExtensionRequest(t, "Get", fmt.Sprintf("%s/extensions/%s/", ts.URL, backendEndpoint)) - app := getApp(clusterName, clusterURL, defaultProjectName) + app := getApp("", clusterURL, defaultProjectName) proj := getProjectWithDestinations("project-name", nil, []string{clusterURL}) f.appGetterMock.On("Get", mock.Anything, mock.Anything).Return(app, nil) withProject(proj, f) @@ -658,7 +662,7 @@ func TestCallExtension(t *testing.T) { require.NotNil(t, resp) assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) }) - t.Run("will return 400 if application defines name and server destination", func(t *testing.T) { + t.Run("will return 401 if application defines name and server destination", func(t *testing.T) { // This test is to validate a security risk with malicious application // trying to gain access to execute extensions in clusters it doesn't // have access. @@ -693,11 +697,11 @@ func TestCallExtension(t *testing.T) { // then require.NotNil(t, resp1) - assert.Equal(t, http.StatusBadRequest, resp1.StatusCode) + assert.Equal(t, http.StatusUnauthorized, resp1.StatusCode) body, err := io.ReadAll(resp1.Body) require.NoError(t, err) actual := strings.TrimSuffix(string(body), "\n") - assert.Equal(t, "invalid extension", actual) + assert.Equal(t, "Unauthorized extension request", actual) }) t.Run("will return 400 if no extension name is provided", func(t *testing.T) { // given diff --git a/server/project/project.go b/server/project/project.go index 62487b268a705..94cd8763eda03 100644 --- a/server/project/project.go +++ b/server/project/project.go @@ -398,43 +398,39 @@ func (s *Server) Update(ctx context.Context, q *project.ProjectUpdateRequest) (* return nil, err } - var srcValidatedApps []v1alpha1.Application - var dstValidatedApps []v1alpha1.Application getProjectClusters := func(project string) ([]*v1alpha1.Cluster, error) { return s.db.GetProjectClusters(ctx, project) } - for _, a := range argo.FilterByProjects(appsList.Items, []string{q.Project.Name}) { - if oldProj.IsSourcePermitted(a.Spec.GetSource()) { - srcValidatedApps = append(srcValidatedApps, a) - } - - dstPermitted, err := oldProj.IsDestinationPermitted(a.Spec.Destination, getProjectClusters) - if err != nil { - return nil, err - } - - if dstPermitted { - dstValidatedApps = append(dstValidatedApps, a) - } - } - invalidSrcCount := 0 invalidDstCount := 0 - for _, a := range srcValidatedApps { - if !q.Project.IsSourcePermitted(a.Spec.GetSource()) { + for _, a := range argo.FilterByProjects(appsList.Items, []string{q.Project.Name}) { + if oldProj.IsSourcePermitted(a.Spec.GetSource()) && !q.Project.IsSourcePermitted(a.Spec.GetSource()) { invalidSrcCount++ } - } - for _, a := range dstValidatedApps { - dstPermitted, err := q.Project.IsDestinationPermitted(a.Spec.Destination, getProjectClusters) + + destCluster, err := argo.GetDestinationCluster(ctx, a.Spec.Destination, s.db) + if err != nil { + if err.Error() == argo.ErrDestinationMissing { + invalidDstCount++ + } else { + return nil, err + } + } + dstPermitted, err := oldProj.IsDestinationPermitted(destCluster, a.Spec.Destination.Namespace, getProjectClusters) if err != nil { return nil, err } - if !dstPermitted { - invalidDstCount++ + if dstPermitted { + dstPermitted, err = q.Project.IsDestinationPermitted(destCluster, a.Spec.Destination.Namespace, getProjectClusters) + if err != nil { + return nil, err + } + if !dstPermitted { + invalidDstCount++ + } } } diff --git a/server/project/project_test.go b/server/project/project_test.go index cd5a8de5fee53..8fd1317b38e07 100644 --- a/server/project/project_test.go +++ b/server/project/project_test.go @@ -58,6 +58,42 @@ func TestProjectServer(t *testing.T) { "admin.password": []byte("test"), "server.secretkey": []byte("test"), }, + }, &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: "cluster-1", + Namespace: testNamespace, + Labels: map[string]string{ + common.LabelKeySecretType: common.LabelValueSecretTypeCluster, + }, + }, + Data: map[string][]byte{ + "name": []byte("server1"), + "server": []byte("https://server1"), + }, + }, &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: "cluster-2", + Namespace: testNamespace, + Labels: map[string]string{ + common.LabelKeySecretType: common.LabelValueSecretTypeCluster, + }, + }, + Data: map[string][]byte{ + "name": []byte("server2"), + "server": []byte("https://server2"), + }, + }, &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: "cluster-3", + Namespace: testNamespace, + Labels: map[string]string{ + common.LabelKeySecretType: common.LabelValueSecretTypeCluster, + }, + }, + Data: map[string][]byte{ + "name": []byte("server3"), + "server": []byte("https://server3"), + }, }) settingsMgr := settings.NewSettingsManager(context.Background(), kubeclientset, testNamespace) enforcer := newEnforcer(kubeclientset) @@ -201,7 +237,7 @@ func TestProjectServer(t *testing.T) { t.Run("TestRemoveSourceSuccessful", func(t *testing.T) { existingApp := v1alpha1.Application{ ObjectMeta: v1.ObjectMeta{Name: "test", Namespace: "default"}, - Spec: v1alpha1.ApplicationSpec{Source: &v1alpha1.ApplicationSource{}, Project: "test"}, + Spec: v1alpha1.ApplicationSpec{Destination: v1alpha1.ApplicationDestination{Server: "https://server1"}, Source: &v1alpha1.ApplicationSource{}, Project: "test"}, } argoDB := db.NewDB("default", settingsMgr, kubeclientset) @@ -231,7 +267,7 @@ func TestProjectServer(t *testing.T) { require.Error(t, err) statusCode, _ := status.FromError(err) - assert.Equal(t, codes.InvalidArgument, statusCode.Code()) + assert.Equalf(t, codes.InvalidArgument, statusCode.Code(), "Got unexpected error code with error: %v", err) }) t.Run("TestRemoveSourceUsedByAppSuccessfulIfPermittedByAnotherSrc", func(t *testing.T) { @@ -239,7 +275,7 @@ func TestProjectServer(t *testing.T) { proj.Spec.SourceRepos = []string{"https://github.com/argoproj/argo-cd.git", "https://github.com/argoproj/*"} existingApp := v1alpha1.Application{ ObjectMeta: v1.ObjectMeta{Name: "test", Namespace: "default"}, - Spec: v1alpha1.ApplicationSpec{Project: "test", Source: &v1alpha1.ApplicationSource{RepoURL: "https://github.com/argoproj/argo-cd.git"}}, + Spec: v1alpha1.ApplicationSpec{Destination: v1alpha1.ApplicationDestination{Server: "https://server1"}, Project: "test", Source: &v1alpha1.ApplicationSource{RepoURL: "https://github.com/argoproj/argo-cd.git"}}, } argoDB := db.NewDB("default", settingsMgr, kubeclientset) projectServer := NewServer("default", fake.NewSimpleClientset(), apps.NewSimpleClientset(proj, &existingApp), enforcer, sync.NewKeyLock(), nil, nil, projInformer, settingsMgr, argoDB, testEnableEventList) diff --git a/server/server.go b/server/server.go index eaef1262cf9ba..aacadf96354d5 100644 --- a/server/server.go +++ b/server/server.go @@ -335,7 +335,7 @@ func NewServer(ctx context.Context, opts ArgoCDServerOpts, appsetOpts Applicatio ag := extension.NewDefaultApplicationGetter(appLister) pg := extension.NewDefaultProjectGetter(projLister, dbInstance) ug := extension.NewDefaultUserGetter(policyEnf) - em := extension.NewManager(logger, opts.Namespace, sg, ag, pg, enf, ug) + em := extension.NewManager(logger, opts.Namespace, sg, ag, pg, dbInstance, enf, ug) noopShutdown := func() { log.Error("API Server Shutdown function called but server is not started yet.") } diff --git a/util/argo/argo.go b/util/argo/argo.go index a965ebd5cd56d..e5e3ba4d5acc8 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -3,6 +3,7 @@ package argo import ( "context" "encoding/json" + "errors" "fmt" "regexp" "sort" @@ -33,7 +34,7 @@ import ( ) const ( - errDestinationMissing = "Destination server missing from app spec" + ErrDestinationMissing = "Destination server missing from app spec" ) var ErrAnotherOperationInProgress = status.Errorf(codes.FailedPrecondition, "another operation is already in progress") @@ -321,7 +322,7 @@ func ValidateRepo( return nil, fmt.Errorf("error getting permitted repo creds: %w", err) } - cluster, err := db.GetCluster(context.Background(), spec.Destination.Server) + cluster, err := GetDestinationCluster(context.Background(), spec.Destination, db) if err != nil { conditions = append(conditions, argoappv1.ApplicationCondition{ Type: argoappv1.ApplicationConditionInvalidSpecError, @@ -485,46 +486,6 @@ func GetRefSources(ctx context.Context, sources argoappv1.ApplicationSources, pr return refSources, nil } -// 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. 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) - 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() && !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 -} - func validateSourcePermissions(source argoappv1.ApplicationSource, hasMultipleSources bool) []argoappv1.ApplicationCondition { var conditions []argoappv1.ApplicationCondition if hasMultipleSources { @@ -588,8 +549,8 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p } } - // ValidateDestination will resolve the destination's server address from its name for us, if possible - if err := ValidateDestination(ctx, &spec.Destination, db); err != nil { + destCluster, err := GetDestinationCluster(ctx, spec.Destination, db) + if err != nil { conditions = append(conditions, argoappv1.ApplicationCondition{ Type: argoappv1.ApplicationConditionInvalidSpecError, Message: err.Error(), @@ -597,8 +558,8 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p return conditions, nil } - if spec.Destination.Server != "" { - permitted, err := proj.IsDestinationPermitted(spec.Destination, func(project string) ([]*argoappv1.Cluster, error) { + if destCluster.Server != "" { + permitted, err := proj.IsDestinationPermitted(destCluster, spec.Destination.Namespace, func(project string) ([]*argoappv1.Cluster, error) { return db.GetProjectClusters(ctx, project) }) if err != nil { @@ -610,20 +571,8 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p Message: fmt.Sprintf("application destination server '%s' and namespace '%s' do not match any of the allowed destinations in project '%s'", spec.Destination.Server, spec.Destination.Namespace, spec.Project), }) } - // Ensure the k8s cluster the app is referencing, is configured in Argo CD - _, err = db.GetCluster(ctx, spec.Destination.Server) - if err != nil { - if errStatus, ok := status.FromError(err); ok && errStatus.Code() == codes.NotFound { - conditions = append(conditions, argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: fmt.Sprintf("cluster '%s' has not been configured", spec.Destination.Server), - }) - } else { - return nil, fmt.Errorf("error getting cluster: %w", err) - } - } - } else if spec.Destination.Server == "" { - conditions = append(conditions, argoappv1.ApplicationCondition{Type: argoappv1.ApplicationConditionInvalidSpecError, Message: errDestinationMissing}) + } else if destCluster.Server == "" { + conditions = append(conditions, argoappv1.ApplicationCondition{Type: argoappv1.ApplicationConditionInvalidSpecError, Message: ErrDestinationMissing}) } return conditions, nil } @@ -732,12 +681,6 @@ func verifyGenerateManifests( refSources argoappv1.RefTargetRevisionMapping, ) []argoappv1.ApplicationCondition { var conditions []argoappv1.ApplicationCondition - if app.Spec.Destination.Server == "" { - conditions = append(conditions, argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: errDestinationMissing, - }) - } // If source is Kustomize add build options kustomizeSettings, err := settingsMgr.GetKustomizeSettings() if err != nil { @@ -959,33 +902,39 @@ func GetPermittedRepos(proj *argoappv1.AppProject, repos []*argoappv1.Repository return permittedRepos, nil } -func getDestinationServer(ctx context.Context, db db.ArgoDB, clusterName string) (string, error) { - servers, err := db.GetClusterServersByName(ctx, clusterName) - if err != nil { - return "", fmt.Errorf("error getting cluster server by name %q: %w", clusterName, err) - } - 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 servers[0], nil +type ClusterGetter interface { + GetCluster(ctx context.Context, name string) (*argoappv1.Cluster, error) + GetClusterServersByName(ctx context.Context, server string) ([]string, error) } -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) +func GetDestinationCluster(ctx context.Context, destination argoappv1.ApplicationDestination, db ClusterGetter) (*argoappv1.Cluster, error) { + if destination.Name != "" && destination.Server != "" { + return nil, fmt.Errorf("application destination can't have both name and server defined: %s %s", destination.Name, destination.Server) } - - if cluster.Name == "" { - return "", fmt.Errorf("there are no clusters with this URL: %s", server) + if destination.Server != "" { + cluster, err := db.GetCluster(ctx, destination.Server) + if err != nil { + return cluster, fmt.Errorf("error getting cluster by server %q: %w", destination.Server, err) + } + return cluster, nil + } else if destination.Name != "" { + clusterURLs, err := db.GetClusterServersByName(ctx, destination.Name) + if err != nil { + return nil, fmt.Errorf("error getting cluster by name %q: %w", destination.Name, err) + } + if len(clusterURLs) == 0 { + return nil, fmt.Errorf("there are no clusters with this name: %s", destination.Name) + } + if len(clusterURLs) > 1 { + return nil, fmt.Errorf("there are %d clusters with the same name: [%s]", len(clusterURLs), strings.Join(clusterURLs, " ")) + } + cluster, err := db.GetCluster(ctx, clusterURLs[0]) + if err != nil { + return nil, fmt.Errorf("error getting cluster by URL: %w", err) + } + return cluster, nil } - return cluster.Name, nil + return nil, errors.New(ErrDestinationMissing) } func GetGlobalProjects(proj *argoappv1.AppProject, projLister applicationsv1.AppProjectLister, settingsManager *settings.SettingsManager) []*argoappv1.AppProject { diff --git a/util/argo/argo_test.go b/util/argo/argo_test.go index d07360665b20f..d8af72ef399b1 100644 --- a/util/argo/argo_test.go +++ b/util/argo/argo_test.go @@ -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, "unable to find destination server") + assert.Contains(t, conditions[0].Message, "Cluster does not exist") }) t.Run("Destination cluster name does not exist", func(t *testing.T) { @@ -805,7 +805,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, "unable to find destination server: there are no clusters with this name: does-not-exist") + assert.Contains(t, conditions[0].Message, "there are no clusters with this name: does-not-exist") }) t.Run("Cannot get cluster info from DB", func(t *testing.T) { @@ -926,16 +926,21 @@ func TestSetAppOperations(t *testing.T) { }) } -func TestValidateDestination(t *testing.T) { +func TestGetDestinationCluster(t *testing.T) { t.Run("Validate destination with server url", func(t *testing.T) { dest := argoappv1.ApplicationDestination{ Server: "https://127.0.0.1:6443", Namespace: "default", } - appCond := ValidateDestination(context.Background(), &dest, nil) - require.Error(t, appCond) - assert.False(t, dest.IsServerInferred()) + expectedCluster := &argoappv1.Cluster{Server: "https://127.0.0.1:6443"} + db := &dbmocks.ArgoDB{} + db.On("GetCluster", context.Background(), "https://127.0.0.1:6443").Return(expectedCluster, nil) + + cluster, err := GetDestinationCluster(context.Background(), dest, db) + require.NoError(t, err) + require.NotNil(t, expectedCluster) + assert.Equal(t, expectedCluster, cluster) }) t.Run("Validate destination with server name", func(t *testing.T) { @@ -945,11 +950,11 @@ func TestValidateDestination(t *testing.T) { db := &dbmocks.ArgoDB{} db.On("GetClusterServersByName", context.Background(), "minikube").Return([]string{"https://127.0.0.1:6443"}, nil) + db.On("GetCluster", context.Background(), "https://127.0.0.1:6443").Return(&argoappv1.Cluster{Server: "https://127.0.0.1:6443", Name: "minikube"}, nil) - appCond := ValidateDestination(context.Background(), &dest, db) - require.NoError(t, appCond) - assert.Equal(t, "https://127.0.0.1:6443", dest.Server) - assert.True(t, dest.IsServerInferred()) + destCluster, err := GetDestinationCluster(context.Background(), dest, db) + require.NoError(t, err) + assert.Equal(t, "https://127.0.0.1:6443", destCluster.Server) }) t.Run("Error when having both server url and name", func(t *testing.T) { @@ -959,9 +964,8 @@ func TestValidateDestination(t *testing.T) { Namespace: "default", } - err := ValidateDestination(context.Background(), &dest, nil) + _, err := GetDestinationCluster(context.Background(), dest, nil) assert.Equal(t, "application destination can't have both name and server defined: minikube https://127.0.0.1:6443", err.Error()) - assert.False(t, dest.IsServerInferred()) }) t.Run("GetClusterServersByName fails", func(t *testing.T) { @@ -972,9 +976,8 @@ func TestValidateDestination(t *testing.T) { db := &dbmocks.ArgoDB{} db.On("GetClusterServersByName", context.Background(), mock.Anything).Return(nil, fmt.Errorf("an error occurred")) - err := ValidateDestination(context.Background(), &dest, db) + _, err := GetDestinationCluster(context.Background(), dest, db) require.ErrorContains(t, err, "an error occurred") - assert.False(t, dest.IsServerInferred()) }) t.Run("Destination cluster does not exist", func(t *testing.T) { @@ -985,9 +988,8 @@ func TestValidateDestination(t *testing.T) { db := &dbmocks.ArgoDB{} 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()) - assert.False(t, dest.IsServerInferred()) + _, err := GetDestinationCluster(context.Background(), dest, db) + assert.Equal(t, "there are no clusters with this name: minikube", err.Error()) }) t.Run("Validate too many clusters with the same name", func(t *testing.T) { @@ -998,9 +1000,8 @@ func TestValidateDestination(t *testing.T) { db := &dbmocks.ArgoDB{} 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()) - assert.False(t, dest.IsServerInferred()) + _, err := GetDestinationCluster(context.Background(), dest, db) + assert.Equal(t, "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/webhook/webhook.go b/util/webhook/webhook.go index df5c1fecc1273..03b7770bf9fd9 100644 --- a/util/webhook/webhook.go +++ b/util/webhook/webhook.go @@ -350,7 +350,7 @@ func getWebUrlRegex(webURL string) (*regexp.Regexp, error) { } func (a *ArgoCDWebhookHandler) storePreviouslyCachedManifests(app *v1alpha1.Application, change changeInfo, trackingMethod string, appInstanceLabelKey string, installationID string) error { - err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, a.db) + _, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, a.db) if err != nil { return fmt.Errorf("error validating destination: %w", err) }