diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 3dea14193e..f640f4d3d5 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -504,10 +504,6 @@ func (ctrl *ApplicationController) processAppOperationQueueItem() (processNext b log.Warnf("Key '%s' in index is not an application", appKey) return } - if destCond := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); destCond != nil { - ctrl.setAppCondition(app, *destCond) - ctrl.auditLogger.LogAppEvent(app, argo.EventInfo{Reason: argo.EventReasonStatusRefreshed, Type: v1.EventTypeWarning}, destCond.Message) - } if app.Operation != nil { ctrl.processRequestedAppOperation(app) } else if app.DeletionTimestamp != nil && app.CascadedDeletion() { @@ -707,7 +703,12 @@ func (ctrl *ApplicationController) processRequestedAppOperation(app *appv1.Appli logCtx.Infof("Initialized new operation: %v", *app.Operation) } - ctrl.appStateManager.SyncAppState(app, state) + if err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); err != nil { + state.Phase = synccommon.OperationFailed + state.Message = err.Error() + } else { + ctrl.appStateManager.SyncAppState(app, state) + } if state.Phase == synccommon.OperationRunning { // It's possible for an app to be terminated while we were operating on it. We do not want @@ -828,12 +829,12 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo log.Warnf("Key '%s' in index is not an application", appKey) return } + origApp = origApp.DeepCopy() needRefresh, refreshType, comparisonLevel := ctrl.needRefreshAppStatus(origApp, ctrl.statusRefreshTimeout) if !needRefresh { return } - argo.ValidateDestination(context.Background(), &origApp.Spec.Destination, ctrl.db) app := origApp.DeepCopy() logCtx := log.WithFields(log.Fields{"application": app.Name}) @@ -845,6 +846,7 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo "time_ms": reconcileDuration.Milliseconds(), "level": comparisonLevel, "dest-server": origApp.Spec.Destination.Server, + "dest-name": origApp.Spec.Destination.Name, "dest-namespace": origApp.Spec.Destination.Namespace, }).Info("Reconciliation completed") }() @@ -998,10 +1000,13 @@ func (ctrl *ApplicationController) refreshAppConditions(app *appv1.Application) }) } } else { - destCondition := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db) - if destCondition != nil { - errorConditions = append(errorConditions, *destCondition) + if err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); err != nil { + errorConditions = append(errorConditions, appv1.ApplicationCondition{ + Message: err.Error(), + Type: appv1.ApplicationConditionInvalidSpecError, + }) } + specConditions, err := argo.ValidatePermissions(context.Background(), &app.Spec, proj, ctrl.db) if err != nil { errorConditions = append(errorConditions, appv1.ApplicationCondition{ diff --git a/controller/cache/cache.go b/controller/cache/cache.go index bacb139c04..97652d506b 100644 --- a/controller/cache/cache.go +++ b/controller/cache/cache.go @@ -18,6 +18,7 @@ import ( "github.com/argoproj/argo-cd/controller/metrics" appv1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/util/argo" "github.com/argoproj/argo-cd/util/db" "github.com/argoproj/argo-cd/util/lua" "github.com/argoproj/argo-cd/util/settings" @@ -351,9 +352,17 @@ func (c *liveStateCache) GetVersionsInfo(serverURL string) (string, []metav1.API return clusterInfo.GetServerVersion(), clusterInfo.GetAPIGroups(), nil } -func isClusterHasApps(apps []interface{}, cluster *appv1.Cluster) bool { +func (c *liveStateCache) isClusterHasApps(apps []interface{}, cluster *appv1.Cluster) bool { for _, obj := range apps { - if app, ok := obj.(*appv1.Application); ok && app.Spec.Destination.Server == cluster.Server { + app, ok := obj.(*appv1.Application) + if !ok { + continue + } + err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, c.db) + if err != nil { + continue + } + if app.Spec.Destination.Server == cluster.Server { return true } } @@ -420,7 +429,7 @@ func (c *liveStateCache) handleAddEvent(cluster *appv1.Cluster) { _, ok := c.clusters[cluster.Server] c.lock.Unlock() if !ok { - if isClusterHasApps(c.appInformer.GetStore().List(), cluster) { + if c.isClusterHasApps(c.appInformer.GetStore().List(), cluster) { go func() { // warm up cache for cluster with apps _, _ = c.getSyncedCluster(cluster.Server) diff --git a/server/application/application.go b/server/application/application.go index cdcf7bd11d..a68279a8b6 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -689,8 +689,8 @@ func (s *Server) validateAndNormalizeApp(ctx context.Context, app *appv1.Applica return err } - if cond := argo.ValidateDestination(ctx, &app.Spec.Destination, s.db); cond != nil { - return status.Errorf(codes.InvalidArgument, "application destination spec is invalid: %s", argo.FormatAppConditions([]appv1.ApplicationCondition{*cond})) + if err := argo.ValidateDestination(ctx, &app.Spec.Destination, s.db); err != nil { + return status.Errorf(codes.InvalidArgument, "application destination spec is invalid: %s", err.Error()) } conditions, err := argo.ValidateRepo(ctx, app, s.repoClientset, s.db, kustomizeOptions, plugins, s.kubectl) @@ -714,8 +714,8 @@ func (s *Server) validateAndNormalizeApp(ctx context.Context, app *appv1.Applica } func (s *Server) getApplicationClusterConfig(ctx context.Context, a *appv1.Application) (*rest.Config, error) { - if condition := argo.ValidateDestination(ctx, &a.Spec.Destination, s.db); condition != nil { - return nil, errors.New(condition.Message) + 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) if err != nil { diff --git a/util/argo/argo.go b/util/argo/argo.go index 3d4c5a407a..8c6cd1f70c 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -269,29 +269,20 @@ func enrichSpec(spec *argoappv1.ApplicationSpec, appDetails *apiclient.RepoAppDe // 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 *argoappv1.ApplicationDestination, db db.ArgoDB) *argoappv1.ApplicationCondition { +func ValidateDestination(ctx context.Context, dest *argoappv1.ApplicationDestination, db db.ArgoDB) error { if dest.Name != "" { if dest.Server == "" { server, err := getDestinationServer(ctx, db, dest.Name) if err != nil { - return &argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: fmt.Sprintf("unable to find destination server: %v", err), - } + return fmt.Errorf("unable to find destination server: %v", err) } if server == "" { - return &argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: fmt.Sprintf("application references destination cluster %s which does not exist", dest.Name), - } + return fmt.Errorf("application references destination cluster %s which does not exist", dest.Name) } dest.SetInferredServer(server) } else { if !dest.IsServerInferred() { - return &argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: fmt.Sprintf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server), - } + return fmt.Errorf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server) } } } diff --git a/util/argo/argo_test.go b/util/argo/argo_test.go index 2cf9d1ee26..472eac4254 100644 --- a/util/argo/argo_test.go +++ b/util/argo/argo_test.go @@ -623,9 +623,8 @@ func TestValidateDestination(t *testing.T) { Namespace: "default", } - appCond := ValidateDestination(context.Background(), &dest, nil) - assert.Equal(t, argoappv1.ApplicationConditionInvalidSpecError, appCond.Type) - assert.Equal(t, "application destination can't have both name and server defined: minikube https://127.0.0.1:6443", appCond.Message) + err := ValidateDestination(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()) }) @@ -637,9 +636,8 @@ func TestValidateDestination(t *testing.T) { db := &dbmocks.ArgoDB{} db.On("ListClusters", context.Background()).Return(nil, fmt.Errorf("an error occured")) - appCond := ValidateDestination(context.Background(), &dest, db) - assert.Equal(t, argoappv1.ApplicationConditionInvalidSpecError, appCond.Type) - assert.Equal(t, "unable to find destination server: an error occured", appCond.Message) + err := ValidateDestination(context.Background(), &dest, db) + assert.Equal(t, "unable to find destination server: an error occured", err.Error()) assert.False(t, dest.IsServerInferred()) }) @@ -658,9 +656,8 @@ func TestValidateDestination(t *testing.T) { }, }, nil) - appCond := ValidateDestination(context.Background(), &dest, db) - assert.Equal(t, argoappv1.ApplicationConditionInvalidSpecError, appCond.Type) - assert.Equal(t, "unable to find destination server: there are no clusters with this name: minikube", appCond.Message) + 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()) }) @@ -683,9 +680,8 @@ func TestValidateDestination(t *testing.T) { }, }, nil) - appCond := ValidateDestination(context.Background(), &dest, db) - assert.Equal(t, argoappv1.ApplicationConditionInvalidSpecError, appCond.Type) - 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]", appCond.Message) + 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()) })