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 Terraform var for Tilegarden function name #729

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

KlaasH
Copy link
Contributor

@KlaasH KlaasH commented Mar 7, 2019

Overview

Adds a tfvar for the name of the Tilegarden Lambda function and uses it to restrict the Lambda execution permission on the TilegardenExecutor role.

See discussion on PR #719

Also

  • Uses the tfvar to set the name of the executor role.
  • Renames the variable on the Tilegarden side to LAMBDA_FUNCTION_NAME, since that's what it's used for.
  • Removes the LAMBDA_ROLE variable from the Tilegarden .env example and uses the LAMBDA_FUNCTION_NAME value with 'Executor' appended to match what Terraform does when creating it.

What this does not do is resolve #714 by incorporating the Lambda-warming scheduled task into Terraform. It turns out giving CloudWatch the permission to invoke a Lambda function is done by defining a policy on the Lambda function and can't be done via IAM role or any sort of resource wildcard.

So trying to define the scheduled warming event before the actual Lambda function exists isn't possible, which means putting it in the Terraform config would add a bunch more complexity to the circular dependency between the Terraform and Claudia deployment steps.

Using a variable to set the function name is useful, though, and since I did that first on the way to configuring the CloudWatch rule and discovering this problem, we might as well keep it.

Notes

Here's a branch with the CloudWatch rule/event config.

Testing Instructions

If run for Staging, starting with current develop deployed:

  • With the tilegarden_function_name variable set to the existing function name (pfbTilegardenStaging) infra plan (after the usual setup per the deployment instructions) should show only the usual service task updates (caused by the batch job revision changing) plus a change to the InvokeLambda permission, changing it to a more restrictive resources clause.
  • With the variable set to something else, infra plan should show that plus a bunch of changes to the Executor role to remove the one with the old name and create one with the new name.

Checklist

- [ ] Add entry to CHANGELOG.md

Adds a tfvar for the name of the Tilegarden Lambda function and uses it
to restrict the Lambda execution permission on the TilegardenExecutor role.

See [discussion on PR #719](#719 (comment)))

Also
- Uses the tfvar to set the name of the executor role.
- Renames the variable on the Tilegarden side to 'LAMBDA_FUNCTION_NAME',
since that's what it's used for.
- Removes the 'LAMBDA_ROLE' variable from the Tilegarden .env example and
uses the 'LAMBDA_FUNCTION_NAME' value with 'Executor' appended to match
what Terraform does when creating it.
@KlaasH KlaasH requested a review from ddohler March 7, 2019 20:02
Copy link
Contributor

@ddohler ddohler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected. Super annoying that Lambda uses its own special permissions system for this though. Aside from the question I asked, I noticed that GitHub's issue-resolving logic isn't smart enough to understand the negation in What this does not do is resolve #714 so I think #714 will get closed if this is merged without editing the PR text.

@@ -162,3 +162,6 @@ variable "tilegarden_api_gateway_domain_name" {}
variable "cloudfront_price_class" {
default = "PriceClass_100"
}

# Should be environment-specific and match the value in the Tilegarden .env file
variable "tilegarden_function_name" {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to get added to the .tfvars files on S3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, will do.

@KlaasH
Copy link
Contributor Author

KlaasH commented Mar 8, 2019

I noticed that GitHub's issue-resolving logic isn't smart enough to understand the negation

I wondered whether it would pick that out. I wasn't sure if keywords like that had to be at the beginning of a line. I guess it's probably nice that they don't in some cases, but yeah, false positives...

@KlaasH KlaasH merged commit cc276ab into develop Mar 8, 2019
@KlaasH KlaasH mentioned this pull request Mar 8, 2019
@KlaasH KlaasH deleted the feature/kjh/lambda-function-name-tfvar branch March 8, 2019 16:01
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.

Add lambda-warming scheduled task to Terraform config
3 participants