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

aws-apprunner-alpha: missing option for referencing environment variables via SM parameters #32770

Open
1 task
garysassano opened this issue Jan 7, 2025 · 6 comments
Labels
@aws-cdk/aws-apprunner Related to the apprunner package feature-request A feature should be added or improved. p2

Comments

@garysassano
Copy link

Describe the bug

There's no option in the current Service L2 construct for referencing environment variables using SecureString parameters in Systems Manager.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

I expected to be able to reference environment variables from SM Parameter Store like from AWS Console.

image

Current Behavior

image

You only have the following two options available within the Service construct props:

  • environmentVariables ➜ equivalent to Plain text in AWS Console
  • environmentSecrets ➜ equivalent to Secrets Manager in AWS Console

Reproduction Steps

See above.

Possible Solution

Introduce a new environmentParameters option following the same principle as the other two props.

Additional Information/Context

No response

CDK CLI Version

2.174.0

Framework Version

No response

Node.js Version

22.12.0

OS

Ubuntu 24.04.1

Language

TypeScript

Language Version

No response

Other information

No response

@garysassano garysassano added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 7, 2025
@github-actions github-actions bot added the @aws-cdk/aws-apprunner Related to the apprunner package label Jan 7, 2025
@khushail khushail self-assigned this Jan 7, 2025
@khushail khushail added p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 7, 2025
@garysassano
Copy link
Author

The SM Parameter can be used as part of environmentSecrets, even though it's confusing (see here).

@khushail
Copy link
Contributor

khushail commented Jan 9, 2025

Hi @garysassano , I also noticed that in this code example, SSM Parameters have been mentioned as well -

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-apprunner-alpha-readme.html#secrets-manager

Screenshot 2025-01-09 at 8 44 34 AM

So I assume it should be feasible to reference SM Parameters. wdyt?

@khushail khushail added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jan 9, 2025
@garysassano
Copy link
Author

I believe that since the construct is still in alpha, it would be more logical to move the SSM parameters into a dedicated environmentParameters property. Additionally, I'm not entirely sure why the documentation lists SSM Parameters under the Secrets Manager section.

You should decide on a clear structure: either use three distinct properties, such as environmentVariables, environmentSecrets, and environmentParameters, or consolidate everything into a single environment property. Mixing these approaches creates unnecessary confusion.

// Proposed structure for clarity:

// Option 1: Separate properties
const config = {
  source: apprunner.Source.fromEcr({
    imageConfiguration: {
      environmentVariables: {
        'ENV_VAR': 'value',
      },
      environmentSecrets: {
        'SECRET_VAR': apprunner.Secret.fromSecretsManager(secret),
      },
      environmentParameters: {
        'PARAM_VAR': apprunner.Parameter.fromSsmParameter(parameter),
      },
    },
  });
};

// OR

// Option 2: Unified environment property with inferred types
const config = {
  source: apprunner.Source.fromEcr({
    imageConfiguration: {
      environment: [
        { key: 'ENV_VAR', value: 'value' }, 
        // If the value is a string, it's treated as a plain environment variable.
        // During synthesis, it is internally converted into an enum type like EnvironmentVariableType.VARIABLE.
        
        { key: 'SECRET_VAR', value: apprunner.Secret.fromSecretsManager(secret) }, 
        // If the value is an instance of apprunner.Secret, it's automatically recognized as a secret.
        // It is then converted into the corresponding enum type, EnvironmentVariableType.SECRET.

        { key: 'PARAM_VAR', value: apprunner.Parameter.fromSsmParameter(parameter) }, 
        // If the value is an instance of apprunner.Parameter, it's treated as an SSM parameter.
        // This is internally converted into EnvironmentVariableType.PARAMETER.
      ],
    },
  }),
};

I personally prefer Option 2 for the better developer experience, but also Option 1 is acceptable.

@garysassano
Copy link
Author

Putting an SSM parameter under environmentSecrets is also not very idiomatic, since a parameter isn't necessarily a SecureString, and SSM parameters can vary in sensitivity.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 9, 2025
@khushail
Copy link
Contributor

khushail commented Jan 9, 2025

@garysassano , In the CDK code, for environment variables, the implementation is -

private getEnvironmentVariables(): { [key: string]: string } {

However for secrets, this is implemented through-

private getEnvironmentSecrets(): { [key: string]: Secret } {

which are further invoked when initialising construct-

https://github.com/aws/aws-cdk/blob/bb59b5a40582e0d23bf8e0c20de4ef1bf17eb352/packages/%40aws-cdk/aws-apprunner-alpha/lib/service.ts#L1278C1-L1279C61

    const environmentVariables = this.getEnvironmentVariables();
    const environmentSecrets = this.getEnvironmentSecrets();

@khushail
Copy link
Contributor

khushail commented Jan 9, 2025

From the code implementation,it might have been thoughtful to put SSM parameters under Secrets but your suggestion also makes sense. Keeping it under environmentVariables would impose direct mapping and remove the confusion of its implementation, considering the varying sensitivity.

However IMO , this should be marked as Feature request , than bug. and your suggested approaches here seem logical. Please feel free to revert back if you think otherwise.

For now, I would be marking it as P2 which means it won't be immediately addressed by the team but would be on their radar.

I am also requesting team's input here as it something they would like to consider, with the suggested approaches(#32770 (comment)) and share insights if possible.

@khushail khushail added feature-request A feature should be added or improved. and removed bug This issue is a bug. labels Jan 9, 2025
@khushail khushail removed their assignment Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apprunner Related to the apprunner package feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

2 participants