Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in setting dynamic encryption_configuration value #206

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Fix bug in setting dynamic encryption_configuration value #206

merged 5 commits into from
Nov 15, 2023

Conversation

LawrenceWarren
Copy link
Contributor

@LawrenceWarren LawrenceWarren commented Nov 10, 2023

what

  • When trying to create an S3 bucket, the following error is encountered:
Error: Invalid dynamic for_each value

  on .terraform/main.tf line 225, in resource "aws_s3_bucket_replication_configuration" "default":
 225:           for_each = try(compact(concat(
 226:             [try(rule.value.destination.encryption_configuration.replica_kms_key_id, "")],
 227:             [try(rule.value.destination.replica_kms_key_id, "")]
 228:           ))[0], [])
    ├────────────────
    │ rule.value.destination.encryption_configuration is null
    │ rule.value.destination.replica_kms_key_id is "arn:aws:kms:my-region:my-account-id:my-key-alias"

Cannot use a string value in for_each. An iterable collection is required.
  • This is caused in my case by having s3_replication_rules.destination.encryption_configuration.replica_kms_key_id set.

why

  • There is a bug when trying to create an S3 bucket, which causes an error that stops the bucket being created

    • Basically, there are two attributes that do the same thing (for backwards compatability)
      • s3_replication_rules.destination.encryption_configuration.replica_kms_key_id (newer)
      • s3_replication_rules.destination.replica_kms_key_id (older)
    • There is logic to:
      • A) use the newer of these two attributes
      • B) fall back to the older of the attributes if it is set and the newer is not
      • C) fall back to an empty array if nothing is set
    • There is a bug in steps A/B, where by selecting one or the other, we end up with the string value, and not an iterable
    • The simplest solution, which I have tested successfully on existing buckets, is to wrap the output of that logic in a list
  • This error is easily replicable by trying compact(concat([try("string", "")], [try("string", "")]))[0] in the Terraform console, which is a simplified version of the existing logic used above

  • The table below demonstrates the possible values of the existing code - you can see the outputs for value 2, value 3, and value 4 are not lists:

Key Value 1 Value 2 Value 3 Value 4
newer null "string1" null "string1"
older null null "string2" "string2"
output [] "string1" "string2" "string1"

@LawrenceWarren
Copy link
Contributor Author

@johncblandii @joe-niland can I get a review please?

@joe-niland
Copy link
Member

Hi @LawrenceWarren

Could you please run the following and commit the changes?

make init
make github/init
make readme

@LawrenceWarren
Copy link
Contributor Author

That's done.

@Nuru
Copy link
Contributor

Nuru commented Nov 15, 2023

/terratest

@Nuru Nuru added the bugfix Change that restores intended behavior label Nov 15, 2023
@Nuru Nuru enabled auto-merge (squash) November 15, 2023 21:41
@Nuru Nuru merged commit eaaee29 into cloudposse:main Nov 15, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Change that restores intended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants