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 support for well known policies with IRSA #3045

Merged
merged 9 commits into from Jan 28, 2021
5 changes: 5 additions & 0 deletions examples/13-iamserviceaccounts.yaml
Expand Up @@ -18,6 +18,11 @@ iam:
labels: {aws-usage: "application"}
attachPolicyARNs:
- "arn:aws:iam::aws:policy/AmazonS3ReadOnlyAccess"
- metadata:
name: aws-load-balancer-controller
namespace: kube-system
wellKnownPolicies:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

awsLoadBalancerController: true
- metadata:
name: cache-access
namespace: backend-apps
Expand Down
48 changes: 48 additions & 0 deletions pkg/apis/eksctl.io/v1alpha5/assets/schema.json
Expand Up @@ -343,6 +343,9 @@
"description": "AWS tags for the service account",
"x-intellij-html-description": "AWS tags for the service account",
"default": "{}"
},
"wellKnownPolicies": {
"$ref": "#/definitions/WellKnownPolicies"
}
},
"preferredOrder": [
Expand All @@ -351,6 +354,7 @@
"labels",
"annotations",
"attachPolicyARNs",
"wellKnownPolicies",
"attachPolicy",
"permissionsBoundary",
"status",
Expand Down Expand Up @@ -1703,6 +1707,50 @@
"description": "defines the configuration for KMS encryption provider",
"x-intellij-html-description": "defines the configuration for KMS encryption provider"
},
"WellKnownPolicies": {
"properties": {
"autoScaler": {
"type": "boolean",
"description": "adds policies for cluster-autoscaler. See [autoscaler AWS docs](https://docs.aws.amazon.com/eks/latest/userguide/cluster-autoscaler.html).",
"x-intellij-html-description": "adds policies for cluster-autoscaler. See <a href=\"https://docs.aws.amazon.com/eks/latest/userguide/cluster-autoscaler.html\">autoscaler AWS docs</a>.",
"default": "false"
},
"awsLoadBalancerController": {
"type": "boolean",
"description": "adds policies for using the aws-load-balancer-controller. See [Load Balancer docs](https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html).",
"x-intellij-html-description": "adds policies for using the aws-load-balancer-controller. See <a href=\"https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html\">Load Balancer docs</a>.",
"default": "false"
},
"certManager": {
"type": "boolean",
"description": "adds cert-manager policies. See [cert-manager docs](https://cert-manager.io/docs/configuration/acme/dns01/route53).",
"x-intellij-html-description": "adds cert-manager policies. See <a href=\"https://cert-manager.io/docs/configuration/acme/dns01/route53\">cert-manager docs</a>.",
"default": "false"
},
"externalDNS": {
"type": "boolean",
"description": "adds external-dns policies for Amazon Route 53. See [external-dns docs](https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/aws.md).",
"x-intellij-html-description": "adds external-dns policies for Amazon Route 53. See <a href=\"https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/aws.md\">external-dns docs</a>.",
"default": "false"
},
"imageBuilder": {
"type": "boolean",
"description": "allows for full ECR (Elastic Container Registry) access.",
"x-intellij-html-description": "allows for full ECR (Elastic Container Registry) access.",
"default": "false"
}
},
"preferredOrder": [
"imageBuilder",
"autoScaler",
"awsLoadBalancerController",
"externalDNS",
"certManager"
],
"additionalProperties": false,
"description": "for attaching common IAM policies",
"x-intellij-html-description": "for attaching common IAM policies"
},
"github.com|weaveworks|eksctl|pkg|utils|ipnet.IPNet": {
"type": "string",
"description": "an IP address in CIDR notation",
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/eksctl.io/v1alpha5/iam.go
Expand Up @@ -80,6 +80,8 @@ type ClusterIAMServiceAccount struct {
// +optional
AttachPolicyARNs []string `json:"attachPolicyARNs,omitempty"`

WellKnownPolicies WellKnownPolicies `json:"wellKnownPolicies,omitempty"`

// AttachPolicy holds a policy document to attach to this service account
// +optional
AttachPolicy InlineDocument `json:"attachPolicy,omitempty"`
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/eksctl.io/v1alpha5/schema.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions pkg/apis/eksctl.io/v1alpha5/validation.go
Expand Up @@ -53,8 +53,8 @@ func ValidateClusterConfig(cfg *ClusterConfig) error {
if ok, err := saNames.checkUnique("<namespace>/<name> of "+path, sa.NameString()); !ok {
return err
}
if len(sa.AttachPolicyARNs) == 0 && sa.AttachPolicy == nil {
return fmt.Errorf("%s.attachPolicyARNs or %s.attachPolicy must be set", path, path)
if !sa.WellKnownPolicies.HasPolicy() && len(sa.AttachPolicyARNs) == 0 && sa.AttachPolicy == nil {
return fmt.Errorf("%[1]s.wellKnownPolicies, %[1]s.attachPolicyARNs or %[1]s.attachPolicy must be set", path)
}
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/apis/eksctl.io/v1alpha5/validation_test.go
Expand Up @@ -255,7 +255,11 @@ var _ = Describe("ClusterConfig validation", func() {
err = ValidateClusterConfig(cfg)
Expect(err).To(HaveOccurred())

Expect(err.Error()).To(HavePrefix("iam.serviceAccounts[1].attachPolicyARNs or iam.serviceAccounts[1].attachPolicy must be set"))
Expect(err.Error()).To(SatisfyAll(
ContainSubstring("iam.serviceAccounts[1]"),
ContainSubstring("attachPolicy"),
ContainSubstring("must be set"),
))
})

It("should fail when non-uniquely named iam.serviceAccounts are given", func() {
Expand Down
25 changes: 25 additions & 0 deletions pkg/apis/eksctl.io/v1alpha5/well_known_iam_policy.go
@@ -0,0 +1,25 @@
package v1alpha5

// WellKnownPolicies for attaching common IAM policies
type WellKnownPolicies struct {
// ImageBuilder allows for full ECR (Elastic Container Registry) access.
ImageBuilder bool `json:"imageBuilder,inline"`
// AutoScaler adds policies for cluster-autoscaler. See [autoscaler AWS
// docs](https://docs.aws.amazon.com/eks/latest/userguide/cluster-autoscaler.html).
AutoScaler bool `json:"autoScaler,inline"`
// AWSLoadBalancerController adds policies for using the
// aws-load-balancer-controller. See [Load Balancer
// docs](https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html).
AWSLoadBalancerController bool `json:"awsLoadBalancerController,inline"`
// ExternalDNS adds external-dns policies for Amazon Route 53.
// See [external-dns
// docs](https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/aws.md).
ExternalDNS bool `json:"externalDNS,inline"`
// CertManager adds cert-manager policies. See [cert-manager
// docs](https://cert-manager.io/docs/configuration/acme/dns01/route53).
CertManager bool `json:"certManager,inline"`
}

func (p *WellKnownPolicies) HasPolicy() bool {
return p.ImageBuilder || p.AutoScaler || p.AWSLoadBalancerController || p.ExternalDNS || p.CertManager
}
17 changes: 17 additions & 0 deletions pkg/apis/eksctl.io/v1alpha5/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 11 additions & 2 deletions pkg/cfn/builder/api_test.go
Expand Up @@ -1066,13 +1066,22 @@ var _ = Describe("CloudFormation template builder API", func() {
Expect(policy2.PolicyDocument.Statement[0].Action).To(Equal([]string{
"route53:ListResourceRecordSets",
"route53:ListHostedZonesByName",
}))

policy3 := ngTemplate.Resources["PolicyExternalDNSHostedZones"].Properties

Expect(policy3.Roles).To(HaveLen(1))
isRefTo(policy3.Roles[0], "NodeInstanceRole")
Expect(policy3.PolicyDocument.Statement).To(HaveLen(1))
Expect(policy3.PolicyDocument.Statement[0].Effect).To(Equal("Allow"))
Expect(policy3.PolicyDocument.Statement[0].Resource).To(Equal("*"))
Expect(policy3.PolicyDocument.Statement[0].Action).To(Equal([]string{
"route53:ListHostedZones",
"route53:ListResourceRecordSets",
"route53:ListTagsForResource",
}))

Expect(ngTemplate.Resources).ToNot(HaveKey("PolicyAutoScaling"))
Expect(ngTemplate.Resources).ToNot(HaveKey("PolicyExternalDNSChangeSet"))
Expect(ngTemplate.Resources).ToNot(HaveKey("PolicyExternalDNSHostedZones"))
Expect(ngTemplate.Resources).ToNot(HaveKey("PolicyAppMesh"))
Expect(ngTemplate.Resources).ToNot(HaveKey("PolicyAppMeshPreview"))
Expect(ngTemplate.Resources).ToNot(HaveKey("PolicyEBS"))
Expand Down
23 changes: 21 additions & 2 deletions pkg/cfn/builder/iam.go
Expand Up @@ -224,10 +224,27 @@ func (rs *IAMServiceAccountResourceSet) AddAllResources() error {
PermissionsBoundary: rs.spec.PermissionsBoundary,
RoleName: rs.spec.RoleName,
}
role.ManagedPolicyArns = append(role.ManagedPolicyArns, rs.spec.AttachPolicyARNs...)
for _, arn := range rs.spec.AttachPolicyARNs {
role.ManagedPolicyArns = append(role.ManagedPolicyArns, arn)
}

wellKnownPolicies := createWellKnownPolicies(rs.spec.WellKnownPolicies)

for _, p := range wellKnownPolicies {
for _, np := range p.NamedPolicies {
role.ManagedPolicyArns = append(role.ManagedPolicyArns, makePolicyARN(np))
}
}

roleRef := rs.template.NewResource("Role1", role)

for _, p := range wellKnownPolicies {
if p.Name != "" {
doc := cft.MakePolicyDocument(p.Statements...)
rs.template.AttachPolicy(p.Name, roleRef, doc)
}
Callisto13 marked this conversation as resolved.
Show resolved Hide resolved
}

// TODO: declare output collector automatically when all stack builders migrated to our template package
rs.template.Outputs["Role1"] = cft.Output{
Value: cft.MakeFnGetAttString("Role1.Arn"),
Expand Down Expand Up @@ -321,7 +338,9 @@ func (rs *IAMRoleResourceSet) AddAllResources() error {
role := &cft.IAMRole{
AssumeRolePolicyDocument: assumeRolePolicyDocument,
}
role.ManagedPolicyArns = append(role.ManagedPolicyArns, rs.attachPolicyARNs...)
for _, arn := range rs.attachPolicyARNs {
role.ManagedPolicyArns = append(role.ManagedPolicyArns, arn)
}

roleRef := rs.template.NewResource("Role1", role)

Expand Down
42 changes: 36 additions & 6 deletions pkg/cfn/builder/iam_helper.go
Expand Up @@ -18,6 +18,39 @@ type cfnTemplate interface {
newResource(name string, resource gfn.Resource) *gfnt.Value
}

type customPolicy struct {
Name string
Statements []cft.MapOfInterfaces
NamedPolicies []string
}

func createWellKnownPolicies(wellKnownPolicies api.WellKnownPolicies) []customPolicy {
var policies []customPolicy
if wellKnownPolicies.ImageBuilder {
policies = append(policies, customPolicy{NamedPolicies: []string{iamPolicyAmazonEC2ContainerRegistryPowerUser}})
}
if wellKnownPolicies.AutoScaler {
policies = append(policies, customPolicy{Name: "PolicyAutoScaling", Statements: autoScalerStatements()})
}
if wellKnownPolicies.AWSLoadBalancerController {
policies = append(policies, customPolicy{Name: "PolicyAWSLoadBalancerController", Statements: loadBalancerControllerStatements()})
}
if wellKnownPolicies.ExternalDNS {
policies = append(policies,
customPolicy{Name: "PolicyExternalDNSChangeSet", Statements: changeSetStatements()},
customPolicy{Name: "PolicyExternalDNSHostedZones", Statements: externalDNSHostedZonesStatements()},
)
}
if wellKnownPolicies.CertManager {
policies = append(policies,
customPolicy{Name: "PolicyCertManagerChangeSet", Statements: changeSetStatements()},
customPolicy{Name: "PolicyCertManagerGetChange", Statements: certManagerGetChangeStatements()},
customPolicy{Name: "PolicyCertManagerHostedZones", Statements: certManagerHostedZonesStatements()},
)
}
return policies
}

// createRole creates an IAM role with policies required for the worker nodes and addons
func createRole(cfnTemplate cfnTemplate, clusterIAMConfig *api.ClusterIAM, iamConfig *api.NodeGroupIAM, managed, enableSSM, forceAddCNIPolicy bool) error {
managedPolicyARNs, err := makeManagedPolicies(clusterIAMConfig, iamConfig, managed, enableSSM, forceAddCNIPolicy)
Expand Down Expand Up @@ -46,13 +79,10 @@ func createRole(cfnTemplate cfnTemplate, clusterIAMConfig *api.ClusterIAM, iamCo

if api.IsEnabled(iamConfig.WithAddonPolicies.CertManager) {
cfnTemplate.attachAllowPolicy("PolicyCertManagerChangeSet", refIR, changeSetStatements())
if api.IsEnabled(iamConfig.WithAddonPolicies.ExternalDNS) {
cfnTemplate.attachAllowPolicy("PolicyCertManagerHostedZones", refIR, certManagerHostedZonesStatements("route53:ListHostedZones", "route53:ListTagsForResource"))
} else {
cfnTemplate.attachAllowPolicy("PolicyCertManagerHostedZones", refIR, certManagerHostedZonesStatements())
}
cfnTemplate.attachAllowPolicy("PolicyCertManagerHostedZones", refIR, certManagerHostedZonesStatements())
cfnTemplate.attachAllowPolicy("PolicyCertManagerGetChange", refIR, certManagerGetChangeStatements())
} else if api.IsEnabled(iamConfig.WithAddonPolicies.ExternalDNS) {
}
if api.IsEnabled(iamConfig.WithAddonPolicies.ExternalDNS) {
cfnTemplate.attachAllowPolicy("PolicyExternalDNSChangeSet", refIR, changeSetStatements())
cfnTemplate.attachAllowPolicy("PolicyExternalDNSHostedZones", refIR, externalDNSHostedZonesStatements())
}
Expand Down
38 changes: 38 additions & 0 deletions pkg/cfn/builder/iam_test.go
Expand Up @@ -254,6 +254,44 @@ var _ = Describe("template builder for IAM", func() {
Expect(t).To(HaveOutputWithValue("Role1", `{ "Fn::GetAtt": "Role1.Arn" }`))
})

It("can construct an iamserviceaccount addon template with wellKnownPolicies", func() {
serviceAccount := &api.ClusterIAMServiceAccount{}

serviceAccount.Name = "sa-1"

serviceAccount.WellKnownPolicies = api.WellKnownPolicies{
ImageBuilder: true,
}

appendServiceAccountToClusterConfig(cfg, serviceAccount)

rs := NewIAMServiceAccountResourceSet(serviceAccount, oidc)

templateBody := []byte{}

Expect(rs).To(RenderWithoutErrors(&templateBody))

t := cft.NewTemplate()

Expect(t).To(LoadBytesWithoutErrors(templateBody))

Expect(t.Description).To(Equal("IAM role for serviceaccount \"default/sa-1\" [created and managed by eksctl]"))

Expect(t.Resources).To(HaveLen(1))
Expect(t.Outputs).To(HaveLen(1))

Expect(t).To(HaveResource("Role1", "AWS::IAM::Role"))

Expect(t).To(HaveResourceWithPropertyValue("Role1", "AssumeRolePolicyDocument", expectedServiceAccountAssumeRolePolicyDocument))
Expect(t).To(HaveResourceWithPropertyValue("Role1", "ManagedPolicyArns", `[
{
"Fn::Sub": "arn:${AWS::Partition}:iam::aws:policy/AmazonEC2ContainerRegistryPowerUser"
}
]`))

Expect(t).To(HaveOutputWithValue("Role1", `{ "Fn::GetAtt": "Role1.Arn" }`))
})

It("can parse an iamserviceaccount addon template", func() {
t := cft.NewTemplate()

Expand Down
6 changes: 5 additions & 1 deletion pkg/cfn/builder/partition.go
Expand Up @@ -34,10 +34,14 @@ func MakeServiceRef(servicePrincipalName string) *gfnt.Value {
)
}

func makePolicyARN(policyName string) *gfnt.Value {
return gfnt.MakeFnSubString(fmt.Sprintf("arn:${%s}:iam::aws:policy/%s", gfnt.Partition, policyName))
}

func makePolicyARNs(policyNames ...string) []*gfnt.Value {
policyARNs := make([]*gfnt.Value, len(policyNames))
for i, policy := range policyNames {
policyARNs[i] = gfnt.MakeFnSubString(fmt.Sprintf("arn:${%s}:iam::aws:policy/%s", gfnt.Partition, policy))
policyARNs[i] = makePolicyARN(policy)
}
return policyARNs
}
Expand Down
13 changes: 5 additions & 8 deletions pkg/cfn/builder/statement.go
Expand Up @@ -183,18 +183,15 @@ func cloudWatchMetricsStatements() []cft.MapOfInterfaces {
}
}

func certManagerHostedZonesStatements(appendActions ...string) []cft.MapOfInterfaces {
actions := []string{
"route53:ListResourceRecordSets",
"route53:ListHostedZonesByName",
}
actions = append(actions, appendActions...)

func certManagerHostedZonesStatements() []cft.MapOfInterfaces {
return []cft.MapOfInterfaces{
{
"Effect": effectAllow,
"Resource": resourceAll,
"Action": actions,
"Action": []string{
"route53:ListResourceRecordSets",
"route53:ListHostedZonesByName",
},
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/cfn/template/api_test.go
Expand Up @@ -19,7 +19,7 @@ var _ = Describe("CloudFormation template", func() {

roleRef := t.NewResource("aRole", &IAMRole{
RoleName: "foo",
ManagedPolicyArns: []string{"abc"},
ManagedPolicyArns: []interface{}{"abc"},
})

t.Outputs["aRole"] = Output{
Expand Down
2 changes: 1 addition & 1 deletion pkg/cfn/template/iam.go
Expand Up @@ -25,7 +25,7 @@ type IAMRole struct {
Path string `json:",omitempty"`

AssumeRolePolicyDocument MapOfInterfaces `json:",omitempty"`
ManagedPolicyArns []string `json:",omitempty"`
ManagedPolicyArns []interface{} `json:",omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

PermissionsBoundary string `json:",omitempty"`
}

Expand Down