fix: cluster_iam_role_name logic bug

[daroga0002]

PR o'clock

Description

#1199 introduced some bug in logic of this resource. In both cases was same conditional. It started causing compatibility issues as name_prefix started to be replaced by name, in result cluster wants to recreate as in #1382

Checklist

• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[daroga0002]

Tests

Basic example:

$ terraform state show module.eks.aws_iam_role.cluster[0]
# module.eks.aws_iam_role.cluster[0]:
resource "aws_iam_role" "cluster" {
    arn                   = "arn:aws:iam::3********:role/test-eks-B5SenpZF20210531111707434300000001"
    assume_role_policy    = jsonencode(
        {
            Statement = [
                {
                    Action    = "sts:AssumeRole"
                    Effect    = "Allow"
                    Principal = {
                        Service = "eks.amazonaws.com"
                    }
                    Sid       = "EKSClusterAssumeRole"
                },
            ]
            Version   = "2012-10-17"
        }
    )
    create_date           = "2021-05-31T11:17:08Z"
    force_detach_policies = true
    id                    = "test-eks-B5SenpZF20210531111707434300000001"
    managed_policy_arns   = []
    max_session_duration  = 3600
    name                  = "test-eks-B5SenpZF20210531111707434300000001"
    name_prefix           = "test-eks-B5SenpZF"
    path                  = "/"
    tags                  = {
        "Environment" = "test"
        "GithubOrg"   = "terraform-aws-modules"
        "GithubRepo"  = "terraform-aws-eks"
    }
    tags_all              = {
        "Environment" = "test"
        "GithubOrg"   = "terraform-aws-modules"
        "GithubRepo"  = "terraform-aws-eks"
    }
    unique_id             = "AROAUO****************"

    inline_policy {}
}

Added cluster_iam_role_name = "awesomerole" into basic example:

$ terraform state show module.eks.aws_iam_role.cluster[0]
# module.eks.aws_iam_role.cluster[0]:
resource "aws_iam_role" "cluster" {
    arn                   = "arn:aws:iam::3********:role/awesomerole"
    assume_role_policy    = jsonencode(
        {
            Statement = [
                {
                    Action    = "sts:AssumeRole"
                    Effect    = "Allow"
                    Principal = {
                        Service = "eks.amazonaws.com"
                    }
                    Sid       = "EKSClusterAssumeRole"
                },
            ]
            Version   = "2012-10-17"
        }
    )
    create_date           = "2021-05-31T11:13:53Z"
    force_detach_policies = true
    id                    = "awesomerole"
    managed_policy_arns   = []
    max_session_duration  = 3600
    name                  = "awesomerole"
    path                  = "/"
    tags                  = {
        "Environment" = "test"
        "GithubOrg"   = "terraform-aws-modules"
        "GithubRepo"  = "terraform-aws-eks"
    }
    tags_all              = {
        "Environment" = "test"
        "GithubOrg"   = "terraform-aws-modules"
        "GithubRepo"  = "terraform-aws-eks"
    }
    unique_id             = "AROAUO****************"

    inline_policy {}
}

[barryib]

To me, both writing are similar.

1. name_prefix = var.cluster_iam_role_name != "" ? null : var.cluster_name
   If var.cluster_iam_role_name is not empty, then return null otherwise return the cluster name
   => This mean also: return null if var.cluster_iam_role_name is empty (similar to 2)

2. name_prefix = var.cluster_iam_role_name == "" ? var.cluster_name : null
   If var.cluster_iam_role_name is empty , then return cluster name or null otherwise.
   => This mean also, return null if var.cluster_iam_role_name is not empty (similar to 1).

Please see:

> "x" != "" ? null : "y"
tostring(null)
> "" != "" ? null : "y"
"y"
> "x" == "" ? "y" : null
tostring(null)
> "" == "" ? "y" : null
"y"

[daroga0002]

they are same, but if you will take a look on name conditional this is issue of real problem.

[barryib]

  name_prefix           = var.cluster_iam_role_name != "" ? null : var.cluster_name
  name                  = var.cluster_iam_role_name != "" ? var.cluster_iam_role_name : null

If we managed IAM resources (var.manage_cluster_iam_resources=true) and var.cluster_iam_role_name is defined, then use var.cluster_iam_role_name as role name and don't use name_prefix. Otherwise set the name_prefix.

This is what I read. I'm probably missing something.

[barryib]

Actually, the var.worker_role_name behave the same. See https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/workers.tf#L448-L449

[daroga0002]

ahhh, yup you are right, closing PR

[barryib]

@daroga0002 Thanks for working on this. But your tests results come from your PR or from the master branch ?

[daroga0002]

@daroga0002 Thanks for working on this. But your tests results come from your PR or from the master branch ?

from my fix, master branch has bad logic (conditional in name and name_prefix is same what is making it bug)

[daroga0002]

Behaviour is good, it was my fault in interpreting code

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