diff --git a/internal/policy/source/source.go b/internal/policy/source/source.go index 1c072b83c..52cb78b6b 100644 --- a/internal/policy/source/source.go +++ b/internal/policy/source/source.go @@ -74,6 +74,7 @@ type PolicyUrl struct { Url string Kind PolicyType pinOnce sync.Once + urlMu sync.RWMutex } // downloadCache is a concurrent map used to cache downloaded files. @@ -171,7 +172,7 @@ func (p *PolicyUrl) GetPolicy(ctx context.Context, workDir string, showMsg bool) if trace.IsEnabled() { region := trace.StartRegion(ctx, "ec:get-policy") defer region.End() - trace.Logf(ctx, "", "policy=%q", p.Url) + trace.Logf(ctx, "", "policy=%q", p.url()) } dl := func(source string, dest string) (metadata.Metadata, error) { @@ -189,8 +190,19 @@ func (p *PolicyUrl) GetPolicy(ctx context.Context, workDir string, showMsg bool) var pinErr error p.pinOnce.Do(func() { + originalUrl := p.url() + p.urlMu.Lock() p.Url, pinErr = metadata.GetPinnedURL(p.Url) - log.Debug("Pinned URL: ", p.Url) + p.urlMu.Unlock() + log.Debug("Pinned URL: ", p.url()) + // Register the cached download under the pinned URL too, so + // subsequent lookups (which use the now-mutated p.Url) still hit. + pinnedUrl := p.url() + if pinErr == nil && pinnedUrl != originalUrl { + if cached, ok := downloadCache.Load(originalUrl); ok { + downloadCache.LoadOrStore(pinnedUrl, cached) + } + } }) if pinErr != nil { return "", pinErr @@ -199,10 +211,16 @@ func (p *PolicyUrl) GetPolicy(ctx context.Context, workDir string, showMsg bool) return dest, nil } -func (p *PolicyUrl) PolicyUrl() string { +func (p *PolicyUrl) url() string { + p.urlMu.RLock() + defer p.urlMu.RUnlock() return p.Url } +func (p *PolicyUrl) PolicyUrl() string { + return p.url() +} + func (p *PolicyUrl) Subdir() string { // Be lazy and assume the kind value is the same as the subdirectory we want return string(p.Kind) diff --git a/internal/policy/source/source_test.go b/internal/policy/source/source_test.go index 87af0f351..942e23151 100644 --- a/internal/policy/source/source_test.go +++ b/internal/policy/source/source_test.go @@ -26,7 +26,6 @@ import ( "path" "path/filepath" "regexp" - "sync" "testing" ecc "github.com/conforma/crds/api/v1alpha1" @@ -163,7 +162,7 @@ func TestInlineDataSource(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Clear download cache for each test t.Cleanup(func() { - downloadCache = sync.Map{} + ClearDownloadCache() }) s := InlineData(tt.inputData) @@ -276,7 +275,7 @@ func TestInlineDataGetPolicy(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Clear download cache for each test t.Cleanup(func() { - downloadCache = sync.Map{} + ClearDownloadCache() }) s := InlineData(tt.inputData) @@ -539,7 +538,7 @@ func (m mockPolicySource) Type() PolicyType { func TestGetPolicyThroughCache(t *testing.T) { test := func(t *testing.T, fs afero.Fs, expectedDownloads int) { t.Cleanup(func() { - downloadCache = sync.Map{} + ClearDownloadCache() }) ctx := utils.WithFS(context.Background(), fs) @@ -604,7 +603,7 @@ func TestGetPolicyThroughCache(t *testing.T) { // causing Rego compile issue func TestDownloadCacheWorkdirMismatch(t *testing.T) { t.Cleanup(func() { - downloadCache = sync.Map{} + ClearDownloadCache() }) tmp := t.TempDir() @@ -634,12 +633,55 @@ func TestDownloadCacheWorkdirMismatch(t *testing.T) { assert.Equal(t, destination1, destination2) } +// TestGetPolicyPinnedURLCacheConsistency verifies that URL pinning doesn't +// cause duplicate policy directories. After the first GetPolicy call, the URL +// is pinned (e.g. github.com/org/repo -> git::github.com/org/repo?ref=abc123), +// which changes the cache key. Without the fix, the second call would miss the +// cache and re-download into a new directory, causing OPA duplicate package errors. +func TestGetPolicyPinnedURLCacheConsistency(t *testing.T) { + t.Cleanup(ClearDownloadCache) + + workDir := t.TempDir() + policyDir := filepath.Join(workDir, "policy") + require.NoError(t, os.MkdirAll(policyDir, 0o755)) + + originalUrl := "github.com/org/repo//policy" + p := &PolicyUrl{Url: originalUrl, Kind: PolicyKind} + + dl := &mockDownloader{} + dl.On("Download", mock.Anything, mock.Anything, originalUrl, false). + Run(func(args mock.Arguments) { + dest := args.String(1) + require.NoError(t, os.MkdirAll(dest, 0o755)) + }). + Return(&gitMetadata.GitMetadata{LatestCommit: "abc123def456"}, nil) + + ctx := usingDownloader(context.TODO(), dl) + + // First call: downloads and pins URL + dest1, err := p.GetPolicy(ctx, workDir, false) + require.NoError(t, err) + assert.NotEqual(t, originalUrl, p.Url, "URL should be pinned after first call") + + // Second call: should hit cache despite pinned URL + dest2, err := p.GetPolicy(ctx, workDir, false) + require.NoError(t, err) + assert.Equal(t, dest1, dest2, "second call should return same directory") + + // Verify only one directory exists under policy/ + entries, err := os.ReadDir(policyDir) + require.NoError(t, err) + assert.Len(t, entries, 1, "only one policy directory should exist, not a duplicate from pinned URL") + + dl.AssertNumberOfCalls(t, "Download", 1) +} + // TestConcurrentPolicyCachingRaceCondition reproduces the "file exists" error // that occurs when multiple workers simultaneously try to create symlinks from // cached policy downloads to their individual work directories func TestConcurrentPolicyCachingRaceCondition(t *testing.T) { t.Cleanup(func() { - downloadCache = sync.Map{} + ClearDownloadCache() }) tmp := t.TempDir() diff --git a/internal/server/server.go b/internal/server/server.go index 7ed2edb2d..a35e9cd4a 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -30,6 +30,7 @@ import ( "github.com/conforma/cli/internal/evaluation_target/input" "github.com/conforma/cli/internal/evaluator" "github.com/conforma/cli/internal/policy" + "github.com/conforma/cli/internal/version" ) type Config struct { @@ -69,6 +70,7 @@ func (s *Server) Start(ctx context.Context) error { "address": s.cfg.Address, "port": s.cfg.Port, "sources": len(s.cfg.Policy.Spec().Sources), + "version": version.Version, }).Info("Starting server") log.Info("Loading policy sources...")