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.

Sign up for GitHub

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

feat: Add load_balancer_controller targetgroup binding only role #199

Merged
merged 11 commits into from
Mar 25, 2022
Merged

feat: Add load_balancer_controller targetgroup binding only role #199

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c64be51
add load_balancer_controller targetgroup binding only role
cdhesse Mar 4, 2022
877524d
Merge branch 'master' into master
cdhesse Mar 10, 2022
070c9f9
Update policies.tf
cdhesse Mar 10, 2022
24f902d
adding missing permission for describevpcs
cdhesse Mar 10, 2022
02ea3e0
Merge branch 'master' of github.com:cdhesse/terraform-aws-iam
cdhesse Mar 10, 2022
0c92efa
Merge branch 'master' into master
cdhesse Mar 16, 2022
444b6aa
Merge branch 'master' into master
cdhesse Mar 18, 2022
4ff74f2
Update examples/iam-role-for-service-accounts-eks/main.tf
cdhesse Mar 25, 2022
699865f
Update modules/iam-role-for-service-accounts-eks/variables.tf
cdhesse Mar 25, 2022
1959a37
Merge branch 'master' into master
cdhesse Mar 25, 2022
c203e7e
pre-commit results
cdhesse Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/iam-role-for-service-accounts-eks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ No providers.
| <a name="module_irsa_role"></a> [irsa\_role](#module\_irsa\_role) | ../../modules/iam-role-for-service-accounts-eks | n/a |
| <a name="module_karpenter_controller_irsa_role"></a> [karpenter\_controller\_irsa\_role](#module\_karpenter\_controller\_irsa\_role) | ../../modules/iam-role-for-service-accounts-eks | n/a |
| <a name="module_load_balancer_controller_irsa_role"></a> [load\_balancer\_controller\_irsa\_role](#module\_load\_balancer\_controller\_irsa\_role) | ../../modules/iam-role-for-service-accounts-eks | n/a |
| <a name="module_load_balancer_controller_targetgroup_binding_only_irsa_role"></a> [load\_balancer\_controller\_targetgroup\_binding\_only\_irsa\_role](#module\_load\_balancer\_controller\_targetgroup\_binding\_only\_irsa\_role) | ../../modules/iam-role-for-service-accounts-eks | n/a |
| <a name="module_node_termination_handler_irsa_role"></a> [node\_termination\_handler\_irsa\_role](#module\_node\_termination\_handler\_irsa\_role) | ../../modules/iam-role-for-service-accounts-eks | n/a |
| <a name="module_vpc"></a> [vpc](#module\_vpc) | terraform-aws-modules/vpc/aws | ~> 3.0 |
| <a name="module_vpc_cni_ipv4_irsa_role"></a> [vpc\_cni\_ipv4\_irsa\_role](#module\_vpc\_cni\_ipv4\_irsa\_role) | ../../modules/iam-role-for-service-accounts-eks | n/a |
Expand Down
16 changes: 16 additions & 0 deletions examples/iam-role-for-service-accounts-eks/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,22 @@ module "load_balancer_controller_irsa_role" {
tags = local.tags
}

module "load_balancer_controller_targetgroup_binding_only_irsa_role" {
source = "../../modules/iam-role-for-service-accounts-eks"

role_name = "load_balancer_controller"
cdhesse marked this conversation as resolved.
Show resolved Hide resolved
attach_load_balancer_controller_targetgroup_binding_only_policy = true

oidc_providers = {
ex = {
provider_arn = module.eks.oidc_provider_arn
namespace_service_accounts = ["kube-system:aws-load-balancer-controller"]
}
}

tags = local.tags
}

################################################################################
# Supporting Resources
################################################################################
Expand Down
4 changes: 4 additions & 0 deletions modules/iam-role-for-service-accounts-eks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ No modules.
| [aws_iam_policy.external_dns](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
| [aws_iam_policy.karpenter_controller](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
| [aws_iam_policy.load_balancer_controller](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
| [aws_iam_policy.load_balancer_controller_targetgroup_only](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
| [aws_iam_policy.node_termination_handler](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
| [aws_iam_policy.vpc_cni](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
| [aws_iam_role.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role) | resource |
Expand All @@ -121,6 +122,7 @@ No modules.
| [aws_iam_role_policy_attachment.external_dns](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
| [aws_iam_role_policy_attachment.karpenter_controller](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
| [aws_iam_role_policy_attachment.load_balancer_controller](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
| [aws_iam_role_policy_attachment.load_balancer_controller_targetgroup_only](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
| [aws_iam_role_policy_attachment.node_termination_handler](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
| [aws_iam_role_policy_attachment.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
| [aws_iam_role_policy_attachment.vpc_cni](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
Expand All @@ -129,6 +131,7 @@ No modules.
| [aws_iam_policy_document.external_dns](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.karpenter_controller](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.load_balancer_controller](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.load_balancer_controller_targetgroup_only](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.node_termination_handler](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.vpc_cni](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
Expand All @@ -144,6 +147,7 @@ No modules.
| <a name="input_attach_external_dns_policy"></a> [attach\_external\_dns\_policy](#input\_attach\_external\_dns\_policy) | Determines whether to attach the External DNS IAM policy to the role | `bool` | `false` | no |
| <a name="input_attach_karpenter_controller_policy"></a> [attach\_karpenter\_controller\_policy](#input\_attach\_karpenter\_controller\_policy) | Determines whether to attach the Karpenter Controller policy to the role | `bool` | `false` | no |
| <a name="input_attach_load_balancer_controller_policy"></a> [attach\_load\_balancer\_controller\_policy](#input\_attach\_load\_balancer\_controller\_policy) | Determines whether to attach the Load Balancer Controller policy to the role | `bool` | `false` | no |
| <a name="input_attach_load_balancer_controller_targetgroup_binding_only_policy"></a> [attach\_load\_balancer\_controller\_targetgroup\_binding\_only\_policy](#input\_attach\_load\_balancer\_controller\_targetgroup\_binding\_only\_policy) | Determines whether to attach the Load Balancer Controller policy for the TargetGroup binding only scenario. | `bool` | `false` | no |
| <a name="input_attach_node_termination_handler_policy"></a> [attach\_node\_termination\_handler\_policy](#input\_attach\_node\_termination\_handler\_policy) | Determines whether to attach the Node Termination Handler policy to the role | `bool` | `false` | no |
| <a name="input_attach_vpc_cni_policy"></a> [attach\_vpc\_cni\_policy](#input\_attach\_vpc\_cni\_policy) | Determines whether to attach the VPC CNI IAM policy to the role | `bool` | `false` | no |
| <a name="input_cluster_autoscaler_cluster_ids"></a> [cluster\_autoscaler\_cluster\_ids](#input\_cluster\_autoscaler\_cluster\_ids) | List of cluster IDs to appropriately scope permissions within the Cluster Autoscaler IAM policy | `list(string)` | `[]` | no |
Expand Down
44 changes: 44 additions & 0 deletions modules/iam-role-for-service-accounts-eks/policies.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,3 +740,47 @@ resource "aws_iam_role_policy_attachment" "load_balancer_controller" {
role = aws_iam_role.this[0].name
policy_arn = aws_iam_policy.load_balancer_controller[0].arn
}

################################################################################
# AWS Load Balancer Controller TargetGroup Binding Only Policy
################################################################################

# https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.4/guide/targetgroupbinding/targetgroupbinding/#reference
# https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.4/deploy/installation/#setup-iam-manually
data "aws_iam_policy_document" "load_balancer_controller_targetgroup_only" {
count = var.create_role && var.attach_load_balancer_controller_targetgroup_binding_only_policy ? 1 : 0

statement {
actions = [
"ec2:DescribeSecurityGroups",
"ec2:DescribeInstances",
"ec2:DescribeVpcs",
"elasticloadbalancing:DescribeTargetGroups",
"elasticloadbalancing:DescribeTargetHealth",
"elasticloadbalancing:ModifyTargetGroup",
Copy link
Member

@bryantbiggs bryantbiggs Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For the Modify*/Register*/Deregeister* actions, should we not break these permissions out to avoid the wildcard, similar to the full controller policy. Something like

  statement {
    actions = [
      "ec2:DescribeSecurityGroups",
      "ec2:DescribeInstances",
      "ec2:DescribeVpcs",
      "elasticloadbalancing:DescribeTargetGroups",
      "elasticloadbalancing:DescribeTargetHealth",
    ]

    resources = ["*"]
  }

  statement {
    actions = [
      "elasticloadbalancing:ModifyTargetGroup",
      "elasticloadbalancing:ModifyTargetGroupAttributes",
    ]
    resources = ["*"]

    condition {
      test     = "Null"
      variable = "aws:ResourceTag/elbv2.k8s.aws/cluster"
      values   = ["false"]
    }
  }

  statement {
    actions = [
      "elasticloadbalancing:RegisterTargets",
      "elasticloadbalancing:DeregisterTargets",
    ]
    resources = ["arn:${local.partition}:elasticloadbalancing:*:*:targetgroup/*/*"]
  }

With that we could even go one step further and allow users to restrict which target groups with

  statement {
    actions = [
      "ec2:DescribeSecurityGroups",
      "ec2:DescribeInstances",
      "ec2:DescribeVpcs",
      "elasticloadbalancing:DescribeTargetGroups",
      "elasticloadbalancing:DescribeTargetHealth",
    ]

    resources = ["*"]
  }

  statement {
    actions = [
      "elasticloadbalancing:ModifyTargetGroup",
      "elasticloadbalancing:ModifyTargetGroupAttributes",
    ]
    resources = ["*"]

    condition {
      test     = "Null"
      variable = "aws:ResourceTag/elbv2.k8s.aws/cluster"
      values   = ["false"]
    }
  }

  statement {
    actions = [
      "elasticloadbalancing:RegisterTargets",
      "elasticloadbalancing:DeregisterTargets",
    ]
    resources = [for name in var.load_balancer_controller_targetgroup_names :
      "arn:${local.partition}:elasticloadbalancing:*:*:targetgroup/${name}/*"
    ]
  }

Where the variable load_balancer_controller_targetgroup_names defaults to *. What do you think?

Copy link
Contributor Author

@cdhesse cdhesse Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The use case for TargetGroupBinding only mode, vs. full ALB, is that you want to create (or already have pre-existing) TargetGroups outside of the kubernetes environment. This change would require the TargetGroup in question to have the elbv2.k8s.aws/cluster tag. While on one hand this solution ensures that at least the owner/creator of the TargetGroup intended for it to be updated from a load-balancer-controller, the downside is there may be valid reasons for that tag to not exist.

The second thought - your proposed change is a better version than what is documented as part of the load-balancer-controller intro guide. A person trying for the first time to use this module would quickly realize it didn't work, until they reverse engineer looking at this policy, then see that they need to go back and tag their TargetGroups with this k8s tag. I suggest that if you implement it this way, the ideal would be that the TargetBingingOnly documentation should include this more restricted version of the role vs. their more permissive one. The load-balancer-controller documentation could then describe the requirement to tag the TargetGroup.

However, the Register/Deregister change I think is brilliant. We could make an option as well for var.load_balancer_controller_targetgroup_arns if we wanted to, and then you could pass name or arn...?

Copy link
Member

Choose a reason for hiding this comment

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

For now we'll proceed with what is defined by the upstream documentation. We can revisit later for any additional policy scoping

"elasticloadbalancing:ModifyTargetGroupAttributes",
"elasticloadbalancing:RegisterTargets",
"elasticloadbalancing:DeregisterTargets"
]

resources = ["*"]
}
}

resource "aws_iam_policy" "load_balancer_controller_targetgroup_only" {
count = var.create_role && var.attach_load_balancer_controller_targetgroup_binding_only_policy ? 1 : 0

name_prefix = "AmazonEKS_AWS_Load_Balancer_Controller_TargetGroup_Only-"
path = var.role_path
description = "Provides permissions for AWS Load Balancer Controller addon in TargetGroup binding only scenario."
policy = data.aws_iam_policy_document.load_balancer_controller_targetgroup_only[0].json

tags = var.tags
}

resource "aws_iam_role_policy_attachment" "load_balancer_controller_targetgroup_only" {
count = var.create_role && var.attach_load_balancer_controller_targetgroup_binding_only_policy ? 1 : 0

role = aws_iam_role.this[0].name
policy_arn = aws_iam_policy.load_balancer_controller_targetgroup_only[0].arn
}
8 changes: 8 additions & 0 deletions modules/iam-role-for-service-accounts-eks/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,3 +177,11 @@ variable "attach_load_balancer_controller_policy" {
type = bool
default = false
}

# https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.4/guide/targetgroupbinding/targetgroupbinding/#reference
# https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.4/deploy/installation/#setup-iam-manually
variable "attach_load_balancer_controller_targetgroup_binding_only_policy" {
description = "Determines whether to attach the Load Balancer Controller policy for the TargetGroup binding only scenario."
cdhesse marked this conversation as resolved.
Show resolved Hide resolved
type = bool
default = false
}