Multiple pullSecrets for same registry does not work

[Wouter0100]

When having multiple pullSecrets for the same registry in the same Service Account, Knative fails to detect the correct set of credentials and fails at the first. The kubelet has a loop build in that allows this setup.

What version of Knative?

1.1.0

Expected Behavior

To attempt each imagePullSecret in the associated Service Account when resolving the tag of an image and continue deploying the revision.

Actual Behavior

It fails at the first pull secret, from which the credentials do not match and does not attempt the next pull secret.

Steps to Reproduce the Problem

1. Create 2 kubernetes.io/dockerconfigjson-secrets for the same registry with different username/password.
2. Create a Service Account with both secrets.
3. Create a kservice with the above created Service Account, and include 2 containers in the pod whereby:
   • The first container should only be allowed to be pulled from the first set of credentials (used in the first secret).
   • The second container should only be allowed to be pulled from the registry using the second set of credentials (used in the second secret).

Workaround

We disabled tag resolving by setting registries-skipping-tag-resolving in config-deployment to our registry.

[dprotaso]

/triage accepted

[jwcesign]

Can I take this? @dprotaso

[dprotaso]

Sure - the change will need to be done upstream in ggcr

https://github.com/google/go-containerregistry/blob/7196cf3dc436a7633687b0c83ba76318f70e26f2/pkg/authn/kubernetes/keychain.go#L273

/assign @jwcesign

[dprotaso]

I created an upstream issue and asked for input from the maintainers - https://github.com/google/go-containerregistry/issues/1431

[dprotaso]

@Wouter0100 another workaround is to specify the registry secret with the repository

ie. registry.com/foo and registry.com/bar can you try that?

[jwcesign]

I think only change the logic in knative serving is better. Because if change go-containerregistry, the API return results will be changed, it has influence with who use go-containerregistry:

file: serving/pkg/reconciler/revision/resolve.go
// Resolve resolves the image references that use tags to digests.
func (r *digestResolver) Resolve(
    ctx context.Context,
    image string,
    opt k8schain.Options,
    registriesToSkip sets.String) (string, error) {
    if _, err := name.NewDigest(image, name.WeakValidation); err == nil {
        // Already a digest
        return image, nil
    }

    tag, err := name.NewTag(image, name.WeakValidation)
    if err != nil {
        return "", fmt.Errorf("failed to parse image name %q into a tag: %w", image, err)
    }

    if registriesToSkip.Has(tag.Registry.RegistryStr()) {
        return "", nil
    }

    if len(opt.ImagePullSecrets) == 0 {
        resolveDigest, err := r.ResolveWithSingleImagePullSecret(ctx, tag, opt)
        if err != nil {
            return "", err
        }
        return resolveDigest, nil
    }

    var resolveErrors []error
    for _, v := range opt.ImagePullSecrets {
        singleImagePullSecretOpt := opt
        singleImagePullSecretOpt.ImagePullSecrets = []string{v}
        resolveDigest, err := r.ResolveWithSingleImagePullSecret(ctx, tag, singleImagePullSecretOpt)
        if err == nil {
            return resolveDigest, nil
        }
        resolveErrors = append(resolveErrors, err)
    }

    return "", utilerrors.NewAggregate(resolveErrors)
}

func (r *digestResolver) ResolveWithSingleImagePullSecret(
    ctx context.Context,
    tag name.Tag,
    opt k8schain.Options) (string, error) {
    kc, err := k8schain.New(ctx, r.client, opt)
    if err != nil {
        return "", err
    }

    desc, err := remote.Head(tag, remote.WithContext(ctx), remote.WithTransport(r.transport), remote.WithAuthFromKeychain(kc), remote.WithUserAgent(r.userAgent))
    if err != nil {
        return "", err
    }

    return fmt.Sprintf("%s@%s", tag.Repository.String(), desc.Digest), nil
}

cc @dprotaso , what u think

[dprotaso]

My only concern with that solution is that it'll enumerate over extra keychains excessively - and we've seen some keychains in the past be really slow (ie. 2s).

ie. https://github.com/google/go-containerregistry/blob/7196cf3dc436a7633687b0c83ba76318f70e26f2/pkg/authn/k8schain/k8schain.go#L98-L104

return authn.NewMultiKeychain(
    k8s,
    authn.DefaultKeychain,
    google.Keychain,
    amazonKeychain,
    azureKeychain,
), nil

I think the workaround I mentioned above is fine while we figure out a way to solve this upstream

[jwcesign]

go-containerregistry maintainer tries to work on abother approach, when finished, I will sync and add ut for this issue