Support workers CDK #6449

rupertbates · 2024-10-28T17:42:20Z

What are you doing in this PR?

This PR replaces the existing home rolled way of generating the cloudformation for support-workers with CDK.

This is more standard and a lot simpler generally as the previous approach involved messing about with handlebar templates. It has been successfully tested on CODE.

github-actions · 2024-10-28T17:44:39Z

Deploy build 11901 of `support::stripe-patrons-data` to CODE

All deployment options

From guardian/actions-riff-raff.

github-actions · 2024-10-28T17:45:57Z

Deploy build 21165 of `support:support-workers-mono` to CODE

All deployment options

From guardian/actions-riff-raff.

github-actions · 2024-10-28T17:46:06Z

Size Change: 0 B

Total Size: 2.29 MB

ℹ️ View Unchanged

Filename	Size
`./public/compiled-assets/javascripts/[countryGroupId]/events/router.js`	112 kB
`./public/compiled-assets/javascripts/[countryGroupId]/router.js`	259 kB
`./public/compiled-assets/javascripts/ausMomentMap.js`	108 kB
`./public/compiled-assets/javascripts/contributionsRedirectStyles.js`	20 B
`./public/compiled-assets/javascripts/digitalSubscriptionLandingPage.js`	242 kB
`./public/compiled-assets/javascripts/downForMaintenancePage.js`	69.3 kB
`./public/compiled-assets/javascripts/error404Page.js`	69.3 kB
`./public/compiled-assets/javascripts/error500Page.js`	69.2 kB
`./public/compiled-assets/javascripts/favicons.js`	617 B
`./public/compiled-assets/javascripts/paperSubscriptionCheckoutPage.js`	195 kB
`./public/compiled-assets/javascripts/paperSubscriptionLandingPage.js`	85.8 kB
`./public/compiled-assets/javascripts/payPalErrorPage.js`	67.6 kB
`./public/compiled-assets/javascripts/payPalErrorPageStyles.js`	20 B
`./public/compiled-assets/javascripts/promotionTerms.js`	72.2 kB
`./public/compiled-assets/javascripts/subscriptionsLandingPage.js`	71.5 kB
`./public/compiled-assets/javascripts/subscriptionsRedemptionPage.js`	126 kB
`./public/compiled-assets/javascripts/supporterPlusLandingPage.js`	312 kB
`./public/compiled-assets/javascripts/unsupportedBrowserStyles.js`	20 B
`./public/compiled-assets/javascripts/weeklySubscriptionCheckoutPage.js`	192 kB
`./public/compiled-assets/javascripts/weeklySubscriptionLandingPage.js`	85.8 kB
`./public/compiled-assets/webpack/136.js`	2.17 kB
`./public/compiled-assets/webpack/187.js`	21.5 kB
`./public/compiled-assets/webpack/344.js`	2.01 kB
`./public/compiled-assets/webpack/671.js`	21.8 kB
`./public/compiled-assets/webpack/706.js`	107 kB

_{compressed-size-action}

johnduffell · 2024-10-31T13:44:40Z

cdk/bin/cdk.ts

+  ],
+  s3Files: [
+    "arn:aws:s3:::gu-zuora-catalog/PROD/Zuora-CODE/catalog.json",
+    "arn:aws:s3:::support-workers-private/*",


I assume this is just copied from elsewhere but it does seem a little generous

tomrf1

👍 looks good

johnduffell · 2024-10-31T13:49:56Z

cdk/lib/support-workers.ts

+      this.overrideLogicalId(lambda, {
+        logicalId: lambdaId,
+        reason: "Moving existing lambda to CDK",
+      });


that's a clever trick, it's always tricky to get around this kind of auto generated IDs thing normally. I assume this is a permanent override though?

Suggested change

this.overrideLogicalId(lambda, {

logicalId: lambdaId,

reason: "Moving existing lambda to CDK",

});

this.overrideLogicalId(lambda, {

logicalId: lambdaId,

reason: "Retain pre-CDK logical ID",

});

johnduffell · 2024-10-31T14:01:17Z

cdk/lib/support-workers.ts

+      });
+      return new LambdaInvoke(this, lambdaName, {
+        lambdaFunction: lambda,
+        outputPath: "$.Payload", // Without this, LambdaInvoke wraps the output state as described here: https://github.com/aws/aws-cdk/issues/29473


I couldn't make sense of the bug, but the documentation[1] says that if we set the arn to lambda:invoke.... as we do with the above CDK construct, rather than the arn of the function as we did before, you get that wrapping.

[1] https://docs.aws.amazon.com/step-functions/latest/dg/connect-lambda.html

Suggested change

outputPath: "$.Payload", // Without this, LambdaInvoke wraps the output state as described here: https://github.com/aws/aws-cdk/issues/29473

outputPath: "$.Payload", // LambdaInvoke wraps the output state, so we have to unwrap for the next state as described here: https://docs.aws.amazon.com/step-functions/latest/dg/connect-lambda.html

having said that, could we keep the old way of specifying the ARN directly, as that's what the docs seem to suggest?

johnduffell · 2024-10-31T14:04:35Z

cdk/lib/support-workers.ts

+    );
+    const checkoutSuccess = new Succeed(this, "CheckoutSuccess");
+
+    const parallelSteps = new Parallel(this, "Parallel")


Suggested change

const parallelSteps = new Parallel(this, "Parallel")

const postAcquisitionSteps = new Parallel(this, "Parallel")

johnduffell · 2024-10-31T14:05:44Z

cdk/lib/support-workers.ts

+      .branch(sendAcquisitionEvent)
+      .branch(checkoutSuccess);
+
+    const commonSteps = createZuoraSubscription.next(parallelSteps);


common in what way?

Steps that are carried out for all users whether they have an existing account or not

postAccountExistsSteps ?

johnduffell · 2024-10-31T14:06:12Z

cdk/lib/support-workers.ts

+      .next(createSalesforceContactLambda)
+      .next(commonSteps);
+
+    const startPoint = shouldClonePaymentMethodChoice


slightly more mathematic perhaps?

Suggested change

const startPoint = shouldClonePaymentMethodChoice

const initialState = shouldClonePaymentMethodChoice

johnduffell · 2024-10-31T14:07:28Z

cdk/lib/support-workers.ts

+      .otherwise(stepsForNewAccount);
+
+    const stateMachine = new StateMachine(this, "SupportWorkers", {
+      stateMachineName: `${app}-${this.stage}`,


this is used by support frontend to find the state machine I think? might be worth a comment, now we're blessed with that ability

Suggested change

stateMachineName: `${app}-${this.stage}`,

stateMachineName: `${app}-${this.stage}`,// used by support-frontend to find the state machine

johnduffell · 2024-10-31T14:08:29Z

cdk/lib/support-workers.ts

+    const stateMachine = new StateMachine(this, "SupportWorkers", {
+      stateMachineName: `${app}-${this.stage}`,
+      definitionBody: DefinitionBody.fromChainable(startPoint),
+    });


can't see the overall (24h) timeout anywhere.

johnduffell · 2024-10-31T14:09:08Z

cdk/lib/support-workers.ts

+    };
+
+    const checkoutFailure = new Pass(this, "CheckoutFailure"); // This is a failed execution we don't want to alert on
+    const failState = new Fail(this, "FailState"); // This is a failed execution we do want to alert on


I assume the alarms are still going to be defined in the existing CFN for now (and will still match up with the retained IDs)

Great catch, I'd missed the alarms off. Will add them.

johnduffell · 2024-10-31T14:10:03Z

support-workers/scripts/update-code-cloudformation.sh

@@ -1,24 +1,17 @@
 #!/bin/bash

-./build-cloudformation.sh


do we need to run pnpm build or similar at this point?

Yes, it's running yarn synth

johnduffell · 2024-11-04T09:32:52Z

cdk/lib/support-workers.ts

@@ -232,5 +242,304 @@ export class SupportWorkers extends GuStack {
      logicalId: `SupportWorkers${this.stage}`,
      reason: "Moving existing step function to CDK",
    });
+
+    // Alarms


would it make sense to split it out into several files rather than relying on comments? or is that not the done thing for CDK?

I think it would make it more difficult to get a full picture of what's going on

johnduffell · 2024-11-04T09:35:33Z

cdk/lib/support-workers.ts

+
+    new GuAlarm(this, "ExecutionFailureAlarm", {
+      app,
+      actionsEnabled: isProd,


I prefer how you've done it as then CODE and PROD are more similar, but I remember there was some debate recently about whether we not have the alarm created at all in CODE, I can't remember whether it was causing an irritation for the alarms handler?

cdk/lib/support-workers.ts

johnduffell · 2024-11-04T09:39:14Z

cdk/lib/support-workers.ts

+      evaluationPeriods: 4,
+      treatMissingData: TreatMissingData.BREACHING,
+      threshold: 0,
+    }).node.addDependency(stateMachine);


why does this depend on stateMachine but the above ones don't?

johnduffell · 2024-11-04T09:43:30Z

cdk/lib/support-workers.ts

+      }),
+      comparisonOperator: ComparisonOperator.LESS_THAN_OR_EQUAL_TO_THRESHOLD,
+      evaluationPeriods: 4,
+      treatMissingData: TreatMissingData.BREACHING,


I assume this is how the alarm was already set up rather than a change you made, but actually this will take a fair bit longer than 3600s * 4 (4 hours) to go off, as when there is missing data it looks back to find more data earlier. The usual solution is to use a calculated metric and use FILL(m1, 0) as the expression, where m1 is the actual PaymentSuccess metric.

see example on line 453 (although that also SUMs several metric which is not needed here)

rupertbates added 3 commits October 23, 2024 15:17

Add cdk file for support-workers

747c2a1

Add all the lambdas to the state machine definition

6792ec5

riff-raff.yaml change

43f9db1

rupertbates added 20 commits October 29, 2024 12:30

Imports and logical ids

5b56d9a

Tweaking to get it working (it does now)

b21261d

FailureHandler needs email SQS permissions

e88e77b

Merge branch 'main' into support-workers-cdk

1890669

Remove the kinesis arns, they're handled by the event bus now

3d483ac

Add a snapshot test

d25ecce

Naming improvements

5a58994

Set memorySize and architecture to the same as current cfn

d37b43a

support-workers-build.yml

a6a303f

Snapshot

175ff17

Bucket SSM key

20a849b

Bucket SSM key

d77b814

Bucket SSM key

4165019

Merge branch 'main' into support-workers-cdk

020628a

Bucket SSM key

b5586e5

Try using hardcoded bucket again

d7b69c7

Merge branch 'main' into support-workers-cdk

ed7b3a9

Remove unused support-workers riff-raff.yaml

4689331

Delete old CFN code

890cded

Don't add a cache-dependency-path for old yarn.lock

36ecf42

rupertbates changed the title ~~Support workers cdk~~ Support workers CDK Oct 31, 2024

johnduffell reviewed Oct 31, 2024

View reviewed changes

tomrf1 approved these changes Oct 31, 2024

View reviewed changes

johnduffell reviewed Oct 31, 2024

View reviewed changes

rupertbates added 3 commits November 1, 2024 11:46

Add alarms

1f6c58d

Add label to alarms

8e96537

Add label to alarms

324de20

johnduffell reviewed Nov 4, 2024

View reviewed changes

cdk/lib/support-workers.ts Outdated Show resolved Hide resolved

johnduffell reviewed Nov 4, 2024

View reviewed changes

PR feedback

4865852

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support workers CDK #6449

Support workers CDK #6449

rupertbates commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

johnduffell Oct 31, 2024

tomrf1 left a comment

johnduffell Oct 31, 2024

johnduffell Oct 31, 2024

johnduffell Oct 31, 2024

johnduffell Oct 31, 2024

rupertbates Oct 31, 2024

johnduffell Oct 31, 2024

johnduffell Oct 31, 2024

johnduffell Oct 31, 2024

johnduffell Oct 31, 2024

johnduffell Oct 31, 2024 •

edited

Loading

rupertbates Oct 31, 2024

johnduffell Oct 31, 2024

rupertbates Nov 4, 2024

johnduffell Nov 4, 2024

rupertbates Nov 4, 2024

johnduffell Nov 4, 2024

johnduffell Nov 4, 2024

johnduffell Nov 4, 2024

johnduffell Nov 4, 2024

	outputPath: "$.Payload", // Without this, LambdaInvoke wraps the output state as described here: https://github.com/aws/aws-cdk/issues/29473
	outputPath: "$.Payload", // LambdaInvoke wraps the output state, so we have to unwrap for the next state as described here: https://docs.aws.amazon.com/step-functions/latest/dg/connect-lambda.html

	const parallelSteps = new Parallel(this, "Parallel")
	const postAcquisitionSteps = new Parallel(this, "Parallel")

	const startPoint = shouldClonePaymentMethodChoice
	const initialState = shouldClonePaymentMethodChoice

	stateMachineName: `${app}-${this.stage}`,
	stateMachineName: `${app}-${this.stage}`,// used by support-frontend to find the state machine

Support workers CDK #6449

Are you sure you want to change the base?

Support workers CDK #6449

Conversation

rupertbates commented Oct 28, 2024 • edited Loading

What are you doing in this PR?

github-actions bot commented Oct 28, 2024 • edited Loading

github-actions bot commented Oct 28, 2024 • edited Loading

github-actions bot commented Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

tomrf1 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnduffell Oct 31, 2024 • 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

rupertbates commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

johnduffell Oct 31, 2024 •

edited

Loading