Page MenuHomePhabricator

[terraform] [4/n] search_index_lambda terraform configuration options
ClosedPublic

Authored by will on Nov 13 2023, 11:43 PM.
Tags
None
Referenced Files
F3356278: D9878.id35694.diff
Sat, Nov 23, 5:56 PM
F3356276: D9878.id34346.diff
Sat, Nov 23, 5:56 PM
F3356243: D9878.id34760.diff
Sat, Nov 23, 5:39 PM
F3356218: D9878.id33530.diff
Sat, Nov 23, 5:28 PM
F3356139: D9878.id35230.diff
Sat, Nov 23, 5:05 PM
F3356138: D9878.id34759.diff
Sat, Nov 23, 5:04 PM
F3356117: D9878.id35252.diff
Sat, Nov 23, 4:57 PM
F3356101: D9878.id34766.diff
Sat, Nov 23, 4:51 PM
Subscribers

Details

Reviewers
varun
bartek
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rCOMM4caa544f818b: [terraform] [4/n] search_index_lambda terraform configuration options
Summary

This adds support for search_index_lambda. This lambda consumes our DynamoDB stream for identity-users and then pushes these changes to OpenSearch

Depends on D9936
Depends on D10561

Test Plan

Verified terraform configuration on staging

Diff Detail

Repository
rCOMM Comm
Branch
identity-search
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Restrict opensearch domain policy further

will planned changes to this revision.Dec 1 2023, 9:09 AM

Requires further discussion and planning of error handling. We'll need to set retry limits, batch sizes, and a failed-event destination so we can store failed entries in a queue.

Remove rebase changes meant for [1/n]

will planned changes to this revision.Dec 13 2023, 9:08 AM

Add error handling 2 time max retry 60 seconds event time

Remove manage opensearch permissions to add to [1/n]

Set up with new search domain access and add back read stream policy

lambda role iam policy and manage opensearch domain policy

rebase manage opensearch domain policy

Remove leftover merge comments

Need to rebase on terraform fmt

add network interface permissions and output lambda arn

will requested review of this revision.Dec 18 2023, 10:55 PM
will added inline comments.
services/terraform/modules/shared/search_index_lambda.tf
58 ↗(On Diff #34814)

Will change example to search-index-lambda

Will add dummy file and script for terraform apply and cargo lambda build

services/terraform/remote/aws_iam.tf
302 ↗(On Diff #34814)

This will be changed to the actual arn of the lambda, not the iam_role

services/terraform/remote/aws_iam.tf
321 ↗(On Diff #34814)

Try to avoid unnecessary changes like this by reviewing git diff before you put a diff up

Add dummy lambda bootstrap.zip, replace example, and remote terraform apply script

Owners added a reviewer: Restricted Owners Package.Dec 26 2023, 10:50 PM

If bootstrap.zip is a dummy, why is it so large? 3 MiB is a lot to add to the Git repo

lambda local dev support and commtest support

If bootstrap.zip is a dummy, why is it so large? 3 MiB is a lot to add to the Git repo

I was able to reduce the dummy to 725 KB by removing any extraneous dependencies. It's currently a minimally sized lambda but packaging the runtime makes this still decently large.

Hopefully, me and Bartek can figure out S3 buckets relatively soon as to replace this temporary solution. If not, another consideration might be to create a Node.js bootstrap.zip lambda which does not need to include a Rust runtime.

Thanks for explaining! If you end up needing to commit the bootstrap.zip file to the repo, then before you land this diff, can you link a Linear task tracking migrating the file to an S3 bucket?

varun requested changes to this revision.Jan 3 2024, 11:44 PM

how long does it take to build the lambda? if we're just adding the zip file to satisfy buildkite we could instead build and zip the lambda with a script similar to run.sh and call that script in the CI steps

services/docker-compose.yml
99 ↗(On Diff #35117)

why do we need this?

services/terraform/modules/shared/search_index_lambda.tf
39 ↗(On Diff #35117)

might be worth adding a comment here explaining why the count is 0 for dev

services/terraform/remote/run.sh
11 ↗(On Diff #35117)

this seems a little dangerous on staging/prod (especially prod). can we still require the user to confirm the changes before they're applied?

This revision now requires changes to proceed.Jan 3 2024, 11:44 PM
This comment was removed by ashoat.
In D9878#303855, @varun wrote:

how long does it take to build the lambda? if we're just adding the zip file to satisfy buildkite we could instead build and zip the lambda with a script similar to run.sh and call that script in the CI steps

It doesn't take long at all to build the lambda. You're correct; the Zip file is purely there to satisfy buildkite. I definitely believe that removing the zip file and running a script in CI is the better solution.

services/docker-compose.yml
99 ↗(On Diff #35117)

Localstack, when simulating lambdas, uses Docker to run the rust binaries. This line enables the Docker socket in the Localstack container. If removed, there will be a "Docker not available" error

"/var/run/docker.sock:/var/run/docker.sock"

will marked an inline comment as not done.Jan 4 2024, 9:04 AM
services/terraform/modules/shared/search_index_lambda.tf
39 ↗(On Diff #35117)

Making this count = 0 is a mistake. Now that I'm trying to enable local dev with localstack, I'll remove this

Remove bootstrap.zip and build in CI, address feedback

varun added inline comments.
services/terraform/modules/shared/search_index_lambda.tf
39 ↗(On Diff #35117)

so by omitting count entirely does it default to 1? i don't even see count in the docs

services/terraform/remote/run.sh
11 ↗(On Diff #35230)

so when we run this script now, what happens?

This revision is now accepted and ready to land.Jan 4 2024, 3:09 PM
services/terraform/remote/run.sh
11 ↗(On Diff #35230)

It will build the lambda bootstrap.zip and move it to the search-index-lambda folder so that the terraform config files can pick up on it. Afterwards, we can just run terraform apply and it'll wait for user confirmation.

If you don't run it, terraform apply will complain that there is no bootstrap.zip file at the specified path.

services/terraform/modules/shared/search_index_lambda.tf
39 ↗(On Diff #35117)

Yeah, that's right. Omitting count guarantees a single instance of this resource.

does it default to 1?

So I don't believe that's exactly how it works. Whenever count is used in a resource, instances of the resource are then indexed. So if it was count = 1, we'd access this by aws_lambda_event_source_mapping.trigger[0]. If we never use count, there shouldn't be an index.

Now need to include cargo lambda in dev install script

services/terraform/modules/shared/search_index_lambda.tf
39 ↗(On Diff #35117)

Yeah, that's right. Omitting count guarantees a single instance of this resource.

does it default to 1?

So I don't believe that's exactly how it works. Whenever count is used in a resource, instances of the resource are then indexed. So if it was count = 1, we'd access this by aws_lambda_event_source_mapping.trigger[0]. If we never use count, there shouldn't be an index.

Exactly, count is a built-in Terraform meta-param that spawns multiple instances of a resource and gives ways to iterate through them, without code duplication

Commtest and terraform use gitignored lambda target folder