Dependant kms module resource is being created when create_kms_key is set to false. Breaks in air-gapped environment.

[jbehrends]

Description

This is happening in the latest version (v19.18.0) of this TF module.

I'm working in an air-gapped environment with no internet access. The environment where TF is being ran has all required providers present and this module mirrored, so there is no need to fetch them from the internet. We have an internal TF module which fully manages KMS keys already. So when calling this module we set "create_kms_key" to false and then pass our own KMS arn under "cluster_encryption_config" to the module for use by the cluster's resources.

Code snippet

...
  create_kms_key = false 
  cluster_encryption_config = {
    provider_key_arn = local.aws_kms_key.by_alias.arn
    resources        = ["secrets"]
  }
...

and the resulting error:

╷
│ Error: Failed to download module
│ 
│   on .terraform/modules/eks.eks/main.tf line 123:
│  123: module "kms" {
│ 
│ Could not download module "kms" (.terraform/modules/eks.eks/main.tf:123)
│ source code from
│ "git::https://github.com/terraform-aws-modules/terraform-aws-kms?ref=v1.1.0":
│ error downloading
│ 'https://github.com/terraform-aws-modules/terraform-aws-kms?ref=v1.1.0':
│ /usr/bin/git exited with 128: Cloning into
│ '.terraform/modules/eks.eks.kms'...
│ fatal: unable to access
│ 'https://github.com/terraform-aws-modules/terraform-aws-kms/': Failed to
│ connect to github.com port 443 after 7018 ms: Couldn't connect to server
│ .
╵

I would expect that when setting "create_kms_key = false" TF would not attempt to call that module at all.

Possibly adding a "count" argument to the KMS module call would resolve this? Or is there some other solution here I've overlooked?

• [ x ] ✋ I have searched the open/closed issues and my issue is not listed.

[bryantbiggs]

unfortunately this is Terraform's behavior - any source definitions will be pulled whether they are used or not. that goes for providers and modules

This means your options are:

1. Remove the KMS module definition from your mirrored copy
2. Add the KMS module to your mirror

[tu-strobert]

So we attempted to add a count param to the module and although it does stop the data lookup resources. it does look like it is downloading the module source still :(

we'll have to see how our env is getting modules to see if option #2 is viable or not.

Even though it will still download the module source, would it be acceptable to have the change to do the count change so the unused data resources are not utilized?

[bryantbiggs]

so the unused data resources are not utilized

Which data sources?

[tu-strobert]

I think these are the two:
module.eks.module.kms.data.aws_partition.current
module.eks.module.kms.data.aws_caller_identity.current

https://github.com/terraform-aws-modules/terraform-aws-kms/blob/master/main.tf#L1-L2

not huge overhead, but the making the module.kms optional from a terraform sense and not instantiating it if we won't be using it seems to make sense.

[tu-strobert]

Here is our current draft of a diff that we are testing with (I think some of the var.create_key_kms may need to get changed to the local.create_kms, but this is the current rev):

diff --git a/main.tf b/main.tf
index 52d36aa..a8c6419 100644
--- a/main.tf
+++ b/main.tf
@@ -64,7 +64,7 @@ resource "aws_eks_cluster" "this" {

     content {
       provider {
-        key_arn = var.create_kms_key ? module.kms.key_arn : encryption_config.value.provider_key_arn
+        key_arn = var.create_kms_key ? module.kms[0].key_arn : encryption_config.value.provider_key_arn
       }
       resources = encryption_config.value.resources
     }
@@ -120,11 +120,16 @@ resource "aws_cloudwatch_log_group" "this" {
 # KMS Key
 ################################################################################

+locals {
+  create_kms = local.create && var.create_kms_key && local.enable_cluster_encryption_config # not valid on Outposts
+}
+
 module "kms" {
   source  = "terraform-aws-modules/kms/aws"
   version = "1.1.0" # Note - be mindful of Terraform/provider version compatibility between modules
+  count   = local.create_kms ? 1 : 0

-  create = local.create && var.create_kms_key && local.enable_cluster_encryption_config # not valid on Outposts
+  create = local.create_kms

   description             = coalesce(var.kms_key_description, "${var.cluster_name} cluster encryption key")
   key_usage               = "ENCRYPT_DECRYPT"
@@ -367,7 +372,7 @@ resource "aws_iam_policy" "cluster_encryption" {
           "kms:DescribeKey",
         ]
         Effect   = "Allow"
-        Resource = var.create_kms_key ? module.kms.key_arn : var.cluster_encryption_config.provider_key_arn
+        Resource = var.create_kms_key ? module.kms[0].key_arn : var.cluster_encryption_config.provider_key_arn
       },
     ]
   })
diff --git a/outputs.tf b/outputs.tf
index ea02a3a..6c8edab 100644
--- a/outputs.tf
+++ b/outputs.tf
@@ -58,17 +58,17 @@ output "cluster_primary_security_group_id" {

 output "kms_key_arn" {
   description = "The Amazon Resource Name (ARN) of the key"
-  value       = module.kms.key_arn
+  value       = local.create_kms ? module.kms[0].key_arn : null
 }

 output "kms_key_id" {
   description = "The globally unique identifier for the key"
-  value       = module.kms.key_id
+  value       = local.create_kms ? module.kms[0].key_id : null
 }

 output "kms_key_policy" {
   description = "The IAM resource policy set on the key"
-  value       = module.kms.key_policy
+  value       = local.create_kms ? module.kms[0].key_policy : null
 }

 ################################################################################

[bryantbiggs]

I've updated those here terraform-aws-modules/terraform-aws-kms#25 - we should be able bump the module version used once thats released

[tu-strobert]

ah, that works too. :)
(and I can see probably better aligns with the rest of the kms module design)

Appreciate the quick response!

Hey, on this line in the eks module:
key_arn = var.create_kms_key ? module.kms.key_arn : var.encryption_config.value.provider_key_arn

(and I haven't dug deep to see if this boolean condition could even be valid)
if var.create_kms_key is true and local.enable_cluster_encryption_config ends up false, the kms module wouldn't be created but that key_arn would still ref it...

[bryantbiggs]

If encryption is not enabled, then the KMS module does not create a key

terraform-aws-eks/main.tf

Line 127 in e4c5098

create = local.create && var.create_kms_key && local.enable_cluster_encryption_config # not valid on Outposts

even if you have create_kms_key = true

and we don't enter the loop that assigns the key (either created or provided) to be used

terraform-aws-eks/main.tf

Line 63 in e4c5098

for_each = local.enable_cluster_encryption_config ? [var.cluster_encryption_config] : []

This line

terraform-aws-eks/main.tf

Line 67 in e4c5098

key_arn = var.create_kms_key ? module.kms.key_arn : encryption_config.value.provider_key_arn

may seem redundant, but its really just there to determine if you want the module to use the created key or the key thats been externally created

Does that help clarify?

[tu-strobert]

it does. thank you.

yeah, I hadn't dove into the boolean conditions fully, the conditional caught my eye when I was seeing what ref'd module.kms. so I was thinking you really couldn't get into that condition, appreciate you clarifying the why that is the case :)

[bryantbiggs]

It can be a fickle beast to navigate 😅

[jbehrends]

Thanks for the improvements made here.
Ok! So it appears we're going to mirror the kms module internally and modify our mirrored copy of this module. I don't think there is anything else to discuss at this point. So unless there are any objections I'll close this issue.

[github-actions]

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.