Merge 23a91ef68f into d3908e6a3c

pkg/remote/remote.go

@@ -241,7 +241,17 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {
 	cacheDirPath := filepath.Join(r.Home, getterDst)
 	if u.Getter == "normal" {
 		srcDir = fmt.Sprintf("%s://%s", u.Scheme, u.Host)
-		cacheKey = replacer.Replace(srcDir)
+		dirKey = replacer.Replace(srcDir)
+		if len(query) > 0 {
+			q := maps.Clone(query)
+			if q.Has("sshkey") {
+				q.Set("sshkey", "redacted")
+			}
+			paramsKey := strings.ReplaceAll(q.Encode(), "&", "_")
+			cacheKey = fmt.Sprintf("%s.%s", dirKey, paramsKey)
+		} else {
+			cacheKey = dirKey
+		}
 		cacheDirPath = filepath.Join(r.Home, cacheKey, u.Dir)
 	}
 

pkg/remote/remote_test.go

@@ -7,7 +7,6 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-
 	"github.com/helmfile/helmfile/pkg/helmexec"
 	"github.com/helmfile/helmfile/pkg/testhelper"
 )
@@ -567,3 +566,119 @@ func TestRemote_Fetch(t *testing.T) {
 		})
 	}
 }
+
+func TestRemote_HttpUrlWithQueryParams(t *testing.T) {
+	// Test what cache paths are actually generated for HTTP URLs with query parameters
+
+	cleanfs := map[string]string{
+		CacheDir(): "",
+	}
+
+	url1 := "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=commit1"
+	url2 := "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=commit2"
+
+	for i, url := range []string{url1, url2} {
+		t.Run(fmt.Sprintf("url%d", i+1), func(t *testing.T) {
+			testfs := testhelper.NewTestFs(cleanfs)
+
+			var capturedDst string
+			mockHttpGetter := &testGetter{
+				get: func(wd, src, dst string) error {
+					capturedDst = dst
+					t.Logf("HttpGetter.Get called with dst: %s", dst)
+					t.Logf("HttpGetter.Get called with src: %s", src)
+					return nil
+				},
+			}
+
+			remote := &Remote{
+				Logger:     helmexec.NewLogger(io.Discard, "debug"),
+				Home:       CacheDir(),
+				HttpGetter: mockHttpGetter,
+				fs:         testfs.ToFileSystem(),
+			}
+
+			file, err := remote.Fetch(url)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			t.Logf("Fetched file path: %s", file)
+			t.Logf("Cache destination: %s", capturedDst)
+		})
+	}
+}
+
+func TestRemote_HttpUrlQueryParamsAvoidCacheCollision(t *testing.T) {
+	// Test that URLs with different query parameters use different cache locations
+	// and don't have cache collisions (this was the original bug)
+
+	url1 := "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=commit1"
+	url2 := "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=commit2"
+
+	// First, populate cache for url1
+	cachefs := map[string]string{
+		CacheDir(): "",
+		filepath.Join(CacheDir(), "https_gitlab_example_com.ref=commit1/api/v4/projects/test/repository/files/values.yaml/raw"): "content from commit1",
+	}
+
+	t.Run("url2 with different query params should not hit cache for url1", func(t *testing.T) {
+		testfs := testhelper.NewTestFs(cachefs)
+
+		httpGetCalled := false
+		mockHttpGetter := &testGetter{
+			get: func(wd, src, dst string) error {
+				httpGetCalled = true
+				t.Logf("HttpGetter.Get called - this is expected because url2 has different query params")
+				return nil
+			},
+		}
+
+		remote := &Remote{
+			Logger:     helmexec.NewLogger(io.Discard, "debug"),
+			Home:       CacheDir(),
+			HttpGetter: mockHttpGetter,
+			fs:         testfs.ToFileSystem(),
+		}
+
+		_, err := remote.Fetch(url2)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		// With the fix, HttpGetter.Get should be called because url2 has different
+		// query parameters than url1, so they should use different cache locations
+		if !httpGetCalled {
+			t.Error("HttpGetter.Get should have been called for URL with different query params")
+		}
+	})
+
+	t.Run("same url should hit cache", func(t *testing.T) {
+		testfs := testhelper.NewTestFs(cachefs)
+
+		httpGetCalled := false
+		mockHttpGetter := &testGetter{
+			get: func(wd, src, dst string) error {
+				httpGetCalled = true
+				return nil
+			},
+		}
+
+		remote := &Remote{
+			Logger:     helmexec.NewLogger(io.Discard, "debug"),
+			Home:       CacheDir(),
+			HttpGetter: mockHttpGetter,
+			fs:         testfs.ToFileSystem(),
+		}
+
+		_, err := remote.Fetch(url1)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		// url1 should hit the cache since we populated it with the same URL
+		if httpGetCalled {
+			t.Error("HttpGetter.Get should NOT have been called for cached URL")
+		}
+	})
+}