-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(apprunner-alpha): env vars and secrets can't solely be added via .add*() methods #24346
Changes from 3 commits
74f6e46
0945f0f
ed5eb0a
2904f2d
f7a56a9
b0e8b26
d43fdc7
e9ebaf0
dd23bb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import * as iam from '@aws-cdk/aws-iam'; | |
import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; | ||
import * as ssm from '@aws-cdk/aws-ssm'; | ||
import * as cdk from '@aws-cdk/core'; | ||
import { Lazy } from '@aws-cdk/core'; | ||
import { Construct } from 'constructs'; | ||
import { CfnService } from './apprunner.generated'; | ||
import { IVpcConnector } from './vpc-connector'; | ||
|
@@ -924,16 +925,6 @@ export class Service extends cdk.Resource { | |
*/ | ||
readonly environment: { [key: string]: string } = {}; | ||
|
||
/** | ||
* Environment variables for this service. | ||
*/ | ||
private environmentVariables: { [key: string]: string } = {}; | ||
|
||
/** | ||
* Environment secrets for this service. | ||
*/ | ||
private environmentSecrets: { [key: string]: Secret; } = {}; | ||
|
||
/** | ||
* Environment secrets for this service. | ||
*/ | ||
|
@@ -981,17 +972,22 @@ export class Service extends cdk.Resource { | |
this.source = source; | ||
this.props = props; | ||
|
||
this.environmentVariables = this.getEnvironmentVariables(); | ||
this.environmentSecrets = this.getEnvironmentSecrets(); | ||
this.instanceRole = this.props.instanceRole; | ||
|
||
const environmentVariables = this.getEnvironmentVariables(); | ||
const environmentSecrets = this.getEnvironmentSecrets(); | ||
|
||
for (const [key, value] of Object.entries(environmentVariables)) { | ||
this.addEnvironmentVariable(key, value); | ||
} | ||
for (const [key, value] of Object.entries(environmentSecrets)) { | ||
this.addSecret(key, value); | ||
} | ||
|
||
// generate an IAM role only when ImageRepositoryType is ECR and props.accessRole is undefined | ||
this.accessRole = (this.source.imageRepository?.imageRepositoryType == ImageRepositoryType.ECR) ? | ||
this.props.accessRole ?? this.generateDefaultRole() : undefined; | ||
|
||
// generalte an IAM role only when environmentSecrets has values and props.instanceRole is undefined | ||
this.instanceRole = (Object.keys(this.environmentSecrets).length > 0 && !this.props.instanceRole) ? | ||
this.createInstanceRole() : this.props.instanceRole; | ||
|
||
if (this.source.codeRepository?.codeConfiguration.configurationSource == ConfigurationSourceType.REPOSITORY && | ||
this.source.codeRepository?.codeConfiguration.configurationValues) { | ||
throw new Error('configurationValues cannot be provided if the ConfigurationSource is Repository'); | ||
|
@@ -1001,7 +997,7 @@ export class Service extends cdk.Resource { | |
instanceConfiguration: { | ||
cpu: this.props.cpu?.unit, | ||
memory: this.props.memory?.unit, | ||
instanceRoleArn: this.instanceRole?.roleArn, | ||
instanceRoleArn: Lazy.string({ produce: () => this.instanceRole?.roleArn }), | ||
}, | ||
sourceConfiguration: { | ||
authenticationConfiguration: this.renderAuthenticationConfiguration(), | ||
|
@@ -1036,13 +1032,19 @@ export class Service extends cdk.Resource { | |
* This method adds an environment variable to the App Runner service. | ||
*/ | ||
public addEnvironmentVariable(name: string, value: string) { | ||
if (name.startsWith('AWSAPPRUNNER')) { | ||
throw new Error(`Environment variable key ${name} with a prefix of AWSAPPRUNNER is not allowed`); | ||
} | ||
this.variables.push({ name: name, value: value }); | ||
} | ||
|
||
/** | ||
* This method adds a secret as environment variable to the App Runner service. | ||
*/ | ||
public addSecret(name: string, secret: Secret) { | ||
if (name.startsWith('AWSAPPRUNNER')) { | ||
throw new Error(`Environment secret key ${name} with a prefix of AWSAPPRUNNER is not allowed`); | ||
} | ||
if (!this.instanceRole) { | ||
this.instanceRole = this.createInstanceRole(); | ||
} | ||
|
@@ -1130,36 +1132,22 @@ export class Service extends cdk.Resource { | |
port: props.port, | ||
buildCommand: props.buildCommand, | ||
runtime: props.runtime.name, | ||
runtimeEnvironmentVariables: this.renderEnvironmentVariables(), | ||
runtimeEnvironmentSecrets: this.renderEnvironmentSecrets(), | ||
runtimeEnvironmentVariables: Lazy.any({ produce: () => this.renderEnvironmentVariables() }), | ||
runtimeEnvironmentSecrets: Lazy.any({ produce: () => this.renderEnvironmentSecrets() }), | ||
startCommand: props.startCommand, | ||
}; | ||
} | ||
|
||
private renderEnvironmentVariables(): EnvironmentVariable[] | undefined { | ||
if (Object.keys(this.environmentVariables).length > 0) { | ||
for (const [key, value] of Object.entries(this.environmentVariables)) { | ||
if (key.startsWith('AWSAPPRUNNER')) { | ||
throw new Error(`Environment variable key ${key} with a prefix of AWSAPPRUNNER is not allowed`); | ||
} | ||
this.variables.push({ name: key, value: value }); | ||
} | ||
Comment on lines
-1140
to
-1146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for throwing the error now happens in the |
||
if (this.variables.length > 0) { | ||
return this.variables; | ||
} else { | ||
return undefined; | ||
} | ||
} | ||
|
||
private renderEnvironmentSecrets(): EnvironmentSecret[] | undefined { | ||
if (Object.keys(this.environmentSecrets).length > 0 && this.instanceRole) { | ||
for (const [key, value] of Object.entries(this.environmentSecrets)) { | ||
if (key.startsWith('AWSAPPRUNNER')) { | ||
throw new Error(`Environment secret key ${key} with a prefix of AWSAPPRUNNER is not allowed`); | ||
} | ||
|
||
value.grantRead(this.instanceRole); | ||
this.secrets.push({ name: key, value: value.arn }); | ||
} | ||
Comment on lines
-1154
to
-1162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for throwing the error now happens in the .addSecret() method. And the prop secrets are added using the same .addSecret() method now, see lines 977:985. I followed the pattern that the |
||
if (this.secrets.length > 0 && this.instanceRole) { | ||
return this.secrets; | ||
} else { | ||
return undefined; | ||
|
@@ -1171,8 +1159,8 @@ export class Service extends cdk.Resource { | |
imageConfiguration: { | ||
port: repo.imageConfiguration?.port?.toString(), | ||
startCommand: repo.imageConfiguration?.startCommand, | ||
runtimeEnvironmentVariables: this.renderEnvironmentVariables(), | ||
runtimeEnvironmentSecrets: this.renderEnvironmentSecrets(), | ||
runtimeEnvironmentVariables: Lazy.any({ produce: () => this.renderEnvironmentVariables() }), | ||
runtimeEnvironmentSecrets: Lazy.any({ produce: () => this.renderEnvironmentSecrets() }), | ||
}, | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"version": "30.1.0", | ||
"files": { | ||
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": { | ||
"source": { | ||
"path": "AppRunnerLaterSecretsEnvVarsDefaultTestDeployAssert07867A67.template.json", | ||
"packaging": "file" | ||
}, | ||
"destinations": { | ||
"current_account-current_region": { | ||
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", | ||
"objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json", | ||
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" | ||
} | ||
} | ||
} | ||
}, | ||
"dockerImages": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
{ | ||
"Parameters": { | ||
"BootstrapVersion": { | ||
"Type": "AWS::SSM::Parameter::Value<String>", | ||
"Default": "/cdk-bootstrap/hnb659fds/version", | ||
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]" | ||
} | ||
}, | ||
"Rules": { | ||
"CheckBootstrapVersion": { | ||
"Assertions": [ | ||
{ | ||
"Assert": { | ||
"Fn::Not": [ | ||
{ | ||
"Fn::Contains": [ | ||
[ | ||
"1", | ||
"2", | ||
"3", | ||
"4", | ||
"5" | ||
], | ||
{ | ||
"Ref": "BootstrapVersion" | ||
} | ||
] | ||
} | ||
] | ||
}, | ||
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI." | ||
} | ||
] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
{"version":"30.1.0"} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"version": "30.1.0", | ||
"files": { | ||
"7cbdc4561bb7693ec11f95d96ee8a14d99732d386c66f27cb36e08a108d4ef30": { | ||
"source": { | ||
"path": "integ-apprunner-later-secrets-env-vars.template.json", | ||
"packaging": "file" | ||
}, | ||
"destinations": { | ||
"current_account-current_region": { | ||
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", | ||
"objectKey": "7cbdc4561bb7693ec11f95d96ee8a14d99732d386c66f27cb36e08a108d4ef30.json", | ||
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" | ||
} | ||
} | ||
} | ||
}, | ||
"dockerImages": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
{ | ||
"Resources": { | ||
"LaterSecretF6C54C5B": { | ||
"Type": "AWS::SecretsManager::Secret", | ||
"Properties": { | ||
"SecretString": "{\"password\":\"mySecretPassword\",\"apikey\":\"mySecretApiKey\"}" | ||
}, | ||
"UpdateReplacePolicy": "Delete", | ||
"DeletionPolicy": "Delete" | ||
}, | ||
"Service9DECC815E": { | ||
"Type": "AWS::AppRunner::Service", | ||
"Properties": { | ||
"SourceConfiguration": { | ||
"AuthenticationConfiguration": {}, | ||
"ImageRepository": { | ||
"ImageConfiguration": { | ||
"Port": "8000", | ||
"RuntimeEnvironmentSecrets": [ | ||
{ | ||
"Name": "LATER_SECRET", | ||
"Value": { | ||
"Fn::Join": [ | ||
"", | ||
[ | ||
{ | ||
"Ref": "LaterSecretF6C54C5B" | ||
}, | ||
":apikey::" | ||
] | ||
] | ||
} | ||
} | ||
], | ||
"RuntimeEnvironmentVariables": [ | ||
{ | ||
"Name": "LATER_ENVVAR", | ||
"Value": "testNewEnvVar" | ||
} | ||
] | ||
}, | ||
"ImageIdentifier": "public.ecr.aws/aws-containers/hello-app-runner:latest", | ||
"ImageRepositoryType": "ECR_PUBLIC" | ||
} | ||
}, | ||
"InstanceConfiguration": { | ||
"InstanceRoleArn": { | ||
"Fn::GetAtt": [ | ||
"Service9InstanceRole8BD2CEE0", | ||
"Arn" | ||
] | ||
} | ||
}, | ||
"NetworkConfiguration": { | ||
"EgressConfiguration": { | ||
"EgressType": "DEFAULT" | ||
} | ||
} | ||
} | ||
}, | ||
"Service9InstanceRole8BD2CEE0": { | ||
"Type": "AWS::IAM::Role", | ||
"Properties": { | ||
"AssumeRolePolicyDocument": { | ||
"Statement": [ | ||
{ | ||
"Action": "sts:AssumeRole", | ||
"Effect": "Allow", | ||
"Principal": { | ||
"Service": "tasks.apprunner.amazonaws.com" | ||
} | ||
} | ||
], | ||
"Version": "2012-10-17" | ||
} | ||
} | ||
}, | ||
"Service9InstanceRoleDefaultPolicy85BF9E64": { | ||
"Type": "AWS::IAM::Policy", | ||
"Properties": { | ||
"PolicyDocument": { | ||
"Statement": [ | ||
{ | ||
"Action": [ | ||
"secretsmanager:DescribeSecret", | ||
"secretsmanager:GetSecretValue" | ||
], | ||
"Effect": "Allow", | ||
"Resource": { | ||
"Ref": "LaterSecretF6C54C5B" | ||
} | ||
} | ||
], | ||
"Version": "2012-10-17" | ||
}, | ||
"PolicyName": "Service9InstanceRoleDefaultPolicy85BF9E64", | ||
"Roles": [ | ||
{ | ||
"Ref": "Service9InstanceRole8BD2CEE0" | ||
} | ||
] | ||
} | ||
} | ||
}, | ||
"Outputs": { | ||
"URL9": { | ||
"Value": { | ||
"Fn::Join": [ | ||
"", | ||
[ | ||
"https://", | ||
{ | ||
"Fn::GetAtt": [ | ||
"Service9DECC815E", | ||
"ServiceUrl" | ||
] | ||
} | ||
] | ||
] | ||
} | ||
} | ||
}, | ||
"Parameters": { | ||
"BootstrapVersion": { | ||
"Type": "AWS::SSM::Parameter::Value<String>", | ||
"Default": "/cdk-bootstrap/hnb659fds/version", | ||
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]" | ||
} | ||
}, | ||
"Rules": { | ||
"CheckBootstrapVersion": { | ||
"Assertions": [ | ||
{ | ||
"Assert": { | ||
"Fn::Not": [ | ||
{ | ||
"Fn::Contains": [ | ||
[ | ||
"1", | ||
"2", | ||
"3", | ||
"4", | ||
"5" | ||
], | ||
{ | ||
"Ref": "BootstrapVersion" | ||
} | ||
] | ||
} | ||
] | ||
}, | ||
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI." | ||
} | ||
] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{ | ||
"version": "30.1.0", | ||
"testCases": { | ||
"AppRunnerLaterSecretsEnvVars/DefaultTest": { | ||
"stacks": [ | ||
"integ-apprunner-later-secrets-env-vars" | ||
], | ||
"assertionStack": "AppRunnerLaterSecretsEnvVars/DefaultTest/DeployAssert", | ||
"assertionStackName": "AppRunnerLaterSecretsEnvVarsDefaultTestDeployAssert07867A67" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This same logic now needs to be performed lazily, no? Not entirely removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is handled in lines 975, 1049, and then lazily produced in line 1000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I missed that. I see, nicely done.