Skip to content

Conversation

@aug24
Copy link

@aug24 aug24 commented Jan 28, 2026

What does this change?

It provides the capability to automatically create an SSM Param for the Arn to be protected by WAF, using a standard form.

This is achieved simply by adding waf:true to GuEc2App.

How to test

Tests have been added to base.test.ts. These can be validated by synthing a cdk template for a stack which uses WAF; removing the WAF Arn param block and adding waf:true to the stack declaration.

How can we measure success?

A standard form Aws::SSM:Parameter is created.

Have we considered potential risks?

Risks are minimised as the default is to not create this param.

Checklist

  • I have listed any breaking changes, along with a migration path 1
  • I have updated the documentation as required for the described changes 2

Footnotes

  1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs.

  2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid?

@changeset-bot
Copy link

changeset-bot bot commented Jan 28, 2026

⚠️ No Changeset found

Latest commit: f1a0fce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@aug24 aug24 added enhancement New feature or request feature Departmental tracking: work on a new feature labels Jan 28, 2026
Comment on lines 306 to 308
/**
* You can specify if the arn of this load balancer should be exposed for protection via WAF
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add the prerequisites for this setting (eg. account needs to have waf set up) and a link to the waf configuration doc.

@aug24 aug24 force-pushed the add-waf-parameter-to-guec2app branch from 71e17c9 to f1a0fce Compare January 29, 2026 08:36
* At present it is necessary to remove the old param and immediately redeploy with the new param.
* This does not affect protection unless and until the WAF configuration is redeployed.
*/
waf?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if calling this exposeArnForWaf (or similar) would make it clearer1 that this boolean alone isn't sufficient to get WAF protection?

I don't feel strongly about this though so feel free to leave it as is if you think that's too verbose.

Footnotes

  1. To someone in a rush who doesn't read the typedocs!

*
* See https://github.com/guardian/waf/tree/main/lib
*
* There is a "gotcha" when migrating to this functionality. You may not change only the Logical
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this gotcha some more, I think the problem is likely to be:

  1. In the original CFN template, the LogicalIdA resource defines a AWS::SSM::Parameter with name /infosec/waf/services/TEST/my-app-alb-arn.
  2. Setting the new waf boolean to true adds the LogicalIdB resource; this is another AWS::SSM::Parameter with name /infosec/waf/services/TEST/my-app-alb-arn.

We cannot deploy a CFN stack which contains both LogicalIdA and LogicalIdB because a parameter name must be unique per account (per region?) - and we want to point at different values!

Even if we delete LogicalIdA in the same PR (CFN update) that provisions LogicalIdB, the update will fail because CFN applies creates before deletes, so LogicalIdA is still 'reserving' the magic parameter name at the crucial moment that we try to provision LogicalIdB.

IIUC this is what forces us to currently use 2 PRs (CFN updates) - one to delete LogicalIdA and one to provision LogicalIdB.

We could also consider allowing users to retain the original logical id (LogicalIdA) so that they can roll the change out in a single PR (CFN update).

In order to achieve this, the prop would need to be something like:

waf?: {
  enabled: boolean;
  existingParamId?: string;
}

And then in our implementation below, we'd have something like:

if (waf.enabled) {
  const stage = scope.stage;
  const wafParam = new StringParameter(this, "AlbSsmParam", {
    parameterName: `/infosec/waf/services/${stage}/${app}-alb-arn`,
    description: `The ARN of the ALB for ${stage}-${app}.`,
    simpleName: false,
    stringValue: loadBalancer.loadBalancerArn,
    tier: ParameterTier.STANDARD,
    dataType: ParameterDataType.TEXT,
  });
  if (waf.existingParamId) { 
    this.overrideLogicalId(wafParam, {
      logicalId: "LogicalIdA",
      reason: "Retaining original parameter's logical ID to avoid deployment issues"
    });
  }
}

This would allows users to delete the LogicalIdA resource from their YAML template in the same PR as enabling the WAF param via the pattern.

From a CloudFormation perspective, this means that the update just looks like a change to the value of the LogicalIdA AWS::SSM::Parameter (i.e. we update the exact same parameter but starting pointing it at the new ALB).

Copy link
Contributor

@jacobwinch jacobwinch Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although that allows a single deployment to work, it's still pretty fiddly, so if you are happy with the 2-step deployment then I think it's fine to leave it as is!

*/
defaultInstanceWarmup?: Duration;

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the helpful docs here.

In order to generate a new library release as part of this PR, you'll need to follow these instructions for adding a changeset. I'd suggest copy/pasting most of these details into the release notes that you add as part of that too.

Copy link
Contributor

@jacobwinch jacobwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving this; I've made some small suggestions but none of them are blocking.

Feel free to merge this in once you've added the changeset. This will trigger some automation which raises a PR similar to this one. Feel free to check/merge that PR yourself too.

Once the relevant Release package updates PR is merged, a new GuCDK version containing this feature should be available via npm.

Copy link
Member

@kelvin-chappell kelvin-chappell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request feature Departmental tracking: work on a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants