diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index a8b1d76db802..face08bc4f46 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -450,6 +450,11 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, close(ch.errCh) close(ch.responseCh) }() + + // GenerateManifests mutates the source (applies overrides). Those overrides shouldn't be reflected in the cache + // key. Overrides will break the cache anyway, because changes to overrides will change the revision. + appSourceCopy := q.ApplicationSource.DeepCopy() + var manifestGenResult *apiclient.ManifestResponse opContext, err := opContextSrc() if err == nil { @@ -462,9 +467,9 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, // Retrieve a new copy (if available) of the cached response: this ensures we are updating the latest copy of the cache, // rather than a copy of the cache that occurred before (a potentially lengthy) manifest generation. innerRes := &cache.CachedManifestResponse{} - cacheErr := s.cache.GetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes) + cacheErr := s.cache.GetManifests(cacheKey, appSourceCopy, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes) if cacheErr != nil && cacheErr != reposervercache.ErrCacheMiss { - log.Warnf("manifest cache set error %s: %v", q.ApplicationSource.String(), cacheErr) + log.Warnf("manifest cache set error %s: %v", appSourceCopy.String(), cacheErr) ch.errCh <- cacheErr return } @@ -478,9 +483,9 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, // Update the cache to include failure information innerRes.NumberOfConsecutiveFailures++ innerRes.MostRecentError = err.Error() - cacheErr = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes) + cacheErr = s.cache.SetManifests(cacheKey, appSourceCopy, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes) if cacheErr != nil { - log.Warnf("manifest cache set error %s: %v", q.ApplicationSource.String(), cacheErr) + log.Warnf("manifest cache set error %s: %v", appSourceCopy.String(), cacheErr) ch.errCh <- cacheErr return } @@ -499,9 +504,9 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, } manifestGenResult.Revision = commitSHA manifestGenResult.VerifyResult = opContext.verificationResult - err = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, &manifestGenCacheEntry) + err = s.cache.SetManifests(cacheKey, appSourceCopy, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, &manifestGenCacheEntry) if err != nil { - log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err) + log.Warnf("manifest cache set error %s/%s: %v", appSourceCopy.String(), cacheKey, err) } ch.responseCh <- manifestGenCacheEntry.ManifestResponse } @@ -866,13 +871,14 @@ func WithCMPTarDoneChannel(ch chan<- bool) GenerateManifestOpt { } } -// GenerateManifests generates manifests from a path +// GenerateManifests generates manifests from a path. Overrides are applied as a side effect on the given ApplicationSource. func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string, q *apiclient.ManifestRequest, isLocal bool, gitCredsStore git.CredsStore, opts ...GenerateManifestOpt) (*apiclient.ManifestResponse, error) { opt := newGenerateManifestOpt(opts...) var targetObjs []*unstructured.Unstructured var dest *v1alpha1.ApplicationDestination resourceTracking := argo.NewResourceTracking() + appSourceType, err := GetAppSourceType(ctx, q.ApplicationSource, appPath, q.AppName, q.EnabledSourceTypes) if err != nil { return nil, err @@ -985,7 +991,7 @@ func mergeSourceParameters(source *v1alpha1.ApplicationSource, path, appName str overrides = append(overrides, filepath.Join(path, fmt.Sprintf(appSourceFile, appName))) } - var merged v1alpha1.ApplicationSource = *source.DeepCopy() + var merged = *source.DeepCopy() for _, filename := range overrides { info, err := os.Stat(filename) diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index c38171c9e21a..1dbb3ae92d51 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -1120,17 +1120,18 @@ func TestListApps(t *testing.T) { assert.NoError(t, err) expectedApps := map[string]string{ - "Kustomization": "Kustomize", - "app-parameters/multi": "Kustomize", - "app-parameters/single-app-only": "Kustomize", - "app-parameters/single-global": "Kustomize", - "invalid-helm": "Helm", - "invalid-kustomize": "Kustomize", - "kustomization_yaml": "Kustomize", - "kustomization_yml": "Kustomize", - "my-chart": "Helm", - "my-chart-2": "Helm", - "values-files": "Helm", + "Kustomization": "Kustomize", + "app-parameters/multi": "Kustomize", + "app-parameters/single-app-only": "Kustomize", + "app-parameters/single-global": "Kustomize", + "app-parameters/single-global-helm": "Helm", + "invalid-helm": "Helm", + "invalid-kustomize": "Kustomize", + "kustomization_yaml": "Kustomize", + "kustomization_yml": "Kustomize", + "my-chart": "Helm", + "my-chart-2": "Helm", + "values-files": "Helm", } assert.Equal(t, expectedApps, res.Apps) } @@ -1482,6 +1483,35 @@ func TestGenerateManifestsWithAppParameterFile(t *testing.T) { }) }) + t.Run("Single global override Helm", func(t *testing.T) { + runWithTempTestdata(t, "single-global-helm", func(t *testing.T, path string) { + service := newService(".") + manifests, err := service.GenerateManifest(context.Background(), &apiclient.ManifestRequest{ + Repo: &argoappv1.Repository{}, + ApplicationSource: &argoappv1.ApplicationSource{ + Path: path, + }, + }) + require.NoError(t, err) + resourceByKindName := make(map[string]*unstructured.Unstructured) + for _, manifest := range manifests.Manifests { + var un unstructured.Unstructured + err := yaml.Unmarshal([]byte(manifest), &un) + if !assert.NoError(t, err) { + return + } + resourceByKindName[fmt.Sprintf("%s/%s", un.GetKind(), un.GetName())] = &un + } + deployment, ok := resourceByKindName["Deployment/guestbook-ui"] + require.True(t, ok) + containers, ok, _ := unstructured.NestedSlice(deployment.Object, "spec", "template", "spec", "containers") + require.True(t, ok) + image, ok, _ := unstructured.NestedString(containers[0].(map[string]interface{}), "image") + require.True(t, ok) + assert.Equal(t, "gcr.io/heptio-images/ks-guestbook-demo:0.2", image) + }) + }) + t.Run("Application specific override", func(t *testing.T) { service := newService(".") runWithTempTestdata(t, "single-app-only", func(t *testing.T, path string) { @@ -1541,6 +1571,28 @@ func TestGenerateManifestsWithAppParameterFile(t *testing.T) { assert.Equal(t, "gcr.io/heptio-images/ks-guestbook-demo:0.1", image) }) }) + + t.Run("Override info does not appear in cache key", func(t *testing.T) { + service := newService(".") + runWithTempTestdata(t, "single-global", func(t *testing.T, path string) { + source := &argoappv1.ApplicationSource{ + Path: path, + } + sourceCopy := source.DeepCopy() // make a copy in case GenerateManifest mutates it. + _, err := service.GenerateManifest(context.Background(), &apiclient.ManifestRequest{ + Repo: &argoappv1.Repository{}, + ApplicationSource: sourceCopy, + AppName: "test", + }) + assert.NoError(t, err) + res := &cache.CachedManifestResponse{} + // Try to pull from the cache with a `source` that does not include any overrides. Overrides should not be + // part of the cache key, because you can't get the overrides without a repo operation. And avoiding repo + // operations is the point of the cache. + err = service.cache.GetManifests(mock.Anything, source, &argoappv1.ClusterInfo{}, "", "", "", "test", res) + assert.NoError(t, err) + }) + }) } func TestGenerateManifestWithAnnotatedAndRegularGitTagHashes(t *testing.T) { diff --git a/reposerver/repository/testdata/app-parameters/single-global-helm/.argocd-source.yaml b/reposerver/repository/testdata/app-parameters/single-global-helm/.argocd-source.yaml new file mode 100644 index 000000000000..14aa17ae2bd5 --- /dev/null +++ b/reposerver/repository/testdata/app-parameters/single-global-helm/.argocd-source.yaml @@ -0,0 +1,4 @@ +helm: + parameters: + - name: image.tag + value: '0.2' diff --git a/reposerver/repository/testdata/app-parameters/single-global-helm/Chart.yaml b/reposerver/repository/testdata/app-parameters/single-global-helm/Chart.yaml new file mode 100644 index 000000000000..0c9526bab8cc --- /dev/null +++ b/reposerver/repository/testdata/app-parameters/single-global-helm/Chart.yaml @@ -0,0 +1,2 @@ +name: my-chart +version: 1.1.0 \ No newline at end of file diff --git a/reposerver/repository/testdata/app-parameters/single-global-helm/templates/deployment.yaml b/reposerver/repository/testdata/app-parameters/single-global-helm/templates/deployment.yaml new file mode 100644 index 000000000000..2702a963beb4 --- /dev/null +++ b/reposerver/repository/testdata/app-parameters/single-global-helm/templates/deployment.yaml @@ -0,0 +1,18 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: guestbook-ui +spec: + selector: + matchLabels: + app: guestbook-ui + template: + metadata: + labels: + app: guestbook-ui + spec: + containers: + - image: gcr.io/heptio-images/ks-guestbook-demo:{{.Values.image.tag}} + name: guestbook-ui + ports: + - containerPort: 81 diff --git a/reposerver/repository/testdata/app-parameters/single-global-helm/values.yaml b/reposerver/repository/testdata/app-parameters/single-global-helm/values.yaml new file mode 100644 index 000000000000..4e549a736a63 --- /dev/null +++ b/reposerver/repository/testdata/app-parameters/single-global-helm/values.yaml @@ -0,0 +1,2 @@ +image: + tag: 0.1