Bump go-containerregistry, remove k8s-pkg-credentialprovider #409
Conversation
benmoss
force-pushed
the
remove-vdemeester
branch
from
bba666e
to
8e706ef
Compare
| @@ -1,760 +0,0 @@ | |||
| // Copyright 2021 VMware, Inc. | |||
There was a problem hiding this comment.
This test is basically just testing the libraries and the ordering of the keychain. It'd be nice to have some automated tests around the libraries behavior but this is the wrong level to test at. Testing the ordering of the keychain doesn't seem valuable.
benmoss
force-pushed
the
remove-vdemeester
branch
from
8e706ef
to
7b7d1d9
Compare
Move to cloud credential helpers recommended by GGCR: https://github.com/google/go-containerregistry/tree/main/pkg/authn#emulating-cloud-provider-credential-helpers
benmoss
force-pushed
the
remove-vdemeester
branch
from
7b7d1d9
to
1eab10b
Compare
| @@ -40,29 +39,6 @@ func (r *RegistryFlags) Set(cmd *cobra.Command) { | |||
|
|
|||
| cmd.Flags().DurationVar(&r.ResponseHeaderTimeout, "registry-response-header-timeout", 30*time.Second, "Maximum time to allow a request to wait for a server's response headers from the registry (ms|s|m|h)") | |||
| cmd.Flags().IntVar(&r.RetryCount, "registry-retry-count", 5, "Set the number of times imgpkg retries to send requests to the registry in case of an error") | |||
|
|
|||
| cmd.Flags().String("registry-azure-cr-config", "", "Path to the file containing Azure container registry configuration information. ($IMGPKG_REGISTRY_AZURE_CR_CONFIG)") | |||
There was a problem hiding this comment.
In the docs we should make sure that we remove the existence of this flag as well
There was a problem hiding this comment.
Need to figure out the implications for the other auth providers as well
There was a problem hiding this comment.
is that somehow magically derived in ggcr or we just no longer support azure configuration?
There was a problem hiding this comment.
The ACR library we're switching to (https://github.com/chrismellard/docker-credential-acr-env/) supports pulling authentication from the environment via AZURE_CLIENT_ID/AZURE_CLIENT_SECRET/AZURE_TENANT_ID. It's what's being used by ko and cosign so it seems like an acceptable choice
| @@ -70,6 +70,9 @@ func (r *RegistryFlags) AsRegistryOpts() registry.Opts { | |||
| if os.Getenv("IMGPKG_ANON") == "true" { | |||
| opts.Anon = true | |||
| } | |||
| if os.Getenv("IMGPKG_ENABLE_IAAS_AUTH") != "false" { | |||
| opts.EnableIaasAuthProviders = true | |||
There was a problem hiding this comment.
Do we really still need this feature?
There was a problem hiding this comment.
@cppforlife thinks so, apparently users run into problems and want to be able to fallback to ignoring ambient credentials sometimes? I think especially since they are higher priority than the Docker config now, another piece of feedback
benmoss
force-pushed
the
remove-vdemeester
branch
from
64931fb
to
2edbbea
Compare
benmoss
force-pushed
the
remove-vdemeester
branch
from
2edbbea
to
9294f48
Compare
|
Maybe we should consider only using the Docker/env keychains by default, and enabling workload identity/ambient credentials via a flag |
|
The main change that we are currently missing is:
To keep backward compatibility |
Move to cloud credential helpers recommended by GGCR:
https://github.com/google/go-containerregistry/tree/main/pkg/authn#emulating-cloud-provider-credential-helpers
We had to fix our test registry to reject mount requests properly with a 202 due to the changes in google/go-containerregistry#1388
Manually validated against GCP and Azure ambient credentials, both of them work and fail gracefully when access to the metadata server is blocked.