Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AWS: Allow cross-region image pulling with ECR #24369

Merged
merged 3 commits into from
May 13, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 2 additions & 16 deletions pkg/cloudprovider/providers/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import (

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/cloudprovider"
"k8s.io/kubernetes/pkg/credentialprovider/aws"
aws_credentials "k8s.io/kubernetes/pkg/credentialprovider/aws"
Copy link
Member

@justinsb justinsb May 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a confusing alias (I would have guessed "github.com/aws/aws-sdk-go/aws/credentials"), but I can't think of a better one that isn't 30 characters long.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aws_cred_provider?

"k8s.io/kubernetes/pkg/types"

"github.com/golang/glog"
Expand Down Expand Up @@ -566,21 +566,7 @@ func getAvailabilityZone(metadata EC2Metadata) (string, error) {
}

func isRegionValid(region string) bool {
regions := [...]string{
"us-east-1",
"us-west-1",
"us-west-2",
"eu-west-1",
"eu-central-1",
"ap-southeast-1",
"ap-southeast-2",
"ap-northeast-1",
"ap-northeast-2",
"cn-north-1",
"us-gov-west-1",
"sa-east-1",
}
for _, r := range regions {
for _, r := range aws_credentials.AWSRegions {
if r == region {
return true
}
Expand Down
135 changes: 99 additions & 36 deletions pkg/credentialprovider/aws/aws_credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package aws_credentials
package credentials

import (
"encoding/base64"
"fmt"
"strings"
"time"

Expand All @@ -26,23 +27,40 @@ import (
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/ecr"
"github.com/golang/glog"
"k8s.io/kubernetes/pkg/cloudprovider"
"k8s.io/kubernetes/pkg/credentialprovider"
)

var registryUrls = []string{"*.dkr.ecr.*.amazonaws.com"}
// AWSRegions is the complete list of regions known to the AWS cloudprovider
// and credentialprovider.
var AWSRegions = [...]string{
"us-east-1",
"us-west-1",
"us-west-2",
"eu-west-1",
"eu-central-1",
"ap-southeast-1",
"ap-southeast-2",
"ap-northeast-1",
"ap-northeast-2",
"cn-north-1",
"us-gov-west-1",
"sa-east-1",
}

const registryURLTemplate = "*.dkr.ecr.%s.amazonaws.com"

// awsHandlerLogger is a handler that logs all AWS SDK requests
// Copied from cloudprovider/aws/log_handler.go
func awsHandlerLogger(req *request.Request) {
service := req.ClientInfo.ServiceName
region := req.Config.Region

name := "?"
if req.Operation != nil {
name = req.Operation.Name
}

glog.V(4).Infof("AWS request: %s %s", service, name)
glog.V(3).Infof("AWS request: %s:%s in %s", service, name, *region)
Copy link
Member

@justinsb justinsb May 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably should guard against the region being nil, but there's only a tiny number of non-regional calls AFAIK, and I think they are all S3 calls, so maybe it doesn't matter.

}

// An interface for testing purposes.
Expand All @@ -59,56 +77,101 @@ func (p *ecrTokenGetter) GetAuthorizationToken(input *ecr.GetAuthorizationTokenI
return p.svc.GetAuthorizationToken(input)
}

// lazyEcrProvider is a DockerConfigProvider that creates on demand an
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: I personally like to do this:

var _ DockerConfigProvider = &lazyEcrProvider{}

Apparently there's some debate as to whether or not to do it, but I like that it makes the intent very clear and that it is then checked immediately by the golang compiler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added both.

// 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 {
getter tokenGetter
region string
regionURL string
getter tokenGetter
}

var _ credentialprovider.DockerConfigProvider = &ecrProvider{}

// Init 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.
// Not using the package init() function: this module should be initialized only
// if using the AWS cloud provider. This way, we avoid timeouts waiting for a
// non-existent provider.
func Init() {
credentialprovider.RegisterCredentialProvider("aws-ecr-key",
&credentialprovider.CachingDockerConfigProvider{
Provider: &ecrProvider{},
// Refresh credentials a little earlier before they expire
for _, region := range AWSRegions {
credentialprovider.RegisterCredentialProvider("aws-ecr-"+region,
&lazyEcrProvider{
region: region,
regionURL: fmt.Sprintf(registryURLTemplate, 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actualProvider.Enabled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual provider (p.actualProvider) may not exist, IIUC. So, the lazy provider reports as "always enabled" to the credentialprovider and when it is used, the lazy provider creates an actual provider. IIUC

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rata is correct. actualProvider is only created the first time an image is pulled. Enabled() gets called much earlier than that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment explaining why we always 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 {
Copy link
Member

@rata rata Apr 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think instead of having the code here inside the if, move the code to create the actualProvider to another function, and do something like:

if p.actualProvider == nil {
  p.actualProvider = <func to create the actualProvider here>
}
entry := p.actualProvider.Provide()[p.regionURL]
return &entry

I'm not sure how the k8s code styling is (will check next week :)), but this is a small detail but it seems slightly more readable for me. And it's not like TONs of parameters will be needed in the func to create the actual provider.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was going to try a helper function next. And perhaps, instead of creating N lazy providers, just one, which does a string match against the image getting loaded, figuring the region, then maintaining a map of ecrProviders.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that. The code this way is pretty simple and the N lazy providers shouldn't make noticiable difference in mem or cpu, and as no real provider is created for the unused, not even messages with the cloud provider are used.

So, I'd do it only if it is equally or more readable than now, and equally or less error prone. If the code gets more messy, I don't think it's worth the effort.

But your call :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper/constructor added. I'm still tempted to have one lazy provider (which would definitely require more tests).

glog.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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably guard against an error meaning the region is not present in the DockerConfig map.

return &entry
}

// Provide implements DockerConfigProvider.Provide, creating dummy credentials.
// Client code will call Provider.LazyProvide() at image pulling time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the Provide() method from the DockerConfigProvider interface? I find it confusing that there are now two methods on the interface - it really isn't clear (to me) what is called when. But I guess that isn't this PR.

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: fmt.Sprintf(registryURLTemplate, 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 {
provider, err := cloudprovider.GetCloudProvider("aws", nil)
if err != nil {
glog.Errorf("while initializing AWS cloud provider %v", err)
return false
}
if provider == nil {
return false
}

zones, ok := provider.Zones()
if !ok {
glog.Errorf("couldn't get Zones() interface")
return false
}
zone, err := zones.GetZone()
if err != nil {
glog.Errorf("while getting zone %v", err)
return false
}
if zone.Region == "" {
glog.Errorf("Region information is empty")
if p.region == "" {
glog.Errorf("Called ecrProvider.Enabled() with no region set")
return false
}

getter := &ecrTokenGetter{svc: ecr.New(session.New(&aws.Config{
Credentials: nil,
Region: &zone.Region,
Region: &p.region,
}))}
getter.svc.Handlers.Sign.PushFrontNamed(request.NamedHandler{
Name: "k8s/logger",
Expand Down Expand Up @@ -158,10 +221,10 @@ func (p *ecrProvider) Provide() credentialprovider.DockerConfig {
Email: "not@val.id",
}

// Add our entry for each of the supported container registry URLs
for _, k := range registryUrls {
cfg[k] = entry
}
glog.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
Expand Down
13 changes: 7 additions & 6 deletions pkg/credentialprovider/aws/aws_credentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package aws_credentials
package credentials

import (
"encoding/base64"
Expand Down Expand Up @@ -58,17 +58,18 @@ func (p *testTokenGetter) GetAuthorizationToken(input *ecr.GetAuthorizationToken

func TestEcrProvide(t *testing.T) {
registry := "123456789012.dkr.ecr.lala-land-1.amazonaws.com"
otherRegistries := []string{"private.registry.com",
otherRegistries := []string{
"private.registry.com",
"gcr.io",
}
image := "foo/bar"

provider := &ecrProvider{
getter: &testTokenGetter{
provider := newEcrProvider("lala-land-1",
&testTokenGetter{
user: user,
password: password,
endpoint: registry},
}
endpoint: registry,
})

keyring := &credentialprovider.BasicDockerKeyring{}
keyring.Add(provider.Provide())
Expand Down