diff --git a/pkg/credentialprovider/keyring.go b/pkg/credentialprovider/keyring.go index 63005f6322b..08997c234db 100644 --- a/pkg/credentialprovider/keyring.go +++ b/pkg/credentialprovider/keyring.go @@ -87,7 +87,8 @@ func NewTrackedAuthConfig(c *AuthConfig, src *CredentialSource) *TrackedAuthConf } type CredentialSource struct { - Secret SecretCoordinates + Secret *SecretCoordinates + ServiceAccount *ServiceAccountCoordinates } type SecretCoordinates struct { @@ -96,6 +97,12 @@ type SecretCoordinates struct { Name string } +type ServiceAccountCoordinates struct { + UID string + Namespace string + Name string +} + // AuthConfig contains authorization information for connecting to a Registry // This type mirrors "github.com/docker/docker/api/types.AuthConfig" type AuthConfig struct { @@ -316,8 +323,6 @@ func (dk *providersDockerKeyring) Lookup(image string) ([]TrackedAuthConfig, boo keyring := &BasicDockerKeyring{} for _, p := range dk.Providers { - // TODO: the source should probably change once we depend on service accounts (KEP-4412). - // Perhaps `Provide()` should return the source modified to accommodate this? keyring.Add(nil, p.Provide(image)) } diff --git a/pkg/credentialprovider/plugin/plugin.go b/pkg/credentialprovider/plugin/plugin.go index b528602ccdc..5c38823c8f1 100644 --- a/pkg/credentialprovider/plugin/plugin.go +++ b/pkg/credentialprovider/plugin/plugin.go @@ -401,20 +401,22 @@ type perPodPluginProvider struct { serviceAccountName string } -// Enabled always returns true since registration of the plugin via kubelet implies it should be enabled. -func (p *perPodPluginProvider) Enabled() bool { - return true -} - -func (p *perPodPluginProvider) Provide(image string) credentialprovider.DockerConfig { +// provideWithCoordinates returns the DockerConfig and, if available, the ServiceAccountCoordinates +// used for credential resolution. If ServiceAccountCoordinates is nil, it means no service account +// context was used (e.g., the plugin is not operating in service account token mode or no service +// account was provided for the request). +func (p *perPodPluginProvider) provideWithCoordinates(image string) (credentialprovider.DockerConfig, *credentialprovider.ServiceAccountCoordinates) { return p.provider.provide(image, p.podNamespace, p.podName, p.podUID, p.serviceAccountName) } // provide returns a credentialprovider.DockerConfig based on the credentials returned -// from cache or the exec plugin. -func (p *pluginProvider) provide(image, podNamespace, podName string, podUID types.UID, serviceAccountName string) credentialprovider.DockerConfig { +// from cache or the exec plugin. The returned ServiceAccountCoordinates may be nil. +// If ServiceAccountCoordinates is nil, it means no service account context was used +// (e.g., the plugin is not operating in service account token mode or no service account +// was provided for the request). +func (p *pluginProvider) provide(image, podNamespace, podName string, podUID types.UID, serviceAccountName string) (credentialprovider.DockerConfig, *credentialprovider.ServiceAccountCoordinates) { if !p.isImageAllowed(image) { - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } var serviceAccountUID types.UID @@ -423,11 +425,12 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ var err error var serviceAccountCacheKey string var serviceAccountTokenHash string + var serviceAccountCoordinates *credentialprovider.ServiceAccountCoordinates if p.serviceAccountProvider != nil { if len(serviceAccountName) == 0 && p.serviceAccountProvider.requireServiceAccount { klog.V(5).Infof("Service account name is empty for pod %s/%s", podNamespace, podName) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } // If the service account name is empty and the plugin has indicated that invoking the plugin @@ -441,16 +444,16 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ // for image pull. If any of the required annotation is missing, we will not invoke the plugin. We will log the error // at higher verbosity level as it could be noisy. klog.V(5).Infof("Failed to get service account data %s/%s: %v", podNamespace, serviceAccountName, err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } klog.Errorf("Failed to get service account %s/%s: %v", podNamespace, serviceAccountName, err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } if serviceAccountToken, err = p.serviceAccountProvider.getServiceAccountToken(podNamespace, podName, serviceAccountName, serviceAccountUID, podUID); err != nil { klog.Errorf("Error getting service account token %s/%s: %v", podNamespace, serviceAccountName, err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } serviceAccountTokenHash = getHashIfNotEmpty(serviceAccountToken) @@ -468,7 +471,13 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ serviceAccountCacheKey, err = generateServiceAccountCacheKey(c) if err != nil { klog.Errorf("Error generating service account cache key: %v", err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil + } + + serviceAccountCoordinates = &credentialprovider.ServiceAccountCoordinates{ + UID: string(serviceAccountUID), + Namespace: podNamespace, + Name: serviceAccountName, } } } @@ -477,11 +486,11 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ cachedConfig, found, errCache := p.getCachedCredentials(image, serviceAccountCacheKey) if errCache != nil { klog.Errorf("Failed to get cached docker config: %v", err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } if found { - return cachedConfig + return cachedConfig, serviceAccountCoordinates } // ExecPlugin is wrapped in single flight to exec plugin once for concurrent same image request. @@ -500,7 +509,7 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ // and the workload is using the same service account. if singleFlightKey, err = generateSingleFlightKey(image, serviceAccountTokenHash, saAnnotations); err != nil { klog.Errorf("Error generating singleflight key: %v", err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } } res, err, _ := p.group.Do(singleFlightKey, func() (interface{}, error) { @@ -509,13 +518,13 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ if err != nil { klog.Errorf("Failed getting credential from external registry credential provider: %v", err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } response, ok := res.(*credentialproviderapi.CredentialProviderResponse) if !ok { klog.Errorf("Invalid response type returned by external credential provider") - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } if len(serviceAccountToken) > 0 && p.serviceAccountProvider.cacheType != kubeletconfig.TokenServiceAccountTokenCacheType { // validate that the response credentials are not the echoed token back verbatim when cache @@ -524,7 +533,7 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ for _, authConfig := range response.Auth { if authConfig.Password == serviceAccountToken { klog.Errorf("Credential provider plugin returned the service account token as the password for image %q, which is not allowed when service account cache type is not set to 'Token'", image) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } } } @@ -540,7 +549,7 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ cacheKey = globalCacheKey default: klog.Errorf("credential provider plugin did not return a valid cacheKeyType: %q", cacheKeyType) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } dockerConfig := make(credentialprovider.DockerConfig, len(response.Auth)) @@ -553,14 +562,14 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ // cache duration was explicitly 0 so don't cache this response at all. if response.CacheDuration != nil && response.CacheDuration.Duration == 0 { - return dockerConfig + return dockerConfig, serviceAccountCoordinates } var expiresAt time.Time // nil cache duration means use the default cache duration if response.CacheDuration == nil { if p.defaultCacheDuration == 0 { - return dockerConfig + return dockerConfig, serviceAccountCoordinates } expiresAt = p.clock.Now().Add(p.defaultCacheDuration) } else { @@ -570,7 +579,7 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ cacheKey, err = generateCacheKey(cacheKey, serviceAccountCacheKey) if err != nil { klog.Errorf("Error generating cache key: %v", err) - return credentialprovider.DockerConfig{} + return credentialprovider.DockerConfig{}, nil } cachedEntry := &cacheEntry{ @@ -583,12 +592,7 @@ func (p *pluginProvider) provide(image, podNamespace, podName string, podUID typ klog.Errorf("Error adding auth entry to cache: %v", err) } - return dockerConfig -} - -// Enabled always returns true since registration of the plugin via kubelet implies it should be enabled. -func (p *pluginProvider) Enabled() bool { - return true + return dockerConfig, serviceAccountCoordinates } // isImageAllowed returns true if the image matches against the list of allowed matches by the plugin. diff --git a/pkg/credentialprovider/plugin/plugin_test.go b/pkg/credentialprovider/plugin/plugin_test.go index dfe1e228a69..ab088e99989 100644 --- a/pkg/credentialprovider/plugin/plugin_test.go +++ b/pkg/credentialprovider/plugin/plugin_test.go @@ -125,7 +125,7 @@ func TestSingleflightProvide(t *testing.T) { wg.Add(1) go func(i int) { defer wg.Done() - result := dynamicProvider.Provide(image) + result, _ := dynamicProvider.provideWithCoordinates(image) results[i] = result }(i) } @@ -158,7 +158,7 @@ func TestSingleflightProvide(t *testing.T) { wg.Add(1) go func(i int) { defer wg.Done() - result := dynamicProvider.Provide(image) + result, _ := dynamicProvider.provideWithCoordinates(image) results[i] = result }(i) } @@ -179,7 +179,7 @@ func TestSingleflightProvide(t *testing.T) { wg.Add(1) go func(i int) { defer wg.Done() - result := dynamicProvider.Provide(image) + result, _ := dynamicProvider.provideWithCoordinates(image) results[i] = result }(i) } @@ -191,7 +191,7 @@ func TestSingleflightProvide(t *testing.T) { } } -func Test_Provide(t *testing.T) { +func Test_ProvideWithCoordinates(t *testing.T) { klog.InitFlags(nil) if err := flag.Set("v", "6"); err != nil { t.Fatalf("failed to set log level: %v", err) @@ -200,11 +200,12 @@ func Test_Provide(t *testing.T) { tclock := clock.RealClock{} testcases := []struct { - name string - pluginProvider *perPodPluginProvider - image string - dockerconfig credentialprovider.DockerConfig - wantLog string + name string + pluginProvider *perPodPluginProvider + image string + dockerconfig credentialprovider.DockerConfig + wantLog string + expectedServiceAccount *credentialprovider.ServiceAccountCoordinates }{ { name: "exact image match, with Registry cache key", @@ -529,6 +530,7 @@ func Test_Provide(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "sa-name", Namespace: "ns", + UID: "sa-uid", Annotations: map[string]string{ "domain.io/identity-id": "id", "domain.io/identity-type": "type", @@ -552,6 +554,11 @@ func Test_Provide(t *testing.T) { Password: "password", }, }, + expectedServiceAccount: &credentialprovider.ServiceAccountCoordinates{ + Namespace: "ns", + Name: "sa-name", + UID: "sa-uid", + }, }, } @@ -563,8 +570,17 @@ func Test_Provide(t *testing.T) { klog.LogToStderr(false) defer klog.LogToStderr(true) - if got := testcase.pluginProvider.Provide(testcase.image); !reflect.DeepEqual(got, testcase.dockerconfig) { - t.Errorf("unexpected docker config: %v, expected: %v", got, testcase.dockerconfig) + gotConfig, gotCoordinates := testcase.pluginProvider.provideWithCoordinates(testcase.image) + if !reflect.DeepEqual(gotConfig, testcase.dockerconfig) { + t.Errorf("unexpected docker config from ProvideWithCoordinates: %v, expected: %v", gotConfig, testcase.dockerconfig) + } + + if testcase.expectedServiceAccount != nil { + if !reflect.DeepEqual(gotCoordinates, testcase.expectedServiceAccount) { + t.Errorf("unexpected service account coordinates: %v, expected: %v", gotCoordinates, testcase.expectedServiceAccount) + } + } else if gotCoordinates != nil { + t.Errorf("expected nil service account coordinates but got: %v", gotCoordinates) } klog.Flush() @@ -641,11 +657,9 @@ func Test_ProvideParallel(t *testing.T) { for i := 0; i < 5; i++ { go func(w *sync.WaitGroup) { image := fmt.Sprintf(testcase.registry+"/%s", rand.String(5)) - dockerconfigResponse := pluginProvider.Provide(image) + dockerconfigResponse, _ := pluginProvider.provideWithCoordinates(image) if !reflect.DeepEqual(dockerconfigResponse, dockerconfig) { - t.Logf("actual docker config: %v", dockerconfigResponse) - t.Logf("expected docker config: %v", dockerconfig) - t.Error("unexpected docker config") + t.Errorf("unexpected docker config for image %s: %v, expected: %v", image, dockerconfigResponse, dockerconfig) } w.Done() }(&wg) @@ -1110,7 +1124,7 @@ func Test_NoCacheResponse(t *testing.T) { }, } - dockerConfig := pluginProvider.Provide("test.registry.io/foo/bar") + dockerConfig, _ := pluginProvider.provideWithCoordinates("test.registry.io/foo/bar") if !reflect.DeepEqual(dockerConfig, expectedDockerConfig) { t.Logf("actual docker config: %v", dockerConfig) t.Logf("expected docker config: %v", expectedDockerConfig) @@ -1544,7 +1558,7 @@ func Test_CacheKeyGeneration(t *testing.T) { if tc.serviceAccountCacheType != "" { saTokenCacheType = &tc.serviceAccountCacheType } - pluginProvider := createTestPluginProvider(t, tc.pluginCacheKeyType, saTokenCacheType) + pluginProvider := createTestPerPodPluginProvider(t, tc.pluginCacheKeyType, saTokenCacheType) // Test image and expected SA parameters testImage := "test.registry.io/foo/bar" @@ -1566,7 +1580,7 @@ func Test_CacheKeyGeneration(t *testing.T) { cacheType: cacheTypeValue, } - dockerConfig := pluginProvider.Provide(testImage) + dockerConfig, _ := pluginProvider.provideWithCoordinates(testImage) verifyDockerConfig(t, dockerConfig) // Verify the cache key is as expected @@ -1579,13 +1593,13 @@ func Test_CacheKeyGeneration(t *testing.T) { // Test cache hit (nil out plugin to ensure cache is used) pluginProvider.provider.plugin = nil - cachedConfig := pluginProvider.Provide(testImage) + cachedConfig, _ := pluginProvider.provideWithCoordinates(testImage) verifyDockerConfig(t, cachedConfig) }) } } -func createTestPluginProvider(t *testing.T, pluginCacheKeyType credentialproviderapi.PluginCacheKeyType, saTokenCacheType *kubeletconfig.ServiceAccountTokenCacheType) *perPodPluginProvider { +func createTestPerPodPluginProvider(t *testing.T, pluginCacheKeyType credentialproviderapi.PluginCacheKeyType, saTokenCacheType *kubeletconfig.ServiceAccountTokenCacheType) *perPodPluginProvider { t.Helper() tclock := clock.RealClock{} diff --git a/pkg/credentialprovider/plugin/plugins.go b/pkg/credentialprovider/plugin/plugins.go index fbadc80eddf..e37d908e1a5 100644 --- a/pkg/credentialprovider/plugin/plugins.go +++ b/pkg/credentialprovider/plugin/plugins.go @@ -17,6 +17,7 @@ limitations under the License. package plugin import ( + "fmt" "sync" "k8s.io/apimachinery/pkg/types" @@ -27,6 +28,11 @@ import ( "k8s.io/kubernetes/pkg/features" ) +type dockerConfigProviderWithCoordinates interface { + // provideWithCoordinates returns the DockerConfig and service account coordinates for the given image. + provideWithCoordinates(image string) (credentialprovider.DockerConfig, *credentialprovider.ServiceAccountCoordinates) +} + type provider struct { name string impl *pluginProvider @@ -41,7 +47,7 @@ func registerCredentialProviderPlugin(name string, p *pluginProvider) { defer providersMutex.Unlock() if seenProviderNames.Has(name) { - klog.Fatalf("Credential provider %q was registered twice", name) + panic(fmt.Sprintf("Credential provider %q was registered twice", name)) } seenProviderNames.Insert(name) @@ -50,7 +56,7 @@ func registerCredentialProviderPlugin(name string, p *pluginProvider) { } type externalCredentialProviderKeyring struct { - providers []credentialprovider.DockerConfigProvider + providers []dockerConfigProviderWithCoordinates } func NewExternalCredentialProviderDockerKeyring(podNamespace, podName, podUID, serviceAccountName string) credentialprovider.DockerKeyring { @@ -58,14 +64,10 @@ func NewExternalCredentialProviderDockerKeyring(podNamespace, podName, podUID, s defer providersMutex.RUnlock() keyring := &externalCredentialProviderKeyring{ - providers: make([]credentialprovider.DockerConfigProvider, 0, len(providers)), + providers: make([]dockerConfigProviderWithCoordinates, 0, len(providers)), } for _, p := range providers { - if !p.impl.Enabled() { - continue - } - pp := &perPodPluginProvider{ name: p.name, provider: p.impl, @@ -91,8 +93,12 @@ func (k *externalCredentialProviderKeyring) Lookup(image string) ([]credentialpr keyring := &credentialprovider.BasicDockerKeyring{} for _, p := range k.providers { - // TODO: modify the credentialprovider.CredentialSource to contain the SA/pod information - keyring.Add(nil, p.Provide(image)) + dockerConfig, saCoords := p.provideWithCoordinates(image) + if saCoords != nil { + keyring.Add(&credentialprovider.CredentialSource{ServiceAccount: saCoords}, dockerConfig) + } else { + keyring.Add(nil, dockerConfig) + } } return keyring.Lookup(image) diff --git a/pkg/credentialprovider/plugin/plugins_test.go b/pkg/credentialprovider/plugin/plugins_test.go new file mode 100644 index 00000000000..d4f10f939ef --- /dev/null +++ b/pkg/credentialprovider/plugin/plugins_test.go @@ -0,0 +1,517 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plugin + +import ( + "sync" + "testing" + "time" + + "k8s.io/apimachinery/pkg/util/sets" + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/tools/cache" + featuregatetesting "k8s.io/component-base/featuregate/testing" + credentialproviderapi "k8s.io/kubelet/pkg/apis/credentialprovider" + "k8s.io/kubernetes/pkg/credentialprovider" + "k8s.io/kubernetes/pkg/features" + testingclock "k8s.io/utils/clock/testing" +) + +// fakedockerConfigProviderWithCoordinates implements dockerConfigProviderWithCoordinates for testing +type fakedockerConfigProviderWithCoordinates struct { + dockerConfig credentialprovider.DockerConfig + serviceAccountCoords *credentialprovider.ServiceAccountCoordinates + callCount int + lastImageRequested string + mu sync.Mutex +} + +func (f *fakedockerConfigProviderWithCoordinates) provideWithCoordinates(image string) (credentialprovider.DockerConfig, *credentialprovider.ServiceAccountCoordinates) { + f.mu.Lock() + defer f.mu.Unlock() + f.callCount++ + f.lastImageRequested = image + + return f.dockerConfig, f.serviceAccountCoords +} + +func (f *fakedockerConfigProviderWithCoordinates) getCallCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.callCount +} + +func (f *fakedockerConfigProviderWithCoordinates) getLastImageRequested() string { + f.mu.Lock() + defer f.mu.Unlock() + return f.lastImageRequested +} + +// createTestPluginProvider creates a pluginProvider for testing +func createTestPluginProvider(dockerConfig credentialprovider.DockerConfig) *pluginProvider { + testClock := testingclock.NewFakeClock(time.Now()) + + authConfigMap := make(map[string]credentialproviderapi.AuthConfig) + for registry, config := range dockerConfig { + authConfigMap[registry] = credentialproviderapi.AuthConfig{ + Username: config.Username, + Password: config.Password, + } + } + + mockPlugin := &fakeExecPlugin{ + cacheKeyType: credentialproviderapi.ImagePluginCacheKeyType, + cacheDuration: time.Hour, + auth: authConfigMap, + } + + provider := &pluginProvider{ + clock: testClock, + matchImages: []string{"*"}, + cache: cache.NewExpirationStore(cacheKeyFunc, &cacheExpirationPolicy{clock: testClock}), + defaultCacheDuration: time.Hour, + lastCachePurge: testClock.Now(), + plugin: mockPlugin, + } + + return provider +} + +// setupTestProviders sets up test providers and cleans up after the test +func setupTestProviders(t *testing.T) { + t.Helper() + + // Save original state + providersMutex.Lock() + originalProviders := providers + originalSeenProviderNames := seenProviderNames + + // Reset to clean state + providers = make([]provider, 0) + seenProviderNames = sets.NewString() + providersMutex.Unlock() + + t.Cleanup(func() { + // Restore original state + providersMutex.Lock() + providers = originalProviders + seenProviderNames = originalSeenProviderNames + providersMutex.Unlock() + }) +} + +func TestRegisterCredentialProviderPlugin(t *testing.T) { + testCases := []struct { + name string + firstProvider string + secondProvider string // empty means no second provider + shouldPanic bool + }{ + { + name: "successful registration", + firstProvider: "test-provider", + shouldPanic: false, + }, + { + name: "duplicate registration should panic", + firstProvider: "duplicate-provider", + secondProvider: "duplicate-provider", + shouldPanic: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + setupTestProviders(t) + + mockProvider1 := createTestPluginProvider(credentialprovider.DockerConfig{ + "test.registry.io": credentialprovider.DockerConfigEntry{ + Username: "user", + Password: "pass", + }, + }) + + // Register first provider + registerCredentialProviderPlugin(tc.firstProvider, mockProvider1) + + // Verify first registration + providersMutex.RLock() + if len(providers) != 1 { + t.Errorf("Expected 1 provider after first registration, got %d", len(providers)) + } + if providers[0].name != tc.firstProvider { + t.Errorf("Expected provider name '%s', got %s", tc.firstProvider, providers[0].name) + } + if providers[0].impl != mockProvider1 { + t.Errorf("Expected provider implementation to match") + } + if !seenProviderNames.Has(tc.firstProvider) { + t.Errorf("Expected '%s' to be in seenProviderNames", tc.firstProvider) + } + providersMutex.RUnlock() + + // If we have a second provider to test, register it + if tc.secondProvider != "" { + mockProvider2 := createTestPluginProvider(credentialprovider.DockerConfig{}) + + if tc.shouldPanic { + // Test that registering duplicate provider name panics + defer func() { + if r := recover(); r == nil { + t.Errorf("Expected panic for duplicate provider registration") + } + }() + } + + registerCredentialProviderPlugin(tc.secondProvider, mockProvider2) + + if !tc.shouldPanic { + // If we didn't expect a panic, verify the second provider was registered + providersMutex.RLock() + if len(providers) != 2 { + t.Errorf("Expected 2 providers after second registration, got %d", len(providers)) + } + providersMutex.RUnlock() + } + } + }) + } +} + +func TestNewExternalCredentialProviderDockerKeyring(t *testing.T) { + testCases := []struct { + name string + setupProviders func() + featureGateEnabled bool + expectedProviderCount int + expectedPodNamespace string + expectedPodName string + expectedPodUID string + expectedServiceAccountName string + validatePerPodProviderFields bool + }{ + { + name: "no providers registered", + setupProviders: func() {}, + expectedProviderCount: 0, + }, + { + name: "multiple providers registered", + setupProviders: func() { + mockProvider1 := createTestPluginProvider(credentialprovider.DockerConfig{}) + mockProvider2 := createTestPluginProvider(credentialprovider.DockerConfig{}) + registerCredentialProviderPlugin("enabled-provider-1", mockProvider1) + registerCredentialProviderPlugin("enabled-provider-2", mockProvider2) + }, + expectedProviderCount: 2, + }, + { + name: "feature gate enabled - pod information should be set", + setupProviders: func() { + mockProvider := createTestPluginProvider(credentialprovider.DockerConfig{}) + registerCredentialProviderPlugin("test-provider", mockProvider) + }, + featureGateEnabled: true, + expectedProviderCount: 1, + expectedPodNamespace: "test-namespace", + expectedPodName: "test-pod", + expectedPodUID: "test-uid", + expectedServiceAccountName: "test-sa", + validatePerPodProviderFields: true, + }, + { + name: "feature gate disabled - pod information should be empty", + setupProviders: func() { + mockProvider := createTestPluginProvider(credentialprovider.DockerConfig{}) + registerCredentialProviderPlugin("test-provider", mockProvider) + }, + featureGateEnabled: false, + expectedProviderCount: 1, + expectedPodNamespace: "", + expectedPodName: "", + expectedPodUID: "", + expectedServiceAccountName: "", + validatePerPodProviderFields: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + setupTestProviders(t) + + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.KubeletServiceAccountTokenForCredentialProviders, tc.featureGateEnabled) + + tc.setupProviders() + + keyring := NewExternalCredentialProviderDockerKeyring("test-namespace", "test-pod", "test-uid", "test-sa") + + externalKeyring, ok := keyring.(*externalCredentialProviderKeyring) + if !ok { + t.Fatalf("Expected externalCredentialProviderKeyring, got %T", keyring) + } + + if len(externalKeyring.providers) != tc.expectedProviderCount { + t.Errorf("Expected %d providers, got %d", tc.expectedProviderCount, len(externalKeyring.providers)) + } + + if tc.validatePerPodProviderFields && len(externalKeyring.providers) > 0 { + perPodProvider, ok := externalKeyring.providers[0].(*perPodPluginProvider) + if !ok { + t.Fatalf("Expected perPodPluginProvider, got %T", externalKeyring.providers[0]) + } + + if perPodProvider.podNamespace != tc.expectedPodNamespace { + t.Errorf("Expected podNamespace '%s', got %s", tc.expectedPodNamespace, perPodProvider.podNamespace) + } + if perPodProvider.podName != tc.expectedPodName { + t.Errorf("Expected podName '%s', got %s", tc.expectedPodName, perPodProvider.podName) + } + if string(perPodProvider.podUID) != tc.expectedPodUID { + t.Errorf("Expected podUID '%s', got %s", tc.expectedPodUID, string(perPodProvider.podUID)) + } + if perPodProvider.serviceAccountName != tc.expectedServiceAccountName { + t.Errorf("Expected serviceAccountName '%s', got %s", tc.expectedServiceAccountName, perPodProvider.serviceAccountName) + } + } + }) + } +} + +func TestExternalCredentialProviderKeyringLookupNoProviders(t *testing.T) { + keyring := &externalCredentialProviderKeyring{ + providers: []dockerConfigProviderWithCoordinates{}, + } + + configs, found := keyring.Lookup("test.registry.io/image:tag") + + if found { + t.Errorf("Expected not found, got found=true") + } + if len(configs) != 0 { + t.Errorf("Expected 0 configs, got %d", len(configs)) + } +} + +func TestExternalCredentialProviderKeyringLookupWithProviders(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.KubeletEnsureSecretPulledImages, true) + + testCases := []struct { + name string + image string + providers []dockerConfigProviderWithCoordinates + expectedConfigCount int + expectedFound bool + expectedServiceAccountOK bool + }{ + { + name: "single provider with config", + image: "test.registry.io/image:tag", + providers: []dockerConfigProviderWithCoordinates{ + &fakedockerConfigProviderWithCoordinates{ + dockerConfig: credentialprovider.DockerConfig{ + "test.registry.io": credentialprovider.DockerConfigEntry{ + Username: "user1", + Password: "pass1", + }, + }, + serviceAccountCoords: nil, + }, + }, + expectedConfigCount: 1, + expectedFound: true, + expectedServiceAccountOK: false, + }, + { + name: "single provider with service account coordinates", + image: "test.registry.io/image:tag", + providers: []dockerConfigProviderWithCoordinates{ + &fakedockerConfigProviderWithCoordinates{ + dockerConfig: credentialprovider.DockerConfig{ + "test.registry.io": credentialprovider.DockerConfigEntry{ + Username: "user1", + Password: "pass1", + }, + }, + serviceAccountCoords: &credentialprovider.ServiceAccountCoordinates{ + Namespace: "test-namespace", + Name: "test-sa", + UID: "test-uid", + }, + }, + }, + expectedConfigCount: 1, + expectedFound: true, + expectedServiceAccountOK: true, + }, + { + name: "multiple providers", + image: "test.registry.io/image:tag", + providers: []dockerConfigProviderWithCoordinates{ + &fakedockerConfigProviderWithCoordinates{ + dockerConfig: credentialprovider.DockerConfig{ + "test.registry.io": credentialprovider.DockerConfigEntry{ + Username: "user1", + Password: "pass1", + }, + }, + serviceAccountCoords: &credentialprovider.ServiceAccountCoordinates{ + Namespace: "test-namespace", + Name: "test-sa-1", + UID: "test-uid-1", + }, + }, + &fakedockerConfigProviderWithCoordinates{ + dockerConfig: credentialprovider.DockerConfig{ + "test.registry.io": credentialprovider.DockerConfigEntry{ + Username: "user2", + Password: "pass2", + }, + }, + serviceAccountCoords: &credentialprovider.ServiceAccountCoordinates{ + Namespace: "test-namespace", + Name: "test-sa-2", + UID: "test-uid-2", + }, + }, + }, + expectedConfigCount: 2, + expectedFound: true, + expectedServiceAccountOK: true, + }, + { + name: "provider with empty config", + image: "test.registry.io/image:tag", + providers: []dockerConfigProviderWithCoordinates{ + &fakedockerConfigProviderWithCoordinates{ + dockerConfig: credentialprovider.DockerConfig{}, + serviceAccountCoords: nil, + }, + }, + expectedConfigCount: 0, + expectedFound: false, + expectedServiceAccountOK: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + keyring := &externalCredentialProviderKeyring{ + providers: tc.providers, + } + + configs, found := keyring.Lookup(tc.image) + + if found != tc.expectedFound { + t.Errorf("Expected found=%v, got found=%v", tc.expectedFound, found) + } + + if len(configs) != tc.expectedConfigCount { + t.Errorf("Expected %d configs, got %d", tc.expectedConfigCount, len(configs)) + } + + // Verify that each provider was called with the correct image + for i, provider := range tc.providers { + mockProvider := provider.(*fakedockerConfigProviderWithCoordinates) + if mockProvider.getCallCount() != 1 { + t.Errorf("Provider %d expected 1 call, got %d", i, mockProvider.getCallCount()) + } + if mockProvider.getLastImageRequested() != tc.image { + t.Errorf("Provider %d expected image %s, got %s", i, tc.image, mockProvider.getLastImageRequested()) + } + } + + // Verify service account coordinates in TrackedAuthConfig + if tc.expectedServiceAccountOK && len(configs) > 0 { + foundServiceAccountCoords := false + for _, config := range configs { + if config.Source != nil && config.Source.ServiceAccount != nil { + foundServiceAccountCoords = true + break + } + } + if !foundServiceAccountCoords { + t.Errorf("Expected to find service account coordinates in TrackedAuthConfig") + } + } + }) + } +} + +func TestExternalCredentialProviderKeyringLookupConcurrency(t *testing.T) { + // Test concurrent access to the keyring + mockProvider := &fakedockerConfigProviderWithCoordinates{ + dockerConfig: credentialprovider.DockerConfig{ + "test.registry.io": credentialprovider.DockerConfigEntry{ + Username: "user1", + Password: "pass1", + }, + }, + serviceAccountCoords: &credentialprovider.ServiceAccountCoordinates{ + Namespace: "test-namespace", + Name: "test-sa", + UID: "test-uid", + }, + } + + keyring := &externalCredentialProviderKeyring{ + providers: []dockerConfigProviderWithCoordinates{mockProvider}, + } + + const numGoroutines = 10 + const numCallsPerGoroutine = 5 + + var wg sync.WaitGroup + var errorOccurred bool + var mu sync.Mutex + + wg.Add(numGoroutines) + + for i := range numGoroutines { + go func(goroutineID int) { + defer wg.Done() + for j := range numCallsPerGoroutine { + image := "test.registry.io/image:tag" + configs, found := keyring.Lookup(image) + + if !found { + mu.Lock() + errorOccurred = true + t.Errorf("Goroutine %d call %d: Expected found=true, got found=false", goroutineID, j) + mu.Unlock() + return + } + if len(configs) != 1 { + mu.Lock() + errorOccurred = true + t.Errorf("Goroutine %d call %d: Expected 1 config, got %d", goroutineID, j, len(configs)) + mu.Unlock() + return + } + } + }(i) + } + + wg.Wait() + + if !errorOccurred { + expectedTotalCalls := numGoroutines * numCallsPerGoroutine + actualCalls := mockProvider.getCallCount() + if actualCalls != expectedTotalCalls { + t.Errorf("Expected %d total calls, got %d", expectedTotalCalls, actualCalls) + } + } +} diff --git a/pkg/credentialprovider/secrets/secrets.go b/pkg/credentialprovider/secrets/secrets.go index eab1e22ab40..0f5c28f63e0 100644 --- a/pkg/credentialprovider/secrets/secrets.go +++ b/pkg/credentialprovider/secrets/secrets.go @@ -49,7 +49,7 @@ func secretsToTrackedDockerConfigs(secrets []v1.Secret) (credentialprovider.Dock return nil, err } - coords := credentialprovider.SecretCoordinates{ + coords := &credentialprovider.SecretCoordinates{ UID: string(passedSecret.UID), Namespace: passedSecret.Namespace, Name: passedSecret.Name} @@ -62,7 +62,7 @@ func secretsToTrackedDockerConfigs(secrets []v1.Secret) (credentialprovider.Dock return nil, err } - coords := credentialprovider.SecretCoordinates{ + coords := &credentialprovider.SecretCoordinates{ UID: string(passedSecret.UID), Namespace: passedSecret.Namespace, Name: passedSecret.Name} diff --git a/pkg/credentialprovider/secrets/secrets_test.go b/pkg/credentialprovider/secrets/secrets_test.go index e96112fd3b3..90d516c5c27 100644 --- a/pkg/credentialprovider/secrets/secrets_test.go +++ b/pkg/credentialprovider/secrets/secrets_test.go @@ -67,7 +67,7 @@ func Test_MakeDockerKeyring(t *testing.T) { expectedAuthConfigs: []credentialprovider.TrackedAuthConfig{ { Source: &credentialprovider.CredentialSource{ - Secret: credentialprovider.SecretCoordinates{ + Secret: &credentialprovider.SecretCoordinates{ Name: "s1", Namespace: "ns1", UID: "uid1"}, }, AuthConfig: credentialprovider.AuthConfig{ @@ -95,7 +95,7 @@ func Test_MakeDockerKeyring(t *testing.T) { expectedAuthConfigs: []credentialprovider.TrackedAuthConfig{ { Source: &credentialprovider.CredentialSource{ - Secret: credentialprovider.SecretCoordinates{ + Secret: &credentialprovider.SecretCoordinates{ Name: "s1", Namespace: "ns1", UID: "uid1"}, }, AuthConfig: credentialprovider.AuthConfig{ @@ -123,7 +123,7 @@ func Test_MakeDockerKeyring(t *testing.T) { expectedAuthConfigs: []credentialprovider.TrackedAuthConfig{ { Source: &credentialprovider.CredentialSource{ - Secret: credentialprovider.SecretCoordinates{ + Secret: &credentialprovider.SecretCoordinates{ Name: "s1", Namespace: "ns1", UID: "uid1"}, }, AuthConfig: credentialprovider.AuthConfig{ @@ -151,7 +151,7 @@ func Test_MakeDockerKeyring(t *testing.T) { expectedAuthConfigs: []credentialprovider.TrackedAuthConfig{ { Source: &credentialprovider.CredentialSource{ - Secret: credentialprovider.SecretCoordinates{ + Secret: &credentialprovider.SecretCoordinates{ Name: "s1", Namespace: "ns1", UID: "uid1"}, }, AuthConfig: credentialprovider.AuthConfig{ @@ -314,7 +314,7 @@ func Test_MakeDockerKeyring(t *testing.T) { expectedAuthConfigs: []credentialprovider.TrackedAuthConfig{ { Source: &credentialprovider.CredentialSource{ - Secret: credentialprovider.SecretCoordinates{ + Secret: &credentialprovider.SecretCoordinates{ Name: "s1", Namespace: "ns1", UID: "uid1"}, }, AuthConfig: credentialprovider.AuthConfig{