From 81c4fde489f7d32431c4fd693e49dae56286a284 Mon Sep 17 00:00:00 2001 From: tiffany jernigan Date: Fri, 22 Mar 2019 07:42:35 +0000 Subject: [PATCH 1/5] Remove aws cred provider dep on cloud provider --- pkg/cloudprovider/providers/aws/regions.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/regions.go b/pkg/cloudprovider/providers/aws/regions.go index f19bab6eb55..f834ee1aa79 100644 --- a/pkg/cloudprovider/providers/aws/regions.go +++ b/pkg/cloudprovider/providers/aws/regions.go @@ -22,7 +22,6 @@ import ( "k8s.io/klog" "k8s.io/apimachinery/pkg/util/sets" - awscredentialprovider "k8s.io/kubernetes/pkg/credentialprovider/aws" ) // wellKnownRegions is the complete list of regions known to the AWS cloudprovider @@ -78,8 +77,6 @@ func recognizeRegion(region string) { klog.V(4).Infof("found AWS region %q", region) - awscredentialprovider.RegisterCredentialsProvider(region) - awsRegions.Insert(region) } From 847cb24aa1960e86e9c7c160ce6b97bab0e9fdea Mon Sep 17 00:00:00 2001 From: tiffany jernigan Date: Fri, 22 Mar 2019 07:49:17 +0000 Subject: [PATCH 2/5] Credential provider Provide takes image (general) --- pkg/credentialprovider/keyring.go | 8 ++--- pkg/credentialprovider/keyring_test.go | 4 +-- pkg/credentialprovider/provider.go | 32 ++++++++++++-------- pkg/credentialprovider/provider_test.go | 26 ++++++++-------- pkg/kubelet/dockershim/helpers.go | 2 +- pkg/kubelet/kuberuntime/kuberuntime_image.go | 2 +- 6 files changed, 41 insertions(+), 33 deletions(-) diff --git a/pkg/credentialprovider/keyring.go b/pkg/credentialprovider/keyring.go index 6f5fad5fc47..9f2d3b87606 100644 --- a/pkg/credentialprovider/keyring.go +++ b/pkg/credentialprovider/keyring.go @@ -261,11 +261,9 @@ func (dk *BasicDockerKeyring) Lookup(image string) ([]LazyAuthConfiguration, boo for _, k := range dk.index { // both k and image are schemeless URLs because even though schemes are allowed // in the credential configurations, we remove them in Add. - if matched, _ := urlsMatchStr(k, image); !matched { - continue + if matched, _ := urlsMatchStr(k, image); matched { + ret = append(ret, dk.creds[k]...) } - - ret = append(ret, dk.creds[k]...) } if len(ret) > 0 { @@ -288,7 +286,7 @@ func (dk *lazyDockerKeyring) Lookup(image string) ([]LazyAuthConfiguration, bool keyring := &BasicDockerKeyring{} for _, p := range dk.Providers { - keyring.Add(p.Provide()) + keyring.Add(p.Provide(image)) } return keyring.Lookup(image) diff --git a/pkg/credentialprovider/keyring_test.go b/pkg/credentialprovider/keyring_test.go index 2b36bde889a..128670d19c4 100644 --- a/pkg/credentialprovider/keyring_test.go +++ b/pkg/credentialprovider/keyring_test.go @@ -464,12 +464,12 @@ func (d *testProvider) Enabled() bool { } // LazyProvide implements dockerConfigProvider. Should never be called. -func (d *testProvider) LazyProvide() *DockerConfigEntry { +func (d *testProvider) LazyProvide(image string) *DockerConfigEntry { return nil } // Provide implements dockerConfigProvider -func (d *testProvider) Provide() DockerConfig { +func (d *testProvider) Provide(image string) DockerConfig { d.Count += 1 return DockerConfig{} } diff --git a/pkg/credentialprovider/provider.go b/pkg/credentialprovider/provider.go index 16b4e601a10..245810722e5 100644 --- a/pkg/credentialprovider/provider.go +++ b/pkg/credentialprovider/provider.go @@ -33,15 +33,23 @@ type DockerConfigProvider interface { Enabled() bool // Provide returns docker configuration. // Implementations can be blocking - e.g. metadata server unavailable. - Provide() DockerConfig - // LazyProvide() gets called after URL matches have been performed, so the - // location used as the key in DockerConfig would be redundant. - LazyProvide() *DockerConfigEntry + // The image is passed in as context in the event that the + // implementation depends on information in the image name to return + // credentials; implementations are safe to ignore the image. + Provide(image string) DockerConfig + // LazyProvide gets called after URL matches have been + // performed, so the location used as the key in DockerConfig would be + // redundant. + // The image is passed in as context in the event that the + // implementation depends on information in the image name to return + // credentials; implementations are safe to ignore the image. + LazyProvide(image string) *DockerConfigEntry } -func LazyProvide(creds LazyAuthConfiguration) AuthConfig { +//LazyProvide returns an Lazy AuthConfig +func LazyProvide(creds LazyAuthConfiguration, image string) AuthConfig { if creds.Provider != nil { - entry := *creds.Provider.LazyProvide() + entry := *creds.Provider.LazyProvide(image) return DockerConfigEntryToLazyAuthConfiguration(entry).AuthConfig } return creds.AuthConfig @@ -77,8 +85,8 @@ func (d *defaultDockerConfigProvider) Enabled() bool { return true } -// Provide implements dockerConfigProvider -func (d *defaultDockerConfigProvider) Provide() DockerConfig { +// LazyProvide implements dockerConfigProvider +func (d *defaultDockerConfigProvider) Provide(image string) DockerConfig { // Read the standard Docker credentials from .dockercfg if cfg, err := ReadDockerConfigFile(); err == nil { return cfg @@ -89,7 +97,7 @@ func (d *defaultDockerConfigProvider) Provide() DockerConfig { } // LazyProvide implements dockerConfigProvider. Should never be called. -func (d *defaultDockerConfigProvider) LazyProvide() *DockerConfigEntry { +func (d *defaultDockerConfigProvider) LazyProvide(image string) *DockerConfigEntry { return nil } @@ -99,12 +107,12 @@ func (d *CachingDockerConfigProvider) Enabled() bool { } // LazyProvide implements dockerConfigProvider. Should never be called. -func (d *CachingDockerConfigProvider) LazyProvide() *DockerConfigEntry { +func (d *CachingDockerConfigProvider) LazyProvide(image string) *DockerConfigEntry { return nil } // Provide implements dockerConfigProvider -func (d *CachingDockerConfigProvider) Provide() DockerConfig { +func (d *CachingDockerConfigProvider) Provide(image string) DockerConfig { d.mu.Lock() defer d.mu.Unlock() @@ -114,7 +122,7 @@ func (d *CachingDockerConfigProvider) Provide() DockerConfig { } klog.V(2).Infof("Refreshing cache for provider: %v", reflect.TypeOf(d.Provider).String()) - d.cacheDockerConfig = d.Provider.Provide() + d.cacheDockerConfig = d.Provider.Provide(image) d.expiration = time.Now().Add(d.Lifetime) return d.cacheDockerConfig } diff --git a/pkg/credentialprovider/provider_test.go b/pkg/credentialprovider/provider_test.go index 4d70689532a..44a2f581976 100644 --- a/pkg/credentialprovider/provider_test.go +++ b/pkg/credentialprovider/provider_test.go @@ -31,31 +31,33 @@ func TestCachingProvider(t *testing.T) { Lifetime: 1 * time.Second, } + image := "image" + if provider.Count != 0 { t.Errorf("Unexpected number of Provide calls: %v", provider.Count) } - cache.Provide() - cache.Provide() - cache.Provide() - cache.Provide() + cache.Provide(image) + cache.Provide(image) + cache.Provide(image) + cache.Provide(image) if provider.Count != 1 { t.Errorf("Unexpected number of Provide calls: %v", provider.Count) } time.Sleep(cache.Lifetime) - cache.Provide() - cache.Provide() - cache.Provide() - cache.Provide() + cache.Provide(image) + cache.Provide(image) + cache.Provide(image) + cache.Provide(image) if provider.Count != 2 { t.Errorf("Unexpected number of Provide calls: %v", provider.Count) } time.Sleep(cache.Lifetime) - cache.Provide() - cache.Provide() - cache.Provide() - cache.Provide() + cache.Provide(image) + cache.Provide(image) + cache.Provide(image) + cache.Provide(image) if provider.Count != 3 { t.Errorf("Unexpected number of Provide calls: %v", provider.Count) } diff --git a/pkg/kubelet/dockershim/helpers.go b/pkg/kubelet/dockershim/helpers.go index 21166b82d1f..924715a4861 100644 --- a/pkg/kubelet/dockershim/helpers.go +++ b/pkg/kubelet/dockershim/helpers.go @@ -344,7 +344,7 @@ func ensureSandboxImageExists(client libdocker.Interface, image string) error { var pullErrs []error for _, currentCreds := range creds { - authConfig := dockertypes.AuthConfig(credentialprovider.LazyProvide(currentCreds)) + authConfig := dockertypes.AuthConfig(credentialprovider.LazyProvide(currentCreds, repoToPull)) err := client.PullImage(image, authConfig, dockertypes.ImagePullOptions{}) // If there was no error, return success if err == nil { diff --git a/pkg/kubelet/kuberuntime/kuberuntime_image.go b/pkg/kubelet/kuberuntime/kuberuntime_image.go index 0823088bd8a..66886a8604a 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_image.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_image.go @@ -57,7 +57,7 @@ func (m *kubeGenericRuntimeManager) PullImage(image kubecontainer.ImageSpec, pul var pullErrs []error for _, currentCreds := range creds { - authConfig := credentialprovider.LazyProvide(currentCreds) + authConfig := credentialprovider.LazyProvide(currentCreds, repoToPull) auth := &runtimeapi.AuthConfig{ Username: authConfig.Username, Password: authConfig.Password, From ecbb090f325c171f703f46eb1245308226802e7b Mon Sep 17 00:00:00 2001 From: tiffany jernigan Date: Fri, 22 Mar 2019 07:50:05 +0000 Subject: [PATCH 3/5] Credential provider Provide takes image (clouds) --- pkg/credentialprovider/azure/azure_credentials.go | 4 ++-- .../azure/azure_credentials_test.go | 2 +- pkg/credentialprovider/gcp/metadata.go | 12 ++++++------ pkg/credentialprovider/gcp/metadata_test.go | 6 +++--- .../rancher/rancher_registry_credentials.go | 4 ++-- .../rancher/rancher_registry_credentials_test.go | 2 +- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/credentialprovider/azure/azure_credentials.go b/pkg/credentialprovider/azure/azure_credentials.go index a2759f525a5..199addf78aa 100644 --- a/pkg/credentialprovider/azure/azure_credentials.go +++ b/pkg/credentialprovider/azure/azure_credentials.go @@ -173,7 +173,7 @@ func (a *acrProvider) Enabled() bool { return true } -func (a *acrProvider) Provide() credentialprovider.DockerConfig { +func (a *acrProvider) Provide(image string) credentialprovider.DockerConfig { cfg := credentialprovider.DockerConfig{} ctx, cancel := getContextWithCancel() defer cancel() @@ -246,6 +246,6 @@ func getACRDockerEntryFromARMToken(a *acrProvider, loginServer string) (*credent }, nil } -func (a *acrProvider) LazyProvide() *credentialprovider.DockerConfigEntry { +func (a *acrProvider) LazyProvide(image string) *credentialprovider.DockerConfigEntry { return nil } diff --git a/pkg/credentialprovider/azure/azure_credentials_test.go b/pkg/credentialprovider/azure/azure_credentials_test.go index 6e5434ee285..3c106f796c4 100644 --- a/pkg/credentialprovider/azure/azure_credentials_test.go +++ b/pkg/credentialprovider/azure/azure_credentials_test.go @@ -74,7 +74,7 @@ func Test(t *testing.T) { } provider.loadConfig(bytes.NewBufferString(configStr)) - creds := provider.Provide() + creds := provider.Provide("") if len(creds) != len(result)+1 { t.Errorf("Unexpected list: %v, expected length %d", creds, len(result)+1) diff --git a/pkg/credentialprovider/gcp/metadata.go b/pkg/credentialprovider/gcp/metadata.go index 8fdc940c9fc..e484902ddde 100644 --- a/pkg/credentialprovider/gcp/metadata.go +++ b/pkg/credentialprovider/gcp/metadata.go @@ -130,12 +130,12 @@ func (g *metadataProvider) Enabled() bool { } // LazyProvide implements DockerConfigProvider. Should never be called. -func (g *dockerConfigKeyProvider) LazyProvide() *credentialprovider.DockerConfigEntry { +func (g *dockerConfigKeyProvider) LazyProvide(image string) *credentialprovider.DockerConfigEntry { return nil } // Provide implements DockerConfigProvider -func (g *dockerConfigKeyProvider) Provide() credentialprovider.DockerConfig { +func (g *dockerConfigKeyProvider) Provide(image string) credentialprovider.DockerConfig { // Read the contents of the google-dockercfg metadata key and // parse them as an alternate .dockercfg if cfg, err := credentialprovider.ReadDockerConfigFileFromUrl(dockerConfigKey, g.Client, metadataHeader); err != nil { @@ -148,12 +148,12 @@ func (g *dockerConfigKeyProvider) Provide() credentialprovider.DockerConfig { } // LazyProvide implements DockerConfigProvider. Should never be called. -func (g *dockerConfigUrlKeyProvider) LazyProvide() *credentialprovider.DockerConfigEntry { +func (g *dockerConfigUrlKeyProvider) LazyProvide(image string) *credentialprovider.DockerConfigEntry { return nil } // Provide implements DockerConfigProvider -func (g *dockerConfigUrlKeyProvider) Provide() credentialprovider.DockerConfig { +func (g *dockerConfigUrlKeyProvider) Provide(image string) credentialprovider.DockerConfig { // Read the contents of the google-dockercfg-url key and load a .dockercfg from there if url, err := credentialprovider.ReadUrl(dockerConfigUrlKey, g.Client, metadataHeader); err != nil { klog.Errorf("while reading 'google-dockercfg-url' metadata: %v", err) @@ -258,12 +258,12 @@ type tokenBlob struct { } // LazyProvide implements DockerConfigProvider. Should never be called. -func (g *containerRegistryProvider) LazyProvide() *credentialprovider.DockerConfigEntry { +func (g *containerRegistryProvider) LazyProvide(image string) *credentialprovider.DockerConfigEntry { return nil } // Provide implements DockerConfigProvider -func (g *containerRegistryProvider) Provide() credentialprovider.DockerConfig { +func (g *containerRegistryProvider) Provide(image string) credentialprovider.DockerConfig { cfg := credentialprovider.DockerConfig{} tokenJsonBlob, err := credentialprovider.ReadUrl(metadataToken, g.Client, metadataHeader) diff --git a/pkg/credentialprovider/gcp/metadata_test.go b/pkg/credentialprovider/gcp/metadata_test.go index 6a31c562c50..439d56c6375 100644 --- a/pkg/credentialprovider/gcp/metadata_test.go +++ b/pkg/credentialprovider/gcp/metadata_test.go @@ -91,7 +91,7 @@ func TestDockerKeyringFromGoogleDockerConfigMetadata(t *testing.T) { t.Errorf("Provider is unexpectedly disabled") } - keyring.Add(provider.Provide()) + keyring.Add(provider.Provide("")) creds, ok := keyring.Lookup(registryUrl) if !ok { @@ -169,7 +169,7 @@ func TestDockerKeyringFromGoogleDockerConfigMetadataUrl(t *testing.T) { t.Errorf("Provider is unexpectedly disabled") } - keyring.Add(provider.Provide()) + keyring.Add(provider.Provide("")) creds, ok := keyring.Lookup(registryUrl) if !ok { @@ -253,7 +253,7 @@ func TestContainerRegistryBasics(t *testing.T) { t.Errorf("Provider is unexpectedly disabled") } - keyring.Add(provider.Provide()) + keyring.Add(provider.Provide("")) creds, ok := keyring.Lookup(registryUrl) if !ok { diff --git a/pkg/credentialprovider/rancher/rancher_registry_credentials.go b/pkg/credentialprovider/rancher/rancher_registry_credentials.go index d122308ed82..5725d9263c7 100644 --- a/pkg/credentialprovider/rancher/rancher_registry_credentials.go +++ b/pkg/credentialprovider/rancher/rancher_registry_credentials.go @@ -79,12 +79,12 @@ func (p *rancherProvider) Enabled() bool { } // LazyProvide implements DockerConfigProvider. Should never be called. -func (p *rancherProvider) LazyProvide() *credentialprovider.DockerConfigEntry { +func (p *rancherProvider) LazyProvide(image string) *credentialprovider.DockerConfigEntry { return nil } // Provide implements DockerConfigProvider.Provide, refreshing Rancher tokens on demand -func (p *rancherProvider) Provide() credentialprovider.DockerConfig { +func (p *rancherProvider) Provide(image string) credentialprovider.DockerConfig { cfg := credentialprovider.DockerConfig{} for _, cred := range p.credGetter.getCredentials() { entry := credentialprovider.DockerConfigEntry{ diff --git a/pkg/credentialprovider/rancher/rancher_registry_credentials_test.go b/pkg/credentialprovider/rancher/rancher_registry_credentials_test.go index 41e56b8076a..64037df4bc1 100644 --- a/pkg/credentialprovider/rancher/rancher_registry_credentials_test.go +++ b/pkg/credentialprovider/rancher/rancher_registry_credentials_test.go @@ -82,7 +82,7 @@ func TestRancherCredentialsProvide(t *testing.T) { } keyring := &credentialprovider.BasicDockerKeyring{} - keyring.Add(provider.Provide()) + keyring.Add(provider.Provide("")) for _, registry := range serverAddresses { fullImagePath := path.Join(registry, image) From 11efc013288a15322a6abc0852a4859f7b0785dc Mon Sep 17 00:00:00 2001 From: tiffany jernigan Date: Fri, 22 Mar 2019 07:51:00 +0000 Subject: [PATCH 4/5] Refactors and fixes bugs in AWS credentialprovider Adds caching per registry. Fixes caching of invalid ECR tokens. --- pkg/credentialprovider/aws/aws_credentials.go | 417 +++++++++++------- .../aws/aws_credentials_test.go | 270 +++++++++--- 2 files changed, 468 insertions(+), 219 deletions(-) diff --git a/pkg/credentialprovider/aws/aws_credentials.go b/pkg/credentialprovider/aws/aws_credentials.go index 05ce549d844..5493d17802c 100644 --- a/pkg/credentialprovider/aws/aws_credentials.go +++ b/pkg/credentialprovider/aws/aws_credentials.go @@ -18,24 +18,190 @@ package credentials import ( "encoding/base64" + "errors" "fmt" + "net/url" + "regexp" "strings" + "sync" "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/request" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/ecr" - "k8s.io/klog" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/tools/cache" + "k8s.io/klog" "k8s.io/kubernetes/pkg/credentialprovider" "k8s.io/kubernetes/pkg/version" ) -const awsChinaRegionPrefix = "cn-" -const awsStandardDNSSuffix = "amazonaws.com" -const awsChinaDNSSuffix = "amazonaws.com.cn" -const registryURLTemplate = "*.dkr.ecr.%s.%s" +var ecrPattern = regexp.MustCompile(`^(\d{12})\.dkr\.ecr(\-fips)?\.([a-zA-Z0-9][a-zA-Z0-9-_]*)\.amazonaws\.com(\.cn)?$`) + +// init registers a credential provider for each registryURLTemplate and creates +// an ECR token getter factory with a new cache to store token getters +func init() { + credentialprovider.RegisterCredentialProvider("amazon-ecr", + newECRProvider(&ecrTokenGetterFactory{cache: make(map[string]tokenGetter)})) +} + +// ecrProvider is a DockerConfigProvider that gets and refreshes tokens +// from AWS to access ECR. +type ecrProvider struct { + cache cache.Store + getterFactory tokenGetterFactory +} + +var _ credentialprovider.DockerConfigProvider = &ecrProvider{} + +func newECRProvider(getterFactory tokenGetterFactory) *ecrProvider { + return &ecrProvider{ + cache: cache.NewExpirationStore(stringKeyFunc, &ecrExpirationPolicy{}), + getterFactory: getterFactory, + } +} + +// Enabled implements DockerConfigProvider.Enabled. Enabled is true if AWS +// credentials are found. +func (p *ecrProvider) Enabled() bool { + sess, err := session.NewSessionWithOptions(session.Options{ + SharedConfigState: session.SharedConfigEnable, + }) + if err != nil { + klog.Errorf("while validating AWS credentials %v", err) + return false + } + if _, err := sess.Config.Credentials.Get(); err != nil { + klog.Errorf("while getting AWS credentials %v", err) + return false + } + return true +} + +// LazyProvide is lazy +// TODO: the LazyProvide methods will be removed in a future PR +func (p *ecrProvider) LazyProvide(image string) *credentialprovider.DockerConfigEntry { + return nil +} + +// Provide returns a DockerConfig with credentials from the cache if they are +// found, or from ECR +func (p *ecrProvider) Provide(image string) credentialprovider.DockerConfig { + parsed, err := parseRepoURL(image) + if err != nil { + klog.V(3).Info(err) + return credentialprovider.DockerConfig{} + } + + if cfg, exists := p.getFromCache(parsed); exists { + klog.V(6).Infof("Got ECR credentials from cache for %s", parsed.registry) + return cfg + } + klog.V(3).Info("unable to get ECR credentials from cache, checking ECR API") + + cfg, err := p.getFromECR(parsed) + if err != nil { + klog.Errorf("error getting credentials from ECR for %s %v", parsed.registry, err) + return credentialprovider.DockerConfig{} + } + klog.V(3).Infof("Got ECR credentials from ECR API for %s", parsed.registry) + return cfg +} + +// getFromCache attempts to get credentials from the cache +func (p *ecrProvider) getFromCache(parsed *parsedURL) (credentialprovider.DockerConfig, bool) { + cfg := credentialprovider.DockerConfig{} + + obj, exists, err := p.cache.GetByKey(parsed.registry) + if err != nil { + klog.Errorf("error getting ECR credentials from cache: %v", err) + return cfg, false + } + + if !exists { + return cfg, false + } + + entry := obj.(*cacheEntry) + cfg[entry.registry] = entry.credentials + return cfg, true +} + +// getFromECR gets credentials from ECR since they are not in the cache +func (p *ecrProvider) getFromECR(parsed *parsedURL) (credentialprovider.DockerConfig, error) { + cfg := credentialprovider.DockerConfig{} + getter, err := p.getterFactory.GetTokenGetterForRegion(parsed.region) + if err != nil { + return cfg, err + } + params := &ecr.GetAuthorizationTokenInput{RegistryIds: []*string{aws.String(parsed.registryID)}} + output, err := getter.GetAuthorizationToken(params) + if err != nil { + return cfg, err + } + if output == nil { + return cfg, errors.New("authorization token is nil") + } + if len(output.AuthorizationData) == 0 { + return cfg, errors.New("authorization data from response is empty") + } + data := output.AuthorizationData[0] + if data.AuthorizationToken == nil { + return cfg, errors.New("authorization token in response is nil") + } + entry, err := makeCacheEntry(data, parsed.registry) + if err != nil { + return cfg, err + } + if err := p.cache.Add(entry); err != nil { + return cfg, err + } + cfg[entry.registry] = entry.credentials + return cfg, nil +} + +type parsedURL struct { + registryID string + region string + registry string +} + +// parseRepoURL parses and splits the registry URL into the registry ID, +// region, and registry. +// .dkr.ecr(-fips)..amazonaws.com(.cn) +func parseRepoURL(image string) (*parsedURL, error) { + parsed, err := url.Parse("https://" + image) + if err != nil { + return nil, fmt.Errorf("error parsing image %s %v", image, err) + } + splitURL := ecrPattern.FindStringSubmatch(parsed.Hostname()) + if len(splitURL) == 0 { + return nil, fmt.Errorf("%s is not a valid ECR repository URL", parsed.Hostname()) + } + return &parsedURL{ + registryID: splitURL[1], + region: splitURL[3], + registry: parsed.Hostname(), + }, nil +} + +// tokenGetter is for testing purposes +type tokenGetter interface { + GetAuthorizationToken(input *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) +} + +// tokenGetterFactory is for testing purposes +type tokenGetterFactory interface { + GetTokenGetterForRegion(string) (tokenGetter, error) +} + +// ecrTokenGetterFactory stores a token getter per region +type ecrTokenGetterFactory struct { + cache map[string]tokenGetter + mutex sync.Mutex +} // awsHandlerLogger is a handler that logs all AWS SDK requests // Copied from pkg/cloudprovider/providers/aws/log_handler.go @@ -51,125 +217,15 @@ func awsHandlerLogger(req *request.Request) { klog.V(3).Infof("AWS request: %s:%s in %s", service, name, *region) } -// An interface for testing purposes. -type tokenGetter interface { - GetAuthorizationToken(input *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) -} - -// The canonical implementation -type ecrTokenGetter struct { - svc *ecr.ECR -} - -func (p *ecrTokenGetter) GetAuthorizationToken(input *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) { - return p.svc.GetAuthorizationToken(input) -} - -// lazyEcrProvider is a DockerConfigProvider that creates on demand an -// ecrProvider for a given region and then proxies requests to it. -type lazyEcrProvider struct { - region string - regionURL string - actualProvider *credentialprovider.CachingDockerConfigProvider -} - -var _ credentialprovider.DockerConfigProvider = &lazyEcrProvider{} - -// ecrProvider is a DockerConfigProvider that gets and refreshes 12-hour tokens -// from AWS to access ECR. -type ecrProvider struct { - region string - regionURL string - getter tokenGetter -} - -var _ credentialprovider.DockerConfigProvider = &ecrProvider{} - -// registryURL has different suffix in AWS China region -func registryURL(region string) string { - dnsSuffix := awsStandardDNSSuffix - // deal with aws none standard regions - if strings.HasPrefix(region, awsChinaRegionPrefix) { - dnsSuffix = awsChinaDNSSuffix +func newECRTokenGetter(region string) (tokenGetter, error) { + sess, err := session.NewSessionWithOptions(session.Options{ + Config: aws.Config{Region: aws.String(region)}, + SharedConfigState: session.SharedConfigEnable, + }) + if err != nil { + return nil, err } - return fmt.Sprintf(registryURLTemplate, region, dnsSuffix) -} - -// RegisterCredentialsProvider registers a credential provider for the specified region. -// It creates a lazy provider for each AWS region, in order to support -// cross-region ECR access. They have to be lazy because it's unlikely, but not -// impossible, that we'll use more than one. -// This should be called only if using the AWS cloud provider. -// This way, we avoid timeouts waiting for a non-existent provider. -func RegisterCredentialsProvider(region string) { - klog.V(4).Infof("registering credentials provider for AWS region %q", region) - - credentialprovider.RegisterCredentialProvider("aws-ecr-"+region, - &lazyEcrProvider{ - region: region, - regionURL: registryURL(region), - }) -} - -// Enabled implements DockerConfigProvider.Enabled for the lazy provider. -// Since we perform no checks/work of our own and actualProvider is only created -// later at image pulling time (if ever), always return true. -func (p *lazyEcrProvider) Enabled() bool { - return true -} - -// LazyProvide implements DockerConfigProvider.LazyProvide. It will be called -// by the client when attempting to pull an image and it will create the actual -// provider only when we actually need it the first time. -func (p *lazyEcrProvider) LazyProvide() *credentialprovider.DockerConfigEntry { - if p.actualProvider == nil { - klog.V(2).Infof("Creating ecrProvider for %s", p.region) - p.actualProvider = &credentialprovider.CachingDockerConfigProvider{ - Provider: newEcrProvider(p.region, nil), - // Refresh credentials a little earlier than expiration time - Lifetime: 11*time.Hour + 55*time.Minute, - } - if !p.actualProvider.Enabled() { - return nil - } - } - entry := p.actualProvider.Provide()[p.regionURL] - return &entry -} - -// Provide implements DockerConfigProvider.Provide, creating dummy credentials. -// Client code will call Provider.LazyProvide() at image pulling time. -func (p *lazyEcrProvider) Provide() credentialprovider.DockerConfig { - entry := credentialprovider.DockerConfigEntry{ - Provider: p, - } - cfg := credentialprovider.DockerConfig{} - cfg[p.regionURL] = entry - return cfg -} - -func newEcrProvider(region string, getter tokenGetter) *ecrProvider { - return &ecrProvider{ - region: region, - regionURL: registryURL(region), - getter: getter, - } -} - -// Enabled implements DockerConfigProvider.Enabled for the AWS token-based implementation. -// For now, it gets activated only if AWS was chosen as the cloud provider. -// TODO: figure how to enable it manually for deployments that are not on AWS but still -// use ECR somehow? -func (p *ecrProvider) Enabled() bool { - if p.region == "" { - klog.Errorf("Called ecrProvider.Enabled() with no region set") - return false - } - - getter := &ecrTokenGetter{svc: ecr.New(session.New(&aws.Config{ - Credentials: nil, - Region: &p.region, - }))} + getter := &ecrTokenGetter{svc: ecr.New(sess)} getter.svc.Handlers.Build.PushFrontNamed(request.NamedHandler{ Name: "k8s/user-agent", Fn: request.MakeAddToUserAgentHandler("kubernetes", version.Get().String()), @@ -178,55 +234,78 @@ func (p *ecrProvider) Enabled() bool { Name: "k8s/logger", Fn: awsHandlerLogger, }) - p.getter = getter - - return true + return getter, nil } -// LazyProvide implements DockerConfigProvider.LazyProvide. Should never be called. -func (p *ecrProvider) LazyProvide() *credentialprovider.DockerConfigEntry { - return nil -} +// GetTokenGetterForRegion gets the token getter for the requested region. If it +// doesn't exist, it creates a new ECR token getter +func (f *ecrTokenGetterFactory) GetTokenGetterForRegion(region string) (tokenGetter, error) { + f.mutex.Lock() + defer f.mutex.Unlock() -// Provide implements DockerConfigProvider.Provide, refreshing ECR tokens on demand -func (p *ecrProvider) Provide() credentialprovider.DockerConfig { - cfg := credentialprovider.DockerConfig{} - - // TODO: fill in RegistryIds? - params := &ecr.GetAuthorizationTokenInput{} - output, err := p.getter.GetAuthorizationToken(params) + if getter, ok := f.cache[region]; ok { + return getter, nil + } + getter, err := newECRTokenGetter(region) if err != nil { - klog.Errorf("while requesting ECR authorization token %v", err) - return cfg + return nil, fmt.Errorf("unable to create token getter for region %v %v", region, err) } - if output == nil { - klog.Errorf("Got back no ECR token") - return cfg - } - - for _, data := range output.AuthorizationData { - if data.ProxyEndpoint != nil && - data.AuthorizationToken != nil { - decodedToken, err := base64.StdEncoding.DecodeString(aws.StringValue(data.AuthorizationToken)) - if err != nil { - klog.Errorf("while decoding token for endpoint %v %v", data.ProxyEndpoint, err) - return cfg - } - parts := strings.SplitN(string(decodedToken), ":", 2) - user := parts[0] - password := parts[1] - entry := credentialprovider.DockerConfigEntry{ - Username: user, - Password: password, - // ECR doesn't care and Docker is about to obsolete it - Email: "not@val.id", - } - - klog.V(3).Infof("Adding credentials for user %s in %s", user, p.region) - // Add our config entry for this region's registry URLs - cfg[p.regionURL] = entry - - } - } - return cfg + f.cache[region] = getter + return getter, nil +} + +// The canonical implementation +type ecrTokenGetter struct { + svc *ecr.ECR +} + +// GetAuthorizationToken gets the ECR authorization token using the ECR API +func (p *ecrTokenGetter) GetAuthorizationToken(input *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) { + return p.svc.GetAuthorizationToken(input) +} + +type cacheEntry struct { + expiresAt time.Time + credentials credentialprovider.DockerConfigEntry + registry string +} + +// makeCacheEntry decodes the ECR authorization entry and re-packages it into a +// cacheEntry. +func makeCacheEntry(data *ecr.AuthorizationData, registry string) (*cacheEntry, error) { + decodedToken, err := base64.StdEncoding.DecodeString(aws.StringValue(data.AuthorizationToken)) + if err != nil { + return nil, fmt.Errorf("error decoding ECR authorization token: %v", err) + } + parts := strings.SplitN(string(decodedToken), ":", 2) + if len(parts) < 2 { + return nil, errors.New("error getting username and password from authorization token") + } + creds := credentialprovider.DockerConfigEntry{ + Username: parts[0], + Password: parts[1], + Email: "not@val.id", // ECR doesn't care and Docker is about to obsolete it + } + if data.ExpiresAt == nil { + return nil, errors.New("authorization data expiresAt is nil") + } + return &cacheEntry{ + expiresAt: data.ExpiresAt.Add(-1 * wait.Jitter(30*time.Minute, 0.2)), + credentials: creds, + registry: registry, + }, nil +} + +// ecrExpirationPolicy implements ExpirationPolicy from client-go. +type ecrExpirationPolicy struct{} + +// stringKeyFunc returns the cache key as a string +func stringKeyFunc(obj interface{}) (string, error) { + key := obj.(*cacheEntry).registry + return key, nil +} + +// IsExpired checks if the ECR credentials are expired. +func (p *ecrExpirationPolicy) IsExpired(entry *cache.TimestampedEntry) bool { + return time.Now().After(entry.Obj.(*cacheEntry).expiresAt) } diff --git a/pkg/credentialprovider/aws/aws_credentials_test.go b/pkg/credentialprovider/aws/aws_credentials_test.go index 499af17b78b..7161fc136d1 100644 --- a/pkg/credentialprovider/aws/aws_credentials_test.go +++ b/pkg/credentialprovider/aws/aws_credentials_test.go @@ -19,7 +19,9 @@ package credentials import ( "encoding/base64" "fmt" + "math/rand" "path" + "strconv" "testing" "time" @@ -34,15 +36,30 @@ const password = "1234567890abcdef" const email = "not@val.id" // Mock implementation +// randomizePassword is used to check for a cache hit to verify the password +// has not changed type testTokenGetter struct { - user string - password string - endpoint string + user string + password string + endpoint string + randomizePassword bool +} + +type testTokenGetterFactory struct { + getter tokenGetter +} + +func (f *testTokenGetterFactory) GetTokenGetterForRegion(region string) (tokenGetter, error) { + return f.getter, nil } func (p *testTokenGetter) GetAuthorizationToken(input *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) { - + if p.randomizePassword { + rand.Seed(int64(time.Now().Nanosecond())) + p.password = strconv.Itoa(rand.Int()) + } expiration := time.Now().Add(1 * time.Hour) + // expiration := time.Now().Add(5 * time.Second) //for testing with the cache expiring creds := []byte(fmt.Sprintf("%s:%s", p.user, p.password)) data := &ecr.AuthorizationData{ AuthorizationToken: aws.String(base64.StdEncoding.EncodeToString(creds)), @@ -52,112 +69,265 @@ func (p *testTokenGetter) GetAuthorizationToken(input *ecr.GetAuthorizationToken output := &ecr.GetAuthorizationTokenOutput{ AuthorizationData: []*ecr.AuthorizationData{data}, } - return output, nil //p.svc.GetAuthorizationToken(input) } -func TestEcrProvide(t *testing.T) { +func TestRegistryPatternMatch(t *testing.T) { + grid := []struct { + Registry string + Expected bool + }{ + {"123456789012.dkr.ecr.lala-land-1.amazonaws.com", true}, + // fips + {"123456789012.dkr.ecr-fips.lala-land-1.amazonaws.com", true}, + // .cn + {"123456789012.dkr.ecr.lala-land-1.amazonaws.com.cn", true}, + // registry ID too long + {"1234567890123.dkr.ecr.lala-land-1.amazonaws.com", false}, + // registry ID too short + {"12345678901.dkr.ecr.lala-land-1.amazonaws.com", false}, + // registry ID has invalid chars + {"12345678901A.dkr.ecr.lala-land-1.amazonaws.com", false}, + // region has invalid chars + {"123456789012.dkr.ecr.lala-land-1!.amazonaws.com", false}, + // region starts with invalid char + {"123456789012.dkr.ecr.#lala-land-1.amazonaws.com", false}, + // invalid host suffix + {"123456789012.dkr.ecr.lala-land-1.amazonaws.hacker.com", false}, + // invalid host suffix + {"123456789012.dkr.ecr.lala-land-1.hacker.com", false}, + // invalid host suffix + {"123456789012.dkr.ecr.lala-land-1.amazonaws.lol", false}, + // without dkr + {"123456789012.dog.ecr.lala-land-1.amazonaws.com", false}, + // without ecr + {"123456789012.dkr.cat.lala-land-1.amazonaws.com", false}, + // without amazonaws + {"123456789012.dkr.cat.lala-land-1.awsamazon.com", false}, + // too short + {"123456789012.lala-land-1.amazonaws.com", false}, + } + for _, g := range grid { + actual := ecrPattern.MatchString(g.Registry) + if actual != g.Expected { + t.Errorf("unexpected pattern match value, want %v for %s", g.Expected, g.Registry) + } + } +} + +func TestParseRepoURLPass(t *testing.T) { + registryID := "123456789012" + region := "lala-land-1" + port := "9001" + registry := "123456789012.dkr.ecr.lala-land-1.amazonaws.com" + image := path.Join(registry, port, "foo/bar") + parsedURL, err := parseRepoURL(image) + + if err != nil { + t.Errorf("Could not parse URL: %s, err: %v", image, err) + } + if registryID != parsedURL.registryID { + t.Errorf("Unexpected registryID value, want: %s, got: %s", registryID, parsedURL.registryID) + } + if region != parsedURL.region { + t.Errorf("Unexpected region value, want: %s, got: %s", region, parsedURL.region) + } + if registry != parsedURL.registry { + t.Errorf("Unexpected registry value, want: %s, got: %s", registry, parsedURL.registry) + } +} + +func TestParseRepoURLFail(t *testing.T) { + registry := "123456789012.foo.bar.baz" + image := path.Join(registry, "foo/bar") + parsedURL, err := parseRepoURL(image) + expectedErr := "123456789012.foo.bar.baz is not a valid ECR repository URL" + + if err == nil { + t.Errorf("Should fail to parse URL %s", image) + } + if err.Error() != expectedErr { + t.Errorf("Unexpected error, want: %s, got: %v", expectedErr, err) + } + if parsedURL != nil { + t.Errorf("Expected parsedURL to be nil") + } +} + +func TestECRProvide(t *testing.T) { registry := "123456789012.dkr.ecr.lala-land-1.amazonaws.com" otherRegistries := []string{ "123456789012.dkr.ecr.cn-foo-1.amazonaws.com.cn", "private.registry.com", "gcr.io", } - image := "foo/bar" - - provider := newEcrProvider("lala-land-1", - &testTokenGetter{ + image := path.Join(registry, "foo/bar") + p := newECRProvider(&testTokenGetterFactory{ + getter: &testTokenGetter{ user: user, password: password, endpoint: registry, - }) - + }, + }) keyring := &credentialprovider.BasicDockerKeyring{} - keyring.Add(provider.Provide()) + keyring.Add(p.Provide(image)) // Verify that we get the expected username/password combo for // an ECR image name. - fullImage := path.Join(registry, image) - creds, ok := keyring.Lookup(fullImage) + creds, ok := keyring.Lookup(image) if !ok { - t.Errorf("Didn't find expected URL: %s", fullImage) + t.Errorf("Didn't find expected URL: %s", image) return } if len(creds) > 1 { t.Errorf("Got more hits than expected: %s", creds) } - val := creds[0] - - if user != val.Username { - t.Errorf("Unexpected username value, want: _token, got: %s", val.Username) + cred := creds[0] + if user != cred.Username { + t.Errorf("Unexpected username value, want: %s, got: %s", user, cred.Username) } - if password != val.Password { - t.Errorf("Unexpected password value, want: %s, got: %s", password, val.Password) + if password != creds[0].Password { + t.Errorf("Unexpected password value, want: %s, got: %s", password, cred.Password) } - if email != val.Email { - t.Errorf("Unexpected email value, want: %s, got: %s", email, val.Email) + if email != creds[0].Email { + t.Errorf("Unexpected email value, want: %s, got: %s", email, cred.Email) } // Verify that we get an error for other images. for _, otherRegistry := range otherRegistries { - fullImage = path.Join(otherRegistry, image) - creds, ok = keyring.Lookup(fullImage) + image = path.Join(otherRegistry, "foo/bar") + creds, ok = keyring.Lookup(image) if ok { - t.Errorf("Unexpectedly found image: %s", fullImage) + t.Errorf("Unexpectedly found image: %s", image) return } } } -func TestChinaEcrProvide(t *testing.T) { +func TestECRProvideCached(t *testing.T) { + registry := "123456789012.dkr.ecr.lala-land-1.amazonaws.com" + p := newECRProvider(&testTokenGetterFactory{ + getter: &testTokenGetter{ + user: user, + password: password, + endpoint: registry, + randomizePassword: true, + }, + }) + image1 := path.Join(registry, "foo/bar") + image2 := path.Join(registry, "bar/baz") + keyring := &credentialprovider.BasicDockerKeyring{} + keyring.Add(p.Provide(image1)) + // time.Sleep(6 * time.Second) //for testing with the cache expiring + keyring.Add(p.Provide(image2)) + // Verify that we get the credentials from the + // cache the second time + creds1, ok := keyring.Lookup(image1) + if !ok { + t.Errorf("Didn't find expected URL: %s", image1) + return + } + if len(creds1) != 2 { + t.Errorf("Got more hits than expected: %s", creds1) + } + + if creds1[0].Password != creds1[1].Password { + t.Errorf("cached credentials do not match") + } + + creds2, ok := keyring.Lookup(image2) + if !ok { + t.Errorf("Didn't find expected URL: %s", image1) + return + } + if len(creds2) != 2 { + t.Errorf("Got more hits than expected: %s", creds2) + } + + if creds2[0].Password != creds2[1].Password { + t.Errorf("cached credentials do not match") + } + if creds1[0].Password != creds2[0].Password { + t.Errorf("cached credentials do not match") + } +} + +func TestChinaECRProvide(t *testing.T) { registry := "123456789012.dkr.ecr.cn-foo-1.amazonaws.com.cn" otherRegistries := []string{ "123456789012.dkr.ecr.lala-land-1.amazonaws.com", "private.registry.com", "gcr.io", } - image := "foo/bar" - - provider := newEcrProvider("cn-foo-1", - &testTokenGetter{ + image := path.Join(registry, "foo/bar") + p := newECRProvider(&testTokenGetterFactory{ + getter: &testTokenGetter{ user: user, password: password, endpoint: registry, - }) - + }, + }) keyring := &credentialprovider.BasicDockerKeyring{} - keyring.Add(provider.Provide()) - + keyring.Add(p.Provide(image)) // Verify that we get the expected username/password combo for // an ECR image name. - fullImage := path.Join(registry, image) - creds, ok := keyring.Lookup(fullImage) + creds, ok := keyring.Lookup(image) if !ok { - t.Errorf("Didn't find expected URL: %s", fullImage) + t.Errorf("Didn't find expected URL: %s", image) return } if len(creds) > 1 { t.Errorf("Got more hits than expected: %s", creds) } - val := creds[0] - - if user != val.Username { - t.Errorf("Unexpected username value, want: _token, got: %s", val.Username) + cred := creds[0] + if user != cred.Username { + t.Errorf("Unexpected username value, want: %s, got: %s", user, cred.Username) } - if password != val.Password { - t.Errorf("Unexpected password value, want: %s, got: %s", password, val.Password) + if password != cred.Password { + t.Errorf("Unexpected password value, want: %s, got: %s", password, cred.Password) } - if email != val.Email { - t.Errorf("Unexpected email value, want: %s, got: %s", email, val.Email) + if email != cred.Email { + t.Errorf("Unexpected email value, want: %s, got: %s", email, cred.Email) } // Verify that we get an error for other images. for _, otherRegistry := range otherRegistries { - fullImage = path.Join(otherRegistry, image) - creds, ok = keyring.Lookup(fullImage) + image = path.Join(otherRegistry, image) + creds, ok = keyring.Lookup(image) if ok { - t.Errorf("Unexpectedly found image: %s", fullImage) + t.Errorf("Unexpectedly found image: %s", image) return } } } + +func TestChinaECRProvideCached(t *testing.T) { + registry := "123456789012.dkr.ecr.cn-foo-1.amazonaws.com.cn" + p := newECRProvider(&testTokenGetterFactory{ + getter: &testTokenGetter{ + user: user, + password: password, + endpoint: registry, + randomizePassword: true, + }, + }) + image := path.Join(registry, "foo/bar") + keyring := &credentialprovider.BasicDockerKeyring{} + keyring.Add(p.Provide(image)) + // time.Sleep(6 * time.Second) //for testing with the cache expiring + keyring.Add(p.Provide(image)) + // Verify that we get the credentials from the + // cache the second time + creds, ok := keyring.Lookup(image) + if !ok { + t.Errorf("Didn't find expected URL: %s", image) + return + } + if len(creds) != 2 { + t.Errorf("Got more hits than expected: %s", creds) + } + + if creds[0].Password != creds[1].Password { + t.Errorf("cached credentials do not match") + } +} From 0d63fa4543b022a9bcb47cde8a6d2f98f4ea429c Mon Sep 17 00:00:00 2001 From: tiffany jernigan Date: Fri, 22 Mar 2019 18:42:28 +0000 Subject: [PATCH 5/5] Update aws provider build files --- pkg/cloudprovider/providers/aws/BUILD | 1 - pkg/credentialprovider/aws/BUILD | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cloudprovider/providers/aws/BUILD b/pkg/cloudprovider/providers/aws/BUILD index 6087aa7c8b8..84513427c6e 100644 --- a/pkg/cloudprovider/providers/aws/BUILD +++ b/pkg/cloudprovider/providers/aws/BUILD @@ -27,7 +27,6 @@ go_library( ], importpath = "k8s.io/kubernetes/pkg/cloudprovider/providers/aws", deps = [ - "//pkg/credentialprovider/aws:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/pkg/credentialprovider/aws/BUILD b/pkg/credentialprovider/aws/BUILD index 7a6b52b0b92..8ba6f1e8fe5 100644 --- a/pkg/credentialprovider/aws/BUILD +++ b/pkg/credentialprovider/aws/BUILD @@ -13,6 +13,8 @@ go_library( deps = [ "//pkg/credentialprovider:go_default_library", "//pkg/version:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", + "//staging/src/k8s.io/client-go/tools/cache:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/request:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/session:go_default_library",