feat: Add load_balancer_controller targetgroup binding only role

[cdhesse]

Description

This implements a least privilege role for load_balancer_controller when using TargetGroup binding only as documented here.

Motivation and Context

The current module has a role for load_balancer_controller, but it is the full set of permissions, which are not required when implementing TargetGroup binding only.

Breaking Changes

No

How Has This Been Tested?

• I have tested and validated these changes using one or more of the provided examples/* projects

I tested the changes by executing the examples/iam-role-for-service-accounts-eks, and added a module call there to create the new role.

[cdhesse]

kubernetes-sigs/aws-load-balancer-controller#2542 was also created to address the missing describevps iam permission.

[cdhesse]

kubernetes-sigs/aws-load-balancer-controller#2542 was also created to address the missing describevps iam permission.

Issue 2542 has been closed as of today in the aws-load-balancer-controller repo.

[bryantbiggs]

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?

[cdhesse]

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

[bryantbiggs]

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

[bryantbiggs]

@cdhesse if you could make the requested tweaks, update your branch with the latest, and re-run pre-commit run -a, we should be set to merge this then. Thank you for the PR!

[cdhesse]

@cdhesse if you could make the requested tweaks, update your branch with the latest, and re-run pre-commit run -a, we should be set to merge this then. Thank you for the PR!

I've done as you requested, should be ready to merge now, thank you!

[bryantbiggs]

thanks again for the PR @cdhesse - good to go @antonbabenko 👍🏽

[antonbabenko]

This PR is included in version 4.16.0 🎉

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.