Allow the re-use of a IAM role that is not created by kops. #2440

chrislovecnm · 2017-04-26T16:52:26Z

This is the original PR submitted by @sp-borja-juncosa with some tweaks.

renamed the API value to match the name of the AdditionalPolicies map.
added feature flag
removed CLI options

Again thanks @sp-borja-juncosa!! Closing this: #2139

TODO

Next Steps

I have various next steps documented here: #2440

When I get past the first two TODOs, this will be ready for merge.

This change is

justinsb · 2017-05-02T05:00:37Z

pkg/apis/kops/cluster.go

+	// Use an existing custom cloud security policy for the instances.  One example is to specify the name
+	// of an AWS IAM role for the master and another for the nodes.
+	// Map is keyed by: master, node
+	CustomPolicies *map[string]string `json:"customPolicies,omitempty"`


No reason to have a pointer to a map, I believe; just use a map.

And should we just have a strongly typed object? i.e.

iam: master: override-master node: override-node

If a strongly typed object isn't right, we should use the InstanceGroups roles as the keys, either explicitly (map[Role]string) or just in comments / code.

I like a typed object

I was following the custom roles as an example, which probably should be a object as well

justinsb · 2017-05-02T05:02:29Z

pkg/apis/kops/v1alpha1/componentconfig.go

+	AdmissionControl      []string `json:"admissionControl,omitempty" flag:"admission-control"`
+	ServiceClusterIPRange string   `json:"serviceClusterIPRange,omitempty" flag:"service-cluster-ip-range"`
+
+	// TODO: Remove unused BasicAuthFile


Is there a reason to add these comments in this PR?

justinsb · 2017-05-02T05:03:17Z

pkg/apis/kops/v1alpha1/componentconfig.go

 	VSphereResourcePool  *string `json:"vSphereResourcePool,omitempty"`
+	VSphereDatastore     *string `json:"vSphereDatastore,omitempty"`


Any reason to move this?

justinsb · 2017-05-02T05:06:21Z

pkg/model/iam.go

@@ -64,33 +68,63 @@ func (b *IAMModelBuilder) Build(c *fi.ModelBuilderContext) error {
 	// Generate IAM objects etc for each role
 	for _, role := range roles {
 		name := b.IAMName(role)
+		roleAsString := reflect.ValueOf(role).String()


Is this string(role) ?

justinsb · 2017-05-02T05:07:54Z

pkg/model/iam.go

+
+		arn := ""
+
+		if b.Cluster.Spec.CustomPolicies != nil && featureflag.CustomPoliciesSupport.Enabled() {


If the feature flag isn't set, this should be an error, not silently ignored. Probably in validation.

This may not need a feature flag.

justinsb · 2017-05-02T05:11:28Z

pkg/model/iam.go

+				Name: &roleName,
+				ID:   &arn,
+
+				// We set Policy Document to nil as this role will be managed externally


I thought this deleted the role?

More context please

justinsb · 2017-05-02T05:13:00Z

upup/pkg/fi/cloudup/awstasks/iaminstanceprofilerole.go

@@ -87,6 +82,9 @@ func (s *IAMInstanceProfileRole) CheckChanges(a, e, changes *IAMInstanceProfileR
 		if e.InstanceProfile == nil {
 			return fi.RequiredField("InstanceProfile")
 		}
+		if a.Role != e.Role {


I think you need to check the ID here, not reference equality.

justinsb · 2017-05-02T05:13:24Z

upup/pkg/fi/cloudup/awstasks/iaminstanceprofilerole.go

@@ -60,9 +58,6 @@ func (e *IAMInstanceProfileRole) Find(c *fi.Context) (*IAMInstanceProfileRole, e

 	ip := response.InstanceProfile
 	for _, role := range ip.Roles {
-		if aws.StringValue(role.RoleId) != roleID {


Not sure why we removed this?

justinsb · 2017-05-02T05:14:14Z

tests/integration/create_cluster/custom_iam_role/options.yaml

+- us-test-1a
+Cloud: aws
+KubernetesVersion: v1.4.8
+MasterIAMRole: foo


Are MasterIAMRole / NodeIAMRole mapped somewhere?

justinsb · 2017-05-02T05:17:28Z

cmd/kops/create_cluster_integration_test.go

@@ -38,6 +39,13 @@ func TestCreateClusterMinimal(t *testing.T) {
 	runCreateClusterIntegrationTest(t, "../../tests/integration/create_cluster/minimal", "v1alpha2")
 }

+// TODO: https://github.com/kubernetes/kops/issues/2438


Answered how to set a FF. Though a FF might not even be necessary here, particularly if we have validation.

jordanjennings · 2017-05-02T15:14:36Z

docs/iam_roles.md

+
+```yaml
+  customPolicies:
+    node: "arn:aws:iam::123456789012:role/kops-node"


FYI - looks like the code is only matching "Node" and "Master" (first letter capitalized)

I am going to make it an object

zyonash · 2017-05-03T15:05:29Z

@chrislovecnm @justinsb Hey guys - we're trying to use this feature to set up our cluster using pre-made IAM roles. We have very limited access to create roles, so this feature seems to fit our needs. However, although it seems to picking up on the roles that our administrator created for us, the cluster creation is still trying to edit the inline policies that we had set, which is failing due to the fact that we have no control at all over IAM resources.

Any ideas? For reference, here's exactly what we're seeing:

W0502 21:03:57.681592 506 executor.go:109] error running task "IAMRolePolicy/nodes.k8s.fscom.clouddev.thermofisher.net" (9m59s remaining to succeed): error creating/updating IAMRolePolicy: AccessDenied: User: arn:aws:sts::066574023230:assumed-role/TFAccountAutomationApp/i-042417434d053d0cb is not authorized to perform: iam:PutRolePolicy on resource: role nodes.k8s.fscom.clouddev.thermofisher.net status code: 403, request id: dbb5ce8d-2f7a-11e7-a3f1-d760f5827906 W0502 21:03:57.681623 506 executor.go:109] error running task "IAMRolePolicy/masters.k8s.fscom.clouddev.thermofisher.net" (9m59s remaining to succeed): error creating/updating IAMRolePolicy: AccessDenied: User: arn:aws:sts::066574023230:assumed-role/TFAccountAutomationApp/i-042417434d053d0cb is not authorized to perform: iam:PutRolePolicy on resource: role masters.k8s.fscom.clouddev.thermofisher.net status code: 403, request id: dbb495b9-2f7a-11e7-bc1a-2b7ba7392c49

chrislovecnm · 2017-05-05T20:37:17Z

@zyonash take a look at #2497 - the admin policy has the needed perms.

Vince-Cercury · 2017-06-26T04:06:56Z

Hi what is the status for this? It seems there are multiple PR such as this one #2139

Which one to track? Is that already released in its simplest form?
Our company has a different department for managing roles.

chrislovecnm · 2017-06-26T14:51:22Z

#2139 was closed, and this will hopefully be merged

justinsb · 2017-07-16T01:36:10Z

Blocked on #2763

StevenACoffman · 2017-07-21T17:01:24Z

#2763 was merged, so this is unblocked, and just needs the merge conflict resolved, right?

k8s-github-robot · 2017-07-21T17:01:51Z

@chrislovecnm PR needs rebase

k8s-github-robot · 2017-08-11T11:40:03Z

@chrislovecnm PR needs rebase

chrislovecnm · 2017-08-15T20:33:18Z

@justinsb thanks for the response. I do not understand how your different use cases would allow a user to reuse an IAM instance profile. The kops user IAM perms would not have the capability to make any modifications to IAM. You mention modifying the role in multiple parts of your use case.

Second question 'adding' permissions. IAM roles are very complex with the use of wild cards. How can we tell that we are missing a permission? Parsing the IAM profile information is quite challenging. Maybe we can use the IAM test API, but I am not certain.

The need that we have is that we cannot do any CRUD with IAM. If the name is different we cannot delete the profile. If the permissions are different we cannot update the role. We need to reuse an existing profile instance. How does your design satisfy that need?

justinsb · 2017-09-15T17:09:12Z

So as discussed in office hours today, a plan for getting this merged (not necessarily in order):

a) ability to change the names of the IAM resources that kops creates (and kops still creates them here)
b) look at the phases behaviour for the IAM phase and determine what changes are needed. The idea being that you can run kops update cluster --phases cluster and you'll be warned/errored if the IAM policies are wrong, but kops won't create them. Hopefully there's not a huge delta here.
c) changes to the IAM policies are separate, I believe (and happening already!)

chrislovecnm · 2017-09-15T18:11:46Z

Can we make

a) ability to change the names of the IAM resources that kops creates (and kops still creates them here)
a - 1.1 change the name and have kops not create or delete the policy?

This way Cluster IAM roles can be managed externally, either manually, using cloudformation or any other tool.

k8s-github-robot · 2017-10-04T08:01:19Z

@chrislovecnm PR needs rebase

rifelpet · 2017-12-14T19:20:03Z

Just curious what needs to be done to move this forward? My use case is identical to @dcowden's, we need roles and policies managed externally not by Kops and to instead pass the instance profile ARNs to the cluster spec. I can offer to help if its a time/resource issue.

chrislovecnm · 2017-12-14T21:56:20Z

@rifelpet would welcome the help! I am still working on security groups and file assets.

We have decided to modify the api from how it was implemented here. There is another PR open for the new api. Do you want to chat?

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

This was referenced Apr 26, 2017

E2E testing for customer roles #2441

Closed

Add feature: Custom IAM Roles #2139

Closed

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch 2 times, most recently from 3192dcd to 4937329 Compare April 27, 2017 01:48

justinsb reviewed May 2, 2017

View reviewed changes

justinsb self-assigned this May 2, 2017

jordanjennings reviewed May 2, 2017

View reviewed changes

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch 3 times, most recently from 017a3d0 to 631db08 Compare May 17, 2017 17:21

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from 631db08 to f4dfcc7 Compare May 18, 2017 18:34

chrislovecnm added the WIP: Work In Progress label May 18, 2017

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch 3 times, most recently from cf367a7 to 7f651ed Compare June 15, 2017 23:45

chrislovecnm removed the WIP: Work In Progress label Jun 16, 2017

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from 7f651ed to a471b93 Compare June 16, 2017 20:42

egeland mentioned this pull request Jun 30, 2017

Feature request: Option to use existing IAM Role in AWS #1166

Closed

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from a471b93 to a4d34cf Compare July 12, 2017 18:28

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2017

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from a4d34cf to 116b029 Compare July 21, 2017 17:21

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2017

chrislovecnm mentioned this pull request Sep 4, 2017

Security improvements on IAM policies #2497

Closed

8 tasks

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2017

justinsb modified the milestones: 1.7.1, 1.8.0 Sep 23, 2017

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from f543698 to 9548cf4 Compare October 1, 2017 07:02

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 1, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2017

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from 63a1f48 to e88c6f5 Compare October 1, 2017 08:27

chrislovecnm added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2017

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from e88c6f5 to bb9182a Compare October 1, 2017 08:35

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 Oct 1, 2017

chrislovecnm added the WIP: Work In Progress label Oct 1, 2017

Add feature: Custom IAM Instance Profile

7d4329a

This way Cluster IAM roles can be managed externally, either manually, using cloudformation or any other tool.

chrislovecnm force-pushed the custom-iam-role-sp-borja-juncosa branch from bb9182a to 7d4329a Compare October 3, 2017 05:48

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2017

justinsb added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed WIP: Work In Progress labels Nov 4, 2017

justinsb modified the milestones: 1.8.0, backlog Nov 22, 2017

rifelpet mentioned this pull request Dec 29, 2017

Add support for external IAM Instance Profiles #4171

Merged

chrislovecnm closed this Dec 30, 2017

chrislovecnm deleted the custom-iam-role-sp-borja-juncosa branch December 30, 2017 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the re-use of a IAM role that is not created by kops. #2440

Allow the re-use of a IAM role that is not created by kops. #2440

chrislovecnm commented Apr 26, 2017 •

edited

Loading

justinsb May 2, 2017 •

edited

Loading

justinsb May 2, 2017

chrislovecnm May 5, 2017

chrislovecnm May 5, 2017

justinsb May 2, 2017

chrislovecnm May 5, 2017

justinsb May 2, 2017

chrislovecnm May 5, 2017

justinsb May 2, 2017

justinsb May 2, 2017

justinsb May 2, 2017

chrislovecnm May 5, 2017

justinsb May 2, 2017

chrislovecnm May 5, 2017

chrislovecnm May 11, 2017

justinsb May 2, 2017

justinsb May 2, 2017

justinsb May 2, 2017

jordanjennings May 2, 2017

chrislovecnm May 5, 2017

zyonash commented May 3, 2017

chrislovecnm commented May 5, 2017

Vince-Cercury commented Jun 26, 2017

chrislovecnm commented Jun 26, 2017

justinsb commented Jul 16, 2017

StevenACoffman commented Jul 21, 2017 •

edited

Loading

k8s-github-robot commented Jul 21, 2017

k8s-github-robot commented Aug 11, 2017

chrislovecnm commented Aug 15, 2017

justinsb commented Sep 15, 2017

chrislovecnm commented Sep 15, 2017

k8s-github-robot commented Oct 4, 2017

rifelpet commented Dec 14, 2017

chrislovecnm commented Dec 14, 2017

		VSphereResourcePool *string `json:"vSphereResourcePool,omitempty"`
		VSphereDatastore *string `json:"vSphereDatastore,omitempty"`


		arn := ""

		if b.Cluster.Spec.CustomPolicies != nil && featureflag.CustomPoliciesSupport.Enabled() {

Allow the re-use of a IAM role that is not created by kops. #2440

Allow the re-use of a IAM role that is not created by kops. #2440

Conversation

chrislovecnm commented Apr 26, 2017 • edited Loading

justinsb May 2, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyonash commented May 3, 2017

chrislovecnm commented May 5, 2017

Vince-Cercury commented Jun 26, 2017

chrislovecnm commented Jun 26, 2017

justinsb commented Jul 16, 2017

StevenACoffman commented Jul 21, 2017 • edited Loading

k8s-github-robot commented Jul 21, 2017

k8s-github-robot commented Aug 11, 2017

chrislovecnm commented Aug 15, 2017

justinsb commented Sep 15, 2017

chrislovecnm commented Sep 15, 2017

k8s-github-robot commented Oct 4, 2017

rifelpet commented Dec 14, 2017

chrislovecnm commented Dec 14, 2017

chrislovecnm commented Apr 26, 2017 •

edited

Loading

justinsb May 2, 2017 •

edited

Loading

StevenACoffman commented Jul 21, 2017 •

edited

Loading