feat(route53): Domain redirect pattern #3946

hoegertn · 2019-09-04T23:05:31Z

This implements an HTTPS redirect using a CloudFront distribution and an S3 bucket. (fix #3893)

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

hoegertn · 2019-09-04T23:06:00Z

This is currently work-in-progress but I wanted to get feedback early in the process.

mergify · 2019-09-04T23:06:09Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-09-04T23:42:51Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

eladb · 2019-09-05T05:22:47Z

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

+   * The domain name
+   *
+   * @default - the domain name of the zone
+   */


Should this only be a subdomain? Will this also support redirecting the root domain?

Perhaps call it recordName to align with route53 terminology? Provide an @example

@hoegertn what do you think?

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

eladb · 2019-09-05T05:25:06Z

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

+      originConfigs: [{
+        behaviors: [{ isDefaultBehavior: true }],
+        customOriginSource: {
+          domainName: Fn.select(2, Fn.split('/', redirectBucket.bucketWebsiteUrl)),


Are you sure there isn’t an attribute for this already?

I presume that this is to remove the http:// from the website url attribute

Wouldn't that be redirectBucket.bucketDomainName?

bucketDomainName points to the REST API endpoint, not the website endpoint. The REST API endpoint doesn't support redirects.

mystack-mybucket-kdwwxmddtr2g.s3.amazonaws.com vs http://mystack-mybucket-kdwwxmddtr2g.s3-website-us-east-2.amazonaws.com/

https://docs.aws.amazon.com/AmazonS3/latest/dev/WebsiteEndpoints.html#WebsiteRestEndpointDiff

@hoegertn may be add a comment explaining this line.

the bucketDomainName is the name for S3 operations but not the website domain

Should we simply add another attribute to s3.Bucket? Sounds like something others can benefit from...

@hoegertn care to raise an issue (and make sure to reference this usage so we can track it down).

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

eladb · 2019-09-05T05:53:20Z

packages/@aws-cdk/aws-route53-patterns/README.md

+This library contains commonly used patterns for Route53:
+* HTTP Redirect
+  ```ts
+  new HTTPSRedirect(stack, 'Redirect', {


Should be called HttpsRedirect

eladb · 2019-09-05T05:54:47Z

packages/@aws-cdk/aws-route53-patterns/README.md

+
+This library contains commonly used patterns for Route53:
+* HTTP Redirect
+  ```ts


Provide some details about what this construct does and a glimpse to the underlying implementation in a sentence.

eladb · 2019-09-05T07:16:38Z

@rix0rrr what do you think about the module? Does it make sense to file this under "route53"? I am not 100% convinced.

jogold

I think we should offer a way to redirect multiple domains to the same target domain: domain.com -> www.domain.com, domain.net -> www.domain.com, domian.com -> www.domain.com

This can be achieved using a single CloudFront distribution with multiple alternate domain names (and multiple S3 buckets and A records).

jogold · 2019-09-05T07:29:31Z

packages/@aws-cdk/aws-route53-patterns/README.md

+<!--END STABILITY BANNER-->
+
+This library contains commonly used patterns for Route53:
+* HTTP Redirect


Suggested change

* HTTP Redirect

* HTTPS Redirect

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

jogold · 2019-09-05T07:32:05Z

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

+   */
+  readonly domainName?: string;
+  /**
+   * The ARN of the certificate; Has to be in us-east-1


Suggested change

* The ARN of the certificate; Has to be in us-east-1

* The ACM certificate; Has to be in us-east-1

jogold · 2019-09-05T07:33:52Z

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

+      originConfigs: [{
+        behaviors: [{ isDefaultBehavior: true }],
+        customOriginSource: {
+          domainName: Fn.select(2, Fn.split('/', redirectBucket.bucketWebsiteUrl)),


I presume that this is to remove the http:// from the website url attribute

mergify · 2019-09-05T10:07:33Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

Update ALL jsii dependencies to 0.16, instead of just the "jsii" tool proper.

Bumps [@types/semver](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/semver) from 6.0.1 to 6.0.2. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/semver) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Bumps [aws-sdk](https://github.com/aws/aws-sdk-js) from 2.522.0 to 2.524.0. - [Release notes](https://github.com/aws/aws-sdk-js/releases) - [Changelog](https://github.com/aws/aws-sdk-js/blob/master/CHANGELOG.md) - [Commits](aws/aws-sdk-js@v2.522.0...v2.524.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

See CHANGELOG

…3914) * fix(certificatemanager): add minimum backoff Add a minimum component to the backoff and retry timer * fix(certificatemanager): stub out sleep Provide stub function for sleep to allow long running sleep to cause test to fail * fix(certificatemanager): spy on sleep spy on sleep function to determine total time attempted to sleep * fix(certificatemanager): Failing test Failing test demonstrating min sleep is as low as 4.6 seconds. * fix(certificatemanager): increase attempts Increase max attempts in validation, still under 10, but such that minimum sleep duration of one minute is reached (but not more than 3 minutes). Can adjust tests if window is not suitable, but I find that at least 30 seconds is needed in my region

Removes the dependabot autolabeling functionality (unnecessary and confusing)

mergify · 2019-09-06T12:58:34Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

eladb

There are still some unaddressed/unresolved comments.

eladb · 2019-09-08T07:22:30Z

packages/@aws-cdk/aws-route53-patterns/README.md

+## HTTPS Redirect
+
+This construct allows creating a simple HTTP->HTTPS and domainA->domainB redirect using CloudFront and S3.
+


Add an snippet on how to create an HTTP=>HTTPS redirect. It's not clear from this.

I will reword this sentence. It is doing a domain redirect and http->https redirect for domainA. You cannot use this construct to redirect http->https on your app domain, as you can only have of DNS entry for your domain.

eladb · 2019-09-08T07:23:31Z

packages/@aws-cdk/aws-route53-patterns/lib/website-redirect.ts

+      originConfigs: [{
+        behaviors: [{ isDefaultBehavior: true }],
+        customOriginSource: {
+          domainName: Fn.select(2, Fn.split('/', redirectBucket.bucketWebsiteUrl)),


Should we simply add another attribute to s3.Bucket? Sounds like something others can benefit from...

jogold · 2019-09-09T13:19:48Z

packages/@aws-cdk/aws-route53-patterns/README.md

+
+```ts
+new HttpsRedirect(stack, 'Redirect', {
+  domainName: 'foo.example.com',


This is now recordNames

eladb · 2019-09-12T11:45:19Z

Any updates here?

hoegertn · 2019-09-12T11:51:45Z

We had AWS Community Days in Germany and I did not have time. Will try to fix it tonight.

eladb · 2019-09-12T11:53:54Z

No worries. Just checking in. No rush of course.

mergify · 2019-09-12T17:42:12Z

Continuous integration build failed

hoegertn · 2019-09-12T19:57:27Z

@eladb Something strange is happening on build. If I remove the dependency in decdk it fails and if I add it it does not find it. What do I have to do?

mergify · 2019-09-13T00:18:35Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

jogold · 2019-09-13T13:09:10Z

@eladb Something strange is happening on build. If I remove the dependency in decdk it fails and if I add it it does not find it. What do I have to do?

Seems like your package-lock.json is not in sync anymore with your package.json:

update package version to 1.8.0 in package.json
update all cdk dependencies in your package.json to ^1.8.0 (remove unused @aws-cdk/region-info)
inside aws-route53-patterns: rm -rf package-lock.json && npm i

aws-cdk-automation · 2019-09-13T16:21:54Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-13T16:33:57Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-13T17:24:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

This implements an HTTPS redirect using a CloudFront distribution and an S3 bucket. (fix #3893)

feat: add HTTPS redirect construct

209c123

mergify bot added the automerge-enabled label Sep 4, 2019

eladb self-assigned this Sep 5, 2019

eladb reviewed Sep 5, 2019

View reviewed changes

jogold reviewed Sep 5, 2019

View reviewed changes

Merge branch 'master' into https-redirect

2734446

rix0rrr and others added 11 commits September 6, 2019 14:12

chore(jsii): update to version 0.16 (#3932)

bba6db3

Update ALL jsii dependencies to 0.16, instead of just the "jsii" tool proper.

chore(aws-apigateway): fix typo (#3955)

5e9f38b

v1.7.0 (#3960)

1fc002d

See CHANGELOG

chore: bump year in root LICENSE (#3966)

112986d

chore(repo): remove dpbot autolabel (#3964)

252639f

Removes the dependabot autolabeling functionality (unnecessary and confusing)

chore: merge master

06a2878

chore: review feedback

6cea035

Merge branch 'master' into https-redirect

3a52141

hoegertn and others added 3 commits September 6, 2019 18:57

fix: decdk

7cd6afa

Merge branch 'master' into https-redirect

2597f25

Update README.md

48ee78e

eladb reviewed Sep 8, 2019

View reviewed changes

Merge branch 'master' into https-redirect

dc7bafe

eladb changed the title ~~WIP: feat(route53-patterns): add HTTPS redirect construct~~ WIP: feat(route53): https redirect pattern construct Sep 8, 2019

eladb changed the title ~~WIP: feat(route53): https redirect pattern construct~~ feat(route53): https redirect pattern Sep 9, 2019

jogold reviewed Sep 9, 2019

View reviewed changes

Merge branch 'master' into https-redirect

ac08674

hoegertn added 2 commits September 12, 2019 20:17

review

f71f935

fix test

9a8ea24

NGL321 removed the automerge-enabled label Sep 13, 2019

hoegertn changed the title ~~feat(route53): https redirect pattern~~ feat(route53): Domain redirect pattern Sep 13, 2019

Merge branch 'master' into https-redirect

8781042

nmussy mentioned this pull request Sep 13, 2019

fix(route53): remove http:// from bucket target #4070

Merged

hoegertn added 2 commits September 13, 2019 18:19

Merge branch 'master' into https-redirect

e034580

fix package.json

34b6dc7

fix imports

05a2b57

eladb approved these changes Sep 14, 2019

View reviewed changes

eladb merged commit 3ac4671 into aws:master Sep 16, 2019

eladb pushed a commit that referenced this pull request Sep 23, 2019

feat(route53): Domain redirect pattern (#3946)

c8abd35

This implements an HTTPS redirect using a CloudFront distribution and an S3 bucket. (fix #3893)

hoegertn deleted the https-redirect branch September 27, 2019 07:09

	* The ARN of the certificate; Has to be in us-east-1
	* The ACM certificate; Has to be in us-east-1

		## HTTPS Redirect

		This construct allows creating a simple HTTP->HTTPS and domainA->domainB redirect using CloudFront and S3.

feat(route53): Domain redirect pattern #3946

feat(route53): Domain redirect pattern #3946

Conversation

hoegertn commented Sep 4, 2019 • edited by eladb

hoegertn commented Sep 4, 2019

mergify bot commented Sep 4, 2019

Pull Request Checklist

mergify bot commented Sep 4, 2019

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

eladb Sep 5, 2019 • edited

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

eladb commented Sep 5, 2019

jogold 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

mergify bot commented Sep 5, 2019

mergify bot commented Sep 6, 2019

eladb 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

eladb commented Sep 12, 2019

hoegertn commented Sep 12, 2019

eladb commented Sep 12, 2019

mergify bot commented Sep 12, 2019

hoegertn commented Sep 12, 2019

mergify bot commented Sep 13, 2019

jogold commented Sep 13, 2019

aws-cdk-automation commented Sep 13, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 13, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 13, 2019

AWS CodeBuild CI Report

hoegertn commented Sep 4, 2019 •

edited by eladb

eladb Sep 5, 2019 •

edited