Skip to content

fix: Correct logic to try to use module created IAM role before falli… #3433

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

Merged
merged 3 commits into from
Jul 24, 2025
Merged

fix: Correct logic to try to use module created IAM role before falli… #3433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b4bfd9
fix: Correct logic to try to use module created IAM role before falli…
bryantbiggs Jul 24, 2025
318d61f
fix: Add missing tags to identity providers
bryantbiggs Jul 24, 2025
60096fc
fix: Revert changs to OIDC dual stack issuer url
bryantbiggs Jul 24, 2025
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ We are grateful to the community for contributing bugfixes and improvements! Ple
| <a name="input_iam_role_permissions_boundary"></a> [iam\_role\_permissions\_boundary](#input\_iam\_role\_permissions\_boundary) | ARN of the policy that is used to set the permissions boundary for the IAM role | `string` | `null` | no |
| <a name="input_iam_role_tags"></a> [iam\_role\_tags](#input\_iam\_role\_tags) | A map of additional tags to add to the IAM role created | `map(string)` | `{}` | no |
| <a name="input_iam_role_use_name_prefix"></a> [iam\_role\_use\_name\_prefix](#input\_iam\_role\_use\_name\_prefix) | Determines whether the IAM role name (`iam_role_name`) is used as a prefix | `bool` | `true` | no |
| <a name="input_identity_providers"></a> [identity\_providers](#input\_identity\_providers) | Map of cluster identity provider configurations to enable for the cluster. Note - this is different/separate from IRSA | <pre>map(object({<br/> client_id = string<br/> groups_claim = optional(string)<br/> groups_prefix = optional(string)<br/> identity_provider_config_name = optional(string) # will fall back to map key<br/> issuer_url = string<br/> required_claims = optional(map(string))<br/> username_claim = optional(string)<br/> username_prefix = optional(string)<br/> }))</pre> | `null` | no |
| <a name="input_identity_providers"></a> [identity\_providers](#input\_identity\_providers) | Map of cluster identity provider configurations to enable for the cluster. Note - this is different/separate from IRSA | <pre>map(object({<br/> client_id = string<br/> groups_claim = optional(string)<br/> groups_prefix = optional(string)<br/> identity_provider_config_name = optional(string) # will fall back to map key<br/> issuer_url = string<br/> required_claims = optional(map(string))<br/> username_claim = optional(string)<br/> username_prefix = optional(string)<br/> tags = optional(map(string), {})<br/> }))</pre> | `null` | no |
| <a name="input_include_oidc_root_ca_thumbprint"></a> [include\_oidc\_root\_ca\_thumbprint](#input\_include\_oidc\_root\_ca\_thumbprint) | Determines whether to include the root CA thumbprint in the OpenID Connect (OIDC) identity provider's server certificate(s) | `bool` | `true` | no |
| <a name="input_ip_family"></a> [ip\_family](#input\_ip\_family) | The IP family used to assign Kubernetes pod and service addresses. Valid values are `ipv4` (default) and `ipv6`. You can only specify an IP family when you create a cluster, changing this value will force a new cluster to be created | `string` | `"ipv4"` | no |
| <a name="input_kms_key_administrators"></a> [kms\_key\_administrators](#input\_kms\_key\_administrators) | A list of IAM ARNs for [key administrators](https://docs.aws.amazon.com/kms/latest/developerguide/key-policy-default.html#key-policy-default-allow-administrators). If no value is provided, the current caller identity is used to ensure at least one key admin is available | `list(string)` | `[]` | no |
Expand Down
5 changes: 2 additions & 3 deletions examples/eks-managed-node-group/eks-al2023.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module "eks_al2023" {
# source = "terraform-aws-modules/eks/aws"
# version = "~> 20.0"
source = "../.."
source = "terraform-aws-modules/eks/aws"
version = "~> 21.0"

name = "${local.name}-al2023"
kubernetes_version = "1.33"
Expand Down
5 changes: 2 additions & 3 deletions examples/eks-managed-node-group/eks-bottlerocket.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module "eks_bottlerocket" {
# source = "terraform-aws-modules/eks/aws"
# version = "~> 20.0"
source = "../.."
source = "terraform-aws-modules/eks/aws"
version = "~> 21.0"

name = "${local.name}-bottlerocket"
kubernetes_version = "1.33"
Expand Down
5 changes: 2 additions & 3 deletions examples/self-managed-node-group/eks-al2023.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module "eks_al2023" {
# source = "terraform-aws-modules/eks/aws"
# version = "~> 20.0"
source = "../.."
source = "terraform-aws-modules/eks/aws"
version = "~> 21.0"

name = "${local.name}-al2023"
kubernetes_version = "1.33"
Expand Down
5 changes: 2 additions & 3 deletions examples/self-managed-node-group/eks-bottlerocket.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module "eks_bottlerocket" {
# source = "terraform-aws-modules/eks/aws"
# version = "~> 20.0"
source = "../.."
source = "terraform-aws-modules/eks/aws"
version = "~> 21.0"

name = "${local.name}-bottlerocket"
kubernetes_version = "1.33"
Expand Down
8 changes: 4 additions & 4 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ resource "aws_eks_cluster" "this" {
content {
enabled = compute_config.value.enabled
node_pools = compute_config.value.node_pools
node_role_arn = compute_config.value.node_pools != null ? try(compute_config.value.node_role_arn, aws_iam_role.eks_auto[0].arn, null) : null
node_role_arn = compute_config.value.node_pools != null ? try(aws_iam_role.eks_auto[0].arn, compute_config.value.node_role_arn) : null
Copy link

@matheuscscp matheuscscp Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If compute_config.value.node_role_arn is specified (module input), then is it guaranteed that aws_iam_role.eks_auto[0].arn will be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

it will not be null - this would throw an error if create_node_iam_role = false which is what try() catches and moves down the chain to compute_config.value.node_role_arn

Copy link

Choose a reason for hiding this comment

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

From

resource "aws_iam_role" "eks_auto" {
  count = local.create_node_iam_role ? 1 : 0

and

locals {
  create_node_iam_role = local.create && var.create_node_iam_role && local.auto_mode_enabled

and

  auto_mode_enabled = try(var.compute_config.enabled, false)

I think this change is still not right. If I understand correctly, the compute_config.value.node_role_arn module input will never be used. For this to be correct you need to modify local.create_node_iam_role to be true only if compute_config.value.node_role_arn is not specified.

Copy link

@matheuscscp matheuscscp Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Make it like this:

locals {
  create_node_iam_role = local.create && var.create_node_iam_role && local.auto_mode_enabled && 
  var.compute_config.node_role_arn != null && var.compute_config.node_role_arn != ""

Copy link
Member Author

Choose a reason for hiding this comment

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

You can think its not right, but I can tell you it is 😬

the previous form for the initial v21 release did not work because try() only falls down the chain when it encounters errors. Plucking a value off compute_config.value.node_role_arn does not throw an error because of the new variable optional attributes; it simply returns a null and never moves down the chain

Copy link

Choose a reason for hiding this comment

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

So if I set var.compute_config.node_role_arn to a value then I also have to set var.create_node_iam_role to false, is that the intended use?

Copy link

Choose a reason for hiding this comment

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

Makes sense!

Copy link

Choose a reason for hiding this comment

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

You can think its not right, but I can tell you it is 😬

Wow, easy there

Copy link
Member Author

Choose a reason for hiding this comment

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

#3429 and this one are very bad regressions, we need fixes ASAP.

right back at ya 😉

Copy link

Choose a reason for hiding this comment

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

Thanks for the fixes! 🙏

}
}

Expand Down Expand Up @@ -444,7 +444,7 @@ data "tls_certificate" "this" {
# Not available on outposts
count = local.create_oidc_provider && var.include_oidc_root_ca_thumbprint ? 1 : 0

url = local.dualstack_oidc_issuer_url
url = aws_eks_cluster.this[0].identity[0].oidc[0].issuer
}

resource "aws_iam_openid_connect_provider" "oidc_provider" {
Expand All @@ -453,7 +453,7 @@ resource "aws_iam_openid_connect_provider" "oidc_provider" {

client_id_list = distinct(compact(concat(["sts.amazonaws.com"], var.openid_connect_audiences)))
thumbprint_list = concat(local.oidc_root_ca_thumbprint, var.custom_oidc_thumbprints)
url = local.dualstack_oidc_issuer_url
url = aws_eks_cluster.this[0].identity[0].oidc[0].issuer
Copy link

Choose a reason for hiding this comment

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

👍


tags = merge(
{ Name = "${var.name}-eks-irsa" },
Expand Down Expand Up @@ -856,7 +856,7 @@ resource "aws_eks_identity_provider_config" "this" {
client_id = each.value.client_id
groups_claim = each.value.groups_claim
groups_prefix = each.value.groups_prefix
identity_provider_config_name = try(each.value.identity_provider_config_name, each.key)
identity_provider_config_name = coalesce(each.value.identity_provider_config_name, each.key)
issuer_url = each.value.issuer_url
required_claims = each.value.required_claims
username_claim = each.value.username_claim
Expand Down
6 changes: 3 additions & 3 deletions outputs.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
locals {
# https://github.com/aws/containers-roadmap/issues/2038#issuecomment-2278450601
dualstack_oidc_issuer_url = try(replace(replace(aws_eks_cluster.this[0].identity[0].oidc[0].issuer, "https://oidc.eks.", "https://oidc-eks."), ".amazonaws.com/", ".api.aws/"), null)

}

################################################################################
Expand Down Expand Up @@ -59,7 +58,8 @@ output "cluster_oidc_issuer_url" {

output "cluster_dualstack_oidc_issuer_url" {
description = "Dual-stack compatible URL on the EKS cluster for the OpenID Connect identity provider"
value = local.dualstack_oidc_issuer_url
# https://github.com/aws/containers-roadmap/issues/2038#issuecomment-2278450601
value = try(replace(replace(aws_eks_cluster.this[0].identity[0].oidc[0].issuer, "https://oidc.eks.", "https://oidc-eks."), ".amazonaws.com/", ".api.aws/"), null)
}

output "cluster_version" {
Expand Down
1 change: 1 addition & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,7 @@ variable "identity_providers" {
required_claims = optional(map(string))
username_claim = optional(string)
username_prefix = optional(string)
tags = optional(map(string), {})
}))
default = null
}
Expand Down