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

adding EKS Account ID for security measures when using for AMIs #733

Merged

Conversation

christopherhein
Copy link
Contributor

Signed-off-by: Christopher Hein [email protected]

Description

closes #732

This checks the right Account ID for AL2 AMIs so that they always come from the EKS team.

Checklist

  • Code compiles correctly (i.e make build)
  • All unit tests passing (i.e. make test)
  • All integration tests passing (i.e. make integration-test)

pkg/ami/api.go Outdated Show resolved Hide resolved
@errordeveloper
Copy link
Contributor

It would be also great to not spoil the coverage, this package has pretty good coverage and we should keep it that way. So please take a look if you can add a meaningful test for this!

@christopherhein
Copy link
Contributor Author

Once tests pass let me know what you think @errordeveloper

@christopherhein
Copy link
Contributor Author

@errordeveloper everything looks good now on this if you get a second to merge.

@martina-if
Copy link
Contributor

LGTM

@nrdlngr
Copy link

nrdlngr commented Apr 24, 2019

Any update on this?

Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

This is better, just a few somewhat cosmetic things please! :)

pkg/ami/api.go Outdated
input := &ec2.DescribeImagesInput{
Owners: []*string{aws.String(ImageFamilyToAccountID[imageFamily])},
Copy link
Contributor

Choose a reason for hiding this comment

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

This a little too optimistic, we should check if the key exists, so we don't panic if it doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the look-up into the caller, and the map declaration should live in the same file also:

https://github.com/weaveworks/eksctl/blob/3d20b573fcc5b495ba6729ba4affdc25a285639a/pkg/ami/auto_resolver.go#L50

pkg/ami/api.go Outdated Show resolved Hide resolved
pkg/ami/api.go Outdated Show resolved Hide resolved
@errordeveloper
Copy link
Contributor

We should be able to include this in the next release, but due to undergoing API changes, we cannot cut it before all of milestone issues are closed, as we are half-way-through.

@errordeveloper errordeveloper self-assigned this Apr 29, 2019
@errordeveloper
Copy link
Contributor

@christopherhein I'll have to finish this up myself, as we are looking to cut 0.1.30 today.

errordeveloper
errordeveloper previously approved these changes Apr 29, 2019
@errordeveloper errordeveloper merged commit c2f0728 into eksctl-io:master Apr 29, 2019
@christopherhein christopherhein deleted the feature/732-ami-id-finder branch April 29, 2019 16:46
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.

Contain AMI Search Based on EKS Account ID for AL2 images
4 participants