feat: Create GuSSMParameter using AWS Custom Resources #336

stephengeller · 2021-03-18T13:36:28Z

What does this change?

Problem

At the moment, the general practice for injecting secrets into CloudFormation templates is through CFN parameters. In CDK, this looks like this:

DatabaseUser: new GuStringParameter(this, "DatabaseUser", {
        description: "Username of the RDS DB",
        default: "grafana",
}),

This is a bit gross, as it:

Doesn't have a sensible default (and it wouldn't make sense to have one here), meaning you have to input it yourself every time you create the stack and somehow keep it safe in an unspecified place
Requires you to go into the UI every time you want to change it
Doesn't make it clear that it is a secure/sensitive value
Makes the CFN more unwieldy as more parameters == more things to remember and update and maintain

Solution

This PR creates a GuSSMParameter construct, making use of the SSM Parameter Store to fetch values dynamically.

DatabaseUser: GuSSMParameter(this, "/PATH/TO/database-user")

Using AWS Custom Resources, the GuSSMParameter construct can dynamically fetch the latest version of an SSM parameter, something that AWS' own CDK construct cannot do yet.

This also creates GuSSMDefaultParam (would prefer a new name, suggestions welcome!) that assumes the default path of /$STAGE/$STACK/$APP/$PARAM_NAME as a prefix to the parameter name, for added simplicity.

All three of the below implementations work:

import type { App } from "@aws-cdk/core";
import { CfnOutput } from "@aws-cdk/core";
import type { GuStackProps } from "@guardian/cdk/lib/constructs/core";
import { GuStack } from "@guardian/cdk/lib/constructs/core";
import { GuSSMDefaultParam, GuSSMParameter } from "./ssmParams";

export class CdkStack extends GuStack {
  constructor(scope: App, id: string, props: GuStackProps) {
    super(scope, id, props);

    const secrets = {
      fullPath: new GuSSMParameter(this, "/CODE/test/tag-janitor/test-output").getValue(),
      usingDefaultValues: GuSSMDefaultParam(this, "test-output").getValue(),
      usingTokensInPath: new GuSSMParameter(this, `/${this.stage}/${this.stack}/${this.app}/test-output`).getValue(),
    };

    new CfnOutput(this, "fullPath", { value: secrets.fullPath });
    new CfnOutput(this, "usingDefaultValues", { value: secrets.usingDefaultValues });
    new CfnOutput(this, "usingTokensInPath", { value: secrets.usingTokensInPath });
  }
}

This offers numerous advantages:

It's less code (yay!) and easier to declare
It requires no user input to deploy
Updating the value is easier, as you can navigate to SSM on the CLI as well as the UI and then redeploy the app in RiffRaff
You can optionally and easily make use of the /$STAGE/$STACK/$APP/$PARAM_NAME path prefix with ease and have it dynamically resolve based on the value of those parameters

Usage

Abridged from tag-janitor

export class CdkStack extends GuStack {
  constructor(scope: App, id: string, props: GuStackProps) {
    super(scope, id, props);

    const secrets = {
      prismUrl: new GuSSMParameter(this, `/${this.stage}/${this.stack}/${this.app}/prism-url`).getValue(),
    };

    new GuScheduledLambda(this, `${this.app}-lambda`, {
      handler: "dist/src/handler.handler",
      functionName: `${this.app}-${this.stage}`,
      runtime: Runtime.NODEJS_12_X,
      code: {
        bucket: parameters.bucketName.valueAsString,
        key: `${this.stack}/${this.stage}/${this.app}/${this.app}.zip`,
      },
      environment: { PRISM_URL: parameters.prismUrl }, // Used here
      vpc: GuVpc.fromIdParameter(this, "vpc"),
      vpcSubnets: {
        subnets: GuVpc.subnetsfromParameter(this),
      },
      schedule: Schedule.rate(lambdaFrequency),
      monitoringConfiguration: {
        snsTopicName: "devx-alerts",
        toleratedErrorPercentage: 99,
      },
    });
  }
}

How does it work?

This uses AWS Custom Resouces, which creates a few things alongside your normal resources. Namely, this creates a lambda which will essentially execute anything you want, but we've restricted ours to just call SSM getParameter for our purposes. When you create/update a stack, the lambda will run, fetch the latest version of the SSM parameters you need and populate the relevant resources with them.

Does this change require changes to existing projects or CDK CLI?

Not yet, as this would be something to begin replacing traditional GuParameters with.

How to test

Tests will be added before moving this from draft to live.

How can we measure success?

We pretty much eliminate all manually filled-in CFN parameters from CDK stacks
We can always fetch the latest version of an SSM parameter for use in CDK stacks
We can dymically fetch parameters for appropriate $STAGE/$STACK/$APP paths

Have we considered potential risks?

Not risks as such, but AWS Custom Resources will somewhat unavoidably create additional resources per stack. This isn't necessarily a problem (and should be fine cost-wise due to the low number of executions), but is a bit of a gotcha that's worth knowing about. However, users will not need to touch these resources and can happily ignore them.

jacobwinch · 2021-03-18T14:38:20Z

src/constructs/core/ssm.ts

+  private readonly customResource: CustomResource;
+  readonly grantPrincipal: IPrincipal;
+
+  // eslint-disable-next-line custom-rules/valid-constructors -- I think stating an ID would be overkill for this, but happy to discuss


I think there are plans to change this; see similar a discussion here. However perhaps param should be part of props, for consistency?

Good shout, and done. It's actually becoming apparent that passing in an ID might solve a couple of issues down the line but will investigate.

The problem is essentially that it looks like we need to ensure we create new custom resources every time (not the lambda, but possibly everything else) in order to trigger the fetching of an SSM parameter every time. If you have a fixed ID, it might be the case that it won't update past creation unless you somehow inform it that there's been a change.

Will investigate though.

#352 revises the linting rules.

jacobwinch · 2021-03-18T14:39:37Z

src/constructs/core/ssm.ts

+
+    const provider = new SingletonFunction(scope, id("Provider"), {
+      code: Code.fromInline(readFileSync(join(__dirname, "/custom-resources/runtime/lambda.js")).toString()),
+      // runtime: new Runtime("nodejs14.x", RuntimeFamily.NODEJS, { supportsInlineCode: true }), -- we can use Node14 once we bump the version of @aws-cdk to v1.94 https://github.com/aws/aws-cdk/releases/tag/v1.94.0


I think you're in luck!

Yay! Updated to use Node 14.

jacobwinch · 2021-03-18T14:46:55Z

src/constructs/core/ssm.ts

+  }
+}
+
+export function GuSSMDefaultParam(scope: GuStack, param: string): GuSSMParameter {


If GuSSMParameterProps was:

export interface GuSSMParameterProps { secure?: boolean; /* * If no path is provided we will assume it is`/${STAGE}/${STACK}/${APP}/${param}` * */ overridePath?: string; // not sure about the name, but hopefully you get the idea }

Then we could consider dropping GuSSMDefaultParam entirely?

Although it would still be possible to stray from the well-trodden path (no pun intended), it would be a little harder to do so accidentally.

Yeah, maybe if we called the prop fullPath instead, that might be clearer? eg:

const withFullPath = new GuSSMParameter(this, { parameter: "/path/to/param", fullPath: true }) const assumedPath = new GuSSMParameter(this, { parameter: "param" }) // assumes /stage/stack/app/param

jacobwinch · 2021-03-18T14:49:31Z

package.json

    "@types/jest": "^26.0.20",
    "@types/node": "14.14.32",
    "@typescript-eslint/eslint-plugin": "^4.16.1",
    "@typescript-eslint/parser": "^4.16.1",
+    "aws-sdk": "^2.866.0",


I think that you could use v3 of this sdk. I had a few problems migrating a project to it recently, but unless you are aware of any known issues with the functionality you're using it could be worth a try?

Good shout! I've tried it and looks fine, thanks for the heads up about this.

Sadly, I've had to revert to v2 of the library since an AWS Node Lambda only has v2 of the library available. If and when this changes we can definitely upgrade though, but not sure we can make this work with v3 at the moment.

jacobwinch · 2021-03-18T14:50:58Z

Great PR description, thanks for taking the time to write this up! I've left a few comments but it's looking good so far!

stephengeller · 2021-03-19T09:14:31Z

I've got a couple of things to work out before this is good to go (getting CI to pass included), so will work those out before requesting another review but all comments so far should be addressed

stephengeller · 2021-03-22T13:59:23Z

This is now ready for review! Good to get eyes on it from @jacobwinch @akash1810 and/or @sihil if possible.

akash1810

Looks good! Added a few comments.

Could we add some usage guidance to the PR description? The example use case is a database user - when should this be normal application config vs a GuSSMParameter in the CFN template?

Lastly, should the PR's prefix be "feat" as we're adding a feature and a new minor version should be released?

akash1810 · 2021-03-24T08:27:23Z

src/constructs/core/custom-resources/runtime/lambda.test.ts

+/*
+ * You can't specify the file extension (eg .ts) of the file to import,
+ * and lambda.js exists as a dummy file, so we'd always import that over lambda.ts (the actual code to be tested) if
+ * we imported `lambda`
+ * lambda.symlink is a symlink to lambda.ts, so we can test it without importing lambda.js
+ * */
+import { flatten } from "./lambda.symlink";


Oh wow! I'd be curious to know the output of npm pack now? IIUC npm pack will create a local bundle that's representative of what gets published to NPM - a sort of publish dry run.

I'm not sure what JS file will be added - the empty lambda.js or the one that tsc will create?

Just ran it and it looks like it packs fine! By that I mean the resulting lambda.js file is the expected compiled TS lambda, not the dummy file. I assume it simply overwrites it no matter what, as the dummy file could equally be seen as a stale JS file in need of updating

akash1810 · 2021-03-24T08:30:25Z

src/constructs/core/ssm.test.ts

+    beforeAll(() => {
+      // TODO: Experiment with compiling `custom-resources/runtime/lambda.ts` to JS so that tests run
+    });


Don't think this is needed?

Good find, thanks!

akash1810 · 2021-03-24T08:36:14Z

src/constructs/core/ssm.ts

+
+    const provider = new SingletonFunction(scope, id("Provider"), {
+      code: Code.fromInline(readFileSync(join(__dirname, "/custom-resources/runtime/lambda.js")).toString()),
+      runtime: Runtime.NODEJS_12_X, // TODO: Ensure that the TS -> JS compile creates a NODE12-compliant file


Node 12 is in maintenance LTS. AWS recently announced support for node 14 in AWS Lambda.

How does the output for a node 14 runtime break? There might be some TS config tweaks we can make? Or raise it with our AWS TAM?

I think this TODO is outdated, as the TS configuration in the app creates Node 12 compliant JS code. The problem with upgrading to Node 14 is that it for some reason prevents us from using inline code (ie Code.fromInline), which is necessary here. I can't find anything online yet that specifically explains why, but it means that we're somewhat restricted to 12 until we can inline the code or find a better solution.

akash1810 · 2021-03-24T08:38:26Z

src/constructs/core/ssm.ts

+    const id = (id: string) => {
+      const now = Date.now();
+      // We need to create UIDs for the resources in this construct, as otherwise CFN will not trigger the lambda on updates for resources that appear to be the same
+      const uid = now.toString().substr(now.toString().length - 4);
+      return parameter.toUpperCase().includes("TOKEN") ? `${id}-token-${uid}` : `${id}-${stripped(parameter)}-${uid}`;
+    };


The comment suggests this is pretty crucial, should we explicitly test it?

Yeah, we should. I've added two tests that hopefully capture this behaviour!

Would be good, but not a blocker, to isolate this function to make it super obvious that given this input, we expect this output. That is, test the id function outside of the constructor. We can do this later as needed though.

akash1810 · 2021-03-24T09:30:50Z

package-lock.json

@@ -1,8 +1,19472 @@
 {
  "name": "@guardian/cdk",
  "version": "4.0.0",
-  "lockfileVersion": 1,
+  "lockfileVersion": 2,


I think you're on NPM v7? You'll want to downgrade to v6, I think . See #340 and #335 (comment).

D'oh, thanks! Resolved in a coming push.

…etch latest version

stephengeller · 2021-03-29T11:43:38Z

Thanks for the comments @akash1810 , I've now addressed them all as well as updated the PR name and added a use case, hope that's what you were after?

We have a few too many instances of // eslint-disable-next-line custom-rules/valid-constructors 😅 . It looks like this is mainly because the id field is static (for example the Stage parameter) or generated by the props passed in (for example InstanceType). This PR revises the rules to be: 1. Private constructors don't get linted 2. Must be 1, 2 or 3 parameters 3. First parameter must be called scope 4. First parameter must be of type GuStack 5. If 2 parameters: - The second parameter must be called props - The second parameter must be a custom type 6. If 3 parameters: - The second parameter must be called id - The second parameter must be of type string - The third parameter must be called props - The third parameter must be a custom type 7. Only the third parameter can be optional or have a default value

akash1810

Looks great. A couple of minor comments to consider, but feel free to address them in future PRs.

akash1810 · 2021-03-29T13:16:30Z

src/constructs/core/custom-resources/runtime/lambda.ts

+      hostname: parsedUrl.hostname,
+      path: parsedUrl.path,
+      method: "PUT",
+      headers: { "content-type": "", "content-length": responseBody.length },


What does content-type of empty string do/mean? Would omitting it yield the same behaviour?

I assume it's a bad thing, if anything. However, it was copied straight from aws's source code, so I'm tempted to leave it for now and only change if it causes issues?

akash1810 · 2021-03-29T13:24:27Z

src/constructs/core/ssm.ts

+    const id = (id: string) => {
+      const now = Date.now();
+      // We need to create UIDs for the resources in this construct, as otherwise CFN will not trigger the lambda on updates for resources that appear to be the same
+      const uid = now.toString().substr(now.toString().length - 4);
+      return parameter.toUpperCase().includes("TOKEN") ? `${id}-token-${uid}` : `${id}-${stripped(parameter)}-${uid}`;
+    };


Would be good, but not a blocker, to isolate this function to make it super obvious that given this input, we expect this output. That is, test the id function outside of the constructor. We can do this later as needed though.

akash1810 · 2021-03-29T13:26:36Z

src/constructs/core/ssm.ts

+  readonly grantPrincipal: IPrincipal;
+
+  // eslint-disable-next-line custom-rules/valid-constructors -- TODO: Remove once linting rules have been relaxed for this
+  constructor(scope: GuStack, props: GuSSMParameterProps) {


#352 allows for this constructor 🎉 a rebase of main should allow you to remove this line.

Nice one, done.

akash1810 · 2021-03-29T13:26:39Z

src/constructs/core/ssm.ts

+ * */
+export class GuSSMIdentityParameter extends GuSSMParameter {
+  // eslint-disable-next-line custom-rules/valid-constructors -- this may not be necessary going forward
+  constructor(scope: GuStack, props: GuSSMIdentityParameterProps) {


#352 allows for this constructor 🎉 a rebase of main should allow you to remove this line.

stephengeller · 2021-03-29T14:43:23Z

Would be good, but not a blocker, to isolate this function to make it super obvious that given this input, we expect this output. That is, test the id function outside of the constructor. We can do this later as needed though.

I isolated the function and added some tests, hopefully that should do the trick @akash1810 .

github-actions · 2021-03-29T14:45:48Z

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

First introduced in #336 and later removed in #1157, this change removes some code (and a dependency) for an old custom resource.

stephengeller requested review from akash1810 and jacobwinch March 18, 2021 13:37

jacobwinch reviewed Mar 18, 2021

View reviewed changes

stephengeller force-pushed the sg/custom-resource-ssm-param branch from ca1d230 to f328c36 Compare March 19, 2021 09:12

stephengeller force-pushed the sg/custom-resource-ssm-param branch 4 times, most recently from 6d3f92b to 007dfb5 Compare March 22, 2021 10:05

stephengeller marked this pull request as ready for review March 22, 2021 11:24

stephengeller requested a review from a team March 22, 2021 11:24

stephengeller marked this pull request as draft March 22, 2021 11:25

stephengeller force-pushed the sg/custom-resource-ssm-param branch 3 times, most recently from cc9ed72 to d4ce39c Compare March 22, 2021 13:54

stephengeller marked this pull request as ready for review March 22, 2021 13:58

akash1810 reviewed Mar 24, 2021

View reviewed changes

stephengeller changed the title ~~chore: Create GuSSMParameter using AWS Custom Resources~~ feat: Create GuSSMParameter using AWS Custom Resources Mar 29, 2021

stephengeller added 5 commits March 29, 2021 12:42

Create GuSSMParameter and the AWS Custom Resource that allows it to f…

9d27f82

…etch latest version

Create GuSSMParameter

9120e6d

remove extras

5663942

Add tests for random UIDs

5c30c11

update package-lock

6be4b43

stephengeller force-pushed the sg/custom-resource-ssm-param branch from 83f64c2 to 6be4b43 Compare March 29, 2021 11:46

Stephen Geller and others added 2 commits March 29, 2021 12:47

Merge branch 'main' into sg/custom-resource-ssm-param

58477db

akash1810 approved these changes Mar 29, 2021

View reviewed changes

guardian-ci and others added 4 commits March 29, 2021 13:29

chore(release): 6.1.0 (#360)

39934c6

extract id to fn outside constructor

bfe6cb6

Merge branch 'sg/custom-resource-ssm-param' into main

91499e6

Add tests for random UIDs

23914ea

stephengeller merged commit b65a4f0 into main Mar 29, 2021

stephengeller deleted the sg/custom-resource-ssm-param branch March 29, 2021 14:43

github-actions bot added the released label Mar 29, 2021

nicl mentioned this pull request Jan 7, 2022

Do we really need GuSSMParameter? #1032

Closed

akash1810 added a commit that referenced this pull request Sep 2, 2022

refactor: Remove unused code

0363222

First introduced in #336 and later removed in #1157, this change removes some code (and a dependency) for an old custom resource.

akash1810 mentioned this pull request Sep 2, 2022

refactor: Remove unused code #1472

Merged

2 tasks

feat: Create GuSSMParameter using AWS Custom Resources #336

feat: Create GuSSMParameter using AWS Custom Resources #336

Conversation

stephengeller commented Mar 18, 2021 • edited Loading

What does this change?

Problem

Solution

Usage

How does it work?

Does this change require changes to existing projects or CDK CLI?

How to test

How can we measure success?

Have we considered potential risks?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobwinch commented Mar 18, 2021

stephengeller commented Mar 19, 2021

stephengeller commented Mar 22, 2021

akash1810 left a comment

Choose a reason for hiding this comment

akash1810 Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephengeller commented Mar 29, 2021

akash1810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephengeller commented Mar 29, 2021

github-actions bot commented Mar 29, 2021

stephengeller commented Mar 18, 2021 •

edited

Loading

akash1810 Mar 24, 2021 •

edited

Loading