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

Upgrade Serverless Framework to V4 #60

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sannidhishukla
Copy link

Description

Upgrade Serverless Framework to V4

Steps to Test

  1. Pull in AWS Credentials from the Innov-RES-Dev AWS account
  2. Run locally using the steps in the README

Copy link
Contributor

@namanaman namanaman left a comment

Choose a reason for hiding this comment

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

Looks good! One question, can we also remove @serverless/typescript and make sure it compiles? Note: Adding in ts-node as well. Ideally, if serverless has typescript built in, we don't need to have our own TS packages being pulled in which could eventually become incompatible and are not being used directly by our project

serverless.ts Outdated
@@ -32,7 +28,8 @@ const serverlessConfiguration: AWS = {
GOOGLE_PRIVATE_KEY: '${ssm:feedback-api-sheets-private-key}',
CLIENT_EMAIL: '${ssm:feedback-api-sheets-email}',
AZURE_OPENAI_ENDPOINT: '${ssm:feedback-api-azure-openai-endpoint}',
AZURE_OPENAI_KEY: '${ssm:feedback-api-azure-openai-key}'
AZURE_OPENAI_KEY: '${ssm:feedback-api-azure-openai-key}',
SERVERLESS_ACCESS_KEY: '${ssm:feedback-api-serverless-access-key}'
Copy link

@casewalker casewalker Dec 23, 2024

Choose a reason for hiding this comment

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

boulder - I might be misunderstanding, but I believe this only serves to make the Access Key into an environment variable, available inside of the Lambdas, which:

  1. Should not be necessary
  2. Is setting up the Access Key as a plaintext environment variable, viewable in the Lambda configuration, which is a little less secure than hoped

To my knowledge, the Access Key only needs to be setup as an environment variable in the CodeBuild pipeline, used when CodeBuild calls serverless deploy, and should be removed from the Lambda environment here.

Choose a reason for hiding this comment

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

OH, my bad, this is feedback-api, not UI, so you are not using CodeBuild.

Still, the more manual deployment approach which uses npx sls deploy ... in your local CLI is the place where you need to include the Access Key as an environment variable, so I would instead recommend editing the README to explain that.

Copy link
Author

Choose a reason for hiding this comment

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

just updated the readme!

Copy link

@casewalker casewalker Dec 23, 2024

Choose a reason for hiding this comment

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

Thank you for doing that!

I still believe there is zero need/use in having the Access Key added to the Lambda environment, there are only downsides in making the key less secure. I recommend removing it here.

Even if it was useful in the Lambda environment, which it isn't, I would recommend a getSsmParam approach like I described above instead of adding it in plaintext to the environment here.

Copy link

@casewalker casewalker left a comment

Choose a reason for hiding this comment

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

See comment above

serverless.ts Outdated
Comment on lines 28 to 31
GOOGLE_PRIVATE_KEY: '${ssm:feedback-api-sheets-private-key}',
CLIENT_EMAIL: '${ssm:feedback-api-sheets-email}',
AZURE_OPENAI_ENDPOINT: '${ssm:feedback-api-azure-openai-endpoint}',
AZURE_OPENAI_KEY: '${ssm:feedback-api-azure-openai-key}'
AZURE_OPENAI_KEY: '${ssm:feedback-api-azure-openai-key}',

Choose a reason for hiding this comment

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

pebble (out-of-scope for PR) - Seeing this as well, it looks like GOOGLE_PRIVATE_KEY and AZURE_OPENAI_KEY are probably sensitive values which should also not be exposed as plaintext data in the Lambdas' environments. For me, if something is stored in SSM as a plaintext string, it is fine to import the value this way, but if it is stored in SSM as a SecureString, you should be much more careful with it.

Noah previously implemented a more secure way to handle SecureString SSM parameters in UI, see here.

This is pretty far outside the scope for this PR, but I would recommend opening a ticket and using that kind of approach here too, where the parameter-name is kept as an environment variable (like IDME_VERIFICATION_DYNAMODB_ENCRYPTION_KEY_ARN_PARAM_NAME), and then the Lambda which needs it calls something like getSsmParam to fetch the value itself.

Copy link
Contributor

@noah-marcus noah-marcus left a comment

Choose a reason for hiding this comment

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

🥳 YAY! Great job getting this ready before v3 reaches EOL!!

Wondering if we can remove theserverless-offline-ssm plugin as well for similar reasons Naman described for the ts plugin. It doesn't look like it is being used in the Serverless file (unless I am reading it wrong) so it should be safe to remove.

Other than that, LOOKS GOOD TO ME!!! Let's wait to merge until we have a paid license key which we can coordinate over Slack 🙂

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.

4 participants