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 <michael@crenshaw.dev> * fix Helm regression Signed-off-by: Michael Crenshaw <michael@crenshaw.dev> * fix test Signed-off-by: Michael Crenshaw <michael@crenshaw.dev> * fix test again Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
argoproj · Jun 15, 2022 · 91de2f3 · 91de2f3
1 parent 56eb095
commit 91de2f3
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 19 deletions.
diff --git 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
@@ -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
@@ -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
@@ -0,0 +1,2 @@
+name: my-chart
+version: 1.1.0
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
@@ -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
@@ -0,0 +1,2 @@
+image:
+  tag: 0.1