cloudposse / terraform-aws-lambda-function

A module for launching Lambda Fuctions
https://cloudposse.com/accelerate
Apache License 2.0
30 stars 40 forks source link

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

Closed jpalomaki closed 3 weeks ago

jpalomaki commented 2 months ago

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 commented 2 months ago

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

dudymas commented 2 months ago

/terratest

jpalomaki commented 1 month ago

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

dudymas commented 1 month ago

/terratest

dudymas commented 1 month ago

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: https://github.com/cloudposse/terraform-aws-lambda-function/blob/db40e8895f1a4ff77c82d5961fb84efdf86acd7d/test/src/examples_complete_test.go#L62

jpalomaki commented 4 weeks ago

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

dudymas commented 4 weeks ago

/terratest

dudymas commented 4 weeks ago

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: https://github.com/cloudposse/terraform-aws-lambda-function/blob/b9923cf8a0f9b6cfd2664565f17f75c57bc51f24/examples/complete/main.tf#L10

jpalomaki commented 3 weeks ago

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:

https://github.com/cloudposse/terraform-aws-lambda-function/blob/b9923cf8a0f9b6cfd2664565f17f75c57bc51f24/examples/complete/main.tf#L10

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

dudymas commented 3 weeks ago

/terratest

gberenice commented 3 weeks ago

/terratest

gberenice commented 3 weeks ago

@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 commented 3 weeks ago

/terratest

jpalomaki commented 3 weeks ago

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

gberenice commented 3 weeks ago

/terratest

jpalomaki commented 3 weeks ago

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

gberenice commented 3 weeks ago

/terratest

gberenice commented 3 weeks ago

@jpalomaki thanks for contribution!

github-actions[bot] commented 3 weeks ago

These changes were released in v0.5.6.