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

feat: Add support for secrets manager environment variables #287

Merged

Conversation

christophersjchow
Copy link
Contributor

Add support for setting environment variables that pull from Secrets Manager.

I basically copied the pattern that environment variables are currently being set so would appreciate anything I've missed.

@pgrzesik
Copy link
Contributor

pgrzesik commented Feb 8, 2022

Hello @christophersjchow - could you please link to the documetation that shows how that secrets data should be passed? It would help me better understand how we could shape it to be convenient for the plugin users

@christophersjchow
Copy link
Contributor Author

@pgrzesik The resource for the Cloud Function is described here in the REST API reference: https://cloud.google.com/functions/docs/reference/rest/v1/projects.locations.functions

In there there's a field:

"secretEnvironmentVariables": [
    {
      object ([SecretEnvVar]
    }
  ],

Which links to the SecretsEnvVar doc here: https://cloud.google.com/functions/docs/reference/rest/v1/projects.locations.functions#SecretEnvVar

That describes the object structure as:

{
  "key": string,
  "projectId": string,
  "secret": string,
  "version": string
}

I tested this locally by linking this package into one of our projects and successfully deployed through Deployment manager.

@christophersjchow
Copy link
Contributor Author

Any chance you could approve me for GitHub Actions too so CI will run?

@pgrzesik
Copy link
Contributor

pgrzesik commented Feb 9, 2022

Thanks a lot for the update @christophersjchow - I won't be able to look at it more closely this week, but I will find some time for it at the beginning of the next week.

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @christophersjchow - I was swamped with other priorities - it looks good in general, but I'm afraid we need to figure out the best representation of this setting in serverless configuration - could you propose how it could look like?

@christophersjchow
Copy link
Contributor Author

christophersjchow commented Feb 23, 2022

Sorry for the delay @christophersjchow - I was swamped with other priorities - it looks good in general, but I'm afraid we need to figure out the best representation of this setting in serverless configuration - could you propose how it could look like?

@pgrzesik The PR simply passes it through to the Deployment manager config. Here is an example as per the docs I've added here: serverless/serverless#10660

I think leaving the options the same as what Deployment Manager would expect is reasonable here, as this relies heavily on those specific options. The ID of the secret, version of the secret and the environment variable key to set are all compulsory and I don't think it's helpful to invent a syntax we would have to parse to set these values (like the example @sambauers suggested above).

In the current form, it would be possible to define at most a single secret variable - how do you think it would be best to expose these settings in serverless configuration?

The config on the serverless side allows for an array of secrets to be set. From the docs PR:

provider:
  secrets:
  - key: 'API_KEY'
    secret: 'secret-id'
    version: 'latest'
    projectId: 'project-id' # Optional, defaults to function project

@christophersjchow
Copy link
Contributor Author

@pgrzesik I can see that there's a call for maintainers for this plugin. I'd be willing to help out.

@R3VoLuT1OneR
Copy link

@pgrzesik This future is super helpful. Currently, I can't set secret env variable out of the scope of the serverless deployment.
Want to set it one time in a secure way.

Environment variables that are set manually are overridden by the next deployment.

The implementation looks good, let's merge and release it :)

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Hey @christophersjchow - sorry for the delay - I was unavailable for a past few days. The implementation looks good to me, let's merge it 👍

@pgrzesik pgrzesik changed the title Add support for secrets manager environment variables feat: Add support for secrets manager environment variables May 8, 2022
@pgrzesik pgrzesik merged commit 4e59429 into serverless:master May 8, 2022
@christophersjchow christophersjchow deleted the secrets-manager-support branch May 9, 2022 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants