-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix AWS ECR private repository usage #858
Conversation
Here is a job that uses this solution successfully:
|
I am not sure I understand the CI failure here. The client driver tests pass on my local machine. |
@ErikEvenson That test has become flakey recently, don't worry about that one. |
Great -- I'll take that as good news. |
driverConfig.SSL = true | ||
driverConfig.ImageName = strings.Replace(driverConfig.ImageName, "https://", "", 1) | ||
} else { | ||
driverConfig.SSL = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to set this as false as default value for bools are false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed -- will change.
@ErikEvenson Thanks for taking a stab at this! I left a few comments, I hope after a few iterations we can get this merged and get it out for 0.3.1 which is going to come out in a few days. |
I incorporated/answered your comments @diptanu and brought the PR up to date with master. Cheers. |
@ErikEvenson I just had one last comment about introducing an |
@diptanu great -- I'll try to do this later today. |
I have brought this PR up to date with master and made the changes @diptanu requested regarding the Init() function. Note: master is not passing the |
Fix AWS ECR private repository usage
@ErikEvenson out of curiosity, how does your Nomad cluster authenticate with ECR? My understanding is ECR requires a call to IAM for GetAuthorizationToken, then invoke a docker login. However, this token is only valid for 12 hours. Are you handling this token out of band on your cluster? |
@steve-jansen typically, before I run a nomad job, I have the cluster authenticate itself via a consul command. So something like this:
|
@ErikEvenson Oh I see. Can we improve this? I would like this to be a no-op and Nomad handling this for the user. |
@diptanu I'd love to see this being handled by nomad end-to-end, but I'm not sure how to have nomad do this for the general user. Everyone is going to have a different opinion on how to handle AWS authentication. I use vault to populate the AWS credentials that the This is a general problem for secured, private repositories as well. Non-AWS repos will still have to do Thoughts? |
@pshima Any thoughts here? |
If you are running nomad on an EC2 host, you could give the EC2 instance a role that gives the instance full access to your ECR repos. Then, you wouldn't have to store or even log in to AWS ECR. |
Very interesting. Have you been able to bypass the need for |
@steve-jansen I haven't actually used this technique with nomad as it is a sort of lock-in to the AWS platform. I would like my nomad scripts to be just as useful on other cloud provider platforms. My expectation is that you would not need to do |
@ErikEvenson This is something we might be able to pull off by having the concept of image providers for different type of environments - so we could have a image provider which works well with AWS ECR and another with GCP for example. |
@diptanu Right, but you are always going to want to have the ability to, say, pull a docker image from AWS for use on a, say, Azure instance. |
I didn't test this but I don't believe it's accurate to say you don't need to use The point of the command is still to run a |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is my attempt to allow nomad to access AWS ECR private docker repositories. The problem is per the commit discussion between me and @diptanu here: c4fd236
This solution does require the user to add
https://
to the image name in the docker config. I am not sure if that is in line with nomad's design vision. Other declarative approaches are possible, but this does work for my AWS ECR usage.