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.

Sign up for GitHub

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

Jump to bottom

✨ Look up official EKS AMI when appropriate #1817

Merged
merged 1 commit into from
Sep 1, 2020
Merged

✨ Look up official EKS AMI when appropriate #1817

merged 1 commit into from
Sep 1, 2020

Conversation

rudoi
Copy link
Contributor

@rudoi rudoi commented Jul 13, 2020

What this PR does / why we need it:
This adds a lookup for the official EKS AMI.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1778

This is WIP until #1724 goes in. Will be using those types to determine if using the EKS AMI is appropriate. I'll also need some additional changes when #1810 goes in.

Also still working on finding the right IAM updates. I'm having an obnoxiously difficult time getting a policy statement that is properly scoped. Wildcards are not appropriate at all for SSM Parameter Store.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 13, 2020
@k8s-ci-robot k8s-ci-robot requested review from chuckha and justinsb July 13, 2020 23:45
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 13, 2020
@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch from 0b70aaf to 1548531 Compare July 13, 2020 23:47
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2020
@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch from 1548531 to 524aa97 Compare July 16, 2020 20:39
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 16, 2020
@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch 3 times, most recently from 898ef8a to b1de831 Compare July 17, 2020 00:51
@detiber detiber added this to the v0.5.x milestone Jul 27, 2020
detiber
detiber reviewed Jul 29, 2020
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

Changes look pretty sane to me. This doesn't introduce any permissions issues if someone is running this against a non-eks managed cluster but does not have ssm permissions, does it?

pkg/cloud/services/ec2/instances.go Outdated Show resolved Hide resolved
@rudoi
Copy link
Contributor Author

rudoi commented Aug 1, 2020

@detiber the SSM code path is only used when there's an AWSManagedControlPlane in play, so I don't expect any IAM errors when using a non-EKS cluster.

This PR needs #1724 anyway, so I think we hold this until it's in and I can test both code paths at once.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2020
@ncdc
Copy link
Contributor

ncdc commented Aug 5, 2020

/retitle [WIP] ✨ Look up official EKS AMI when appropriate

@k8s-ci-robot k8s-ci-robot changed the title WIP feat: look up official EKS AMI when appropriate [WIP] ✨ Look up official EKS AMI when appropriate Aug 5, 2020
@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch from b1de831 to b49475a Compare August 10, 2020 22:36
@rudoi rudoi mentioned this pull request Aug 13, 2020
Open
@randomvariable randomvariable modified the milestones: v0.5.x, v0.6.0 Aug 14, 2020
@randomvariable randomvariable added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Aug 14, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2020
randomvariable
randomvariable reviewed Aug 27, 2020
View reviewed changes
cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go Outdated
@@ -147,6 +147,15 @@ func (t Template) controllersPolicy() *iamv1.PolicyDocument {
"secretsmanager:TagResource",
},
},
{
Copy link
Member

Choose a reason for hiding this comment

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

After you've rebased, this should go behind the EKS flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any precedence for feature flags in clusterawsadm yet?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, misspoke. I meant the flag in terms of t.Spec.ClusterAPIControllers.EKS.Enable at https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/master/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go#L149 than a -- features thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@randomvariable
Copy link
Member

Looks fine other than the rebasing.

@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch from b49475a to 949bffb Compare August 28, 2020 21:51
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2020
@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch from 949bffb to 81502b0 Compare August 28, 2020 21:52
@rudoi rudoi changed the title [WIP] ✨ Look up official EKS AMI when appropriate ✨ Look up official EKS AMI when appropriate Aug 28, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2020
@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch from 81502b0 to 275d93e Compare August 28, 2020 21:54
@rudoi rudoi mentioned this pull request Aug 28, 2020
Closed
@randomvariable
Copy link
Member 

=== RUN   Test_RenderCloudformation
--- FAIL: Test_RenderCloudformation (0.04s)
    template_test.go:118: Differing output (default):
        AWSTemplateFormatVersion: 2010-09-09
        Resources:
          AWSIAMInstanceProfileControlPlane:
            Properties:
skipped 257 lines unfold_more
                    - ec2.amazonaws.com
                Version: 2012-10-17
              RoleName: nodes.cluster-api-provider-aws.sigs.k8s.io
            Type: AWS::IAM::Role
        
FAIL
FAIL	sigs.k8s.io/cluster-api-provider-aws/cmd/clusterawsadm/cloudformation/bootstrap	0.111s

@rudoi rudoi force-pushed the rudoi/eks-ami-lookup branch from 275d93e to 27c055d Compare August 31, 2020 15:03
@rudoi
Copy link
Contributor Author

rudoi commented Aug 31, 2020

Ok, fixed up those template tests.

@randomvariable
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2020
@randomvariable
Copy link
Member

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 8a79283 into kubernetes-sigs:master Sep 1, 2020
@luthermonson luthermonson mentioned this pull request Aug 11, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@randomvariable randomvariable randomvariable left review comments

@detiber detiber detiber left review comments

@chuckha chuckha Awaiting requested review from chuckha

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@randomvariable randomvariable

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.6.0
Development

Successfully merging this pull request may close these issues.

Support official EKS AMI lookup
5 participants
@rudoi @ncdc @randomvariable @k8s-ci-robot @detiber