fix(apprunner-alpha): env vars and secrets can't solely be added via .add*() methods #24346

rogerchi · 2023-02-27T05:21:28Z

This fixes the logic for rendering environment variables and environment secrets for the apprunner-alpha module. Previously, .addEnvironmentVariable() and .addSecret() were being ignored if there were not already "seed" values in the input props.

Closes #24345.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

TheRealAmazonKendra · 2023-02-27T22:34:13Z

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

rogerchi · 2023-03-01T04:11:22Z

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)

I've updated the code, tests, and added a new integration test and snapshot.

TheRealAmazonKendra · 2023-03-01T18:52:30Z

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)

I've updated the code, tests, and added a new integration test and snapshot.

I'll take a look but please also address the message above.

rogerchi · 2023-03-01T18:58:03Z

I'll take a look but please also address the message above.

Let me know if the new title works.

packages/@aws-cdk/aws-apprunner/test/service.test.ts

comcalvi · 2023-03-01T22:12:40Z

packages/@aws-cdk/aws-apprunner/lib/service.ts

-    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 });
-      }


Why was this removed?

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 lambda.Function construct established by using the .add* methods for both prop environment variables/secrets and ones added later using the .add* methods.

comcalvi · 2023-03-01T22:12:50Z

packages/@aws-cdk/aws-apprunner/lib/service.ts

-    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 });
-      }


Why was this removed?

The logic for throwing the error now happens in the .addEnvironmentVariable() method (it was not catching variables that started with AWSAPPRUNNER if added through the .add*() methods.

comcalvi · 2023-03-01T22:13:21Z

packages/@aws-cdk/aws-apprunner/lib/service.ts

-    // 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;
-


This same logic now needs to be performed lazily, no? Not entirely removed

This is handled in lines 975, 1049, and then lazily produced in line 1000.

ah, I missed that. I see, nicely done.

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Pull request has been modified.

rogerchi · 2023-03-02T01:27:32Z

@comcalvi I've tried to address/answer all of your comments. Let me know if there's any further suggestions or clarifications required!

comcalvi

nice work!

comcalvi · 2023-03-03T17:55:04Z

packages/@aws-cdk/aws-apprunner/lib/service.ts

-    // 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;
-


ah, I missed that. I see, nicely done.

mergify · 2023-03-03T18:01:22Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-03-03T19:14:38Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: dd23bb4
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-03-03T19:14:57Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

….add*() methods (aws#24346) This fixes the logic for rendering environment variables and environment secrets for the `apprunner-alpha` module. Previously, `.addEnvironmentVariable()` and `.addSecret()` were being ignored if there were not already "seed" values in the input props. Closes aws#24345. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix(apprunner-alpha): adding environment logic fix

74f6e46

github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK bug This issue is a bug. p2 labels Feb 27, 2023

aws-cdk-automation requested a review from a team February 27, 2023 05:21

aws-cdk-automation previously requested changes Feb 27, 2023

View reviewed changes

fix: working logic, update snapshots and tests

0945f0f

Merge branch 'main' into fix/apprunner-env-vars

ed5eb0a

rogerchi changed the title ~~fix(apprunner-alpha): adding environment logic fix~~ fix(apprunner-alpha): env vars and secrets can't solely be added via .add*() methods Mar 1, 2023

github-actions bot added the effort/medium Medium work item – several days of effort label Mar 1, 2023

comcalvi previously requested changes Mar 1, 2023

View reviewed changes

chore: apply suggestions from code review

2904f2d

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

rogerchi added 2 commits March 1, 2023 20:18

feat: split up tests

f7a56a9

chore: test name

b0e8b26

rogerchi added 2 commits March 1, 2023 20:34

feat: add additional tests for catching invalid prefixes added later

d43fdc7

Merge branch 'main' into fix/apprunner-env-vars

e9ebaf0

rogerchi requested a review from comcalvi March 3, 2023 01:50

comcalvi approved these changes Mar 3, 2023

View reviewed changes

Merge branch 'main' into fix/apprunner-env-vars

dd23bb4

mergify bot merged commit 45195b6 into aws:main Mar 3, 2023

rogerchi deleted the fix/apprunner-env-vars branch March 6, 2023 00:26

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apprunner-alpha): env vars and secrets can't solely be added via .add*() methods #24346

fix(apprunner-alpha): env vars and secrets can't solely be added via .add*() methods #24346

rogerchi commented Feb 27, 2023

aws-cdk-automation left a comment

TheRealAmazonKendra commented Feb 27, 2023

rogerchi commented Mar 1, 2023

TheRealAmazonKendra commented Mar 1, 2023

rogerchi commented Mar 1, 2023

comcalvi Mar 1, 2023

rogerchi Mar 2, 2023 •

edited

Loading

comcalvi Mar 1, 2023

rogerchi Mar 2, 2023 •

edited

Loading

comcalvi Mar 1, 2023

rogerchi Mar 2, 2023

comcalvi Mar 3, 2023

rogerchi commented Mar 2, 2023

comcalvi left a comment

comcalvi Mar 3, 2023

mergify bot commented Mar 3, 2023

aws-cdk-automation commented Mar 3, 2023

mergify bot commented Mar 3, 2023

fix(apprunner-alpha): env vars and secrets can't solely be added via .add*() methods #24346

fix(apprunner-alpha): env vars and secrets can't solely be added via .add*() methods #24346

Conversation

rogerchi commented Feb 27, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Feb 27, 2023

rogerchi commented Mar 1, 2023

TheRealAmazonKendra commented Mar 1, 2023

rogerchi commented Mar 1, 2023

comcalvi Mar 1, 2023

Choose a reason for hiding this comment

rogerchi Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

comcalvi Mar 1, 2023

Choose a reason for hiding this comment

rogerchi Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

comcalvi Mar 1, 2023

Choose a reason for hiding this comment

rogerchi Mar 2, 2023

Choose a reason for hiding this comment

comcalvi Mar 3, 2023

Choose a reason for hiding this comment

rogerchi commented Mar 2, 2023

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Mar 3, 2023

Choose a reason for hiding this comment

mergify bot commented Mar 3, 2023

aws-cdk-automation commented Mar 3, 2023

AWS CodeBuild CI Report

mergify bot commented Mar 3, 2023

rogerchi Mar 2, 2023 •

edited

Loading

rogerchi Mar 2, 2023 •

edited

Loading