This looks good to me. BTW, after the feature goes to GA, this if block can be removed.

after the feature goes to GA, this if block can be removed

If this if block is removed, the problem described in #133079 will reappear.

after the feature goes to GA, this if block can be removed

If this if block is removed, the problem described in #133079 will reappear.

Then just fine for now. But it seems that this still needs an update once this is Beta or GA.

This part should not be externalized from MustAttemptImagePull() to keep a single point for authorization decisions based on the policy and cache credentials.

Instead, I would expect the keyring.Lookup(repoToPull) to be called JIT inside of MustAttemptImagePull().

This way, you would end up having something like this:

type CachedImageCredentials interface {
    Get() ([]credentialprovider.TrackedAuthConfig, error)
}

type getCachedImageCredentials func() ([]credentialprovider.TrackedAuthConfig, error)
func (f getCachedCredentials) Get() ([]credentialprovider.TrackedAuthConfig, error) {
    return f()
}

func getImageCredentialsForPodFunc(
    fullyQualifiedImage string,
    pullSecrets []v1.Secret,
    podSandboxConfig *runtimeapi.PodSandboxConfig,
) getCachedImageCredentials {
    var (
        called bool
        credsCached []credentialprovider.TrackedAuthConfig
        errCached error
    )
    return func() (creds []credentialprovider.TrackedAuthConfig, err error)  {
        if called {
            return credsCached, errCached
        }
        defer func() {
            credsCached = creds
            errCached = err
            called = true
        }()
        // setup the keyrings and do a Lookup(fullyQualifiedImage)
    })
}

You can then update the MustAttemptImagePull() to take CachedCredentials as an argument.

From image_manager.go, you then just do:

cachedCredentials := getImageCredentialsForPodFunc(<arguments>)
if len(imageRef) > 0 {
    ...
    m.imagePullManager.MustAttemptImagePull(<arguments>, cachedCredentials)
}
...
credentials, err := cachedCredentials.Get()
if err != nil {
    return "", err.Error(), err
}
return m.pullImage(..., credentials, ...)

This part should not be externalized from MustAttemptImagePull() to keep a single point for authorization decisions based on the policy and cache credentials.

OK, I'll rewrite it that way.

I didn't see a reason to make pullmanager aware of []credentialprovider.TrackedAuthConfig so I sliced it in a slightly different way.

pod is unused

It's used for pod.Spec.ServiceAccountName.

oops, sorry, must have overlooked it

Below cachedCreds.NodePodsAccessible block is where I'd expect the JIT call to CachedCredentials.Get()

// and evaluates the part of the policy which does not depend on credentials.

A function called "Lookup" shouldn't do any kinds of policy evaluations. That's another reason why I'd rather have a JIT credential call fetch built inside of MustAttemptImagePull

We'll probably have to add an error to the return of MustAttemptImagePull() for the cases where initializing the keyrings fails as we would ideally want the caller to short-circuit.

Using sync.OnceValues here like this would likely result in a continuous memory growth, which is why I avoided it in my original suggestion. I was thinking about it too in the very beginning though.

... or maybe not? Would sync.OnceValues ever be garbage collected? Tbh I am not sure at this point.

Either way, I think we can extract lookupPullCredentials from here to its own function.

Using sync.OnceValues here like this would likely result in a continuous memory growth,

Why? Wouldn't that be a bug in Go runtime if sync.Once leaked memory? The getPodCredentials closure isn't stored in any struct. Looking at the source code, I see nothing that would prevent the Once object closed over by OnceValues from getting garbage collected once EnsureImageExists() returns. I tried a simple test that mimics this use of OnceValues and everything seems to get cleaned up.

extract lookupPullCredentials

I.e. so that that place looks like this?

lookupPullCredentials := sync.OnceValues(func() ([]credentialprovider.TrackedAuthConfig, error) {
	return m.lookupPullCredentials(spec.Image, pod, podSandboxConfig, pullSecrets, externalCredentialProviderKeyring)
})

Why? Wouldn't that be a bug in Go runtime if sync.Once leaked memory?

I guess so. sync.Once is generally used a bit differently, even throughout the codebase here, which is what throws me off. I looked at the source, too, aside from the atomic and _nocopy, nothing appears to be that much out of ordinary. I suppose it's safe?

As for the lookup credentials, something more like

func lookupPullCredentialsFunc(<args>) (func() ([]credentialprovider.TrackedAuthConfig, error))  {
    return sync.OnceValues(func() ([]credentialprovider.TrackedAuthConfig, error) {
        // lookup code here
    })
}

I suppose it's safe?

I believe so. sync.OnceFunc() and friends are old and established, if they leaked memory in regular use it would have been caught a decade ago, and their return values being closures users can do nothing bad with them (like copying a sync.Once or a mutex by value).

As for the lookup credentials, something more like

I see what you mean, but what does that achieve? Do you plan to use this new function elsewhere?

It's just for clarity, makes the code less cluttered.

move right above m.lastSecrets, return error if it occured

nit: wantPodCredsRequested would be a more fitting name

Yes, but then there would be a lot of spurious diffs due to go fmt whitespace changes. I prefer not to pollute git blame if I can help it. But if it's okay then sure I'll change it

I see. It's true it was easier to review this way, but feel free to change the name now.

The PR went one more size up :|

Just an idea, wouldn't it be better to disregard whitespace changes when computing PR size?

Just an idea, wouldn't it be better to disregard whitespace changes when computing PR size?

I can see how that would be useful in most if not all cases, yes

why do we need to wrap it in OnceValues? It is only called once anyways as far as I can tell

the returned lookup function might be called twice in some cases:

1. JIT during the MustAttemptImagePull()
2. just before calling m.pullImage()

my question was mostly because I do not know exact behavior of sync.OnceValues. Will it retry if the first attempt resulted in err? Or we want to cache the error. So I thought it is easier just not have it =).

I will need to review the sequence you suggested. I plan to take it again later during 1.35, please ping if you think it is critical to do earleir

Will it retry if the first attempt resulted in err?

No. It caches both values.

Or we want to cache the error

An error in that place shows that something is seriously misconfigured, so it is unlikely that retrying makes sense. A mere failure to lookup credentials from the keyring does not return an error.

A mere failure to lookup credentials from the keyring does not return an error.

Not because of this PR, I mean. The relevant method's signature does not include error.

// pkg/credentialprovider/keyring.go
type DockerKeyring interface {
	Lookup(image string) ([]TrackedAuthConfig, bool)
}

I plan to take it again later during 1.35, please ping if you think it is critical to do earleir

I think this change should be in place when KubeletEnsureSecretPulledImages feature gate goes GA and default to enabled, because at that point it will become enabled by some managed (cloud) Kubernetes providers with no option to disable.

An error in that place shows that something is seriously misconfigured, so it is unlikely that retrying makes sense. A mere failure to lookup credentials from the keyring does not return an error.

Besides, I would like to make sure the result of the credential lookup matches what the authz check received in the context of a single EnsureImageExists() call.

I will need to review the sequence you suggested. I plan to take it again later during 1.35, please ping if you think it is critical to do earlier

This likely prevents us from moving the feature to Beta. It's not critical right now, but let's please make sure we pay attention to this PR early in 1.35.

keeping this code block where it was before will simlpify the review. Took me some time to understand what happened here

This is what the code originally looked like before #133079. I'd rather it stayed here.