More flexible configuration for IAM and stable naming for roles #607

Fsero · 2017-04-27T20:30:01Z

this PR aims to fix issue #500 allowing Node Pools to use an existing instance profile. It also includes a brief improvement for IAM Roles managed by kube-aws supporting customer managed policies (ref #340), it also makes IAM Roles fixed and not depending on the stack name if you have configured managedIAMRoleName.

If kube-aws manages the IAM Role it will create a customer managed policy for controllers and for workers, then it will create an IAM Role with this policy attached, that enables policy updates without the need to recreating the whole stack and kube-aws users can tailor the policies to their current needs.

if kube-aws uses an existing InstanceProfile it will not create an IAM Role and the InstanceProfile used should be attached to an IAM Role with the enough level of grants to make the cluster work.

By default if you don't configure an iam block in your worker/controller kube-aws would manage the role for you.

if you set iam.managedIamRoleName, the IAM Role will be created with the name {AWS::Region}-$managedIamRoleName.

if you set iam.existingInstanceProfile, kube-aws will use that instance profile.

if you set both, kube-aws would fail as these options are incompatible.

#  #iam:
#       # This will create a named IAM Role managed by kube-aws with the name {AWS::Region}-$managedIamRoleName.
#       # It will have attached a customer Managed Policy that you can modify afterwards if you need more permissions for your cluster.
#       # Be careful with the Statements you modify because an update could overwrite your own.
#       # the Statements included in the ManagedPolicy are the minimun ones required for the Controllers to run.
#       # managedIamRoleName: "yourManagedRole"
#       # ATTENTION: if you decide to use an existing InstanceProfile you need to ensure it has the needed permissions for kube-aws to run.
#       # kube-aws will not manage and never manage this InstanceProfile. IAM Roles will not be created by kube-aws in this case.
#       # existingInstanceProfile: "arn:aws:iam::YOURACCOUNTID:instance-profile/INSTANCEPROFILEID"

probably we can expand this to define custom iam policies in cluster.yaml, i think this model brings flexibility without confusing cluster.yaml more, but i'm happy to hear your thoughts.

i will be very glad if this is merged, @jpb (this is related to your #556 work), @c-knowles this is implementing customer managed policies.

k8s-ci-robot · 2017-04-27T20:30:06Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

codecov-io · 2017-04-27T20:39:10Z

Codecov Report

Merging #607 into master will decrease coverage by 0.28%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #607      +/-   ##
=========================================
- Coverage   37.99%   37.7%   -0.29%     
=========================================
  Files          50      51       +1     
  Lines        3277    3302      +25     
=========================================
  Hits         1245    1245              
- Misses       1830    1854      +24     
- Partials      202     203       +1

Impacted Files	Coverage Δ
core/controlplane/cluster/cluster.go	`43.33% <ø> (ø)`	⬆️
core/controlplane/config/stack_config.go	`71.42% <ø> (ø)`	⬆️
model/iamconfig.go	`0% <0%> (ø)`
model/node_pool_config.go	`21.21% <0%> (-2.13%)`	⬇️
core/controlplane/config/config.go	`55.77% <0%> (-0.2%)`	⬇️
model/controller.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8de79d4...4988aa5. Read the comment docs.

mumoshu · 2017-04-28T01:58:36Z

core/controlplane/config/templates/cluster.yaml

+#       # managedIamRoleName: "yourManagedRole"
+#       # ATTENTION: if you decide to use an existing InstanceProfile you need to ensure it has the needed permissions for kube-aws to run.
+#       # kube-aws will not manage and never manage this InstanceProfile. IAM Roles will not be created by kube-aws in this case.
+#       # existingInstanceProfile: "arn:aws:iam::YOURACCOUNTID:instance-profile/INSTANCEPROFILEID"


I guess iam should be iamInstanceProfile so that it is even more clearer that it is used to customize IamInstanceProfile associated to nodes.

If we go that way, iam.existingInstanceProfile could be just iamInstanceProfile.arn. It will correspond more to e.g. subnets[].id or apiEndpoints[].loadBalancer.id which is there for reusing existing AWS resources.

managedIamRoleName changes an IAM Role not the attached IAM Instance Profile, so renaming it to iamInstanceProfile.roleName might be confusing to people, but i don't have hard feelings about it if that key name looks more appealing, i will rename it.

@Fsero I think so too. Then, how about iamInstanceProfile.role.name?
My idea is that it will be explicit about:

iamInstanceProfile is used to customize what the instance profile is attached to nodes

iamInstanceProfile.role is used to customize what the role is associated to the instance profile

iamInstanceProfile.role.name is used to customize the name of the role associated to the instance profile

// I neither have hard feelings but just thought a bit of discussions would result in less explanations required to our users afterwards!

See my comment below.
I like your idea of iam.instanceProfile.

mumoshu · 2017-04-28T02:01:33Z

core/controlplane/config/templates/cluster.yaml

+#       # It will have attached a customer Managed Policy that you can modify afterwards if you need more permissions for your cluster.
+#       # Be careful with the Statements you modify because an update could overwrite your own.
+#       # the Statements included in the ManagedPolicy are the minimun ones required for the Controllers to run.
+#       # managedIamRoleName: "yourManagedRole"


If we go this way, iam.managedIamRoleName could be iamInstanceProfile.roleName which will correspond more to the RoleName key in Cfn's IAMRole resources.

See my comment above. I'm now suggesting iamInstanceProfile.role.name

mumoshu · 2017-04-28T02:08:14Z

core/controlplane/config/templates/stack-template.json

+        {{if .Controller.IAMConfig.ManagedIamRoleName }}
+        "RoleName":  {"Fn::Join": ["",[{"Ref": "AWS::Region"},"-","{{.Controller.IAMConfig.ManagedIamRoleName}}"]]},
+        {{end}}
+        "ManagedPolicyArns": [ {"Ref": "IAMManagedPolicyController"} ]


Not sure it should be addressed in this PR or not but..
I'd appreciate it if I could have iamInstanceProfile.managedPolicyArns to add "additional managed policy arns" to the ManagedPolicyArns array 😄
Please see #340
cc @c-knowles

And I just realized that you addressed it in your description of the PR

It also includes a brief improvement for IAM Roles managed by kube-aws supporting customer managed policies (ref #340)

However it doesn't seem to be implemented yet?

i can change the config block, so instead of being

iam: managedIamRoleName existingInstanceProfile

could be:

iam: role: iamRoleName: "your role" managedPolicyArns: - "arn:aws:iam:YOURACCOUNTID/policy/POLICYID" instanceProfile: arn: "arn:aws:iam::YOURACCOUNTID:instance-profile/INSTANCEPROFILEID"

that would be better @mumoshu ?

Regarding #340 i now realize is half implemented yes, if we agree on that syntax i can adapt the PR.

Thanks for your comments @mumoshu 😃

Sounds great. I'm almost convinced to your idea!

Nit but:

Would you mind renaming iamRoleName to just name so that the whole path would look like iam.role.name rather than iam.role.iamRoleName, which results in less duplication?

Which of them do you think clearer, iam.instanceProfile.role or iam.role? My motivation to nest role under instanceProfile is based on the fact that the role is associated to the instance profile. However I'd really like to hear your idea.

i don't mind at all, i like iam.role.name. Regarding your second point,

between

iam: role: name: "yourRoleName" managedPolicyArns: - "arn:aws:iam:YOURACCOUNTID/policy/POLICYID" instanceProfile: arn: "arn:aws:iam::YOURACCOUNTID:instance-profile/INSTANCEPROFILEID"

and

iam: instanceProfile: role: name: "yourRoleName" managedPolicyArns: - "arn:aws:iam:YOURACCOUNTID/policy/POLICYID" arn: "arn:aws:iam::YOURACCOUNTID:instance-profile/INSTANCEPROFILEID"

there is a link between the role and the instance profile i like the first slightly more because the option is more clear either you go with a role or either you set an instance profile but you cannot do both.

That said, both can work, so if you feel that the second makes more sense i'll do that.

Thanks @mumoshu 🙇‍♂️

@Fsero Thanks for the detailed reply 🙇

i like the first slightly more because the option is more clear either you go with a role or either you set an instance profile but you cannot do both.

It does make sense! Now, I like the first more, too. Let's go ahead with it 👍

mumoshu · 2017-04-28T02:14:26Z

@Fsero Thanks for your efforts here!

I did add some comments but

i think this model brings flexibility without confusing cluster.yaml more

Definitely.
Great job 👍

Fsero · 2017-04-29T10:38:02Z

@mumoshu i've implemented the proposed changes in syntax for cluster.yaml and included the features from #340.

cluster with existing instance profile

cluster with existing instance profile detail from workers

cluster with fixed role name and managed policies

cluster with fixed role name and managed policies detail from workers

i did some validation to prevent mistakes writing the cluster.yaml, i hope this is enough to got it merged :)

let me know wdyt @mumoshu

mumoshu · 2017-05-01T01:03:38Z

@Fsero Really an awesome work! 👍
LGTM overall and this will be merged after v0.9.6 is cut shortly.
I may add nits but they won't prevent this from merging 😄

mumoshu · 2017-05-01T01:04:43Z

model/node_pool_config.go

@@ -99,6 +101,13 @@ func (c NodePoolConfig) Valid() error {
 		fmt.Println(`WARNING: instance types "t2.nano" and "t2.micro" are not recommended. See https://github.com/kubernetes-incubator/kube-aws/issues/258 for more information`)
 	}

+	if c.IAMConfig.InstanceProfile.Arn != "" && (c.IAMConfig.Role.Name != "" || c.DeprecatedNodePoolManagedIamRoleName != "") {
+		return errors.New("Incompatible options either kube-aws attachs instances to an existing InstanceProfile or manages an IAM Role")


nit:`s/attachs/attaches/

nit: "incompatible options: kube-aws either attaches ~~ or manages ~~" may be easier to read/consistent with other error messages?

fixed, setting up an error message more similar to others.

mumoshu · 2017-05-01T01:13:19Z

core/controlplane/config/templates/stack-template.json

        "IamInstanceProfile": {
-          "Ref": "IAMInstanceProfileController"
+          "Ref": "IAMInstanceProfileController" 


nit: redundant whitespace at the end of line?

fixed, yep it was extra

mumoshu · 2017-05-01T14:16:17Z

model/iamconfig.go

+			return fmt.Errorf("invalid instance profile, your instance profile must match (=arn:aws:iam::YOURACCOUNTID:instance-profile/INSTANCEPROFILENAME), provided (%s)", c.InstanceProfile.Arn)
+		}
+
+	}


nit: We'd better emit a validation error also when both instanceProfile.arn and managePolicies[].arns are specified.

Seems like we already had it only for controller nodes above https://github.com/kubernetes-incubator/kube-aws/pull/607/files#diff-75f5c0a1828df862096ed45c755c2990R73
Perhaps we need it for node pools, too?

you are right, i'd forgot about it. Fixed now

mumoshu · 2017-05-01T14:26:16Z

Wow, thanks for the quick turnaround @Fsero!
LGTM and I'm now waiting for the travis build to finish.

mumoshu · 2017-05-01T14:30:21Z

@Fsero Merged. Sry if you've force-pushed one more commit after I've confirmed the build status and merged the PR. If you had further changes, I'd appreciate it if you could open up an another PR!

Thanks again for your efforts 🍻

Fsero · 2017-05-01T14:31:27Z

Thanks for your detailed review @mumoshu and for the efforts for making kube-aws great. See you in another PR 🍻

More flexible configuration for IAM and stable naming for roles

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 27, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 27, 2017

mumoshu reviewed Apr 28, 2017

View reviewed changes

Fsero force-pushed the issue/500 branch 2 times, most recently from e74ce61 to 2baab1f Compare April 29, 2017 10:25

Fsero force-pushed the issue/500 branch from 2baab1f to ad450de Compare April 29, 2017 10:40

mumoshu reviewed May 1, 2017

View reviewed changes

Fsero force-pushed the issue/500 branch from ad450de to c13c18d Compare May 1, 2017 08:07

mumoshu reviewed May 1, 2017

View reviewed changes

More flexible configuration for IAM and stable naming for roles

4988aa5

Fsero force-pushed the issue/500 branch from c13c18d to 4988aa5 Compare May 1, 2017 14:19

mumoshu merged commit 7db8629 into kubernetes-retired:master May 1, 2017

mumoshu added this to the v0.9.7-rc.1 milestone May 1, 2017

This was referenced May 9, 2017

Feature: Add custom IAM policies to worker node pools #340

Closed

Creating cluster with an existing IAM roles #53

Closed

cknowles mentioned this pull request Jun 9, 2017

Feedback on IAM role creation #698

Closed

mclarke47 mentioned this pull request Jul 6, 2017

Reusing existing IAM roles for etcd nodes and ASG lifecycle notification #747

Closed

kylehodgetts pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Mar 27, 2018

Merge pull request kubernetes-retired#607 from Fsero/issue/500

d37db55

More flexible configuration for IAM and stable naming for roles

Fsero deleted the issue/500 branch May 1, 2020 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More flexible configuration for IAM and stable naming for roles #607

More flexible configuration for IAM and stable naming for roles #607

Fsero commented Apr 27, 2017 •

edited

k8s-ci-robot commented Apr 27, 2017

codecov-io commented Apr 27, 2017 •

edited

mumoshu Apr 28, 2017

Fsero Apr 28, 2017

mumoshu Apr 28, 2017

mumoshu Apr 28, 2017 •

edited

mumoshu Apr 28, 2017

mumoshu Apr 28, 2017

mumoshu Apr 28, 2017

mumoshu Apr 28, 2017

Fsero Apr 28, 2017

mumoshu Apr 28, 2017 •

edited

Fsero Apr 28, 2017

mumoshu Apr 28, 2017

mumoshu commented Apr 28, 2017

Fsero commented Apr 29, 2017

mumoshu commented May 1, 2017

mumoshu May 1, 2017

mumoshu May 1, 2017

Fsero May 1, 2017

mumoshu May 1, 2017

Fsero May 1, 2017

mumoshu May 1, 2017

mumoshu May 1, 2017 •

edited

Fsero May 1, 2017

mumoshu commented May 1, 2017

mumoshu commented May 1, 2017

Fsero commented May 1, 2017

More flexible configuration for IAM and stable naming for roles #607

More flexible configuration for IAM and stable naming for roles #607

Conversation

Fsero commented Apr 27, 2017 • edited

k8s-ci-robot commented Apr 27, 2017

codecov-io commented Apr 27, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu Apr 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu Apr 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu commented Apr 28, 2017

Fsero commented Apr 29, 2017

mumoshu commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu May 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu commented May 1, 2017

mumoshu commented May 1, 2017

Fsero commented May 1, 2017

Fsero commented Apr 27, 2017 •

edited

codecov-io commented Apr 27, 2017 •

edited

mumoshu Apr 28, 2017 •

edited

mumoshu Apr 28, 2017 •

edited

mumoshu May 1, 2017 •

edited