diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index c83574694f..1af42f1121 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -1813,6 +1813,110 @@ func shortenRevision(revision string, length int) string { return revision } +// buildEnvForRevision builds an Env containing only the revision-derived variables, which are the +// only ones that can differ between two revisions of the same application source. This is used by +// buildEnvHasChanged to compare parameter substitution results across revisions without needing a +// full ManifestRequest. +func buildEnvForRevision(appSource *v1alpha1.ApplicationSource, repo *v1alpha1.Repository, revision string) *v1alpha1.Env { + shortRevision := shortenRevision(revision, 7) + shortRevision8 := shortenRevision(revision, 8) + repoURL := "" + if repo != nil { + repoURL = repo.Repo + } + sourcePath := "" + sourceTargetRevision := "" + if appSource != nil { + sourcePath = appSource.Path + sourceTargetRevision = appSource.TargetRevision + } + return &v1alpha1.Env{ + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_REVISION", Value: revision}, + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_REVISION_SHORT", Value: shortRevision}, + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_REVISION_SHORT_8", Value: shortRevision8}, + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_SOURCE_REPO_URL", Value: repoURL}, + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_SOURCE_PATH", Value: sourcePath}, + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_SOURCE_TARGET_REVISION", Value: sourceTargetRevision}, + } +} + +// buildEnvHasChanged returns true if any application source parameter that undergoes environment +// variable substitution during manifest generation would produce a different value between +// oldRevision and newRevision. When true, manifest regeneration is required even if no files in +// the manifest-generate-paths have changed. +func buildEnvHasChanged(appSource *v1alpha1.ApplicationSource, repo *v1alpha1.Repository, oldRevision, newRevision string) bool { + if appSource == nil { + return false + } + oldEnv := buildEnvForRevision(appSource, repo, oldRevision) + newEnv := buildEnvForRevision(appSource, repo, newRevision) + return appParametersWouldChange(appSource, oldEnv, newEnv) +} + +// appParametersWouldChange checks whether environment substitution would produce different results +// for any application source parameter between two environment contexts. +func appParametersWouldChange(appSource *v1alpha1.ApplicationSource, oldEnv, newEnv *v1alpha1.Env) bool { + envChanged := func(value string) bool { + return oldEnv.Envsubst(value) != newEnv.Envsubst(value) + } + + if appSource.Helm != nil { + for _, param := range appSource.Helm.Parameters { + if envChanged(param.Value) { + return true + } + } + for _, fileParam := range appSource.Helm.FileParameters { + if envChanged(fileParam.Path) { + return true + } + } + } + + if appSource.Kustomize != nil { + for _, image := range appSource.Kustomize.Images { + if envChanged(string(image)) { + return true + } + } + for _, v := range appSource.Kustomize.CommonLabels { + if envChanged(v) { + return true + } + } + if appSource.Kustomize.CommonAnnotationsEnvsubst { + for _, v := range appSource.Kustomize.CommonAnnotations { + if envChanged(v) { + return true + } + } + } + } + + if appSource.Directory != nil && !appSource.Directory.Jsonnet.IsZero() { + for _, tla := range appSource.Directory.Jsonnet.TLAs { + if envChanged(tla.Value) { + return true + } + } + for _, extVar := range appSource.Directory.Jsonnet.ExtVars { + if envChanged(extVar.Value) { + return true + } + } + } + + if appSource.Plugin != nil { + for _, envEntry := range appSource.Plugin.Env { + if envChanged(envEntry.Value) { + return true + } + } + } + + return false +} + func newEnvRepoQuery(q *apiclient.RepoServerAppDetailsQuery, revision string) *v1alpha1.Env { return &v1alpha1.Env{ &v1alpha1.EnvEntry{Name: "ARGOCD_APP_NAME", Value: q.AppName}, @@ -3456,6 +3560,17 @@ func (s *Service) UpdateRevisionForPaths(_ context.Context, request *apiclient.U }, nil } + // Even when no files in manifest-generate-paths changed, build environment variables like + // $ARGOCD_APP_REVISION that are used in parameters will produce different values for the new + // revision, so we must regenerate. + if buildEnvHasChanged(request.ApplicationSource, request.GetRepo(), sRevision, rRevision) { + logCtx.Debugf("build environment variable changes detected for application %s from revision %s to %s", request.AppName, sRevision, rRevision) + return &apiclient.UpdateRevisionForPathsResponse{ + Revision: rRevision, + Changes: true, + }, nil + } + // No changes detected, update the cache using resolved revisions err := s.updateCachedRevision(logCtx, sRevision, rRevision, request, oldRepoRefs, newRepoRefs) if err != nil { diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index 88ae19bffe..7d8e51289e 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -5127,6 +5127,96 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, want: &apiclient.UpdateRevisionForPathsResponse{ Revision: "0.0.1", Changes: true, }, wantErr: assert.Error, cacheHit: nil}, + {name: "HelmParamWithEnvVarChangesRevision", fields: func() fields { + s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { + gitClient.EXPECT().Init().Return(nil) + gitClient.EXPECT().Fetch(mock.Anything, mock.Anything).Once().Return(nil) + gitClient.EXPECT().IsRevisionPresent("632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false) + gitClient.EXPECT().Checkout(mock.Anything, mock.Anything).Return("", nil) + gitClient.EXPECT().IsRevisionPresent("1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(false) + gitClient.EXPECT().Fetch(mock.Anything, mock.Anything).Once().Return(nil) + gitClient.EXPECT().IsRevisionPresent("1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true) + gitClient.EXPECT().LsRemote("HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil) + gitClient.EXPECT().LsRemote("SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil) + paths.EXPECT().GetPath(mock.Anything).Return(".", nil) + paths.EXPECT().GetPathIfExists(mock.Anything).Return(".") + gitClient.EXPECT().Root().Return("") + gitClient.EXPECT().ChangedFiles(mock.Anything, mock.Anything).Return([]string{}, nil) + }, ".") + return fields{service: s, cache: c} + }(), args: args{ + ctx: t.Context(), + request: &apiclient.UpdateRevisionForPathsRequest{ + Repo: &v1alpha1.Repository{Repo: "a-url.com", Type: "git"}, + Revision: "HEAD", + SyncedRevision: "SYNCEDHEAD", + Paths: []string{"."}, + ApplicationSource: &v1alpha1.ApplicationSource{ + Path: ".", + Helm: &v1alpha1.ApplicationSourceHelm{ + Parameters: []v1alpha1.HelmParameter{ + {Name: "image.tag", Value: "$ARGOCD_APP_REVISION"}, + }, + }, + }, + }, + }, want: &apiclient.UpdateRevisionForPathsResponse{ + Revision: "632039659e542ed7de0c170a4fcc1c571b288fc0", + Changes: true, + }, wantErr: assert.NoError, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 0, + ExternalGets: 1, + ExternalSets: 1, + }}, + {name: "HelmParamWithNoEnvVarNoExtraChanges", fields: func() fields { + s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { + gitClient.EXPECT().Init().Return(nil) + gitClient.EXPECT().Fetch(mock.Anything, mock.Anything).Once().Return(nil) + gitClient.EXPECT().IsRevisionPresent("632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false) + gitClient.EXPECT().Checkout(mock.Anything, mock.Anything).Return("", nil) + gitClient.EXPECT().IsRevisionPresent("1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(false) + gitClient.EXPECT().Fetch(mock.Anything, mock.Anything).Once().Return(nil) + gitClient.EXPECT().IsRevisionPresent("1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true) + gitClient.EXPECT().LsRemote("HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil) + gitClient.EXPECT().LsRemote("SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil) + paths.EXPECT().GetPath(mock.Anything).Return(".", nil) + paths.EXPECT().GetPathIfExists(mock.Anything).Return(".") + gitClient.EXPECT().Root().Return("") + gitClient.EXPECT().ChangedFiles(mock.Anything, mock.Anything).Return([]string{}, nil) + }, ".") + return fields{service: s, cache: c} + }(), args: args{ + ctx: t.Context(), + request: &apiclient.UpdateRevisionForPathsRequest{ + Repo: &v1alpha1.Repository{Repo: "a-url.com", Type: "git"}, + Revision: "HEAD", + SyncedRevision: "SYNCEDHEAD", + Paths: []string{"."}, + AppLabelKey: "app.kubernetes.io/name", + AppName: "static-param-app", + Namespace: "default", + TrackingMethod: "annotation+label", + ApplicationSource: &v1alpha1.ApplicationSource{ + Path: ".", + Helm: &v1alpha1.ApplicationSourceHelm{ + Parameters: []v1alpha1.HelmParameter{ + {Name: "replicas", Value: "3"}, + }, + }, + }, + KubeVersion: "v1.16.0", + }, + }, want: &apiclient.UpdateRevisionForPathsResponse{ + Revision: "632039659e542ed7de0c170a4fcc1c571b288fc0", + Changes: true, // FIXME: need to fix changes=true, because now test can't mock Rename cache + }, wantErr: assert.NoError, cacheHit: &cacheHit{ + previousRevision: "1e67a504d03def3a6a1125d934cb511680f72555", + revision: "632039659e542ed7de0c170a4fcc1c571b288fc0", + }, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 1, + ExternalGets: 1, + ExternalSets: 1, + }}, {name: "IgnoreRefSourcesForGitSource", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { gitClient.EXPECT().Init().Return(nil) @@ -5208,6 +5298,136 @@ func TestUpdateRevisionForPaths(t *testing.T) { } } +func TestAppParametersWouldChange(t *testing.T) { + oldEnv := &v1alpha1.Env{ + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_REVISION", Value: "aaaaaaa"}, + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_REVISION_SHORT", Value: "aaaaaaa"}, + } + newEnv := &v1alpha1.Env{ + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_REVISION", Value: "bbbbbbb"}, + &v1alpha1.EnvEntry{Name: "ARGOCD_APP_REVISION_SHORT", Value: "bbbbbbb"}, + } + + t.Run("HelmParamWithRevisionEnvVar", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Helm: &v1alpha1.ApplicationSourceHelm{ + Parameters: []v1alpha1.HelmParameter{ + {Name: "image.tag", Value: "$ARGOCD_APP_REVISION"}, + }, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("HelmParamWithStaticValue", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Helm: &v1alpha1.ApplicationSourceHelm{ + Parameters: []v1alpha1.HelmParameter{ + {Name: "replicas", Value: "3"}, + }, + }, + } + assert.False(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("HelmFileParamWithRevisionEnvVar", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Helm: &v1alpha1.ApplicationSourceHelm{ + FileParameters: []v1alpha1.HelmFileParameter{ + {Name: "config", Path: "configs/$ARGOCD_APP_REVISION/values.yaml"}, + }, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("KustomizeImageWithRevisionEnvVar", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Kustomize: &v1alpha1.ApplicationSourceKustomize{ + Images: v1alpha1.KustomizeImages{"myrepo/myimage:$ARGOCD_APP_REVISION"}, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("KustomizeCommonLabelWithRevisionEnvVar", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Kustomize: &v1alpha1.ApplicationSourceKustomize{ + CommonLabels: map[string]string{"revision": "$ARGOCD_APP_REVISION"}, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("KustomizeCommonAnnotationWithEnvVarAndEnvsubstEnabled", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Kustomize: &v1alpha1.ApplicationSourceKustomize{ + CommonAnnotations: map[string]string{"revision": "$ARGOCD_APP_REVISION"}, + CommonAnnotationsEnvsubst: true, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("KustomizeCommonAnnotationWithEnvVarButEnvsubstDisabled", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Kustomize: &v1alpha1.ApplicationSourceKustomize{ + CommonAnnotations: map[string]string{"revision": "$ARGOCD_APP_REVISION"}, + CommonAnnotationsEnvsubst: false, + }, + } + assert.False(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("JsonnetTLAWithRevisionEnvVar", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Directory: &v1alpha1.ApplicationSourceDirectory{ + Jsonnet: v1alpha1.ApplicationSourceJsonnet{ + TLAs: []v1alpha1.JsonnetVar{{Name: "revision", Value: "$ARGOCD_APP_REVISION"}}, + }, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("JsonnetExtVarWithRevisionEnvVar", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Directory: &v1alpha1.ApplicationSourceDirectory{ + Jsonnet: v1alpha1.ApplicationSourceJsonnet{ + ExtVars: []v1alpha1.JsonnetVar{{Name: "revision", Value: "$ARGOCD_APP_REVISION"}}, + }, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("PluginEnvWithRevisionEnvVar", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Plugin: &v1alpha1.ApplicationSourcePlugin{ + Env: v1alpha1.Env{ + &v1alpha1.EnvEntry{Name: "MY_REVISION", Value: "$ARGOCD_APP_REVISION"}, + }, + }, + } + assert.True(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("PluginEnvWithStaticValue", func(t *testing.T) { + source := &v1alpha1.ApplicationSource{ + Plugin: &v1alpha1.ApplicationSourcePlugin{ + Env: v1alpha1.Env{ + &v1alpha1.EnvEntry{Name: "ENV", Value: "production"}, + }, + }, + } + assert.False(t, appParametersWouldChange(source, oldEnv, newEnv)) + }) + + t.Run("NilSource", func(t *testing.T) { + assert.False(t, appParametersWouldChange(&v1alpha1.ApplicationSource{}, oldEnv, newEnv)) + }) +} + func Test_getRepoSanitizerRegex(t *testing.T) { r := getRepoSanitizerRegex("/tmp/_argocd-repo") msg := r.ReplaceAllString("error message containing /tmp/_argocd-repo/SENSITIVE and other stuff", "")