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 @@ -1713,6 +1717,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 @@ -63,8 +63,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 @@ -364,7 +364,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
19 changes: 17 additions & 2 deletions pkg/cfn/builder/iam.go
Expand Up @@ -224,10 +224,23 @@ 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)
}

managedPolicies, customPolicies := createWellKnownPolicies(rs.spec.WellKnownPolicies)

for _, p := range managedPolicies {
role.ManagedPolicyArns = append(role.ManagedPolicyArns, makePolicyARN(p.name))
}

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

for _, p := range customPolicies {
doc := cft.MakePolicyDocument(p.Statements...)
rs.template.AttachPolicy(p.Name, roleRef, doc)
}

// 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 +334,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
56 changes: 50 additions & 6 deletions pkg/cfn/builder/iam_helper.go
Expand Up @@ -18,6 +18,53 @@ type cfnTemplate interface {
newResource(name string, resource gfn.Resource) *gfnt.Value
}

type managedPolicyForRole struct {
name string
}

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

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

// 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 +93,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 @@ -184,18 +184,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