diff --git a/pkg/kubelet/images/image_manager.go b/pkg/kubelet/images/image_manager.go index 9a64910548b..1093359ecc1 100644 --- a/pkg/kubelet/images/image_manager.go +++ b/pkg/kubelet/images/image_manager.go @@ -199,48 +199,14 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR return "", message, err } - if imageRef != "" && !utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) { - imagePullRequired = ptr.To(false) - msg := fmt.Sprintf("Container image %q already present on machine", requestedImage) - m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info) + // 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) - // we need to stop recording if it is still in progress, as the image - // has already been pulled by another pod. - m.podPullingTimeRecorder.RecordImageFinishedPulling(pod.UID) - - return imageRef, msg, nil - } - - repoToPull, _, _, err := parsers.ParseImageName(spec.Image) - if err != nil { - return "", err.Error(), 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 "", err.Error(), err - } - - pullCredentials, _ := keyring.Lookup(repoToPull) - - if imageRef != "" { - imagePullRequired = ptr.To(false) // should be overridden right after this if-block in case pull was required + getPodCredentials := func() ([]kubeletconfiginternal.ImagePullSecret, *kubeletconfiginternal.ImagePullServiceAccount, error) { + pullCredentials, err := lookupPullCredentials() + if err != nil { + return nil, nil, err + } var imagePullSecrets []kubeletconfiginternal.ImagePullSecret // we don't take the audience of the service account into account, so there can only @@ -268,8 +234,26 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR } } - pullRequired := m.imagePullManager.MustAttemptImagePull(ctx, requestedImage, imageRef, imagePullSecrets, imagePullServiceAccount) - if !pullRequired { + return imagePullSecrets, imagePullServiceAccount, nil + } + + if imageRef != "" { + imagePullRequired = ptr.To(false) // should be overridden right after this if-block in case pull was required + + 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) + + // we need to stop recording if it is still in progress, as the image + // has already been pulled by another pod. + m.podPullingTimeRecorder.RecordImageFinishedPulling(pod.UID) + + return imageRef, msg, nil + } + if pullRequired, err := m.imagePullManager.MustAttemptImagePull(ctx, requestedImage, imageRef, getPodCredentials); err != nil { + imagePullRequired = 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) @@ -289,9 +273,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 e8ef0c545c1..4d3a9827688 100644 --- a/pkg/kubelet/images/image_manager_test.go +++ b/pkg/kubelet/images/image_manager_test.go @@ -734,9 +734,14 @@ type mockImagePullManager struct { config *mockImagePullManagerConfig } -func (m *mockImagePullManager) MustAttemptImagePull(ctx context.Context, image, _ string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool { +func (m *mockImagePullManager) MustAttemptImagePull(ctx context.Context, 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 @@ -744,7 +749,7 @@ func (m *mockImagePullManager) MustAttemptImagePull(ctx context.Context, image, 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 } } } @@ -754,12 +759,12 @@ func (m *mockImagePullManager) MustAttemptImagePull(ctx context.Context, image, 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 @@ -776,19 +781,25 @@ type mockImagePullManagerWithTracking struct { recordedCredentials *kubeletconfiginternal.ImagePullCredentials } -func (m *mockImagePullManagerWithTracking) MustAttemptImagePull(ctx context.Context, image, imageRef string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool { +func (m *mockImagePullManagerWithTracking) MustAttemptImagePull(ctx context.Context, 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(ctx context.Context, 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 6eb8a32957e..f9750779ba7 100644 --- a/pkg/kubelet/images/pullmanager/benchmarks_test.go +++ b/pkg/kubelet/images/pullmanager/benchmarks_test.go @@ -74,9 +74,11 @@ func directRecordReadFunc(expectHit bool) benchmarkedCheckFunc { func mustAttemptPullReadFunc(expectHit bool) benchmarkedCheckFunc { return func(b *testing.B, pullManager PullManager, imgRef string) { tCtx := ktesting.Init(b) - mustPull := pullManager.MustAttemptImagePull(tCtx, "test.repo/org/"+imgRef, imgRef, nil, nil) - if mustPull != !expectHit { - b.Fatalf("MustAttemptImagePull() expected %t, got %t", !expectHit, mustPull) + mustPull, err := pullManager.MustAttemptImagePull(tCtx, "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 09a9ef73cd7..98fe1b03387 100644 --- a/pkg/kubelet/images/pullmanager/image_pull_manager.go +++ b/pkg/kubelet/images/pullmanager/image_pull_manager.go @@ -182,9 +182,9 @@ func (f *PullManager) decrementImagePullIntent(ctx context.Context, image string f.decrementIntentCounterForImage(image) } -func (f *PullManager) MustAttemptImagePull(ctx context.Context, image, imageRef string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool { +func (f *PullManager) MustAttemptImagePull(ctx context.Context, image, imageRef string, getPodCredentials GetPodCredentials) (bool, error) { if len(imageRef) == 0 { - return true + return true, nil } logger := klog.FromContext(ctx) @@ -238,42 +238,48 @@ func (f *PullManager) MustAttemptImagePull(ctx context.Context, image, imageRef if err != nil { resultForMetrics = checkResultError logger.Error(err, "Unable to access cache records about image pulls") - return true + return true, nil } if !f.imagePolicyEnforcer.RequireCredentialVerificationForImage(image, imagePulledByKubelet) { resultForMetrics = checkResultCredentialPolicyAllowed - 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 resultForMetrics = checkResultMustAuthenticate - return true + return true, nil } sanitizedImage, err := trimImageTagDigest(image) if err != nil { resultForMetrics = checkResultError logger.Error(err, "failed to parse image name, forcing image credentials reverification", "image", sanitizedImage) - return true + return true, nil } cachedCreds, ok := pulledRecord.CredentialMapping[sanitizedImage] if !ok { resultForMetrics = checkResultMustAuthenticate - return true + return true, nil } if cachedCreds.NodePodsAccessible { // anyone on this node can access the image resultForMetrics = checkResultCredentialRecordFound - return false + return false, nil } if len(cachedCreds.KubernetesSecrets) == 0 && len(cachedCreds.KubernetesServiceAccounts) == 0 { resultForMetrics = checkResultMustAuthenticate - return true + return true, nil + } + + podSecrets, podServiceAccount, err := getPodCredentials() + if err != nil { + resultForMetrics = checkResultError + return true, err } for _, podSecret := range podSecrets { @@ -295,7 +301,7 @@ func (f *PullManager) MustAttemptImagePull(ctx context.Context, image, imageRef } } resultForMetrics = checkResultCredentialRecordFound - return false + return false, nil } if secretCoordinatesMatch { @@ -307,7 +313,7 @@ func (f *PullManager) MustAttemptImagePull(ctx context.Context, image, imageRef logger.Error(err, "failed to write an image pulled record", "image", image, "imageRef", imageRef) } resultForMetrics = checkResultCredentialRecordFound - return false + return false, nil } } } @@ -316,11 +322,11 @@ func (f *PullManager) MustAttemptImagePull(ctx context.Context, image, imageRef if podServiceAccount != nil && slices.Contains(cachedCreds.KubernetesServiceAccounts, *podServiceAccount) { // we found a matching service account, no need to pull the image resultForMetrics = checkResultCredentialRecordFound - return false + return false, nil } resultForMetrics = checkResultMustAuthenticate - return true + return true, nil } func (f *PullManager) PruneUnknownRecords(ctx context.Context, 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 3ba831f2154..96821cf11c0 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, + 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, + 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, + 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, + 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, + 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, + 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, + 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, + 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, + 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, }, } @@ -744,6 +770,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, @@ -752,8 +783,14 @@ func TestFileBasedImagePullManager_MustAttemptImagePull(t *testing.T) { pulledAccessors: NewStripedLockSet(10), } - if got := f.MustAttemptImagePull(tCtx, tt.image, tt.imageRef, tt.podSecrets, tt.podServiceAccount); got != tt.want { + if got, err := f.MustAttemptImagePull(tCtx, 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 b113f45c114..7d8812bc735 100644 --- a/pkg/kubelet/images/pullmanager/interfaces.go +++ b/pkg/kubelet/images/pullmanager/interfaces.go @@ -23,6 +23,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 @@ -59,11 +62,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(ctx context.Context, image, imageRef string, credentials []kubeletconfiginternal.ImagePullSecret, serviceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool + MustAttemptImagePull(ctx context.Context, 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/metrics_test.go b/pkg/kubelet/images/pullmanager/metrics_test.go index 02b01ba81fd..e9fe86f7e2d 100644 --- a/pkg/kubelet/images/pullmanager/metrics_test.go +++ b/pkg/kubelet/images/pullmanager/metrics_test.go @@ -17,6 +17,7 @@ limitations under the License. package pullmanager import ( + "context" "fmt" "path/filepath" "strings" @@ -200,43 +201,52 @@ func TestMustAttemptPullMetrics(t *testing.T) { }) expectedMetrics := make(map[string]int) - require.True(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/broken", "testbrokenrecord", nil, nil)) + testMustAttemptImagePull(t, true, imageManager, ctx, "docker.io/testing/broken", "testbrokenrecord", nil) expectedMetrics[string(checkResultError)]++ cmpMustAttemptPullMetrics(t, expectedMetrics) - require.True(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/broken", "testbrokenrecord", nil, nil)) - require.True(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/broken", "testbrokenrecord", nil, nil)) + testMustAttemptImagePull(t, true, imageManager, ctx, "docker.io/testing/broken", "testbrokenrecord", nil) + testMustAttemptImagePull(t, true, imageManager, ctx, "docker.io/testing/broken", "testbrokenrecord", nil) expectedMetrics[string(checkResultError)] += 2 - require.False(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/test", "testimage-anonpull", nil, nil)) + testMustAttemptImagePull(t, false, imageManager, ctx, "docker.io/testing/test", "testimage-anonpull", nil) expectedMetrics[string(checkResultCredentialRecordFound)]++ cmpMustAttemptPullMetrics(t, expectedMetrics) - require.False(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/test", "testimage-anonpull", nil, nil)) - require.False(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/test", "testimage-anonpull", nil, nil)) - require.False(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/test", "testimage-anonpull", nil, nil)) + testMustAttemptImagePull(t, false, imageManager, ctx, "docker.io/testing/test", "testimage-anonpull", nil) + testMustAttemptImagePull(t, false, imageManager, ctx, "docker.io/testing/test", "testimage-anonpull", nil) + testMustAttemptImagePull(t, false, imageManager, ctx, "docker.io/testing/test", "testimage-anonpull", nil) expectedMetrics[string(checkResultCredentialRecordFound)] += 3 cmpMustAttemptPullMetrics(t, expectedMetrics) - require.False(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/policyexempt", "policyallowed", nil, nil)) + testMustAttemptImagePull(t, false, imageManager, ctx, "docker.io/testing/policyexempt", "policyallowed", nil) expectedMetrics[string(checkResultCredentialPolicyAllowed)]++ cmpMustAttemptPullMetrics(t, expectedMetrics) - require.False(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/policyexempt", "policyallowed", nil, nil)) + testMustAttemptImagePull(t, false, imageManager, ctx, "docker.io/testing/policyexempt", "policyallowed", nil) expectedMetrics[string(checkResultCredentialPolicyAllowed)]++ cmpMustAttemptPullMetrics(t, expectedMetrics) - require.True(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/norecords", "somewhatunknown", nil, nil)) + testMustAttemptImagePull(t, true, imageManager, ctx, "docker.io/testing/norecords", "somewhatunknown", nil) expectedMetrics[string(checkResultMustAuthenticate)]++ cmpMustAttemptPullMetrics(t, expectedMetrics) - require.False(t, imageManager.MustAttemptImagePull(ctx, "docker.io/testing/test", "testimageref", []kubeletconfig.ImagePullSecret{ - {UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "testsecrethash"}, - }, nil)) + testMustAttemptImagePull(t, false, imageManager, ctx, "docker.io/testing/test", "testimageref", + func() ([]kubeletconfig.ImagePullSecret, *kubeletconfig.ImagePullServiceAccount, error) { + return []kubeletconfig.ImagePullSecret{ + {UID: "testsecretuid", Namespace: "default", Name: "pull-secret", CredentialHash: "testsecrethash"}, + }, nil, nil + }) expectedMetrics[string(checkResultCredentialRecordFound)]++ cmpMustAttemptPullMetrics(t, expectedMetrics) } +func testMustAttemptImagePull(t *testing.T, expected bool, f *PullManager, ctx context.Context, image, imageRef string, getPodCredentials GetPodCredentials) { + mustPull, err := f.MustAttemptImagePull(ctx, image, imageRef, getPodCredentials) + require.NoError(t, err) + require.Equal(t, expected, mustPull) +} + func cmpFSIntents(t *testing.T, expected uint) { t.Helper() const metricFormat = ` diff --git a/pkg/kubelet/images/pullmanager/noop_pull_manager.go b/pkg/kubelet/images/pullmanager/noop_pull_manager.go index f10d6f39323..be9505824aa 100644 --- a/pkg/kubelet/images/pullmanager/noop_pull_manager.go +++ b/pkg/kubelet/images/pullmanager/noop_pull_manager.go @@ -31,7 +31,7 @@ func (m *NoopImagePullManager) RecordPullIntent(string) error { return nil } func (m *NoopImagePullManager) RecordImagePulled(context.Context, string, string, *kubeletconfiginternal.ImagePullCredentials) { } func (m *NoopImagePullManager) RecordImagePullFailed(context.Context, string) {} -func (m *NoopImagePullManager) MustAttemptImagePull(context.Context, string, string, []kubeletconfiginternal.ImagePullSecret, *kubeletconfiginternal.ImagePullServiceAccount) bool { - return false +func (m *NoopImagePullManager) MustAttemptImagePull(context.Context, string, string, GetPodCredentials) (bool, error) { + return false, nil } func (m *NoopImagePullManager) PruneUnknownRecords(context.Context, []string, time.Time) {}