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

Conversation

therc
Copy link
Member

@therc therc commented Apr 16, 2016

Fixes #23298
Definitely should be in the release notes; should maybe get merged in 1.2 along with #23594 after some soaking. Documentation changes to follow.

cc @justinsb @erictune @rata @miguelfrde

This is step two. We now create long-lived, lazy ECR providers in all regions.
When first used, they will create the actual ECR providers doing the work
behind the scenes, namely talking to ECR in the region where the image lives,
rather than the one our instance is running in.

Also:

  • moved the list of AWS regions out of the AWS cloudprovider and into the
    credentialprovider, then exported it from there.
  • improved logging

Behold, running in us-east-1:

aws_credentials.go:127] Creating ecrProvider for us-west-2
aws_credentials.go:63] AWS request: ecr:GetAuthorizationToken in us-west-2
aws_credentials.go:217] Adding credentials for user AWS in us-west-2
Successfully pulled image "123456789012.dkr.ecr.us-west-2.amazonaws.com/test:latest"

"One small step for a pod, one giant leap for Kube-kind."


This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 16, 2016
// 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).

@rata
Copy link
Member

rata commented Apr 16, 2016

@therc: nice! I did two small questions, but looks great for me (will test it next week :))

@@ -59,56 +76,96 @@ 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.

@justinsb
Copy link
Member

A few code nits/questions, but also:

How does cross-region ECR work in terms of authentication? Are the IAM instance credentials for us-east-1 valid against a us-west ECR?

Do we now construct an ECR provider against every region? I don't quite understand how the laziness works...

@therc
Copy link
Member Author

therc commented Apr 17, 2016

@justinsb in my testing, it looks like my IAM instance credentials in us-east-1 were enough to invoke GetAuthorizationToken in us-west.

We only create empty lazy providers in every region. This layered approach is a compromise between two extremes:

  1. Create a real ecrProvider in every region, which obtains and refreshes credentials on a regular basis. This is wasteful for every node to do, especially for the vast majority of people who'll tend to use a local registry. In addition to being more likely to trigger throttling, this might cause timeouts in regions where are there no registries (and there might be none for a long time).
  2. Get credentials for every image pulled.

The real ECR provider is only created the first time we try to pull an actual image.

@therc
Copy link
Member Author

therc commented Apr 17, 2016

Feedback addessed. I'm still on the fence on whether we should have just one lazy provider vs one for each and every region.

@rata
Copy link
Member

rata commented Apr 17, 2016

@therc: great!

@rata
Copy link
Member

rata commented Apr 17, 2016

@therc: I'm not familiar with the code at all, but I'm unsure why it's not better to move the lazy-ness to the "credentialprovider.CachingDockerConfigProvider". I'll try to have a look next week, but don't worry about me. Thanks! :)

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2016
@therc
Copy link
Member Author

therc commented May 2, 2016

@k8s-bot retest this issue: #21484

@therc
Copy link
Member Author

therc commented May 2, 2016

Rebased to pick up ap-northeast-2 from #24457

@therc
Copy link
Member Author

therc commented May 3, 2016

Ping?

@rata
Copy link
Member

rata commented May 3, 2016

@therc IMHO, I don't know if the laziness can be moved a layer up, or even if it makes sense. But in any case, I'm fine with this and improve later if there is room for improvement.

In any case, it doesn't really counts what I say. Probably the best is to ping @justinsb ? :)

@k8s-github-robot k8s-github-robot assigned erictune and liggitt and unassigned justinsb and erictune May 6, 2016
Rudi Chiarito added 3 commits May 10, 2016 12:03
This is step two. We now create long-lived, lazy ECR providers in all regions.
When first used, they will create the actual ECR providers doing the work
behind the scenes, namely talking to ECR in the region where the image lives,
rather than the one our instance is running in.

Also:

- moved the list of AWS regions out of the AWS cloudprovider and into the
credentialprovider, then exported it from there.
- improved logging

Behold, running in us-east-1:

```
aws_credentials.go:127] Creating ecrProvider for us-west-2
aws_credentials.go:63] AWS request: ecr:GetAuthorizationToken in us-west-2
aws_credentials.go:217] Adding credentials for user AWS in us-west-2
Successfully pulled image 123456789012.dkr.ecr.us-west-2.amazonaws.com/test:latest"
```

*"One small step for a pod, one giant leap for Kube-kind."*
Use constructor for ecrProvider
Rename package to "credentials" like golint requests
Don't wrap the lazy provider with a caching provider
Add immedita compile-time interface conformance checks for the interfaces
Added comments
@k8s-github-robot
Copy link

@therc
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@therc
Copy link
Member Author

therc commented May 10, 2016

@k8s-bot test this issue: #22655

@k8s-bot
Copy link

k8s-bot commented May 10, 2016

GCE e2e build/test passed for commit 684517f.

@liggitt liggitt assigned justinsb and unassigned liggitt May 11, 2016
@@ -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?

@justinsb justinsb changed the title Allow cross-region image pulling with AWS' ECR AWS: Allow cross-region image pulling with ECR May 12, 2016
@justinsb justinsb added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels May 12, 2016
@justinsb
Copy link
Member

I do find this very confusing, so I'm tempted to attempt a refactor, but let's get this merged! Thanks for implementing - will be very helpful to a lot of people.

@therc
Copy link
Member Author

therc commented May 12, 2016

I'll create another PR with a mini design doc, a link to it and miscellaneous remaining fixes, but yes, it has to be either confusing like this or very inefficient, given the existing design of credential providers.

@justinsb
Copy link
Member

@therc let's get the AWS PR backlog (of my fault) merged, and then maybe let's look at reworking the design of the credentials provider? I'm certainly game to take a look, and there's no point doing a lot of documentation it if we're going to change it. If/when I/we discover this is actually harder than I think and the design must stay, then more documentation is very welcome :-)

@justinsb justinsb added this to the v1.3 milestone May 13, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 13, 2016

GCE e2e build/test passed for commit 684517f.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 24c46ac into kubernetes:master May 13, 2016
@therc therc deleted the ecr branch July 18, 2016 21:07
therc added a commit to therc/kubernetes.github.io that referenced this pull request Jan 26, 2017
Fetching images from a registry hosted in a different region has been supported since 1.3 with
kubernetes/kubernetes#24369
chenopis pushed a commit to kubernetes/website that referenced this pull request Jan 26, 2017
Fetching images from a registry hosted in a different region has been supported since 1.3 with
kubernetes/kubernetes#24369
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jan 8, 2020
…iserver-throttling-4.3

Bug 1788621: Improve logging of kubelet-apiserver logging

Origin-commit: 02753713077e72421e6076edc4c32e90312f9660
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants