diff --git a/util/dockerutil/dockerconfig/configprovider.go b/util/dockerutil/dockerconfig/configprovider.go index 1aa257460c58..e3293ec91c73 100644 --- a/util/dockerutil/dockerconfig/configprovider.go +++ b/util/dockerutil/dockerconfig/configprovider.go @@ -42,12 +42,14 @@ type authConfigProvider struct { alternativeConfigs []*alternativeConfig } +func defaultCacheExpireCheck(created time.Time, _ string) bool { + // Tokens for Google Artifact Registry via Workload Identity expire after 5 minutes. + return time.Since(created) > 4*time.Minute+50*time.Second +} + func (ap *authConfigProvider) load(ctx context.Context, host string, scopes []string, cacheExpireCheck authprovider.ExpireCachedAuthCheck) (types.AuthConfig, error) { if cacheExpireCheck == nil { - cacheExpireCheck = func(created time.Time, _ string) bool { - // Tokens for Google Artifact Registry via Workload Identity expire after 5 minutes. - return time.Since(created) > 4*time.Minute+50*time.Second - } + cacheExpireCheck = defaultCacheExpireCheck } ac, err := ap.loadHost(ctx, host, scopes, cacheExpireCheck) @@ -78,6 +80,10 @@ func (ap *authConfigProvider) loadHost(_ context.Context, host string, scopes [] ap.mu.Lock() defer ap.mu.Unlock() + if cacheExpireCheck == nil { + cacheExpireCheck = defaultCacheExpireCheck + } + candidates := []*alternativeConfig{} parsedScopes := parseScopes(scopes) @@ -115,8 +121,11 @@ func (ap *authConfigProvider) loadHost(_ context.Context, host string, scopes [] } entry, exists := ap.authConfigCache[key] - if exists && (cacheExpireCheck == nil || !cacheExpireCheck(entry.Created, key)) { - return *entry.Auth, nil + if exists { + if !cacheExpireCheck(entry.Created, key) { + return *entry.Auth, nil + } + delete(ap.authConfigCache, key) } hostKey := host diff --git a/util/dockerutil/dockerconfig/configprovider_test.go b/util/dockerutil/dockerconfig/configprovider_test.go index d4fce2e83656..719b7178675e 100644 --- a/util/dockerutil/dockerconfig/configprovider_test.go +++ b/util/dockerutil/dockerconfig/configprovider_test.go @@ -3,6 +3,11 @@ package dockerconfig import ( "testing" + "context" + "time" + + "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/cli/config/types" "github.com/stretchr/testify/require" ) @@ -198,3 +203,145 @@ func TestMatchesScopes(t *testing.T) { }) } } + +func TestAuthConfigCache_Hit(t *testing.T) { + cfg := configfile.New("test") + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-1", + } + + ap := &authConfigProvider{ + defaultConfig: cfg, + authConfigCache: map[string]authConfigCacheEntry{}, + } + ap.initOnce.Do(func() {}) + + ctx := context.Background() + ac, err := ap.load(ctx, "europe-west1-docker.pkg.dev", nil, func(created time.Time, serverURL string) bool { + return false + }) + require.NoError(t, err) + require.Equal(t, "token-1", ac.Password) + + // Update config in underlying store + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-2", + } + + // Should still hit cache and return token-1 + ac, err = ap.load(ctx, "europe-west1-docker.pkg.dev", nil, func(created time.Time, serverURL string) bool { + return false + }) + require.NoError(t, err) + require.Equal(t, "token-1", ac.Password) +} + +func TestAuthConfigCache_Expired(t *testing.T) { + cfg := configfile.New("test") + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-1", + } + + ap := &authConfigProvider{ + defaultConfig: cfg, + authConfigCache: map[string]authConfigCacheEntry{}, + } + ap.initOnce.Do(func() {}) + + ctx := context.Background() + ac, err := ap.load(ctx, "europe-west1-docker.pkg.dev", nil, nil) + require.NoError(t, err) + require.Equal(t, "token-1", ac.Password) + + // Update config in underlying store + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-2", + } + + // Force expire check to return true + ac, err = ap.load(ctx, "europe-west1-docker.pkg.dev", nil, func(created time.Time, serverURL string) bool { + return true + }) + require.NoError(t, err) + require.Equal(t, "token-2", ac.Password) +} + +func TestAuthConfigCache_DefaultExpireCheck_Nil(t *testing.T) { + cfg := configfile.New("test") + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-1", + } + + ap := &authConfigProvider{ + defaultConfig: cfg, + authConfigCache: map[string]authConfigCacheEntry{}, + } + ap.initOnce.Do(func() {}) + + ctx := context.Background() + // Initial load + ac, err := ap.load(ctx, "europe-west1-docker.pkg.dev", nil, nil) + require.NoError(t, err) + require.Equal(t, "token-1", ac.Password) + + // Change underlying config + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-2", + } + + // Recent cache entry should not expire with nil check + ac, err = ap.load(ctx, "europe-west1-docker.pkg.dev", nil, nil) + require.NoError(t, err) + require.Equal(t, "token-1", ac.Password) + + // Age the cached entry past 4m50s + entry := ap.authConfigCache["europe-west1-docker.pkg.dev"] + entry.Created = time.Now().Add(-5 * time.Minute) + ap.authConfigCache["europe-west1-docker.pkg.dev"] = entry + + // Should expire with default check and reload token-2 + ac, err = ap.load(ctx, "europe-west1-docker.pkg.dev", nil, nil) + require.NoError(t, err) + require.Equal(t, "token-2", ac.Password) +} + +func TestAuthConfigCache_LoadHost_DirectNilCheck(t *testing.T) { + cfg := configfile.New("test") + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-1", + } + + ap := &authConfigProvider{ + defaultConfig: cfg, + authConfigCache: map[string]authConfigCacheEntry{}, + } + ap.initOnce.Do(func() {}) + + ctx := context.Background() + // Direct call to loadHost with nil cacheExpireCheck + ac, err := ap.loadHost(ctx, "europe-west1-docker.pkg.dev", nil, nil) + require.NoError(t, err) + require.Equal(t, "token-1", ac.Password) + + cfg.AuthConfigs["europe-west1-docker.pkg.dev"] = types.AuthConfig{ + Username: "_dcgcloud_token", + Password: "token-2", + } + + // Age the entry + entry := ap.authConfigCache["europe-west1-docker.pkg.dev"] + entry.Created = time.Now().Add(-5 * time.Minute) + ap.authConfigCache["europe-west1-docker.pkg.dev"] = entry + + // loadHost directly with nil should expire and reload + ac, err = ap.loadHost(ctx, "europe-west1-docker.pkg.dev", nil, nil) + require.NoError(t, err) + require.Equal(t, "token-2", ac.Password) +}