Add the ability to read the license key from AWS secrets manager #18
Conversation
|
|
src/requirements.txt
Outdated
| @@ -2,6 +2,7 @@ | |||
| aiohttp==4.0.0a1 | |||
| async-timeout==3.0.1 | |||
| attrs==19.3.0 | |||
| boto3==1.14.9 | |||
There was a problem hiding this comment.
You'll want to add this dependency to the Pipfile. This requirements.txt is generated.
There was a problem hiding this comment.
Thank you for the quick response!
I made an attempt to resolve this. The Pipfile now looks as I would expect. The Pipfile.lock has a boatload of changes, as does the requirements.txt, generated from pipenv. Here are the steps I took (I'm new to pipenv):
pipenv shell
pipenv install boto3
pipenv lock -r > src/requirements.txt
Assuming the resulting Pipfile.lock and src/requirements.txt are not correct, tips on how to get them properly updated would be most appreciated.
|
Thanks for this PR @dswisher, we are actually torn with putting the license key in plain text environment variable. Is it possible to add the option for ssm parameter store too? |
src/function.py
Outdated
| secrets_client = boto3.client('secretsmanager') | ||
| secret = secrets_client.get_secret_value(SecretId=secret_arn) | ||
| cached_license_key = secret.get('SecretString') | ||
| print("(got license key from secrets manager, len={})".format(len(cached_license_key))) |
There was a problem hiding this comment.
Should only print this if debug mode is enabled.
|
@dswisher There's a merge conflict with the |
|
@oba11 Can you create an issue for SSM parameter store support? While related, it is arguably out of scope for this PR. But it is definitely something we would like to support. |
|
@dswisher Unless you're feeling ambitious to include parameter store with this PR? :) |
|
@dswisher Would like to merge this, but tests appear to be failing. |
I'd like to see it merged, too, but I haven't yet been able to figure out how to get the tests to pass. |
| if secret_arn: | ||
| secrets_client = boto3.client("secretsmanager") | ||
| secret = secrets_client.get_secret_value(SecretId=secret_arn) | ||
| cached_license_key = secret.get("SecretString") | ||
| if _debug_logging_enabled(): | ||
| print( | ||
| "(got license key from secrets manager, len={})".format( | ||
| len(cached_license_key) | ||
| ) | ||
| ) | ||
| else: | ||
| cached_license_key = os.getenv("LICENSE_KEY", "") |
There was a problem hiding this comment.
Let's use the LICENSE_KEY env var first. If it's set, we use it and are done. Otherwise, we fall back to secrets manager, which has considerable overhead.
In addition, we should move the secret manager fetch to the cold start period, rather than first invocation.
Finally, I suspect the global state you've introduced here is responsible for the test failures. You'll need to adjust the tests so that they don't have side effects, and aren't dependent on execution order.
There was a problem hiding this comment.
@MattWhelan You are correct, the global state makes the unit tests challenging. To address that and your other concerns, I'd like to change the code so that _get_license_key is called exactly once, from lambda_handler (which I believe is the "cold start period" to which you're referring).
That is a much more involved change, as the license key then needs to be passed down to everything that lambda_handler calls, but it seems to me like the right approach. Would you agree?
Adds a new environment variable,
SECRET_KEY_ARN. If set, the lambda uses that to read the license key from AWS secrets manager. The_get_license_keymethod is called multiple times during a single lambda run, so I created a global variable to store the result, to avoid multiple calls to secrets manager.This change also adds a simple shell script to automate creation of a
.zipfile for deployment, for those that might be unable to useSAMfor deployment (perhaps they useterraformor some other tool to automate their deployments).