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

feat: CloudFormation Service Role support #330

Merged
merged 1 commit into from Dec 19, 2018

Conversation

@mumoshu
Copy link
Collaborator

commented Nov 30, 2018

This depends on/includes changes made in #328, to avoid obvious conflicts.

Please see only the last commit for effective reviews.


In case your role is too restrictive that eksctl create cluster fails due
to cloudformation reporting insufficient permissions, specify a service
role used by CloudFormation to call AWS API while provisioning stacks on
your behalf.

arn:aws:iam:YOUR_AWS_ACCOUNT_ID:role/eksctl

Also note that eksctl now helps you by printing the guidance like below on
permission errors:

"eksctl-ferocious-gopher-1543570938-cluster" 2018-11-30T01:42:21-08:00 [✖]
ensure that the iam role "arn:aws:iam::YOUR_AWS_ACCOUNT_ID:role/eksctl"
exists, it should also have a trust relationship like the below.

{
 "Version": "2012-10-17",
 "Statement": [
   {
     "Sid": "",
     "Effect": "Allow",
     "Principal": {
       "Service": "cloudformation.amazonaws.com"
     },
     "Action": "sts:AssumeRole"
   }
 ]
}

Resolves #329

@mumoshu mumoshu force-pushed the mumoshu:cfn-svc-role branch 2 times, most recently from 35364b9 to 2f72525 Nov 30, 2018

@@ -54,6 +57,7 @@ func NewStackCollection(provider api.ClusterProvider, spec *api.ClusterConfig) *
}
logger.Debug("tags = %#v", tags)
return &StackCollection{
cfnSvcRoleArn: provider.CloudFormationRoleARN(),

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Dec 7, 2018

Member

Seems redundant, as we already have provider and can call the CloudFormationRoleARN on it to obtain the role ARN.

This comment has been minimized.

Copy link
@mumoshu

mumoshu Dec 7, 2018

Author Collaborator

Ah, good catch! Addressed in 85f6c47

return errors.Wrapf(err, "creating CloudFormation stack %q", *i.StackName)
}
logger.Debug("stack = %#v", s)
i.StackId = s.StackId
return nil
}

func logGuidanceOnAssumeRoleFailure(err error, roleArn *string) {

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Dec 7, 2018

Member

This is an advanced use case, I would expect the user to understand very well what they are doing. I don't see much need for this helper, would you mind to omit it for now before we hear from users?

This comment has been minimized.

Copy link
@mumoshu

mumoshu Dec 7, 2018

Author Collaborator

Completely understood. Removed in 7e45c81

@@ -44,11 +44,16 @@ type ProviderServices struct {
eks eksiface.EKSAPI
ec2 ec2iface.EC2API
sts stsiface.STSAPI

cfnRoleArn string

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Dec 7, 2018

Member

s/Arn/ARN/

This comment has been minimized.

Copy link
@mumoshu

mumoshu Dec 7, 2018

Author Collaborator

Turned out this was also redundant. Removed in 1fcf70d

@@ -262,6 +298,7 @@ func (c *StackCollection) doCreateChangeSetRequest(i *Stack, action string, desc
StackName: i.StackName,
ChangeSetName: &changeSetName,
Description: &description,
RoleARN: aws.String(c.cfnSvcRoleArn),

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Dec 7, 2018

Member

I'd just use &c.cfnSvcRoleArn here, which matches the way it's done above.

This comment has been minimized.

Copy link
@mumoshu

mumoshu Dec 7, 2018

Author Collaborator

Agreed! But I ended up removing this line due to the redundancy 0bea25c

@errordeveloper

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

I've added a few style-related comments, happy to address those myself in this PR, or via a follow-up PR. If you can add functionality for stack updates and deletions, that would be great :)

@mumoshu

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 7, 2018

Thanks for reviewing! The support for stack updates was already there, but I fixed a bug in it. Also, I've added the support for deletion b573000

@mumoshu

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 7, 2018

This should be good to go now.

@errordeveloper

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

@mumoshu I am happy to merge this, but will need to rebase and probably squashing the commits would make sense also. Do you mind I just use your branch to do this? I'll keep you as the author, of course.

@errordeveloper

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

It fits quite nicely into a one simple commit: 1ab84c2.

@mumoshu

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 18, 2018

feat: CloudFormation Service Role support
In case your role is too restrictive that `eksctl create cluster` fails due to cloudformation reporting insufficient permissions, specify a service role used by CloudFormation to call AWS API while provisioning stacks on your behalf.

```
eksctl create cluster --cfn-role-arn arn:aws:iam:YOUR_AWS_ACCOUNT_ID:role/eksctl
```

Resolves #329

@errordeveloper errordeveloper force-pushed the mumoshu:cfn-svc-role branch from d59f3df to c9a10f0 Dec 19, 2018

@errordeveloper errordeveloper merged commit 894daf8 into weaveworks:master Dec 19, 2018

2 checks passed

WIP Ready for review
Details
ci/circleci: make-eksctl-image Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.