Refactors and fixes bugs in AWS credentialprovider

Adds caching per registry. Fixes caching of invalid ECR tokens.

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-"
+var ecrPattern = regexp.MustCompile(`^(\d{12})\.dkr\.ecr(\-fips)?\.([a-zA-Z0-9][a-zA-Z0-9-_]*)\.amazonaws\.com(\.cn)?$`)
-const awsStandardDNSSuffix = "amazonaws.com"
+
-const awsChinaDNSSuffix = "amazonaws.com.cn"
+// init registers a credential provider for each registryURLTemplate and creates
-const registryURLTemplate = "*.dkr.ecr.%s.%s"
+// 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.
+// <registryID>.dkr.ecr(-fips).<region>.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.
+func newECRTokenGetter(region string) (tokenGetter, error) {
-type tokenGetter interface {
+	sess, err := session.NewSessionWithOptions(session.Options{
-	GetAuthorizationToken(input *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error)
+		Config:            aws.Config{Region: aws.String(region)},
-}
+		SharedConfigState: session.SharedConfigEnable,
-
+	})
-// The canonical implementation
+	if err != nil {
-type ecrTokenGetter struct {
+		return nil, err
-	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
 	}
-	return fmt.Sprintf(registryURLTemplate, region, dnsSuffix)
+	getter := &ecrTokenGetter{svc: ecr.New(sess)}
-}
-
-// 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.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 getter, nil
-
-	return true
 }
 
-// LazyProvide implements DockerConfigProvider.LazyProvide. Should never be called.
+// GetTokenGetterForRegion gets the token getter for the requested region. If it
-func (p *ecrProvider) LazyProvide() *credentialprovider.DockerConfigEntry {
+// doesn't exist, it creates a new ECR token getter
-	return nil
+func (f *ecrTokenGetterFactory) GetTokenGetterForRegion(region string) (tokenGetter, error) {
-}
+	f.mutex.Lock()
+	defer f.mutex.Unlock()
 
-// Provide implements DockerConfigProvider.Provide, refreshing ECR tokens on demand
+	if getter, ok := f.cache[region]; ok {
-func (p *ecrProvider) Provide() credentialprovider.DockerConfig {
+		return getter, nil
-	cfg := credentialprovider.DockerConfig{}
+	}
-
+	getter, err := newECRTokenGetter(region)
-	// TODO: fill in RegistryIds?
-	params := &ecr.GetAuthorizationTokenInput{}
-	output, err := p.getter.GetAuthorizationToken(params)
 	if err != nil {
-		klog.Errorf("while requesting ECR authorization token %v", err)
+		return nil, fmt.Errorf("unable to create token getter for region %v %v", region, err)
-		return cfg
 	}
-	if output == nil {
+	f.cache[region] = getter
-		klog.Errorf("Got back no ECR token")
+	return getter, nil
-		return cfg
+}
-	}
+
-
+// The canonical implementation
-	for _, data := range output.AuthorizationData {
+type ecrTokenGetter struct {
-		if data.ProxyEndpoint != nil &&
+	svc *ecr.ECR
-			data.AuthorizationToken != nil {
+}
-			decodedToken, err := base64.StdEncoding.DecodeString(aws.StringValue(data.AuthorizationToken))
+
-			if err != nil {
+// GetAuthorizationToken gets the ECR authorization token using the ECR API
-				klog.Errorf("while decoding token for endpoint %v %v", data.ProxyEndpoint, err)
+func (p *ecrTokenGetter) GetAuthorizationToken(input *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) {
-				return cfg
+	return p.svc.GetAuthorizationToken(input)
-			}
+}
-			parts := strings.SplitN(string(decodedToken), ":", 2)
+
-			user := parts[0]
+type cacheEntry struct {
-			password := parts[1]
+	expiresAt   time.Time
-			entry := credentialprovider.DockerConfigEntry{
+	credentials credentialprovider.DockerConfigEntry
-				Username: user,
+	registry    string
-				Password: password,
+}
-				// ECR doesn't care and Docker is about to obsolete it
+
-				Email: "not@val.id",
+// makeCacheEntry decodes the ECR authorization entry and re-packages it into a
-			}
+// cacheEntry.
-
+func makeCacheEntry(data *ecr.AuthorizationData, registry string) (*cacheEntry, error) {
-			klog.V(3).Infof("Adding credentials for user %s in %s", user, p.region)
+	decodedToken, err := base64.StdEncoding.DecodeString(aws.StringValue(data.AuthorizationToken))
-			// Add our config entry for this region's registry URLs
+	if err != nil {
-			cfg[p.regionURL] = entry
+		return nil, fmt.Errorf("error decoding ECR authorization token: %v", err)
-
+	}
-		}
+	parts := strings.SplitN(string(decodedToken), ":", 2)
-	}
+	if len(parts) < 2 {
-	return cfg
+		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)
 }

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
+	user              string
-	password string
+	password          string
-	endpoint 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"
+	image := path.Join(registry, "foo/bar")
-
+	p := newECRProvider(&testTokenGetterFactory{
-	provider := newEcrProvider("lala-land-1",
+		getter: &testTokenGetter{
-		&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(image)
-	creds, ok := keyring.Lookup(fullImage)
 	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]
+	cred := creds[0]
-
+	if user != cred.Username {
-	if user != val.Username {
+		t.Errorf("Unexpected username value, want: %s, got: %s", user, cred.Username)
-		t.Errorf("Unexpected username value, want: _token, got: %s", val.Username)
 	}
-	if password != val.Password {
+	if password != creds[0].Password {
-		t.Errorf("Unexpected password value, want: %s, got: %s", password, val.Password)
+		t.Errorf("Unexpected password value, want: %s, got: %s", password, cred.Password)
 	}
-	if email != val.Email {
+	if email != creds[0].Email {
-		t.Errorf("Unexpected email value, want: %s, got: %s", email, val.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)
+		image = path.Join(otherRegistry, "foo/bar")
-		creds, ok = keyring.Lookup(fullImage)
+		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"
+	image := path.Join(registry, "foo/bar")
-
+	p := newECRProvider(&testTokenGetterFactory{
-	provider := newEcrProvider("cn-foo-1",
+		getter: &testTokenGetter{
-		&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(image)
-	creds, ok := keyring.Lookup(fullImage)
 	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]
+	cred := creds[0]
-
+	if user != cred.Username {
-	if user != val.Username {
+		t.Errorf("Unexpected username value, want: %s, got: %s", user, cred.Username)
-		t.Errorf("Unexpected username value, want: _token, got: %s", val.Username)
 	}
-	if password != val.Password {
+	if password != cred.Password {
-		t.Errorf("Unexpected password value, want: %s, got: %s", password, val.Password)
+		t.Errorf("Unexpected password value, want: %s, got: %s", password, cred.Password)
 	}
-	if email != val.Email {
+	if email != cred.Email {
-		t.Errorf("Unexpected email value, want: %s, got: %s", email, val.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)
+		image = path.Join(otherRegistry, image)
-		creds, ok = keyring.Lookup(fullImage)
+		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")
+	}
+}