Elgato_dark/argo-cd
1
0
Fork 0
mirror of https://github.com/argoproj/argo-cd synced 2026-04-21 17:07:16 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 22

fix: application controller should not modify cached applications (#3821)

Browse source
This commit is contained in:
Alexander Matyushentsev 2020-06-22 11:04:25 -07:00 committed by GitHub
parent be718e2b61
commit fc2e3f82a2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 42 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
controller/appcontroller.go
View file

@ -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{

15
controller/cache/cache.go vendored
View file

@ -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)

8
server/application/application.go
View file

@ -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 {

17
util/argo/argo.go
View file

@ -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)
}
}
}

20
util/argo/argo_test.go
View file

@ -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())
})