Implement centralized per venue launchpad token dispenser lambda #1
Conversation
There was a problem hiding this comment.
Hey David, I think this is a good first pass! A lot of good implementation for the requirements/technical details we discussed
I have quite a few comments on the current implementation I'd like taken care of before a first release. Let me know if you have any questions or if you'd like to discuss them more in-depth
|
|
||
|
|
||
|
|
||
|
|
terraform/variables.tf
Outdated
| # Launchpad Token Dispensing Lambada timeout in secs | ||
| variable "launchpad_token_dispenser_lambda_timeout" { | ||
| type = number | ||
| default = 20 | ||
| } | ||
|
|
||
| variable "launchpad_token_dispenser_lambda_memory_size" { | ||
| type = number | ||
| default = 128 | ||
| } |
There was a problem hiding this comment.
I'm not really sure we need to make these adjustable since the code is fairly minimal and deterministic. I think keeping the timeout/memory size hardcoded is fine for keeping down the number of variables to track
terraform/variables.tf
Outdated
| # The bucket where launchpad.pfx is stored. Ex. my-sndbx-bucket | ||
| variable "launchpad_pfx_file_s3_bucket"{} | ||
| # The key to point to launchpad.pfx Ex. /folder1/folder2/launchpad.pfx | ||
| variable "launchpad_pfx_file_s3_key" {} |
There was a problem hiding this comment.
Can we combine these two into just "s3_uri" which combines the bucket + path as a URI?
There was a problem hiding this comment.
the s3 download api requires 2 parameters - bucket name and key hence we separate them.
Otherwise, the developers using this module could configure this as
s3://bucket_name/folder1/folder2/xxx.pfx
or maybe no s3:// leading? maybe the code needs to deal with double slash or trailing slash.
It might be better to just keep bucket and key as different variables.
terraform/variables.tf
Outdated
|
|
||
| # sndbx, sit, uat or ops | ||
| variable "stage" { | ||
| default = "sndbx" |
There was a problem hiding this comment.
Let's remove the default so that it has to be explicitly specified by the deployment
terraform/variables.tf
Outdated
| } | ||
|
|
||
| variable "region" { | ||
| default = "us-west-2" |
There was a problem hiding this comment.
Let's remove the default so that it has to be explicitly specified by the deployment
| raise e | ||
|
|
||
|
|
||
| def satisfy_minimum_alive_secs(token_json:json, minimum_alive_secs:int | None) -> bool: |
There was a problem hiding this comment.
minimum_alive_secs should never be None because it defaults to DEFAULT_TOKEN_MIN_ALIVE_TIME in all the function calls. Let's not make that parameter optional
| try: | ||
| # When a request comes in, check dynamoDB first, if not found then get new token | ||
| token_structure_str:str = get_token_by_client_id(client_id) | ||
| if(token_structure_str is not None): |
There was a problem hiding this comment.
Doesn't need parenthesis here
| if minimum_alive_secs is None: | ||
| logger.info(f"client passed in empty minimum_alive_secs, using default value as:" | ||
| f" {config.DEFAULT_TOKEN_MIN_ALIVE_TIME}") | ||
| minimum_alive_secs = config.DEFAULT_TOKEN_MIN_ALIVE_TIME |
There was a problem hiding this comment.
Maybe we need to move this logic to around line #99 so that the logic is all in one place for how this default is set?
| raise e | ||
| # return { | ||
| # "statusCode": 500, | ||
| # "body": json.dumps({"error": f"Failed to process client token. exception: {str(e)}"}) | ||
| # } |
There was a problem hiding this comment.
Was this intended to be left here?
| # Below code only need to deal with the case of cached entry could be found. | ||
| # The is, token_json could not be None | ||
| is_longer_than_minimum_alive_secs = satisfy_minimum_alive_secs(token_json, minimum_alive_secs) | ||
| if is_longer_than_minimum_alive_secs is True: |
There was a problem hiding this comment.
I'm wondering if this logic chain and the one above can be cleaned up a little bit. Here's what I'm thinking in (very rough) psudocode
cached_token = dynamodb.get(client_id)
if cached_token is not None and cached_token is not expired:
return cached_token
retrieve new token from Launchpad
store new token into dynamodb
return new token
Makes the logic flow a little easier to read/follow
README.md
Outdated
| The installation steps involved | ||
| * Downloading the lambda artifact and placing it under the $PROJECT_ROOT/dist directory. | ||
| * Creating a tfvars file or using `TF_VAR |
README.md
Outdated
| | config key | description | | ||
| |--------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------| | ||
| | `stage` | prefix of lambda. If intent to deploy one TDL per env, suggested to set it as sndbx/sit/uat or ops based on the running environment | |
README.md
Outdated
| | `credentials` | full path of credential file. Ex. /User/Mynam/.aws/credentials | | ||
| | `profile` | Name of the AWS profile from the credentials file | | ||
| | `log_retention_days` | Number of days where TDL lambda will be retained | |
README.md
Outdated
| | `launchpad_token_dispenser_lambda_memory_size` | TDL lambda memory size | | ||
| | `launchpad_token_dispenser_lambda_architectures` | TDL lambda running architecture. Current set to x86. ARM is not tested. | |
README.md
Outdated
|
|
||
| ### Configure tfstate file | ||
| The $PROJECT_ROOT/backend directory contains sample venue.tf files being used to configure where the TDL project's tfstate file will be located. |
|
|
||
| resource "aws_lambda_function" "launchpad_token_dispenser_lambda" { | ||
| filename = "../dist/token-dispenser_lambda.zip" | ||
| function_name = "${var.stage}-launchpad_token_dispenser" |
There was a problem hiding this comment.
Can we make this a static name instead of a dynamic name between environments? It makes it easier to write clients against the environments if this is hardcoded
Ticket: [PODAAC-6601]
Description
This PR is for initial implementation of centralized launchpad token dispenser
Overview of work done
Implement the token dispenser and integration testing.
Overview of verification done
Tested in Sandbox
Tested in SIT:
N/A
PR checklist:
See Pull Request Review Checklist for pointers on reviewing this pull request