Skip to content

Commit f920aea

Browse files
atykhyyatykhyy
committed
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.
1 parent 7383971 commit f920aea

File tree

7 files changed

+305
-220
lines changed

7 files changed

+305
-220
lines changed

‎pkg/kubelet/images/image_manager.go

Lines changed: 57 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -178,41 +178,15 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR
178178
return "", message, err
179179
}
180180

181-
if imageRef != "" && !utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) {
182-
msg := fmt.Sprintf("Container image %q already present on machine", requestedImage)
183-
m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info)
184-
return imageRef, msg, nil
185-
}
186-
187-
repoToPull, _, _, err := parsers.ParseImageName(spec.Image)
188-
if err != nil {
189-
return "", err.Error(), err
190-
}
181+
// wrap the lookup in a function to ensure that we only look up credentials once and no earlier than needed
182+
lookupPullCredentials := m.makeLookupPullCredentialsFunc(spec.Image, pod, pullSecrets, podSandboxConfig)
191183

192-
// construct the dynamic keyring using the providers we have in the kubelet
193-
var podName, podNamespace, podUID string
194-
if utilfeature.DefaultFeatureGate.Enabled(features.KubeletServiceAccountTokenForCredentialProviders) {
195-
sandboxMetadata := podSandboxConfig.GetMetadata()
196-
197-
podName = sandboxMetadata.Name
198-
podNamespace = sandboxMetadata.Namespace
199-
podUID = sandboxMetadata.Uid
200-
}
201-
202-
externalCredentialProviderKeyring := credentialproviderplugin.NewExternalCredentialProviderDockerKeyring(
203-
podNamespace,
204-
podName,
205-
podUID,
206-
pod.Spec.ServiceAccountName)
207-
208-
keyring, err := credentialprovidersecrets.MakeDockerKeyring(pullSecrets, credentialprovider.UnionDockerKeyring{m.nodeKeyring, externalCredentialProviderKeyring})
209-
if err != nil {
210-
return "", err.Error(), err
211-
}
212-
213-
pullCredentials, _ := keyring.Lookup(repoToPull)
184+
getPodCredentials := func() ([]kubeletconfiginternal.ImagePullSecret, *kubeletconfiginternal.ImagePullServiceAccount, error) {
185+
pullCredentials, err := lookupPullCredentials()
186+
if err != nil {
187+
return nil, nil, err
188+
}
214189

215-
if imageRef != "" {
216190
var imagePullSecrets []kubeletconfiginternal.ImagePullSecret
217191
// we don't take the audience of the service account into account, so there can only
218192
// 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
239213
}
240214
}
241215

242-
pullRequired := m.imagePullManager.MustAttemptImagePull(requestedImage, imageRef, imagePullSecrets, imagePullServiceAccount)
243-
if !pullRequired {
216+
return imagePullSecrets, imagePullServiceAccount, nil
217+
}
218+
219+
if imageRef != "" {
220+
if !utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) {
221+
msg := fmt.Sprintf("Container image %q already present on machine", requestedImage)
222+
m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info)
223+
return imageRef, msg, nil
224+
}
225+
if pullRequired, err := m.imagePullManager.MustAttemptImagePull(requestedImage, imageRef, getPodCredentials); err != nil {
226+
return "", err.Error(), err
227+
} else if !pullRequired {
244228
msg := fmt.Sprintf("Container image %q already present on machine and can be accessed by the pod", requestedImage)
245229
m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info)
246230
return imageRef, msg, nil
@@ -254,9 +238,47 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR
254238
return "", msg, err
255239
}
256240

241+
pullCredentials, err := lookupPullCredentials()
242+
if err != nil {
243+
return "", err.Error(), err
244+
}
245+
257246
return m.pullImage(ctx, logPrefix, objRef, pod.UID, requestedImage, spec, pullCredentials, podSandboxConfig)
258247
}
259248

249+
func (m *imageManager) makeLookupPullCredentialsFunc(image string, pod *v1.Pod, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) func() ([]credentialprovider.TrackedAuthConfig, error) {
250+
return sync.OnceValues(func() ([]credentialprovider.TrackedAuthConfig, error) {
251+
repoToPull, _, _, err := parsers.ParseImageName(image)
252+
if err != nil {
253+
return nil, err
254+
}
255+
256+
// construct the dynamic keyring using the providers we have in the kubelet
257+
var podName, podNamespace, podUID string
258+
if utilfeature.DefaultFeatureGate.Enabled(features.KubeletServiceAccountTokenForCredentialProviders) {
259+
sandboxMetadata := podSandboxConfig.GetMetadata()
260+
261+
podName = sandboxMetadata.Name
262+
podNamespace = sandboxMetadata.Namespace
263+
podUID = sandboxMetadata.Uid
264+
}
265+
266+
externalCredentialProviderKeyring := credentialproviderplugin.NewExternalCredentialProviderDockerKeyring(
267+
podNamespace,
268+
podName,
269+
podUID,
270+
pod.Spec.ServiceAccountName)
271+
272+
keyring, err := credentialprovidersecrets.MakeDockerKeyring(pullSecrets, credentialprovider.UnionDockerKeyring{m.nodeKeyring, externalCredentialProviderKeyring})
273+
if err != nil {
274+
return nil, err
275+
}
276+
277+
pullCredentials, _ := keyring.Lookup(repoToPull)
278+
return pullCredentials, nil
279+
})
280+
}
281+
260282
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) {
261283
var pullSucceeded bool
262284
var finalPullCredentials *credentialprovider.TrackedAuthConfig

‎pkg/kubelet/images/image_manager_test.go

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -694,17 +694,22 @@ type mockImagePullManager struct {
694694
config *mockImagePullManagerConfig
695695
}
696696

697-
func (m *mockImagePullManager) MustAttemptImagePull(image, _ string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool {
697+
func (m *mockImagePullManager) MustAttemptImagePull(image, _ string, getPodCredentials pullmanager.GetPodCredentials) (bool, error) {
698698
if m.config == nil || m.config.allowAll {
699-
return false
699+
return false, nil
700+
}
701+
702+
podSecrets, podServiceAccount, err := getPodCredentials()
703+
if err != nil {
704+
return true, err
700705
}
701706

702707
// Check secrets
703708
if allowedSecrets, ok := m.config.allowedSecrets[image]; ok {
704709
for _, s := range podSecrets {
705710
for _, allowed := range allowedSecrets {
706711
if s.Namespace == allowed.Namespace && s.Name == allowed.Name && s.UID == allowed.UID {
707-
return false
712+
return false, nil
708713
}
709714
}
710715
}
@@ -714,12 +719,12 @@ func (m *mockImagePullManager) MustAttemptImagePull(image, _ string, podSecrets
714719
if podServiceAccount != nil {
715720
if allowedServiceAccounts, ok := m.config.allowedServiceAccounts[image]; ok {
716721
if slices.Contains(allowedServiceAccounts, *podServiceAccount) {
717-
return false
722+
return false, nil
718723
}
719724
}
720725
}
721726

722-
return true
727+
return true, nil
723728
}
724729

725730
// mockImagePullManagerWithTracking tracks calls to MustAttemptImagePull for service account testing
@@ -736,19 +741,25 @@ type mockImagePullManagerWithTracking struct {
736741
recordedCredentials *kubeletconfiginternal.ImagePullCredentials
737742
}
738743

739-
func (m *mockImagePullManagerWithTracking) MustAttemptImagePull(image, imageRef string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool {
744+
func (m *mockImagePullManagerWithTracking) MustAttemptImagePull(image, imageRef string, getPodCredentials pullmanager.GetPodCredentials) (bool, error) {
740745
m.mustAttemptCalled = true
741746
m.lastImage = image
742747
m.lastImageRef = imageRef
748+
749+
podSecrets, podServiceAccount, err := getPodCredentials()
750+
if err != nil {
751+
return true, err
752+
}
753+
743754
m.lastSecrets = podSecrets
744755
if podServiceAccount != nil {
745756
m.lastServiceAccounts = []kubeletconfiginternal.ImagePullServiceAccount{*podServiceAccount}
746757
}
747758

748759
if m.allowAll {
749-
return false
760+
return false, nil
750761
}
751-
return m.mustAttemptReturn
762+
return m.mustAttemptReturn, nil
752763
}
753764

754765
func (m *mockImagePullManagerWithTracking) RecordImagePulled(image, imageRef string, credentials *kubeletconfiginternal.ImagePullCredentials) {

‎pkg/kubelet/images/pullmanager/benchmarks_test.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,11 @@ func directRecordReadFunc(expectHit bool) benchmarkedCheckFunc {
7272

7373
func mustAttemptPullReadFunc(expectHit bool) benchmarkedCheckFunc {
7474
return func(b *testing.B, pullManager PullManager, imgRef string) {
75-
mustPull := pullManager.MustAttemptImagePull("test.repo/org/"+imgRef, imgRef, nil, nil)
76-
if mustPull != !expectHit {
77-
b.Fatalf("MustAttemptImagePull() expected %t, got %t", !expectHit, mustPull)
75+
mustPull, err := pullManager.MustAttemptImagePull("test.repo/org/"+imgRef, imgRef, func() ([]kubeletconfig.ImagePullSecret, *kubeletconfig.ImagePullServiceAccount, error) {
76+
return nil, nil, nil
77+
})
78+
if mustPull != !expectHit || err != nil {
79+
b.Fatalf("no error expected (got %v); MustAttemptImagePull() expected %t, got %t", err, !expectHit, mustPull)
7880
}
7981
}
8082
}

‎pkg/kubelet/images/pullmanager/image_pull_manager.go

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -179,9 +179,9 @@ func (f *PullManager) decrementImagePullIntent(image string) {
179179
f.decrementIntentCounterForImage(image)
180180
}
181181

182-
func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets []kubeletconfiginternal.ImagePullSecret, podServiceAccount *kubeletconfiginternal.ImagePullServiceAccount) bool {
182+
func (f *PullManager) MustAttemptImagePull(image, imageRef string, getPodCredentials GetPodCredentials) (bool, error) {
183183
if len(imageRef) == 0 {
184-
return true
184+
return true, nil
185185
}
186186

187187
var imagePulledByKubelet bool
@@ -225,36 +225,41 @@ func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets []
225225

226226
if err != nil {
227227
klog.ErrorS(err, "Unable to access cache records about image pulls")
228-
return true
228+
return true, nil
229229
}
230230

231231
if !f.imagePolicyEnforcer.RequireCredentialVerificationForImage(image, imagePulledByKubelet) {
232-
return false
232+
return false, nil
233233
}
234234

235235
if pulledRecord == nil {
236236
// we have no proper records of the image being pulled in the past, we can short-circuit here
237-
return true
237+
return true, nil
238238
}
239239

240240
sanitizedImage, err := trimImageTagDigest(image)
241241
if err != nil {
242242
klog.ErrorS(err, "failed to parse image name, forcing image credentials reverification", "image", sanitizedImage)
243-
return true
243+
return true, nil
244244
}
245245

246246
cachedCreds, ok := pulledRecord.CredentialMapping[sanitizedImage]
247247
if !ok {
248-
return true
248+
return true, nil
249249
}
250250

251251
if cachedCreds.NodePodsAccessible {
252252
// anyone on this node can access the image
253-
return false
253+
return false, nil
254254
}
255255

256256
if len(cachedCreds.KubernetesSecrets) == 0 && len(cachedCreds.KubernetesServiceAccounts) == 0 {
257-
return true
257+
return true, nil
258+
}
259+
260+
podSecrets, podServiceAccount, err := getPodCredentials()
261+
if err != nil {
262+
return true, err
258263
}
259264

260265
for _, podSecret := range podSecrets {
@@ -275,7 +280,7 @@ func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets []
275280
klog.ErrorS(err, "failed to write an image pulled record", "image", image, "imageRef", imageRef)
276281
}
277282
}
278-
return false
283+
return false, nil
279284
}
280285

281286
if secretCoordinatesMatch {
@@ -286,18 +291,18 @@ func (f *PullManager) MustAttemptImagePull(image, imageRef string, podSecrets []
286291
if err := f.writePulledRecordIfChanged(image, imageRef, &kubeletconfiginternal.ImagePullCredentials{KubernetesSecrets: []kubeletconfiginternal.ImagePullSecret{podSecret}}); err != nil {
287292
klog.ErrorS(err, "failed to write an image pulled record", "image", image, "imageRef", imageRef)
288293
}
289-
return false
294+
return false, nil
290295
}
291296
}
292297
}
293298
}
294299

295300
if podServiceAccount != nil && slices.Contains(cachedCreds.KubernetesServiceAccounts, *podServiceAccount) {
296301
// we found a matching service account, no need to pull the image
297-
return false
302+
return false, nil
298303
}
299304

300-
return true
305+
return true, nil
301306
}
302307

303308
func (f *PullManager) PruneUnknownRecords(imageList []string, until time.Time) {

0 commit comments

Comments
 (0)