fix: Correct logic to try to use module created IAM role before falli… #3433
Conversation
| 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 |
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
From
resource "aws_iam_role" "eks_auto" {
count = local.create_node_iam_role ? 1 : 0and
locals {
create_node_iam_role = local.create && var.create_node_iam_role && local.auto_mode_enabledand
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.
There was a problem hiding this comment.
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 != ""There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
You can think its not right, but I can tell you it is 😬
Wow, easy there
There was a problem hiding this comment.
#3429 and this one are very bad regressions, we need fixes ASAP.
right back at ya 😉
|
LGTM! 🚀 |
## [21.0.1](v21.0.0...v21.0.1) (2025-07-24) ### Bug Fixes * Correct logic to try to use module created IAM role before falli… ([#3433](#3433)) ([97d4ebb](97d4ebb))
|
This PR is included in version 21.0.1 🎉 |
|
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. |
Description
tagsto identity providers variable definitionMotivation and Context
identity_providersvariables #3432v21.0.0outputcluster_oidc_issuer_urlcannot be used withaws_iam_openid_connect_providerdata source anymore #3428Breaking Changes
How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request…ng back to user provided IAM role