From 7db5ab71b0a44a3ad5d3eda35218e5d87b251243 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Wed, 15 Jun 2022 08:24:07 -0700 Subject: [PATCH] fix: overrides should not appear in the manifest cache key (#9601) * fix: overrides should not appear in the manifest cache key Signed-off-by: Michael Crenshaw * fix Helm regression Signed-off-by: Michael Crenshaw * fix test Signed-off-by: Michael Crenshaw * fix test again Signed-off-by: Michael Crenshaw --- reposerver/repository/repository.go | 22 ++++-- reposerver/repository/repository_test.go | 78 +++++++++++++++---- .../single-global-helm/.argocd-source.yaml | 4 + .../single-global-helm/Chart.yaml | 2 + .../templates/deployment.yaml | 18 +++++ .../single-global-helm/values.yaml | 2 + 6 files changed, 105 insertions(+), 21 deletions(-) create mode 100644 reposerver/repository/testdata/app-parameters/single-global-helm/.argocd-source.yaml create mode 100644 reposerver/repository/testdata/app-parameters/single-global-helm/Chart.yaml create mode 100644 reposerver/repository/testdata/app-parameters/single-global-helm/templates/deployment.yaml create mode 100644 reposerver/repository/testdata/app-parameters/single-global-helm/values.yaml diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index f0d44d6c0a58..4cb00c811775 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -457,6 +457,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 { @@ -469,9 +474,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 } @@ -485,9 +490,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 } @@ -506,9 +511,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 } @@ -873,13 +878,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, maxCombinedManifestQuantity resource.Quantity, 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 @@ -992,7 +998,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 8f494f1ef915..0f0e15784b7d 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -1122,19 +1122,20 @@ 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", - "in-bounds-values-file-link": "Helm", - "invalid-kustomize": "Kustomize", - "kustomization_yaml": "Kustomize", - "kustomization_yml": "Kustomize", - "my-chart": "Helm", - "my-chart-2": "Helm", - "out-of-bounds-values-file-link": "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", + "in-bounds-values-file-link": "Helm", + "invalid-kustomize": "Kustomize", + "kustomization_yaml": "Kustomize", + "kustomization_yml": "Kustomize", + "my-chart": "Helm", + "my-chart-2": "Helm", + "out-of-bounds-values-file-link": "Helm", + "values-files": "Helm", } assert.Equal(t, expectedApps, res.Apps) } @@ -1486,6 +1487,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) { @@ -1545,6 +1575,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