diff --git a/pkg/remote/remote.go b/pkg/remote/remote.go index 7b870617..61fe442c 100644 --- a/pkg/remote/remote.go +++ b/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) } diff --git a/pkg/remote/remote_test.go b/pkg/remote/remote_test.go index a983acca..4547360c 100644 --- a/pkg/remote/remote_test.go +++ b/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") + } + }) +}