missing conditional for datasource

[daroga0002]

Description

https://github.com/terraform-aws-modules/terraform-aws-eks/pull/1580 introduces bug which occurs when var.create_eks is setup for false. This is because https://github.com/terraform-aws-modules/terraform-aws-eks/blob/f198efd2c4ed695c8018ee343701454069806790/modules/node_groups/locals.tf#L1-L3 always require a input (which in case of create_eks is optional).

In node group we are doing this by if https://github.com/terraform-aws-modules/terraform-aws-eks/blob/f198efd2c4ed695c8018ee343701454069806790/modules/node_groups/locals.tf#L43 so this is respected there.

We need add to data source something like: https://github.com/terraform-aws-modules/terraform-aws-eks/blob/f198efd2c4ed695c8018ee343701454069806790/modules/fargate/main.tf#L13

⚠️ Note

Before you submit an issue, please perform the following first:

1. Remove the local .terraform directory (! ONLY if state is stored remotely, which hopefully you are following that best practice!): rm -rf .terraform/
2. Re-initialize the project root to pull down modules: terraform init
3. Re-attempt your terraform plan or apply and check if the issue still persists

Versions

any versions of tf and aws provider

Reproduction

run examples/complete

Expected behavior

It should not throw error

Actual behavior

$ terraform plan
╷
│ Error: "name" length must be between 1-100 characters: ""
│ 
│   with module.disabled_node_groups.data.aws_eks_cluster.default,
│   on ../../modules/node_groups/locals.tf line 2, in data "aws_eks_cluster" "default":
│    2:   name = var.cluster_name
│ 
╵
╷
│ Error: "name" doesn't comply with restrictions ("^[0-9A-Za-z][A-Za-z0-9\\-_]+$"): ""
│ 
│   with module.disabled_node_groups.data.aws_eks_cluster.default,
│   on ../../modules/node_groups/locals.tf line 2, in data "aws_eks_cluster" "default":
│    2:   name = var.cluster_name
│ 
╵
╷
│ Error: "name" length must be between 1-100 characters: ""
│ 
│   with module.disabled_eks.module.node_groups.data.aws_eks_cluster.default,
│   on ../../modules/node_groups/locals.tf line 2, in data "aws_eks_cluster" "default":
│    2:   name = var.cluster_name
│ 
╵
╷
│ Error: "name" doesn't comply with restrictions ("^[0-9A-Za-z][A-Za-z0-9\\-_]+$"): ""
│ 
│   with module.disabled_eks.module.node_groups.data.aws_eks_cluster.default,
│   on ../../modules/node_groups/locals.tf line 2, in data "aws_eks_cluster" "default":
│    2:   name = var.cluster_name
│ 

[stevehipwell]

@daroga0002 I assume that this issue is only impacting where var.create_eks == false and not what was reported yesterday?

I saw this behaviour yesterday and assumed that it wasn't an issue as it was previously working, but one reviewing again today I can see the issue. Off the top of my head a fix would be to set cluster_name = var.create_eks ? coalescelist(aws_eks_cluster.this[*].name, [""])[0] : var.cluster_name.

Furthermore should the module be able to create MNGs if var.create_eks == false?

[daroga0002]

@daroga0002 I assume that this issue is only impacting where var.create_eks == false and not what was reported yesterday?

yes only case where we have var.create_eks == false

Furthermore should the module be able to create MNGs if var.create_eks == false?

Node groups are not created when we have var.create_eks == false because of this trick: https://github.com/terraform-aws-modules/terraform-aws-eks/blob/f198efd2c4ed695c8018ee343701454069806790/modules/node_groups/locals.tf#L43

[stevehipwell]

@daroga0002 I'm aware of the trick to block their creation, but they could be created if that was changed and the input variables required could be passed in. The question was more if they "should" be able to be created?

RE a fix, do you have a preferred solution? The code I added above won't work as var.cluster_name is optional, although I'm not sure why it needs to be?

[daroga0002]

RE a fix, do you have a preferred solution? The code I added above won't work as var.cluster_name is optional, although I'm not sure why it needs to be?

PR is created, var.cluster_name is just optional because to cover situation when you have create_eks=false.

In general this is some legacy when modules where not supporting count in terraform so then was created this trigger. As of now probably the best should be just removing this but this will be huge breaking change.

[stevehipwell]

Thanks for the fix.

RE empty var.cluster_name and assuming that var.create_eks = false is meant to mimic the official for_each = var.workspace_create_eks ? ["default"] : [] or count = var.workspace_create_eks ? 1 : 0 conditional module patterns, shouldn't the variables be required if they are needed by the module with the only change for create/not-create being var.create_eks? This would still need your fix as it would support something like var.cluster_name = var.workspace_create_eks ? "my-cluster" : "" but the idiomatic use would be var.cluster_name = "my-cluster".

[daroga0002]

in general var.create_eks is as per me bad design as it was never working correctly because other dependencies like kubernetes provider configuration and etc.

I tried to use it in development and changing var.create_eks=true to var.create_eks=false was always dropping a bunch of problems. Ended by just holding EKS definition in eks.tf file which I was renaming/removing in time when not required EKS

[github-actions[bot]]

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.