Add support for well known policies with IRSA #3045

michaelbeaumont · 2021-01-07T14:52:25Z

Description

Fixes #2837
I've only added these 5 for now. I want to confirm the rest of the existing addonPolicies make sense with IRSA

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the userdocs directory)
Manually tested
Added labels for change area (e.g. area/nodegroup), target version (e.g. version/0.12.0) and kind (e.g. kind/improvement)
Make sure the title of the PR is a good description that can go into the release notes

aclevername · 2021-01-07T16:58:01Z

can an example yaml be added please 😄

TBBle · 2021-01-08T02:51:02Z

pkg/cfn/builder/iam_helper.go

+			customPolicy{Name: "PolicyCertManagerGetChange", Statements: certManagerGetChangeStatements()},
+		)
+		if wellKnownPolicies.ExternalDNS {
+			policies = append(policies, customPolicy{Name: "PolicyCertManagerHostedZones", Statements: certManagerHostedZonesStatements("route53:ListHostedZones", "route53:ListTagsForResource")})


This can't be hit, because we're in an else after if wellKnownPolicies.ExternalDNS above. (Which is a good reminder that I don't see any tests for this new feature yet...)

Yes, missed when going from handling elements of a list of policies to handling this struct.

I see this has been updated, but I think the logic has drifted slightly.

My understanding of the similar code in createRole is roughly:

If CertManager or ExternalDNS, changeSetStatements

If Certmanager, certManagerGetChangeStatements

If CertManager and ExternalDNS, certManagerHostedZonesStatements + a couple of things ExternalDNS needs that aren't in that

If CertManager but not ExternalDNS, certManagerHostedZonesStatements

If ExternalDNS but not CertManager, externalDNSHostedZonesStatements.

This logic is currently

If CertManager or ExternalDNS, changeSetStatements

If CertManager, certManagerGetChangeStatements

If CertManager and ExternalDNS, certManagerHostedZonesStatements + a couple of things ExternalDNS needs that aren't in that

If CertManager and not ExternalDNS, certManagerHostedZonesStatements

If ExternalDNS, externalDNSHostedZonesStatements <== Applied even if CertManager.

externalDNSHostedZonesStatements and certManagerHostedZonesStatements both contain route53:ListResourceRecordSets, and the "couple of things" above are the other two statements from externalDNSHostedZonesStatements. So externalDNSHostedZonesStatements is a strict subset of "certManagerHostedZonesStatements + a couple of things ExternalDNS needs that aren't in that", so the former should only if the latter has not been applied.

To be fair, I don't know why the effort is made to avoid the overlap, unless there's an actual failure on the AWS API when a policy contains multiple statements for the same Effect/Resource/Action tuple? It just makes it harder to read and reason about.

I've simplified that code now.

@TBBle please do take a look over the changes if you get a chance

TBBle · 2021-01-08T03:04:04Z

pkg/cfn/template/iam.go

@@ -25,7 +25,7 @@ type IAMRole struct {
 	Path string `json:",omitempty"`

 	AssumeRolePolicyDocument MapOfInterfaces `json:",omitempty"`
-	ManagedPolicyArns        []string        `json:",omitempty"`
+	ManagedPolicyArns        []interface{}   `json:",omitempty"`


I'm not clear why this is changed. Is it just because we're smuggling the return value from makePolicyARN out of (rs *IAMServiceAccountResourceSet) AddAllResources() inside this slice? Would it make sense to just stringify the value before adding it to ManagedPolicyArns instead?

(I'm not clear on what this gfn business is all about, so maybe this comment doesn't make sense.)

Yes it's kind of a less than ideal situation because we do some cloud formation using our own types and some using goformation. It's here that they're being mixed. makePolicyARN uses Cloudformation intrinsics, which aren't serialized as strings. Although in this case it's probably not strictly necessary to use the intrinsic.

I'm going to leave the intrinsics because we use them elsewhere (with makePolicyArns) (and interface{} is sort of how things are done in cfn/template right now)

aclevername · 2021-01-08T09:39:16Z

examples/13-iamserviceaccounts.yaml

+  - metadata:
+      name: aws-load-balancer-controller
+      namespace: kube-system
+    wellKnownPolicies:


would attachWellKnownPolicies be better? I'm still open for WellKnownPolicies to change, but I think having attach prefixed helps to describe the intent

I think it's a little odd that this is would be a verb, when the general idea of declarative config suggests using nouns instead, to describe the resulting state, not how you get there.

attachedWellKnownPolicies perhaps, but that's pretty verbose, and only subtly different, so likely to lead to muscle-memory mistakes.

I think the various uses of attach in eksctl config stand out as verbs in a sea of nouns, but I guess using attach specifically is consistent here, e.g., we also have securityGroups.attachIDs, not securityGroups.attachedIDs.

Yes, I was trying to avoid a verb but maybe withWellKnownPolicies?

Would I be at risk of ruining everything by saying I am not a fan of WellKnown? withCommonPolicies?

the one problem with "common" is that it has two meanings, one "well known", the other "shared"...

yeh that's totally right. withNamedPolicies? although I suppose all policies have names.

Struggling to find a better alternative to WellKnownPolicies so I'm happy to stick with that. I'm still unsure about with vs attach. I know that using the word attach isn't technically correct, but IMO its worth using it to remain consistent with the other fields.

aclevername

LGTM

pkg/cfn/builder/iam.go

michaelbeaumont added the kind/feature New feature or request label Jan 7, 2021

michaelbeaumont force-pushed the irsa_addon_policies branch from f2b011e to 27ccd41 Compare January 7, 2021 15:40

michaelbeaumont force-pushed the irsa_addon_policies branch 2 times, most recently from 4e61994 to 58f186a Compare January 7, 2021 19:59

TBBle reviewed Jan 8, 2021

View reviewed changes

aclevername reviewed Jan 8, 2021

View reviewed changes

michaelbeaumont force-pushed the irsa_addon_policies branch 2 times, most recently from f950aa9 to 4052080 Compare January 11, 2021 14:27

michaelbeaumont added 3 commits January 13, 2021 14:29

Add support for well known policies with IRSA

30a5983

Don't skip policies

0347704

Simplify statements for well known and addon IAM policies

fa4f8f6

michaelbeaumont force-pushed the irsa_addon_policies branch from 4052080 to fa4f8f6 Compare January 13, 2021 13:29

Add test that wellKnownPolicies are respected

7cd2224

michaelbeaumont requested a review from a team January 14, 2021 09:15

aclevername approved these changes Jan 14, 2021

View reviewed changes

Improve docs for wellKnownPolicies

a0e0b0e

michaelbeaumont force-pushed the irsa_addon_policies branch from a78c9c6 to a0e0b0e Compare January 14, 2021 10:33

Callisto13 reviewed Jan 14, 2021

View reviewed changes

pkg/cfn/builder/iam.go Outdated Show resolved Hide resolved

michaelbeaumont and others added 4 commits January 14, 2021 19:47

Simplify irsa well known policy code

ac7bc9d

Merge branch 'master' into irsa_addon_policies

d364084

Merge remote-tracking branch 'upstream/master' into irsa_addon_policies

bc5daf0

Merge branch 'master' into irsa_addon_policies

5260127

michaelbeaumont merged commit 14097d1 into eksctl-io:master Jan 28, 2021

michaelbeaumont deleted the irsa_addon_policies branch January 28, 2021 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for well known policies with IRSA #3045

Add support for well known policies with IRSA #3045

michaelbeaumont commented Jan 7, 2021 •

edited

aclevername commented Jan 7, 2021

TBBle Jan 8, 2021 •

edited

michaelbeaumont Jan 8, 2021

TBBle Jan 11, 2021

michaelbeaumont Jan 11, 2021

michaelbeaumont Jan 14, 2021

TBBle Jan 8, 2021

michaelbeaumont Jan 8, 2021

michaelbeaumont Jan 14, 2021

aclevername Jan 8, 2021 •

edited

TBBle Jan 8, 2021

michaelbeaumont Jan 11, 2021

Callisto13 Jan 14, 2021

michaelbeaumont Jan 14, 2021

Callisto13 Jan 15, 2021

aclevername Jan 27, 2021

aclevername left a comment

Add support for well known policies with IRSA #3045

Add support for well known policies with IRSA #3045

Conversation

michaelbeaumont commented Jan 7, 2021 • edited

Description

Checklist

aclevername commented Jan 7, 2021

TBBle Jan 8, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aclevername Jan 8, 2021 • 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

Choose a reason for hiding this comment

aclevername left a comment

Choose a reason for hiding this comment

michaelbeaumont commented Jan 7, 2021 •

edited

TBBle Jan 8, 2021 •

edited

aclevername Jan 8, 2021 •

edited