From f920aea76cdac875ef2755b588f5e971dc16ffb1 Mon Sep 17 00:00:00 2001 From: Anton Tykhyy Date: Wed, 23 Jul 2025 20:25:39 +0300 Subject: [PATCH] kubelet: delay looking up pod image pull credentials until necessary Some decisions about image pull credential verification can be applied without reference to image pull credentials: e.g. if the policy is NeverVerify, or if it is NeverVerifyPreloadedImages and the image is preloaded, or if is NeverVerifyAllowListedImages and the image is white-listed. In these cases, there is no need to look up credentials. --- pkg/kubelet/images/image_manager.go | 92 +++-- pkg/kubelet/images/image_manager_test.go | 27 +- .../images/pullmanager/benchmarks_test.go | 8 +- .../images/pullmanager/image_pull_manager.go | 31 +- .../pullmanager/image_pull_manager_test.go | 351 ++++++++++-------- pkg/kubelet/images/pullmanager/interfaces.go | 12 +- .../images/pullmanager/noop_pull_manager.go | 4 +- 7 files changed, 305 insertions(+), 220 deletions(-) diff --git a/pkg/kubelet/images/image_manager.go b/pkg/kubelet/images/image_manager.go index f52297c3ec656..2cd10a076457d 100644 --- a/pkg/kubelet/images/image_manager.go +++ b/pkg/kubelet/images/image_manager.go @@ -178,41 +178,15 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR return "", message, err } - if imageRef != "" && !utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) { - msg := fmt.Sprintf("Container image %q already present on machine", requestedImage) - m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info) - return imageRef, msg, nil - } - - repoToPull, _, _, err := parsers.ParseImageName(spec.Image) - if err != nil { - return "", err.Error(), err - } + // wrap the lookup in a function to ensure that we only look up credentials once and no earlier than needed + lookupPullCredentials := m.makeLookupPullCredentialsFunc(spec.Image, pod, pullSecrets, podSandboxConfig) - // construct the dynamic keyring using the providers we have in the kubelet - var podName, podNamespace, podUID string - if utilfeature.DefaultFeatureGate.Enabled(features.KubeletServiceAccountTokenForCredentialProviders) { - sandboxMetadata := podSandboxConfig.GetMetadata() - - podName = sandboxMetadata.Name - podNamespace = sandboxMetadata.Namespace - podUID = sandboxMetadata.Uid - } - - externalCredentialProviderKeyring := credentialproviderplugin.NewExternalCredentialProviderDockerKeyring( - podNamespace, - podName, - podUID, - pod.Spec.ServiceAccountName) - - keyring, err := credentialprovidersecrets.MakeDockerKeyring(pullSecrets, credentialprovider.UnionDockerKeyring{m.nodeKeyring, externalCredentialProviderKeyring}) - if err != nil { - return "", err.Error(), err - } - - pullCredentials, _ := keyring.Lookup(repoToPull) + getPodCredentials := func() ([]kubeletconfiginternal.ImagePullSecret, *kubeletconfiginternal.ImagePullServiceAccount, error) { + pullCredentials, err := lookupPullCredentials() + if err != nil { + return nil, nil, err + } - if imageRef != "" { var imagePullSecrets []kubeletconfiginternal.ImagePullSecret // we don't take the audience of the service account into account, so there can only // be one imagePullServiceAccount per pod when we try to make a decision. @@ -239,8 +213,18 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR } } - pullRequired := m.imagePullManager.MustAttemptImagePull(requestedImage, imageRef, imagePullSecrets, imagePullServiceAccount) - if !pullRequired { + return imagePullSecrets, imagePullServiceAccount, nil + } + + if imageRef != "" { + if !utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) { + msg := fmt.Sprintf("Container image %q already present on machine", requestedImage) + m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info) + return imageRef, msg, nil + } + if pullRequired, err := m.imagePullManager.MustAttemptImagePull(requestedImage, imageRef, getPodCredentials); err != nil { + return "", err.Error(), err + } else if !pullRequired { msg := fmt.Sprintf("Container image %q already present on machine and can be accessed by the pod", requestedImage) m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info) return imageRef, msg, nil @@ -254,9 +238,47 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR return "", msg, err } + pullCredentials, err := lookupPullCredentials() + if err != nil { + return "", err.Error(), err + } + return m.pullImage(ctx, logPrefix, objRef, pod.UID, requestedImage, spec, pullCredentials, podSandboxConfig) } +func (m *imageManager) makeLookupPullCredentialsFunc(image string, pod *v1.Pod, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) func() ([]credentialprovider.TrackedAuthConfig, error) { + return sync.OnceValues(func() ([]credentialprovider.TrackedAuthConfig, error) { + repoToPull, _, _, err := parsers.ParseImageName(image) + if err != nil { + return nil, err + } + + // construct the dynamic keyring using the providers we have in the kubelet + var podName, podNamespace, podUID string + if utilfeature.DefaultFeatureGate.Enabled(features.KubeletServiceAccountTokenForCredentialProviders) { + sandboxMetadata := podSandboxConfig.GetMetadata() + + podName = sandboxMetadata.Name + podNamespace = sandboxMetadata.Namespace + podUID = sandboxMetadata.Uid + } + + externalCredentialProviderKeyring := credentialproviderplugin.NewExternalCredentialProviderDockerKeyring( + podNamespace, + podName, + podUID, + pod.Spec.ServiceAccountName) + + keyring, err := credentialprovidersecrets.MakeDockerKeyring(pullSecrets, credentialprovider.UnionDockerKeyring{m.nodeKeyring, externalCredentialProviderKeyring}) + if err != nil { + return nil, err + } + + pullCredentials, _ := keyring.Lookup(repoToPull) + return pullCredentials, nil + }) +} + func (m *imageManager) pullImage(ctx context.Context, logPrefix string, objRef *v1.ObjectReference, podUID types.UID, image string, imgSpec kubecontainer.ImageSpec, pullCredentials []credentialprovider.TrackedAuthConfig, podSandboxConfig *runtimeapi.PodSandboxConfig) (imageRef, message string, err error) { var pullSucceeded bool var finalPullCredentials *credentialprovider.TrackedAuthConfig diff --git a/pkg/kubelet/images/image_manager_test.go b/pkg/kubelet/images/image_manager_test.go index 1f98d734a72e9..ee5ef0632378b 100644 --- a/pkg/kubelet/images/image_manager_test.go +++ b/pkg/kubelet/images/image_manager_test.go @@ -694,9 +694,14 @@ type mockImagePullManager struct { config *mockImagePullManagerConfig } -func (m *mockImagePullManager) MustAttemptImagePull(image, _ string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool { +func (m *mockImagePullManager) MustAttemptImagePull(image, _ string, getPodCredentials pullmanager.GetPodCredentials) (bool, error) { if m.config == nil || m.config.allowAll { - return false + return false, nil + } + + podSecrets, podServiceAccount, err := getPodCredentials() + if err != nil { + return true, err } // Check secrets @@ -704,7 +709,7 @@ func (m *mockImagePullManager) MustAttemptImagePull(image, _ string, podSecrets for _, s := range podSecrets { for _, allowed := range allowedSecrets { if s.Namespace == allowed.Namespace && s.Name == allowed.Name && s.UID == allowed.UID { - return false + return false, nil } } } @@ -714,12 +719,12 @@ func (m *mockImagePullManager) MustAttemptImagePull(image, _ string, podSecrets if podServiceAccount != nil { if allowedServiceAccounts, ok := m.config.allowedServiceAccounts[image]; ok { if slices.Contains(allowedServiceAccounts, *podServiceAccount) { - return false + return false, nil } } } - return true + return true, nil } // mockImagePullManagerWithTracking tracks calls to MustAttemptImagePull for service account testing @@ -736,19 +741,25 @@ type mockImagePullManagerWithTracking struct { recordedCredentials *kubeletconfiginternal.ImagePullCredentials } -func (m *mockImagePullManagerWithTracking) MustAttemptImagePull(image, imageRef string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool { +func (m *mockImagePullManagerWithTracking) MustAttemptImagePull(image, imageRef string, getPodCredentials pullmanager.GetPodCredentials) (bool, error) { m.mustAttemptCalled = true m.lastImage = image m.lastImageRef = imageRef + + podSecrets, podServiceAccount, err := getPodCredentials() + if err != nil { + return true, err + } + m.lastSecrets = podSecrets if podServiceAccount != nil { m.lastServiceAccounts = []kubeletconfiginternal.ImagePullServiceAccount{*podServiceAccount} } if m.allowAll { - return false + return false, nil } - return m.mustAttemptReturn + return m.mustAttemptReturn, nil } func (m *mockImagePullManagerWithTracking) RecordImagePulled(image, imageRef string, credentials *kubeletconfiginternal.ImagePullCredentials) { diff --git a/pkg/kubelet/images/pullmanager/benchmarks_test.go b/pkg/kubelet/images/pullmanager/benchmarks_test.go index 69c759b30a916..8e4bea4329b7c 100644 --- a/pkg/kubelet/images/pullmanager/benchmarks_test.go +++ b/pkg/kubelet/images/pullmanager/benchmarks_test.go @@ -72,9 +72,11 @@ func directRecordReadFunc(expectHit bool) benchmarkedCheckFunc { func mustAttemptPullReadFunc(expectHit bool) benchmarkedCheckFunc { return func(b *testing.B, pullManager PullManager, imgRef string) { - mustPull := pullManager.MustAttemptImagePull("test.repo/org/"+imgRef, imgRef, nil, nil) - if mustPull != !expectHit { - b.Fatalf("MustAttemptImagePull() expected %t, got %t", !expectHit, mustPull) + mustPull, err := pullManager.MustAttemptImagePull("test.repo/org/"+imgRef, imgRef, func() ([]kubeletconfig.ImagePullSecret, *kubeletconfig.ImagePullServiceAccount, error) { + return nil, nil, nil + }) + if mustPull != !expectHit || err != nil { + b.Fatalf("no error expected (got %v); MustAttemptImagePull() expected %t, got %t", err, !expectHit, mustPull) } } } diff --git a/pkg/kubelet/images/pullmanager/image_pull_manager.go b/pkg/kubelet/images/pullmanager/image_pull_manager.go index 0cf489c39f0b2..ceea2f9620b51 100644 --- a/pkg/kubelet/images/pullmanager/image_pull_manager.go +++ b/pkg/kubelet/images/pullmanager/image_pull_manager.go @@ -179,9 +179,9 @@ func (f *PullManager) decrementImagePullIntent(image string) { f.decrementIntentCounterForImage(image) } -func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool { +func (f *PullManager) MustAttemptImagePull(image, imageRef string, getPodCredentials GetPodCredentials) (bool, error) { if len(imageRef) == 0 { - return true + return true, nil } var imagePulledByKubelet bool @@ -225,36 +225,41 @@ func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets [] if err != nil { klog.ErrorS(err, "Unable to access cache records about image pulls") - return true + return true, nil } if !f.imagePolicyEnforcer.RequireCredentialVerificationForImage(image, imagePulledByKubelet) { - return false + return false, nil } if pulledRecord == nil { // we have no proper records of the image being pulled in the past, we can short-circuit here - return true + return true, nil } sanitizedImage, err := trimImageTagDigest(image) if err != nil { klog.ErrorS(err, "failed to parse image name, forcing image credentials reverification", "image", sanitizedImage) - return true + return true, nil } cachedCreds, ok := pulledRecord.CredentialMapping[sanitizedImage] if !ok { - return true + return true, nil } if cachedCreds.NodePodsAccessible { // anyone on this node can access the image - return false + return false, nil } if len(cachedCreds.KubernetesSecrets) == 0 && len(cachedCreds.KubernetesServiceAccounts) == 0 { - return true + return true, nil + } + + podSecrets, podServiceAccount, err := getPodCredentials() + if err != nil { + return true, err } for _, podSecret := range podSecrets { @@ -275,7 +280,7 @@ func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets [] klog.ErrorS(err, "failed to write an image pulled record", "image", image, "imageRef", imageRef) } } - return false + return false, nil } if secretCoordinatesMatch { @@ -286,7 +291,7 @@ func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets [] if err := f.writePulledRecordIfChanged(image, imageRef, &kubeletconfiginternal.ImagePullCredentials{KubernetesSecrets: []kubeletconfiginternal.ImagePullSecret{podSecret}}); err != nil { klog.ErrorS(err, "failed to write an image pulled record", "image", image, "imageRef", imageRef) } - return false + return false, nil } } } @@ -294,10 +299,10 @@ func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets [] if podServiceAccount != nil && slices.Contains(cachedCreds.KubernetesServiceAccounts, *podServiceAccount) { // we found a matching service account, no need to pull the image - return false + return false, nil } - return true + return true, nil } func (f *PullManager) PruneUnknownRecords(imageList []string, until time.Time) { diff --git a/pkg/kubelet/images/pullmanager/image_pull_manager_test.go b/pkg/kubelet/images/pullmanager/image_pull_manager_test.go index 8e05041857d34..ef36d0ad60d77 100644 --- a/pkg/kubelet/images/pullmanager/image_pull_manager_test.go +++ b/pkg/kubelet/images/pullmanager/image_pull_manager_test.go @@ -425,17 +425,18 @@ func Test_pulledRecordMergeNewCreds(t *testing.T) { func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { tests := []struct { - name string - imagePullPolicy ImagePullPolicyEnforcer - podSecrets []kubeletconfiginternal.ImagePullSecret - podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount - image string - imageRef string - pulledFiles []string - pullingFiles []string - expectedPullRecord *kubeletconfiginternal.ImagePulledRecord - want bool - expectedCacheWrite bool + name string + imagePullPolicy ImagePullPolicyEnforcer + podSecrets []kubeletconfiginternal.ImagePullSecret + podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount + image string + imageRef string + pulledFiles []string + pullingFiles []string + expectedPullRecord *kubeletconfiginternal.ImagePulledRecord + want bool + wantPodCredsRequested bool + expectedCacheWrite bool }{ { name: "image exists and is recorded with pod's exact secret", @@ -445,18 +446,20 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "testsecrethash", }, }, - image: "docker.io/testing/test:latest", - imageRef: "testimageref", - pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, - want: false, + image: "docker.io/testing/test:latest", + imageRef: "testimageref", + pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, + want: false, + wantPodCredsRequested: true, }, { - name: "image exists and is recorded, no pod secrets", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - image: "docker.io/testing/test:latest", - imageRef: "testimageref", - pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, - want: true, + name: "image exists and is recorded, no pod secrets", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + image: "docker.io/testing/test:latest", + imageRef: "testimageref", + pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, + want: true, + wantPodCredsRequested: true, }, { name: "image exists and is recorded with the same secret but different credential hash", @@ -479,8 +482,9 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { }, }, }, - want: false, - expectedCacheWrite: true, + want: false, + wantPodCredsRequested: true, + expectedCacheWrite: true, }, { name: "image exists and is recorded with a different secret with a different UID", @@ -490,10 +494,11 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "different uid", Namespace: "default", Name: "pull-secret", CredentialHash: "differenthash", }, }, - image: "docker.io/testing/test:latest", - imageRef: "testimageref", - pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, - want: true, + image: "docker.io/testing/test:latest", + imageRef: "testimageref", + pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, + want: true, + wantPodCredsRequested: true, }, { name: "image exists and is recorded with a different secret", @@ -503,10 +508,11 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "differentns", Name: "pull-secret", CredentialHash: "differenthash", }, }, - image: "docker.io/testing/test:latest", - imageRef: "testimageref", - pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, - want: true, + image: "docker.io/testing/test:latest", + imageRef: "testimageref", + pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, + want: true, + wantPodCredsRequested: true, }, { name: "image exists and is recorded with a different secret with the same credential hash", @@ -530,8 +536,9 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { }, }, }, - want: false, - expectedCacheWrite: true, + want: false, + wantPodCredsRequested: true, + expectedCacheWrite: true, }, { name: "image exists but the pull is recorded with a different image name but with the exact same secret", @@ -541,39 +548,44 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "testsecrethash", }, }, - image: "docker.io/testing/different:latest", - imageRef: "testimageref", - pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, - want: true, + image: "docker.io/testing/different:latest", + imageRef: "testimageref", + pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, + want: true, + wantPodCredsRequested: false, }, { - name: "image exists and is recorded with empty credential mapping", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - image: "docker.io/testing/test:latest", - imageRef: "testemptycredmapping", - pulledFiles: []string{"sha256-f8778b6393eaf39315e767a58cbeacf2c4b270d94b4d6926ee993d9e49444991"}, - want: true, + name: "image exists and is recorded with empty credential mapping", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + image: "docker.io/testing/test:latest", + imageRef: "testemptycredmapping", + pulledFiles: []string{"sha256-f8778b6393eaf39315e767a58cbeacf2c4b270d94b4d6926ee993d9e49444991"}, + want: true, + wantPodCredsRequested: false, }, { - name: "image does not exist and there are no records of it", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - image: "docker.io/testing/test:latest", - imageRef: "", - want: true, + name: "image does not exist and there are no records of it", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + image: "docker.io/testing/test:latest", + imageRef: "", + want: true, + wantPodCredsRequested: false, }, { - name: "image exists and there are no records of it with NeverVerifyPreloadedImages pull policy", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - image: "docker.io/testing/test:latest", - imageRef: "testexistingref", - want: false, + name: "image exists and there are no records of it with NeverVerifyPreloadedImages pull policy", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + image: "docker.io/testing/test:latest", + imageRef: "testexistingref", + want: false, + wantPodCredsRequested: false, }, { - name: "image exists and there are no records of it with AlwaysVerify pull policy", - imagePullPolicy: AlwaysVerifyImagePullPolicy(), - image: "docker.io/testing/test:latest", - imageRef: "testexistingref", - want: true, + name: "image exists and there are no records of it with AlwaysVerify pull policy", + imagePullPolicy: AlwaysVerifyImagePullPolicy(), + image: "docker.io/testing/test:latest", + imageRef: "testexistingref", + want: true, + wantPodCredsRequested: false, }, { name: "image exists but is only recorded via pulling intent", @@ -583,10 +595,11 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "testsecrethash", }, }, - image: "docker.io/testing/test:latest", - imageRef: "testexistingref", - pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, - want: true, + image: "docker.io/testing/test:latest", + imageRef: "testexistingref", + pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, + want: true, + wantPodCredsRequested: false, }, { name: "image exists but is only recorded via pulling intent - NeverVerify policy", @@ -596,18 +609,20 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "testsecrethash", }, }, - image: "docker.io/testing/test:latest", - imageRef: "testexistingref", - pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, - want: false, + image: "docker.io/testing/test:latest", + imageRef: "testexistingref", + pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, + want: false, + wantPodCredsRequested: false, }, { - name: "image exists and is recorded as node-accessible, no pod secrets", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - image: "docker.io/testing/test:latest", - imageRef: "testimage-anonpull", - pulledFiles: []string{"sha256-a2eace2182b24cdbbb730798e47b10709b9ef5e0f0c1624a3bc06c8ca987727a"}, - want: false, + name: "image exists and is recorded as node-accessible, no pod secrets", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + image: "docker.io/testing/test:latest", + imageRef: "testimage-anonpull", + pulledFiles: []string{"sha256-a2eace2182b24cdbbb730798e47b10709b9ef5e0f0c1624a3bc06c8ca987727a"}, + want: false, + wantPodCredsRequested: false, }, { name: "image exists and is recorded as node-accessible, request with pod secrets", @@ -617,10 +632,11 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "testsecrethash", }, }, - image: "docker.io/testing/test:latest", - imageRef: "testimage-anonpull", - pulledFiles: []string{"sha256-a2eace2182b24cdbbb730798e47b10709b9ef5e0f0c1624a3bc06c8ca987727a"}, - want: false, + image: "docker.io/testing/test:latest", + imageRef: "testimage-anonpull", + pulledFiles: []string{"sha256-a2eace2182b24cdbbb730798e47b10709b9ef5e0f0c1624a3bc06c8ca987727a"}, + want: false, + wantPodCredsRequested: false, }, { name: "image exists and is recorded with empty hash as its hashing originally failed, the same fail for a different pod secret", @@ -630,10 +646,11 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "differentns", Name: "pull-secret", CredentialHash: "", }, }, - image: "docker.io/testing/test:latest", - imageRef: "test-brokenhash", - pulledFiles: []string{"sha256-38a8906435c4dd5f4258899d46621bfd8eea3ad6ff494ee3c2f17ef0321625bd"}, - want: true, + image: "docker.io/testing/test:latest", + imageRef: "test-brokenhash", + pulledFiles: []string{"sha256-38a8906435c4dd5f4258899d46621bfd8eea3ad6ff494ee3c2f17ef0321625bd"}, + want: true, + wantPodCredsRequested: true, }, { name: "image exists and is recorded with empty hash as its hashing originally failed, the same fail for the same pod secret", @@ -643,82 +660,91 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "", }, }, - image: "docker.io/testing/test:latest", - imageRef: "test-brokenhash", - pulledFiles: []string{"sha256-38a8906435c4dd5f4258899d46621bfd8eea3ad6ff494ee3c2f17ef0321625bd"}, - want: false, - }, - - { - name: "image exists and is recorded with pod's exact service account", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, - image: "docker.io/testing/test:latest", - imageRef: "testimageref-sa", - pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, - want: false, - }, - { - name: "image exists and is recorded, no pod service accounts", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - image: "docker.io/testing/test:latest", - imageRef: "testimageref", - pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, - want: true, - }, - { - name: "image exists and is recorded with a different service account with different UID", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "different-sa-uid", Namespace: "default", Name: "test-sa"}, - image: "docker.io/testing/test:latest", - imageRef: "testimageref-sa", - pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, - want: true, - }, - { - name: "image exists and is recorded with a different service account with different namespace", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "different-ns", Name: "test-sa"}, - image: "docker.io/testing/test:latest", - imageRef: "testimageref-sa", - pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, - want: true, - }, - { - name: "image exists but the pull is recorded with a different image name but with the exact same service account", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, - image: "docker.io/testing/different:latest", - imageRef: "testimageref-sa", - pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, - want: true, - }, - { - name: "image exists but is only recorded via pulling intent with service account", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, - image: "docker.io/testing/test:latest", - imageRef: "testexistingref", - pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, - want: true, - }, - { - name: "image exists but is only recorded via pulling intent with service account - NeverVerify policy", - imagePullPolicy: NeverVerifyImagePullPolicy(), - podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, - image: "docker.io/testing/test:latest", - imageRef: "testexistingref", - pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, - want: false, - }, - { - name: "image exists and is recorded as node-accessible, request with pod service accounts", - imagePullPolicy: NeverVerifyPreloadedPullPolicy(), - podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, - image: "docker.io/testing/test:latest", - imageRef: "testimage-anonpull", - pulledFiles: []string{"sha256-a2eace2182b24cdbbb730798e47b10709b9ef5e0f0c1624a3bc06c8ca987727a"}, - want: false, + image: "docker.io/testing/test:latest", + imageRef: "test-brokenhash", + pulledFiles: []string{"sha256-38a8906435c4dd5f4258899d46621bfd8eea3ad6ff494ee3c2f17ef0321625bd"}, + want: false, + wantPodCredsRequested: true, + }, + + { + name: "image exists and is recorded with pod's exact service account", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, + image: "docker.io/testing/test:latest", + imageRef: "testimageref-sa", + pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, + want: false, + wantPodCredsRequested: true, + }, + { + name: "image exists and is recorded, no pod service accounts", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + image: "docker.io/testing/test:latest", + imageRef: "testimageref", + pulledFiles: []string{"sha256-b3c0cc4278800b03a308ceb2611161430df571ca733122f0a40ac8b9792a9064"}, + want: true, + wantPodCredsRequested: true, + }, + { + name: "image exists and is recorded with a different service account with different UID", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "different-sa-uid", Namespace: "default", Name: "test-sa"}, + image: "docker.io/testing/test:latest", + imageRef: "testimageref-sa", + pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, + want: true, + wantPodCredsRequested: true, + }, + { + name: "image exists and is recorded with a different service account with different namespace", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "different-ns", Name: "test-sa"}, + image: "docker.io/testing/test:latest", + imageRef: "testimageref-sa", + pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, + want: true, + wantPodCredsRequested: true, + }, + { + name: "image exists but the pull is recorded with a different image name but with the exact same service account", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, + image: "docker.io/testing/different:latest", + imageRef: "testimageref-sa", + pulledFiles: []string{"sha256-917e8b3439bf8a7a6f37ffd2d2ddfdfafac8a251bf214a0be39675742b420b1a"}, + want: true, + wantPodCredsRequested: false, + }, + { + name: "image exists but is only recorded via pulling intent with service account", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, + image: "docker.io/testing/test:latest", + imageRef: "testexistingref", + pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, + want: true, + wantPodCredsRequested: false, + }, + { + name: "image exists but is only recorded via pulling intent with service account - NeverVerify policy", + imagePullPolicy: NeverVerifyImagePullPolicy(), + podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, + image: "docker.io/testing/test:latest", + imageRef: "testexistingref", + pullingFiles: []string{"sha256-aef2af226629a35d5f3ef0fdbb29fdbebf038d0acd8850590e8c48e1e283aa56"}, + want: false, + wantPodCredsRequested: false, + }, + { + name: "image exists and is recorded as node-accessible, request with pod service accounts", + imagePullPolicy: NeverVerifyPreloadedPullPolicy(), + podServiceAccount: &kubeletconfiginternal.ImagePullServiceAccount{UID: "test-sa-uid", Namespace: "default", Name: "test-sa"}, + image: "docker.io/testing/test:latest", + imageRef: "testimage-anonpull", + pulledFiles: []string{"sha256-a2eace2182b24cdbbb730798e47b10709b9ef5e0f0c1624a3bc06c8ca987727a"}, + want: false, + wantPodCredsRequested: false, }, } @@ -743,6 +769,11 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { }, } + var podCredsRequested bool + s := func() ([]kubeletconfiginternal.ImagePullSecret, *kubeletconfiginternal.ImagePullServiceAccount, error) { + podCredsRequested = true + return tt.podSecrets, tt.podServiceAccount, nil + } f := &PullManager{ recordsAccessor: fsRecordAccessor, imagePolicyEnforcer: tt.imagePullPolicy, @@ -750,8 +781,14 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { intentCounters: &sync.Map{}, pulledAccessors: NewStripedLockSet(10), } - if got := f.MustAttemptImagePull(tt.image, tt.imageRef, tt.podSecrets, tt.podServiceAccount); got != tt.want { + if got, err := f.MustAttemptImagePull(tt.image, tt.imageRef, s); err != nil { + t.Errorf("FileBasedImagePullManager.MustAttemptImagePull() unexpected error %v", err) + } else if got != tt.want { t.Errorf("FileBasedImagePullManager.MustAttemptImagePull() = %v, want %v", got, tt.want) + } else if podCredsRequested && !tt.wantPodCredsRequested { + t.Error("expected FileBasedImagePullManager.MustAttemptImagePull() to not look up pod credentials, but it did") + } else if !podCredsRequested && tt.wantPodCredsRequested { + t.Error("expected FileBasedImagePullManager.MustAttemptImagePull() to look up pod credentials, but it did not") } if tt.expectedCacheWrite != (fsRecordAccessor.imagePulledRecordsWrites != 0) { diff --git a/pkg/kubelet/images/pullmanager/interfaces.go b/pkg/kubelet/images/pullmanager/interfaces.go index db386ef99d49e..4a16dfefb468b 100644 --- a/pkg/kubelet/images/pullmanager/interfaces.go +++ b/pkg/kubelet/images/pullmanager/interfaces.go @@ -22,6 +22,9 @@ import ( kubeletconfiginternal "k8s.io/kubernetes/pkg/kubelet/apis/config" ) +// GetPodCredentials lazily looks up and returns a set of pull credentials. +type GetPodCredentials func() ([]kubeletconfiginternal.ImagePullSecret, *kubeletconfiginternal.ImagePullServiceAccount, error) + // ImagePullManager keeps the state of images that were pulled and which are // currently still being pulled. // It should keep an internal state of images currently being pulled by the kubelet @@ -58,11 +61,16 @@ type ImagePullManager interface { // set of credentials or if the image can be accessed by any of the node's pods // or if the image can be accessed by the specified service account. // - // Returns true if the policy demands verification and no record of the pull + // `getPodCredentials` is invoked to retrieve the set of credentials after + // MustAttemptImagePull evaluates the parts of the policy that do not depend on them. + // + // Returns true and an error if `getPodCredentials` returns an error. + // + // Returns true and nil if the policy demands verification and no record of the pull // was found in the cache. // // `image` is the content of the pod's container `image` field. - MustAttemptImagePull(image, imageRef string, credentials []kubeletconfiginternal.ImagePullSecret, serviceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool + MustAttemptImagePull(image, imageRef string, getPodCredentials GetPodCredentials) (bool, error) // PruneUnknownRecords deletes all of the cache ImagePulledRecords for each of the images // whose imageRef does not appear in the `imageList` iff such an record was last updated // _before_ the `until` timestamp. diff --git a/pkg/kubelet/images/pullmanager/noop_pull_manager.go b/pkg/kubelet/images/pullmanager/noop_pull_manager.go index 9920383b69133..8d1d7c0c8efb4 100644 --- a/pkg/kubelet/images/pullmanager/noop_pull_manager.go +++ b/pkg/kubelet/images/pullmanager/noop_pull_manager.go @@ -30,7 +30,7 @@ func (m *NoopImagePullManager) RecordPullIntent(_ string) error { return nil } func (m *NoopImagePullManager) RecordImagePulled(_, _ string, _ *kubeletconfiginternal.ImagePullCredentials) { } func (m *NoopImagePullManager) RecordImagePullFailed(image string) {} -func (m *NoopImagePullManager) MustAttemptImagePull(_, _ string, _ []kubeletconfiginternal.ImagePullSecret, _ *kubeletconfiginternal.ImagePullServiceAccount) bool { - return false +func (m *NoopImagePullManager) MustAttemptImagePull(_, _ string, _ GetPodCredentials) (bool, error) { + return false, nil } func (m *NoopImagePullManager) PruneUnknownRecords(_ []string, _ time.Time) {}