Page MenuHomePhabricator

[services] Tunnelbroker - Add dynamoDB tables to the Terraform
ClosedPublic

Authored by max on May 9 2022, 4:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 9:08 AM
Unknown Object (File)
Mon, Nov 4, 1:12 AM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM

Details

Summary

Add Tunnelbroker dynamoDB tables configuration to the Terraform test and production config files.
The database model is available in the architecture review block.

Test Plan
  1. Run yarn run-local-cloud to update the structure of the dynamoDB tables inside the localstack docker container due to the changes.
  2. Run yarn run-tunnelbroker-service-dev-mode to run the tunnelbroker server.
  3. The tunnelbroker will connect to the localstack dynamoDB database and check for the tables are exists in the database by the isTableAvailable() internal method on the startup procedure.
  4. If some of the tables do not exist (were not created by the terraform or the name of the expected table is not correct in the terraform configuration) the tunnelbroker will throw an error table X is not available on startup.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.May 9 2022, 7:16 AM

This diff and D3213 use different names of tables

This revision now requires changes to proceed.May 9 2022, 7:16 AM
In D3965#111140, @palys-swm wrote:

This diff and D3213 use different names of tables

Rebased table names on top of the D3213. Thanks for catching this!

This revision is now accepted and ready to land.May 12 2022, 3:22 AM
This revision now requires review to proceed.May 12 2022, 3:22 AM

Would like to make sure @jimpo takes a look too, given he's the only one on the team with Terraform experience

Shouldn't we include running the tunnelbroker's code in the test plan? We may receive an error like "trying to access a table that does not exist" due to a typo even though the terraform would work properly.
Have you performed such tests?

This revision now requires changes to proceed.May 13 2022, 6:17 AM
In D3965#113210, @karol-bisztyga wrote:

Shouldn't we include running the tunnelbroker's code in the test plan? We may receive an error like "trying to access a table that does not exist" due to a typo even though the terraform would work properly.
Have you performed such tests?

That makes sense. According to the terraform documentation, it's safe to run terrafrom init multiple times to sync with the changes and it can be used for the test plan along with the tunnelbroker start to check it. I've updated the test plan.
cc @karol-bisztyga and @jimpo

karol requested changes to this revision.EditedMay 26 2022, 4:53 AM
In D3965#116669, @geekbrother wrote:
In D3965#113210, @karol-bisztyga wrote:

Shouldn't we include running the tunnelbroker's code in the test plan? We may receive an error like "trying to access a table that does not exist" due to a typo even though the terraform would work properly.
Have you performed such tests?

That makes sense. According to the terraform documentation, it's safe to run terrafrom init multiple times to sync with the changes and it can be used for the test plan along with the tunnelbroker start to check it. I've updated the test plan.
cc @karol-bisztyga and @jimpo

I'm not sure if I missed anything but I have no idea what you're talking about. What you mentioned does not seem to be related at all to what I said.

What I was talking about: Verifying the terraform operations with the tunnelbroker code that uses their outcome to see whether we're not going to get any runtime errors.

What you're talking about:

it's safe to run terrafrom init multiple times to sync with the changes
it can be used for the test plan along with the tunnelbroker start to check it

I don't understand what running terraform init has to do with what I said. The fact that we can run it multiple times and nothing's going to blow up is pretty obvious.

These questions remain unanswered:

Shouldn't we include running the tunnelbroker's code in the test plan?

In other words what I meant: you can create a terraform script in which you declare a table worker but in the code, in some DB manager you could try to access a table workers. Then you'd end up with a runtime error like No such table: workers.

Have you performed such tests?

Have you run the tunnelbroker's code and entered the parts where you do database operations and checked that it works properly? Just running the tunnelbroker and seeing that it launched doesn't suffice as it does not check anything related to this revision I believe.

Requesting changes as I don't feel like my questions were properly interpreted.

This revision now requires changes to proceed.May 26 2022, 4:53 AM

Removed Expire fields from attribute section because of the terraform init the field unindexed warnings. We actually don't need to specify it in the terraform.

Shouldn't we include running the tunnelbroker's code in the test plan?

In other words what I meant: you can create a terraform script in which you declare a table worker but in the code, in some DB manager you could try to access a table workers. Then you'd end up with a runtime error like No such table: workers.

Have you performed such tests?

Have you run the tunnelbroker's code and entered the parts where you do database operations and checked that it works properly? Just running the tunnelbroker and seeing that it launched doesn't suffice as it does not check anything related to this revision I believe.

That totally makes sense to me!
I've updated the diff and the test plan with the following updates and now it can be properly tested:

  1. Run yarn run-local-cloud to update the structure of the dynamoDB tables inside the localstack docker container due to the changes.
  2. Run yarn run-tunnelbroker-service-dev-mode to run the tunnelbroker server.
  3. The tunnelbroker will connect to the localstack dynamoDB database and check for the tables are exists in the database by the isTableAvailable() internal method on the startup procedure.
  4. If some of the tables do not exist (were not created by the terraform or the name of the expected table is not correct in the terraform configuration) the tunnelbroker will throw an error on startup.

cc to @karol-bisztyga and @jimpo

Rebase on the stack diff updates.

The duplication between dynamodb and dynamodb-test is a problem. We should investigate this in a follow-up: from the localstack docs, it looks like separate databases with separate tables are created for each credential that connects https://docs.localstack.cloud/localstack/configuration/#dynamodb. So ideally terraform would just create the tables twice somehow with different credentials, one for test and one for dev.

@geekbrother, before landing can you create a Linear task for investigating what @jimpo pointed out above?

@geekbrother, before landing can you create a Linear task for investigating what @jimpo pointed out above?

Yes, sure! ENG-1246 was created as a follow-up to this comment. cc to @karol-bisztyga for a review.

This revision is now accepted and ready to land.Jun 10 2022, 1:21 AM