fix(cloudfront): OriginShield not easily disabled once enabled on an origin #22791

bradlead · 2022-11-04T20:15:47Z

Fixes #22233. Previous PR now closed #22334.

Added new prop originShieldEnabled as suggested by @corymhall which can be set to false if the user needs to explicitly disable origin shield.

Updated unit test origin.test.ts

Added new integ test.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

…origin

…tible with a disabled originShield

"pull in upstream changes"

gitpod-io · 2022-11-04T20:15:55Z

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.

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

mrgrain · 2022-11-11T12:52:33Z

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

+  /**
+   * A flag for disabling Origin Shield on the origin.
+   *
+   * @default - Origin Shield not enabled


I don't think this default description is correct. Based on the code I think we default to something like:

Does originShieldRegion exists? -> YES

Otherwise -> NO

Not sure if true as a default is more accurate.

mrgrain · 2022-11-11T12:57:50Z

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

@@ -82,6 +82,13 @@ export interface OriginOptions {
   */
  readonly originShieldRegion?: string;

+  /**
+   * A flag for disabling Origin Shield on the origin.


Let's be a bite more descriptive here (see also default). Explain how originShieldRegion and originShieldEnabled interact etc.

mrgrain · 2022-11-11T12:58:08Z

packages/@aws-cdk/aws-cloudfront/test/integ.distribution-origin-shield.ts

+  testCases: [stack],
+});
+
+app.synth();


Suggested change

app.synth();

app.synth();

mrgrain · 2022-11-11T13:02:28Z

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

@@ -139,6 +148,7 @@ export abstract class OriginBase implements IOrigin {
    this.customHeaders = props.customHeaders;
    this.originShieldRegion = props.originShieldRegion;
    this.originId = props.originId;
+    this.originShieldEnabled = props.originShieldEnabled;


This code is fine in principle. However boolean variables that might be also be undefined, can easily turn into bugs when not taken care of in if statements. Consider this code:

if (this.originShieldEnabled) { // do stuff when enabled } else { // do stuff when disabled }

In this case it seems to be the default is true, so the above would be broken when the value is undefined.

I'd recommend to always set an explicit default in these cases and adjust the private types accordingly:

Suggested change

this.originShieldEnabled = props.originShieldEnabled;

this.originShieldEnabled = props.originShieldEnabled ?? true;

(L136, L175 and L213 will need a change)

mrgrain · 2022-11-11T13:06:49Z

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

+  private renderOriginShield(originShieldRegion?: string, originShieldEnabled?: boolean ): CfnDistribution.OriginShieldProperty | undefined {
+    if (originShieldEnabled === false) return { enabled: false };
+    return originShieldRegion ? { enabled: true, originShieldRegion } : undefined;


Totally okay in principal. We avoid single line if statements. In combination with my other suggestion it could look like this:

Suggested change

private renderOriginShield(originShieldRegion?: string, originShieldEnabled?: boolean ): CfnDistribution.OriginShieldProperty | undefined {

if (originShieldEnabled === false) return { enabled: false };

return originShieldRegion ? { enabled: true, originShieldRegion } : undefined;

private renderOriginShield(originShieldEnabled: boolean, originShieldRegion?: string): CfnDistribution.OriginShieldProperty | undefined {

if (!originShieldEnabled) {

return { enabled: false };

}

return originShieldRegion ? { enabled: true, originShieldRegion } : undefined;

Pull request has been modified.

bradlead · 2022-11-11T14:54:21Z

Hey @mrgrain I've made the changes in this commit Thanks

mrgrain · 2022-11-11T14:57:38Z

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

@@ -124,7 +132,8 @@ export abstract class OriginBase implements IOrigin {
  private readonly connectionTimeout?: Duration;
  private readonly connectionAttempts?: number;
  private readonly customHeaders?: Record<string, string>;
-  private readonly originShieldRegion?: string
+  private readonly originShieldRegion?: string;
+  private readonly originShieldEnabled?: (true | boolean);


Suggested change

private readonly originShieldEnabled?: (true | boolean);

private readonly originShieldEnabled: boolean;

mrgrain · 2022-11-11T14:58:11Z

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

@@ -162,7 +172,7 @@ export abstract class OriginBase implements IOrigin {
        originCustomHeaders: this.renderCustomHeaders(),
        s3OriginConfig,
        customOriginConfig,
-        originShield: this.renderOriginShield(this.originShieldRegion),
+        originShield: this.renderOriginShield(this.originShieldEnabled ?? true, this.originShieldRegion),


Suggested change

originShield: this.renderOriginShield(this.originShieldEnabled ?? true, this.originShieldRegion),

originShield: this.renderOriginShield(this.originShieldEnabled, this.originShieldRegion),

mrgrain

We can simplify these types now. After that we should be done.

Pull request has been modified.

aws-cdk-automation · 2022-11-11T15:50:56Z

AWS CodeBuild CI Report

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

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

mrgrain

🚀

mergify · 2022-11-11T15:55:15Z

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).

Bradley Leadbetter added 5 commits October 3, 2022 17:46

fix(cloudfront): OriginShield not easily disabled once enabled on an …

8eb4608

…origin

fix(cloudfront-origins): fixed integ and unit tests so they are compa…

8f9114a

…tible with a disabled originShield

Merge branch 'main' of github.com:bradlead/aws-cdk

dc01ab5

"pull in upstream changes"

Added originShieldEnabled and created new tests

d75f098

Returning integ snapshots to previous state

4bc9605

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 4, 2022

aws-cdk-automation requested a review from a team November 4, 2022 20:16

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Nov 4, 2022

aws-cdk-automation previously requested changes Nov 4, 2022

View reviewed changes

Bradley Leadbetter added 3 commits November 4, 2022 20:49

updated dependent integ snapshot

06d8cf5

Removing previous changes to integ snapshots

e9bc21d

Added and ran integ test

6456dd5

Delete =

948728e

mrgrain previously requested changes Nov 11, 2022

View reviewed changes

Bradley Leadbetter and others added 3 commits November 11, 2022 14:44

Improved description and open ended If statement

0585c56

Merge branch 'main' of github.com:bradlead/aws-cdk

d853f84

Merge branch 'main' into main

3212956

Merge branch 'main' of github.com:bradlead/aws-cdk

56f5660

mrgrain reviewed Nov 11, 2022

View reviewed changes

mrgrain previously requested changes Nov 11, 2022

View reviewed changes

simplified types

5487a8c

mrgrain approved these changes Nov 11, 2022

View reviewed changes

mergify bot merged commit 6be4cf6 into aws:main Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudfront): OriginShield not easily disabled once enabled on an origin #22791

fix(cloudfront): OriginShield not easily disabled once enabled on an origin #22791

bradlead commented Nov 4, 2022 •

edited

Loading

gitpod-io bot commented Nov 4, 2022

aws-cdk-automation left a comment

mrgrain Nov 11, 2022

mrgrain Nov 11, 2022

mrgrain Nov 11, 2022

mrgrain Nov 11, 2022

mrgrain Nov 11, 2022

bradlead commented Nov 11, 2022

mrgrain Nov 11, 2022

mrgrain Nov 11, 2022

mrgrain left a comment

aws-cdk-automation commented Nov 11, 2022

mrgrain left a comment

mergify bot commented Nov 11, 2022

	this.originShieldEnabled = props.originShieldEnabled;
	this.originShieldEnabled = props.originShieldEnabled ?? true;

	private readonly originShieldEnabled?: (true \| boolean);
	private readonly originShieldEnabled: boolean;

	originShield: this.renderOriginShield(this.originShieldEnabled ?? true, this.originShieldRegion),
	originShield: this.renderOriginShield(this.originShieldEnabled, this.originShieldRegion),

fix(cloudfront): OriginShield not easily disabled once enabled on an origin #22791

fix(cloudfront): OriginShield not easily disabled once enabled on an origin #22791

Conversation

bradlead commented Nov 4, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Nov 4, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

mrgrain Nov 11, 2022

Choose a reason for hiding this comment

mrgrain Nov 11, 2022

Choose a reason for hiding this comment

mrgrain Nov 11, 2022

Choose a reason for hiding this comment

mrgrain Nov 11, 2022

Choose a reason for hiding this comment

mrgrain Nov 11, 2022

Choose a reason for hiding this comment

bradlead commented Nov 11, 2022

mrgrain Nov 11, 2022

Choose a reason for hiding this comment

mrgrain Nov 11, 2022

Choose a reason for hiding this comment

mrgrain left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 11, 2022

AWS CodeBuild CI Report

mrgrain left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 11, 2022

bradlead commented Nov 4, 2022 •

edited

Loading