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

Add IAM policy allowing authenticated pull from ECR #364

Merged
merged 1 commit into from
Aug 1, 2017
Merged

Add IAM policy allowing authenticated pull from ECR #364

merged 1 commit into from
Aug 1, 2017

Conversation

hobti01
Copy link
Contributor

@hobti01 hobti01 commented Jul 31, 2017

Add an IAM policy on the master and workers to allow image pulling from ECR as implemented in kubernetes/kubernetes#19447 and kubernetes/kubernetes#24369

As noted in kubernetes/kubernetes#19447 (comment) it might be possible to reduce the permission set, but this is the policy implemented upstream.

Copy link
Member

@puja108 puja108 left a comment

Choose a reason for hiding this comment

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

LGTM

@rossf7 @teemow do you guys see any problems with having this in as default?

@rossf7
Copy link
Contributor

rossf7 commented Aug 1, 2017

We may want to make this configurable for clusters that are not using ECR. But this can be done as a later change.

@rossf7 rossf7 self-requested a review August 1, 2017 08:02
Copy link
Contributor

@rossf7 rossf7 left a comment

Choose a reason for hiding this comment

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

LGTM too. I'll make sure this is included in my PR to split the master and worker policies.

@puja108
Copy link
Member

puja108 commented Aug 1, 2017

I'd say having it in without using ECR is not a problem, configuration would be rather if only certain clusters should have access to ECR and others not, but yeah, I'll create an issue for that for later.

@puja108 puja108 merged commit 98f8b98 into giantswarm:master Aug 1, 2017
@puja108
Copy link
Member

puja108 commented Aug 1, 2017

closes #363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants