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: Allow for custom_iam_policy_arns that are unknown at apply #46

Merged
merged 1 commit into from Oct 11, 2023
Merged

fix: Allow for custom_iam_policy_arns that are unknown at apply #46

merged 1 commit into from Oct 11, 2023

Conversation

natemccurdy
Copy link
Contributor

what

Replace the toset() in the aws_iam_role_policy_attachment resource's for_each attribute with a map of name:ARN pairs.

why

Prior to this patch, specifying custom_iam_policy_arns for IAM Policies that do not exist yet and would be created in the same Terraform run that creates the Lambda Execution Role would cause the following error:

│ Error: Invalid for_each argument
│
│   on .terraform/modules/foo.test_lambda/iam-role.tf line 81, in resource "aws_iam_role_policy_attachment" "custom":
│   81:   for_each = local.enabled && length(var.custom_iam_policy_arns) > 0 ? var.custom_iam_policy_arns : toset([])
│     ├────────────────
│     │ local.enabled is true
│     │ var.custom_iam_policy_arns is set of string with 3 elements
│
│ The "for_each" set includes values derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will identify the instances of this resource.
│
│ When working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time results.
│
│ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully converge.

This is due to the ARN's of those policies not being known at apply time and the usage of toset() in the aws_iam_role_policy_attachment resource's for_each parameter. As the set's values are unknown at apply time, Terraform can't create a dependency graph.

references

Similar issues with similar fixes in other CloudPosse modules:

@natemccurdy natemccurdy requested review from a team as code owners October 11, 2023 00:23
@natemccurdy natemccurdy changed the title fix: Allow for custom_iam_policy_arns that don't exist yet fix: Allow for custom_iam_policy_arns that are unknown at apply Oct 11, 2023
iam-role.tf Outdated Show resolved Hide resolved
@Gowiem
Copy link
Member

Gowiem commented Oct 11, 2023

@natemccurdy this looks great. Suggested a simple whitespace change. I just pinged at Cloud Posse again on getting an approval on the README issues you're running into in your other PRs, so we'll get those merged first, you can rebase this PR, and then we'll get it merged. Thanks for the patience!

@aknysh
Copy link
Member

aknysh commented Oct 11, 2023

@natemccurdy thank you

can you please run the following commands and commit the changes

make init
make github/init
make readme

@natemccurdy
Copy link
Contributor Author

natemccurdy commented Oct 11, 2023

@natemccurdy thank you

can you please run the following commands and commit the changes

Hi @aknysh, thanks for looking. I've got 3 other PR's with the exact same issue of the boilerplate and readmes being out of sync. I fixed that in one of those PR's, #45 , so I was hoping that rather than do the same thing 3 times (one for each PR), I can just rebase after merging #45 and only have to do it once.

PROBLEM:

Prior to this, specifying `custom_iam_policy_arns` for IAM Policies that do not
exist yet and would be created in the same Terraform run that creates the
Lambda Execution Role would cause the following error:

```
│ Error: Invalid for_each argument
│
│   on .terraform/modules/foo.test_lambda/iam-role.tf line 81, in resource "aws_iam_role_policy_attachment" "custom":
│   81:   for_each = local.enabled && length(var.custom_iam_policy_arns) > 0 ? var.custom_iam_policy_arns : toset([])
│     ├────────────────
│     │ local.enabled is true
│     │ var.custom_iam_policy_arns is set of string with 3 elements
│
│ The "for_each" set includes values derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will identify the instances of this resource.
│
│ When working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time results.
│
│ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully converge.
```

This is due to the ARN's of those policies not being known and the usage of sets in `for_each` for the `aws_iam_role_policy_attachment` resource. As the set's values are unknown at apply time, Terraform can't create a dependency graph.

SOLUTION:

* Don't use `toset()` for the `for_each` in the `aws_iam_role_policy_attachment` resource.
* Build a map of ARNs to attach to the Lambda execution role. Use this map as the value of `for_each`.

OUTCOME:

* Terraform doesn't error when using `aws_iam_role_policy_attachment` for policies that don't exist yet but will exist after the apply.
@aknysh
Copy link
Member

aknysh commented Oct 11, 2023

@natemccurdy thank you
can you please run the following commands and commit the changes

Hi @aknysh, thanks for looking. I've got 3 other PR's with the exact same issue of the boilerplate and readmes being out of sync. I fixed that in one of those PR's, #45 , so I was hoping that rather than do the same thing 3 times (one for each PR), I can just rebase after merging #45 and only have to do it once.

#45 is approved and merged, thank you. Please rebase the other PRs

@natemccurdy
Copy link
Contributor Author

Done. Thanks for your help @aknysh !

@milldr
Copy link
Sponsor Member

milldr commented Oct 11, 2023

/terratest

@milldr milldr added the no-release Do not create a new release (wait for additional code changes) label Oct 11, 2023
@milldr milldr merged commit 517135d into cloudposse:main Oct 11, 2023
19 checks passed
@natemccurdy natemccurdy deleted the fix/iam_for_each branch October 11, 2023 21:57
@natemccurdy
Copy link
Contributor Author

Thanks @milldr 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-release Do not create a new release (wait for additional code changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants