fix: Replace default in encryption_config on null

[AerDragon]

Description

Replaced the default value in variable - encryption_config because of the check here

Motivation and Context

Fixes #3469
Because if you don't use encryption_config and don't explicitly pass null, then the provider crashes with an error:

  │ Error: Missing required argument
  │ 
  │   with aws_eks_cluster.this[0],
  │   on main.tf line 36, in resource "aws_eks_cluster" "this":
  │   36: resource "aws_eks_cluster" "this" {
  │ 
  │ The argument "encryption_config.0.provider.0.key_arn" is required, but no
  │ definition was found.
  ╵
  
  exit status 1

Lines 558 & 566 (main.tf) - Policy creation:

count = local.create_iam_role && var.attach_encryption_policy && local.enable_encryption_config && var.provider_key_arn != null ? 1 : 0

This tries to create encryption policies even when provider_key_arn is null.

Breaking Changes

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects
• I have executed pre-commit run -a on my pull request

[bryantbiggs]

#3469 (comment)

[adamwolfe-tc]

@bryantbiggs I came here for this same thing while trying to migrate from v20.x.x. i get that "all you need is null" but i respectfully request reconsidering this PR because:

1. if i follow the examples or do any kind of vanilla module usage i'll get encryption that i did not ask for

2. migrating from aws provider v5 and this module v20 will fail for seemingly no reason until you dive into the code (and issues and PRs) here

3. as already mentioned the {} is quite misleading meaning we spend time trying to figure out what is wrong with our usage.

4. if null is all we need, why is it not already that? this is what defaults are intended to do, right?

[bryantbiggs]

1. if i follow the examples or do any kind of vanilla module usage i'll get encryption that i did not ask for

Correct - we default to enabling encryption because its a recommended security practice per AWS. Would we do it differently today? Sure! EKS now encrypts secrets by default with an AWS provided key, but that is a very recent change - for years, secrets were not encrypted unless you enabled it and provided a custom KMS key. Ergo, we tried to do the "right thing" by users to make that easier.

2. migrating from aws provider v5 and this module v20 will fail for seemingly no reason until you dive into the code (and issues and PRs) here

I don't know what this means - what fails? to someone who has zero context about your configurations, how would they approach understanding this 😅

3. as already mentioned the {} is quite misleading meaning we spend time trying to figure out what is wrong with our usage.

What is mis-leading? This is Terraform's way of saying "give me the variable optional attribute defaults". I didn't design it, just used it because folks wanted us to use variable optional attributes because they said it gives more information about the API of the variable shapes ¯\_(ツ)_/¯

4. if null is all we need, why is it not already that? this is what defaults are intended to do, right?

Lots to unpack here so lets break it down further:

if null is all we need, why is it not already that?

it may be what you need, if you want to disable encryption with a custom KMS key. Why its not already that, see what I provided above already

this is what defaults are intended to do, right?

I don't follow - the default of {} is what is intended and its doing what is intended - enabling secret encryption by default with a custom KMS key 😬

[adamwolfe-tc]

2. migrating from aws provider v5 and this module v20 will fail for seemingly no reason until you dive into the code (and issues and PRs) here

I don't know what this means - what fails? to someone who has zero context about your configurations, how would they approach understanding this 😅

this is what the original poster put in their description. the default value expects other values to exist that will not as a default.

  │ 
  │   with aws_eks_cluster.this[0],
  │   on main.tf line 36, in resource "aws_eks_cluster" "this":
  │   36: resource "aws_eks_cluster" "this" {
  │ 
  │ The argument "encryption_config.0.provider.0.key_arn" is required, but no
  │ definition was found.

[bryantbiggs]

2. migrating from aws provider v5 and this module v20 will fail for seemingly no reason until you dive into the code (and issues and PRs) here

I don't know what this means - what fails? to someone who has zero context about your configurations, how would they approach understanding this 😅

this is what the original poster put in their description. the default value expects other values to exist that will not as a default.

  │ 
  │   with aws_eks_cluster.this[0],
  │   on main.tf line 36, in resource "aws_eks_cluster" "this":
  │   36: resource "aws_eks_cluster" "this" {
  │ 
  │ The argument "encryption_config.0.provider.0.key_arn" is required, but no
  │ definition was found.

I would love to see you produce a reproduction for this - the number of unknown unknowns is endless

[adamwolfe-tc]

going a bit further into this, previously on v20.x.x {} would disable the encryption. and leaving it as {} when going to 21.x.x will then put users in the situation OP describes.

[bryantbiggs]

ok - but we've clarified that when upgrading to v21.x that null is to be used to disable, correct? so change {} to null

[adamwolfe-tc]

i'm simply inclined to agree with OP as i ran into the exact same situation independently and came to the same conclusion and eventually ended up here when searching. it really looks like the comparison was goofed from what it used to be vs what OP pointed out

but if it's not gonna change then perhaps it could be mentioned in the upgrade guide? or the variable description? it's an undocumented change in behavior at the least. people shouldn't have to search through issues to find the correct way to deal with defaults.

to be clear i'm not debating this to handle my specific situation. it's on behalf of anyone else that gets pinched by this change in the future.

[bryantbiggs]

Submit a PR if you think it will help