aws-cloudfront: OriginShield not easily disabled once enabled on an origin

[s-j-white]

Describe the bug

When you enable OriginShield on an origin by passing originShieldRegion property (see https://github.com/aws/aws-cdk/blob/7e424fdc45f98c83f0b19846c9683d7eeb767d80/packages/%40aws-cdk/aws-cloudfront/lib/origin.ts), it is not possible to then disable OriginShield without either doing so manually or by using escape hatches to modify the generated CloudFormation.

Adding a value for originShieldRegion will render a OriginShieldProperty in the generated CloudFormation. Removing the value will return undefined for the render. For origins where OriginShield is already enabled this will be a no-op. OriginShield will remain enabled.

This has a side effect where, due to how logical IDs are generated in CDK, if you add a new origin that disrupts the ordering of the origins then you can end up with OriginShield enabled for an origin that you never explicitly enabled it for (more details below).

Expected Behavior

When OriginShield is referenced explicitly it should be enabled but when it isn't mentioned then it should be disabled by default, not just left in the current state.

Current Behavior

The OriginShieldProperty is only ever rendered for the enable case which means that it cannot be disabled without diving into the Cfn* objects and overriding properties.

This can lead to OriginShield being enabled on origins that we don't expect when combined with how CDK generates logical ids for origins.

Probably easiest to illustrate with an example. Let’s say I have two origins configured:
Origin: { domainName: ‘service unsuitable for origin shield’ }
Origin: { domainName: 'service suited to origin shield', originShieldRegion: 'eu-west-1' }

The generated CloudFormation to be deployed looks something like:
Distribution:
origins:
- OriginABC123:
domainName: ‘service unsuitable for origin shield’
- OriginXYZ789:
domainName: 'service suited to origin shield'
originShield:
enabled: true
originShieldRegion: eu-west-1

When this is deployed everything is as expected, we have two origins configured with the correct config.

In our CDK app we then add a new origin and due to how the code is structured it ends up being the first origin referenced in the stack:
Origin: { domainName: 'some new service' }
Origin: { domainName: ‘service unsuitable for origin shield’ }
Origin: { domainName: 'service suited to origin shield', originShieldRegion: 'eu-west-1' }

The generated CloudFormation to be deployed looks something like (and note the logical ids will be generated entirely based on the order of the origins):
Distribution:
origins:
- OriginABC123:
domainName: ‘some new service’
- OriginXYZ789:
domainName: 'service unsuitable for origin shield'
- OriginNMO456:
domainName: 'service suited to origin shield'
originShield:
enabled: true
originShieldRegion: eu-west-1

When we actually deploy this, there is immediate stack drift due to the CDK bug mentioned. Since lack of origin shield configuration in the CloudFormation means no change happens at all, by the time we have deployed and look in console, we see three origins configured as follows:
OriginABC123:
domainName: ‘some new service’
OriginXYZ789:
domainName: 'service unsuitable for origin shield'
originShield:
enabled: true
originShieldRegion: eu-west-1
OriginNMO456:
domainName: 'service suited to origin shield'
originShield:
enabled: true
originShieldRegion: eu-west-1

Reproduction Steps

If you deploy something like this:
new cloudfront.Distribution(this, 'distro', { defaultBehavior: { origin: new origins.HttpOrigin('example.com', { originShieldRegion: 'eu-west-1' }), }, });
And then change to this and redeploy:
new cloudfront.Distribution(this, 'distro', { defaultBehavior: { origin: new origins.HttpOrigin('example.com'), }, });
And check AWS console, the origin will still have Origin Shield enabled on it.

For the side effect if you instead deploy:
new cloudfront.Distribution(this, 'distro', { defaultBehavior: { origin: new origins.HttpOrigin('example.com'), }, additionalBehaviours: { "/api": { origin: new origins.HttpOrigin('other-example.com', { originShieldRegion: 'eu-west-1' }) } }, });

This deploy two origins, one with Origin Shield enabled.

And then modify the code and redeploy like this:
new cloudfront.Distribution(this, 'distro', { defaultBehavior: { origin: new origins.HttpOrigin('example.com'), }, additionalBehaviours: { "/api2": { origin: new origins.HttpOrigin('yet-another-example.com') }, "/api": { origin: new origins.HttpOrigin('other-example.com', { originShieldRegion: 'eu-west-1' }) } }, });

This will modify the existing origin that has Origin Shield enabled and change the domain name to "yet-another-example.com" due to the logical ids matching and create a new origin, also with origin shield enabled going to 'other-example.com'

Possible Solution

On this line

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/origin.ts

Line 197 in 7e424fd

: undefined;

undefined is always returned. If it instead return an OriginShieldProperty like:
{ enabled: false } then OriginShield would always be disabled when a region isn't specified (which I think is the original intent).

Additional Information/Context

No response

CDK CLI Version

2.8.0

Framework Version

No response

Node.js Version

16.16

OS

MacOS 12.4

Language

Typescript

Language Version

4.6.4

Other information

No response

[peterwoodworth]

Thanks for the thorough bug report @s-j-white

We've had issues similar to this before, we should be more careful to set the property when possible instead of setting it to undefined for cases like this.

I am marking this issue as p2, which means that we are unable to work on this immediately.

Check out our contributing guide if you're interested in contributing yourself - there's a low chance the team will be able to address this soon but we'd be happy to review a PR 🙂

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.