Skip to content

kubelet: don't fetch image credentials if the image is present and if we don't need to check if the pod is allowed to pull it #133079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 21, 2025
Merged

kubelet: don't fetch image credentials if the image is present and if we don't need to check if the pod is allowed to pull it #133079

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions pkg/kubelet/images/image_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,12 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR
return "", message, err
}

if imageRef != "" && !utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks fine and likely restores the old behavior. The issue will still exist when the KubeletEnsureSecretPulledImages feature gate is enabled by default.

@stlaz we should think about how to avoid calling credential provider plugins when the image is preloaded or the pull policy is Never.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @liggitt

Copy link
Contributor Author

@atykhyy atykhyy Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue will still exist when the KubeletEnsureSecretPulledImages feature gate is enabled by default.

Yes. I made a note to myself to configure it to not verify when it goes GA.

avoid calling credential provider plugins when the image is preloaded

As I see it, this must rely on some kind of declared dependency between pods such that pod A being allowed to access an image means that pod B (somehow linked to A, perhaps with an annotation) is also allowed without additional checks. The upgrade workflow I stumbled upon this already requires a preflight daemonset, which runs while the previous version of the main daemonset is still running, to pull the new version image, because when the main daemonset pod stops, the kubelet won't be able to pull the new image any more than it will be able to acquire credentials. There is no problem with kubelet checking pod image accessibility for preflight daemonset pods.

It is of course possible to add the critical image to preloadedImagesVerificationAllowlist, but that allows access to it to every pod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrowly moving this if block up is ~fine for now, but I expect the credential verification feature to go to get and default-enable ~soon, at which point credential providers will be called again.

as credentials provider plugins may not be able to acquire credentials while the network management daemonset pod is being recreated

that seems like a potential deadlock issue with those credential plugins that needs resolving independent of the credential reverification feature

Copy link
Contributor Author

@atykhyy atykhyy Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential plugins that needs resolving independent of the credential reverification feature

I.e. to authenticate to CR and pull the image? That's handled by preflight daemonsets, as I mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at which point credential providers will be called again

Yes, but that's not correct behavior, isn't it? E.g. if the image pull credential verification policy is NeverVerify, there is no reason to call them. I added an early check for decisions of this type: #133114

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
Expand Down Expand Up @@ -207,12 +213,6 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR
pullCredentials, _ := keyring.Lookup(repoToPull)

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
}

var imagePullSecrets []kubeletconfiginternal.ImagePullSecret
for _, s := range pullCredentials {
if s.Source == nil {
Expand Down