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

Add support for declaring simple lambda permissions in-module #69

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

jpalomaki
Copy link
Contributor

@jpalomaki jpalomaki commented Jul 5, 2024

what

Allow lambda configuration author to optionally declare lambda:InvokeFunction lambda permissions directly in this module.

More complex permissions configurations could still be done outside of this module.

why

This co-locates permissions related to the lambda in the module configuration (where we also declare lambda IAM role permissions), which can help a reader understand where the lambda is invoked from, e.g. in cases where the actual event sources are declared in a different root configuration.

In our specific use case, we use terragrunt to deploy the lambda function (straight from terraform registry module), so this feature would also help us avoid having to create a wrapper module just to add the necessary permission resources.

questions

  1. Because we support terraform 0.14+ (no default value support for optionals), we scope this to just the specific action lambda:InvokeFunction and keep the number of attributes a user has to fill in, small. Does this look like a sane approach (looks like it could cover a lot of ground already, judging by examples)?
  2. Because we support terraform 0.14+, we can't do replace_triggered_by. Not entirely sure if that is a problem though, since we just attach the permission to the function itself (and not an alias or version)
  3. The resource for_each is keyed by list index, which isn't ideal, since it would force recreations if items are shuffled/inserted

references

Slack discussion, cc/ @osterman

@jpalomaki jpalomaki requested review from a team as code owners July 5, 2024 08:23
@jpalomaki
Copy link
Contributor Author

FWIW, I've run a quick smoke test using my fork branch as source

@mergify mergify bot added the triage Needs triage label Jul 5, 2024
@jpalomaki jpalomaki force-pushed the lambda-permissions branch 5 times, most recently from c6a13d3 to 3e57260 Compare July 8, 2024 07:05
@dudymas
Copy link

dudymas commented Jul 12, 2024

/terratest

@jpalomaki jpalomaki force-pushed the lambda-permissions branch 3 times, most recently from 5c600af to db40e88 Compare August 14, 2024 06:23
@jpalomaki
Copy link
Contributor Author

@dudymas I've now added a depends_on that ought to fix the race, can you re-run the tests?

@dudymas
Copy link

dudymas commented Aug 14, 2024

/terratest

@dudymas
Copy link

dudymas commented Aug 14, 2024

Looking good! Now, you'll just need to make sure when enabled=false that the bucket and other resources aren't created. Sorry for the rigor, but we've got to make sure disabling a component truly does not create any resources. See test step here:

testNoChanges(t, "../../examples/complete")

@jpalomaki
Copy link
Contributor Author

@dudymas All right, I've now added count's to the new resources

@dudymas
Copy link

dudymas commented Aug 16, 2024

/terratest

@dudymas
Copy link

dudymas commented Aug 16, 2024

unfortunately, terraform doesn't handle counts of 0 very well on its own. I recommend using the join() function for the bucket arn, similar to here:

join("", data.aws_partition.current.*.partition),

@jpalomaki jpalomaki force-pushed the lambda-permissions branch 2 times, most recently from 2e0df38 to 16cfd56 Compare August 19, 2024 04:56
@jpalomaki
Copy link
Contributor Author

unfortunately, terraform doesn't handle counts of 0 very well on its own. I recommend using the join() function for the bucket arn, similar to here:

join("", data.aws_partition.current.*.partition),

Oh right, the module instance itself isn't guarded by a count. Should be fixed now.

@dudymas
Copy link

dudymas commented Aug 19, 2024

/terratest

1 similar comment
@gberenice
Copy link

/terratest

@gberenice
Copy link

@jpalomaki could you please address this linter error:

Raw Output:
main.tf:109:50: warning: List items should be accessed using square brackets ()

Should be like:

join("", aws_s3_bucket.example[*].arn)

@jpalomaki
Copy link
Contributor Author

/terratest

@jpalomaki
Copy link
Contributor Author

@dudymas @gberenice I've now fixed that linter warning, but looks like I can't run terratest myself

@gberenice
Copy link

/terratest

@jpalomaki
Copy link
Contributor Author

@gberenice I've now fixed the apparent formatting errors, please retest

@gberenice
Copy link

/terratest

@gberenice
Copy link

@jpalomaki thanks for contribution!

@mergify mergify bot removed the triage Needs triage label Aug 22, 2024
@gberenice gberenice merged commit 670c7fa into cloudposse:main Aug 22, 2024
27 checks passed
Copy link

These changes were released in v0.5.6.

@jpalomaki jpalomaki deleted the lambda-permissions branch August 22, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants