feat: CloudFormation Service Role support #330
Conversation
mumoshu
force-pushed
the
cfn-svc-role
branch
2 times, most recently
from
35364b9 to
2f72525
Compare
pkg/cfn/manager/api.go
Outdated
| @@ -54,6 +57,7 @@ func NewStackCollection(provider api.ClusterProvider, spec *api.ClusterConfig) * | |||
| } | |||
| logger.Debug("tags = %#v", tags) | |||
| return &StackCollection{ | |||
| cfnSvcRoleArn: provider.CloudFormationRoleARN(), | |||
There was a problem hiding this comment.
Seems redundant, as we already have provider and can call the CloudFormationRoleARN on it to obtain the role ARN.
There was a problem hiding this comment.
Ah, good catch! Addressed in 85f6c47
pkg/cfn/manager/api.go
Outdated
| 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Completely understood. Removed in 7e45c81
pkg/eks/api.go
Outdated
| @@ -44,11 +44,16 @@ type ProviderServices struct { | |||
| eks eksiface.EKSAPI | |||
| ec2 ec2iface.EC2API | |||
| sts stsiface.STSAPI | |||
|
|
|||
| cfnRoleArn string | |||
There was a problem hiding this comment.
Turned out this was also redundant. Removed in 1fcf70d
pkg/cfn/manager/api.go
Outdated
| @@ -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), | |||
There was a problem hiding this comment.
I'd just use &c.cfnSvcRoleArn here, which matches the way it's done above.
There was a problem hiding this comment.
Agreed! But I ended up removing this line due to the redundancy 0bea25c
|
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 :) |
|
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 |
|
This should be good to go now. |
|
@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. |
|
It fits quite nicely into a one simple commit: 1ab84c2. |
|
That would be great. Thanks :)
2018年12月19日(水) 4:21 Ilya Dmitrichenko <notifications@github.com>:
… It fits quite nicely into a one simple commit: 1ab84c2
<1ab84c2>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#330 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABV-fTT6RYNmM8V5WGuikHeezu4RCALks5u6UAcgaJpZM4Y7Wqi>
.
|
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 eksctl-io#329
errordeveloper
force-pushed
the
cfn-svc-role
branch
from
d59f3df to
c9a10f0
Compare
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 clusterfails dueto cloudformation reporting insufficient permissions, specify a service
role used by CloudFormation to call AWS API while provisioning stacks on
your behalf.
Also note that eksctl now helps you by printing the guidance like below on
permission errors:
Resolves #329