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

Add External Policies (AWS managed policy attachments) #7837

Merged
merged 1 commit into from Feb 17, 2020

Conversation

@mattouille
Copy link
Contributor

mattouille commented Oct 28, 2019

This feature allows Managed Policies to be attached to Instance Group Roles. It will manage the full lifecycle of all Managed Policies attached to master, node, and bastion by first adding new managed policies followed by removing unspecified policies (policies not present in the config) in order to avoid service disruption.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 28, 2019

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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.


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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 28, 2019

Welcome @mattouille!

It looks like this is your first PR to kubernetes/kops 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kops has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 28, 2019

Hi @mattouille. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Oct 28, 2019

I signed it

@chrisz100

This comment has been minimized.

Copy link
Member

chrisz100 commented Nov 15, 2019

/ok-to-test

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Nov 25, 2019

I'm having a hard time figuring out how a call to AWS is resulting in a nil pointer dereference:

		response, err := cloud.IAM().ListAttachedRolePolicies(request)
		if awsErr, ok := err.(awserr.Error); ok {
			if awsErr.Code() == "NoSuchEntity" {
				return nil, nil
			}

			return nil, fmt.Errorf("error getting policies for role: %v", err)
		}

Any help would be appreciated!

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Nov 25, 2019

I did get my above question figured out. I didn't realize it was throwing a nil pointer dereference because of a missing mock method. That's all fixed now.

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Nov 26, 2019

/test pull-kops-e2e-kubernetes-aws

@rifelpet

This comment has been minimized.

Copy link
Member

rifelpet commented Dec 20, 2019

As mentioned on the call, this is the integration test that you could update to test this functionality:

https://github.com/kubernetes/kops/tree/master/tests/integration/update_cluster/complex

Add uses of the new field to the in-*.yaml files and the new tf resources to the .tf file.

I also noticed that RenderCloudFormation wasnt updated, if its simple enough we should add cloudformation support for this feature but if not we should at least document that its not supported in cloudformation.

@johngmyers

This comment has been minimized.

Copy link
Member

johngmyers commented Dec 23, 2019

/test pull-kops-verify-staticcheck

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Dec 26, 2019

As mentioned on the call, this is the integration test that you could update to test this functionality:

https://github.com/kubernetes/kops/tree/master/tests/integration/update_cluster/complex

Add uses of the new field to the in-*.yaml files and the new tf resources to the .tf file.

I also noticed that RenderCloudFormation wasnt updated, if its simple enough we should add cloudformation support for this feature but if not we should at least document that its not supported in cloudformation.

I will start working on those tests. Unfortunately I couldn't come up with a good way to do this in CloudFormation -- someone else might have a better idea of how to do it. I'll make it clear in the docs that this feature is not supported by CloudFormation.

@rifelpet

This comment has been minimized.

Copy link
Member

rifelpet commented Dec 27, 2019

Yeah I think cloudformation support might be a bit more involved. With terraform its just a different resource type (aws_iam_role_policy vs aws_iam_role_policy_attachment) but looking at the CloudFormation docs there isnt a dedicated resource for the attachment of a policy to a role, instead it is the ManagedPolicyArns property of the Role resource so we may need to modify the iamrole task instead.

type cloudformationIAMRole struct {
RoleName *string `json:"RoleName"`
AssumeRolePolicyDocument map[string]interface{}
}

but I'm not a big fan of having logic split over different tasks depending on the target :(

one short term option in addition to documenting the limitation would be to return an error in RenderCloudFormation if someone is trying to use managed policies with target = cloudformation

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Jan 3, 2020

@rifelpet I think I may be doing something wrong and require a nudge in the right direction.

I'm getting this error:
--- FAIL: TestComplex (5.62s) integration_test.go:284: error running update cluster "complex.example.com": error closing target: error writing terraform data to output: error formatting HCL: At 247:3: Unknown token: 247:3 IDENT role

Originally I thought this was because I made some HCLv2 references, but even since changing them back the error has been consistent.

The "role" token it's referring to is required under aws_iam_role_policy_attachment.

@mikesplain

This comment has been minimized.

Copy link
Member

mikesplain commented Jan 3, 2020

I wonder if this could be a formatting issue @mattouille, see terraform-aws-modules/terraform-aws-vpc#267

I don't see in my looking but figured I'd post here too.

- aws:arn:iam:123456789000:policy:test-policy
```

Managed Policy attachments are treated declaritively. Any policies declared will be attached to the role, any policies not specified will be detached _after_ new policies are attached.

This comment has been minimized.

Copy link
@justinsb

justinsb Jan 5, 2020

Member

I'm a little confused as to whether this replaces the built-in kops policies; I think the answer is "no", but I think a sentence that explicitly spells this out would be great.

pkg/apis/kops/cluster.go Outdated Show resolved Hide resolved
pkg/model/iam.go Outdated Show resolved Hide resolved
pkg/resources/aws/aws.go Outdated Show resolved Hide resolved
cloud := c.Cloud.(awsup.AWSCloud)

// Handle policy overrides
if e.PolicyOverrides != nil {

This comment has been minimized.

Copy link
@justinsb

justinsb Jan 5, 2020

Member

Maybe this should be a separate task (e.g. IAMRolePolicyAttachements) - or we should raise an error if we specify PolicyOverrides and PolicyDocument on the same task.

Creating a new task is a trickier thing normally, so I'm happy to do that separately...

This comment has been minimized.

Copy link
@mattouille

mattouille Feb 3, 2020

Author Contributor

I actually did try! I couldn't quite get it figured out.

return nil, fmt.Errorf("error getting policies for role: %v", err)
}

policies := make([]string, 0)

This comment has been minimized.

Copy link
@justinsb

justinsb Jan 5, 2020

Member

Nit: Normally it's fine just to do var policies []string - unless you're making a distinction between the empty list and nil (which is typically a bad idea, though I'm sure I've done it!)

@@ -121,6 +158,51 @@ func (_ *IAMRolePolicy) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *IAMRoleP
return fmt.Errorf("error rendering PolicyDocument: %v", err)
}

// Handles the full lifecycle of Policy Overrides
if e.Managed {

This comment has been minimized.

Copy link
@justinsb

justinsb Jan 5, 2020

Member

I guess Managed is more "ManageAttachments"?

This comment has been minimized.

Copy link
@mattouille

mattouille Feb 3, 2020

Author Contributor

I was using this so I didn't have to check if the slice was nil and then check it's length. It just looked cleaner.

upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go Outdated Show resolved Hide resolved
@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Jan 5, 2020

Some nits / thoughts - mostly about naming & structure. Figuring out the API field names are blockers because we can't change them, the task structure is internal so we can iterate on those and thus they aren't blockers.

So two blocking questions:

  • name of the field e.g. - externalPolicies
  • should we put this onto each IG - trade off making users repeat themselves for more power (policies per IG) and maybe simplicity (no need to think about roles, just directly specify it)

On the error, I ran make bazel-test (though go test should produce the same output) and we did log the invalid HCL. I scanned through the iam stuff and spotted this one:

E0105 16:16:49.191522   16052 hcl_printer.go:127] 251   
E0105 16:16:49.191532   16052 hcl_printer.go:127] 252   resource "aws_iam_role_policy_attachment" "node-policyoverride" {
E0105 16:16:49.191541   16052 hcl_printer.go:127] 253     name = 
E0105 16:16:49.191551   16052 hcl_printer.go:127] 254     role = "${aws_iam_role.nodes-complex-example-com.name}"
E0105 16:16:49.191560   16052 hcl_printer.go:127] 255     policy = 
E0105 16:16:49.191568   16052 hcl_printer.go:127] 256     policy_arn = "aws:arn:iam:123456789000:policy:test-policy"
E0105 16:16:49.191577   16052 hcl_printer.go:127] 257   }

Note the empty policy = line and name = line

By changing these to omitempty I was able to get the tests to almost pass (after running hack/update-expected.sh):

 type terraformIAMRolePolicy struct {
-       Name           *string            `json:"name"`
+       Name           *string            `json:"name,omitempty"`
        Role           *terraform.Literal `json:"role"`
-       PolicyDocument *terraform.Literal `json:"policy"`
+       PolicyDocument *terraform.Literal `json:"policy,omitempty"`
        PolicyArn      *string            `json:"policy_arn,omitempty"`
 }
 

It might be cleaner to have a separate terraformIAMRolePolicyAttachment struct rather than reusing the same structure - the idea of this being a separate task might actually make stuff simpler :-)

The tests almost passed because I guess that complex uses CloudFormation, and then I hit:

W0105 16:39:49.118646   17836 executor.go:128] error running task "IAMRolePolicy/master-policyoverride" (29s remaining to succeed): CloudFormation not supported for use with PolicyOverrides.
W0105 16:39:49.118670   17836 executor.go:128] error running task "IAMRolePolicy/node-policyoverride" (29s remaining to succeed): CloudFormation not supported for use with PolicyOverrides.

Two options there - we can split complex into complex and complex_cloudformation and just add it to complex, or we can figure out how to support it for CloudFormation also. I can take a look at the latter...

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Jan 5, 2020

Oh - we already have the same problem in the load balancer task, so I just copied the hack-around: https://github.com/justinsb/kops/pull/new/tests_for_7837

(In particular e90b40d )

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Jan 5, 2020

On the second "blocker" - I've changed my mind - I don't think we should define this per IG, because we'd then also need to split the profile name per IG, and we do have the ability to override the profile already, and the interactions would get complicated. So what you've got here is great, and we should just make sure we're happy with the name of the field in the API!

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Feb 3, 2020

Alright, so I've got some updates.

Naming
ExternalPolicies sounds better and is absolutely more accurate. AttachedPolicies is good once the policy has been attached however... in the code I also handle the policy before it's attached and there it's actually called a ManagedPolicy (think of it in terms of like user-managed policy from the AWS end) by AWS.

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/L labels Feb 3, 2020
@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Feb 3, 2020

/retest

@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Feb 3, 2020

/test pull-kops-e2e-kubernetes-aws

@mattouille mattouille force-pushed the mattouille:policy-overrides branch from 09dea2e to 90289a4 Feb 3, 2020
@mattouille mattouille force-pushed the mattouille:policy-overrides branch from 90289a4 to 28a9c55 Feb 3, 2020
@mattouille mattouille changed the title Add Policy Overrides (Managed Policies) Add External Policies (AWS managed policy attachments) Feb 3, 2020
@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Feb 4, 2020

/retest

1 similar comment
@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Feb 5, 2020

/retest

master:
- aws:arn:iam:123456789000:policy:test-policy
bastion:
- aws:arn:iam:123456789000:policy:test-policy

This comment has been minimized.

Copy link
@rifelpet

rifelpet Feb 5, 2020

Member

I dont see this policy arn referenced in the terraform output at all.

I'd expect to see something like:

resource "aws_iam_role_policy_attachment" "node-externalpolicy-test-policy" {
  role       = "${aws_iam_role.nodes-complex-example-com.name}"
  policy_arn = "aws:arn:iam:123456789000:policy:test-policy"
}

somewhere in kubernetes.tf below.

This comment has been minimized.

Copy link
@mattouille

mattouille Feb 6, 2020

Author Contributor

ah, I misunderstood how this worked. I'll fix that.

Role: e.Role.TerraformLink(),
PolicyArn: s(policy),
}
return t.RenderResource("aws_iam_role_policy_attachment", *e.Name, tf)

This comment has been minimized.

Copy link
@rifelpet

rifelpet Feb 5, 2020

Member

This might be related to the resource not showing up in kubernetes.tf, but I think we dont want to return here. We want to render both this aws_iam_role_policy_attachment and the aws_iam_role_policy resource below. I think we may also need to make the resource name (currently *e.Name unique so that multiple aws_iam_role_policy_attachment resources dont conflict. Perhaps the name can include a normalized version of the policy arn?

This comment has been minimized.

Copy link
@mattouille

mattouille Feb 6, 2020

Author Contributor

Let me know if this is working the way it should now. My understanding of how this function was called was wrong.

@rifelpet

This comment has been minimized.

Copy link
Member

rifelpet commented Feb 16, 2020

@mattouille can you squash these commits? I think thats the last blocker to getting this merged (and I remember we have a thread in slack explaining how to do that)

@mattouille mattouille force-pushed the mattouille:policy-overrides branch from 02708c2 to 157f04c Feb 17, 2020
@mattouille

This comment has been minimized.

Copy link
Contributor Author

mattouille commented Feb 17, 2020

@mattouille can you squash these commits? I think thats the last blocker to getting this merged (and I remember we have a thread in slack explaining how to do that)

Done! Sorry for the delay.

@mattouille mattouille force-pushed the mattouille:policy-overrides branch from 157f04c to f025ff0 Feb 17, 2020
@rifelpet

This comment has been minimized.

Copy link
Member

rifelpet commented Feb 17, 2020

Perfect, thanks!
/approve
/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 17, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattouille, rifelpet

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 merged commit b6c849c into kubernetes:master Feb 17, 2020
18 of 19 checks passed
18 of 19 checks passed
tide Not mergeable. Needs approved, lgtm labels.
Details
Header rules No header rules processed
Details
Pages changed 2 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
cla/linuxfoundation mattouille authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
pull-kops-bazel-build Job succeeded.
Details
pull-kops-bazel-test Job succeeded.
Details
pull-kops-e2e-kubernetes-aws Job succeeded.
Details
pull-kops-verify-bazel Job succeeded.
Details
pull-kops-verify-boilerplate Job succeeded.
Details
pull-kops-verify-generated Job succeeded.
Details
pull-kops-verify-gofmt Job succeeded.
Details
pull-kops-verify-gomod Job succeeded.
Details
pull-kops-verify-govet Job succeeded.
Details
pull-kops-verify-packages Job succeeded.
Details
pull-kops-verify-staticcheck Job succeeded.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.