feat(route53): improve constructs for basic records #2741

jogold · 2019-06-04T15:28:26Z

Constructs for records (CNAME, TXT, etc.) now extend the RecordSet construct and
offer better typed properties interfaces.

Add constructs for A, AAAA, CAA, MX and SRV records.

Add support for multiple values in basic records.

Make recordName optional with default to zone root.

Add a "security" CaaAmazonRecord construct to easily restrict certificate authorities
allowed to issue certificates for a domain to Amazon only.

BREAKING CHANGE: recordValue: string prop in route53.TxtRecord changed to values: string[]

recordValue prop in route53.CnameRecord renamed to domainName
route53.AliasRecord has been removed, use route53.ARecord or route53.AaaaRecord with the target prop.

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Constructs for basic records (CNAME, TXT, etc.) now extend the `BasicRecord` construct and offer better typed properties interfaces. Add constructs for A, AAAA, CAA, MX and SRV records. Add support for multiple values in basic records. Make `recordName` optional with default to zone root. Add a "security" `CaaAmazonRecord` construct to easily restrict certificate authorities allowed to issue certificates for a domain to Amazon only. BREAKING CHANGE: `recordValue: string` prop in `route53.TxtRecord` changed to `values: string[]` * `recordValue` prop in `route53.CnameRecord` renamed to `domainName`

rix0rrr · 2019-06-05T08:03:57Z

packages/@aws-cdk/aws-route53/lib/records/basic.ts

+ *
+ * @see https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/resource-record-sets-values-basic.html
+ */
+export class BasicRecord extends Resource {


abstract class?

If not abstract, then it should probably be called RecordSet.

I want to highlight the fact that it's a construct for Basic Records only (not alias, failover, geolocation, etc. see https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/resource-record-sets-values.html).

I don't think it should be abstract, one could use this class to create a record set for record type that is not implemented in an inherited class

Sure. If it mirrors the L1 resource it should have the same name I feel. Shouldn't it be called RecordSet then?

You could say the same for AliasRecord looking at its implementation... 🤔

I let you decide...

Yes, the current design is probably not great. AliasRecord shouldn't be a class by itself, but probably ARecord needs an aliasTarget property.

Do you have any specific changes in mind you would like to propose?

You are right, the best solution here would be a base RecordSet class with aliasTarget moved to both ARecord and AaaaRecord.

Will update accordingly if you're OK with this (requires small changes in aws-ecs-patterns and aws-route53-targets also)

We then need a union type in ARecord right? Whether there are literal targets or an aliasTarget? We do union classes like so these days:

https://github.com/awslabs/aws-cdk/blob/master/packages/@aws-cdk/aws-stepfunctions/lib/input.ts

Something like that?

new ARecord(this, 'IPs', { zone: myZone, target: RecordSetTarget.fromIpAddresses('1.2.3.4', '5.6.7.8') // (...ipAddresses: string[]) });

new ARecord(this, 'Alias', { zone: myZone, target: RecordSetTarget.fromAlias(new targets.LoadBalancerTarget(myAlb)) });

The ARecord would then call super() with either recordValues or aliasTarget.

Another solution is to have both ipAddresses and target props with a runtime check.

Yes, that's what I was thinking. The union type is more in line with what we're currently doing.

rix0rrr · 2019-06-05T08:04:33Z

packages/@aws-cdk/aws-route53/lib/records/basic.ts

+ *
+ * @see https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/resource-record-sets-values-basic.html
+ */
+export class BasicRecord extends Resource {


If not abstract, then it should probably be called RecordSet.

packages/@aws-cdk/aws-route53/lib/records/basic.ts

jogold · 2019-06-05T16:13:27Z

packages/@aws-cdk/aws-route53/lib/record-set.ts

+/**
+ * A record to delegate further lookups to a different set of name servers.
+ */
+export class ZoneDelegationRecord extends RecordSet {


Shouldn't this be renamed to NsRecord for uniformity? (ZoneDelegation is the current name)

packages/@aws-cdk/aws-route53/lib/record-set.ts

rix0rrr

Thanks Jonathan :)

jogold requested a review from a team as a code owner June 4, 2019 15:28

caaAmazon for PublicHostedZone

c1c2263

rix0rrr reviewed Jun 5, 2019

View reviewed changes

RecordSet, refactor and clean up

b1490a6

jogold commented Jun 5, 2019

View reviewed changes

jogold added 2 commits June 5, 2019 18:24

Merge branch 'master' into r53-basic-records

e51eb57

JSDoc and fromAlias

06589ac

jogold commented Jun 5, 2019

View reviewed changes

packages/@aws-cdk/aws-route53/lib/record-set.ts Outdated Show resolved Hide resolved

jogold and others added 2 commits June 6, 2019 19:50

move type union to RecordSet

705bdc7

README

49020c8

rix0rrr approved these changes Jun 7, 2019

View reviewed changes

rix0rrr merged commit 696f53f into aws:master Jun 7, 2019

jogold deleted the r53-basic-records branch June 13, 2019 13:18

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(route53): improve constructs for basic records #2741

feat(route53): improve constructs for basic records #2741

jogold commented Jun 4, 2019 •

edited

Loading

rix0rrr Jun 5, 2019

rix0rrr Jun 5, 2019

jogold Jun 5, 2019

rix0rrr Jun 5, 2019

jogold Jun 5, 2019

rix0rrr Jun 5, 2019 •

edited

Loading

jogold Jun 5, 2019 •

edited

Loading

rix0rrr Jun 5, 2019

jogold Jun 5, 2019 •

edited

Loading

rix0rrr Jun 5, 2019

rix0rrr Jun 5, 2019

jogold Jun 5, 2019

rix0rrr left a comment

feat(route53): improve constructs for basic records #2741

feat(route53): improve constructs for basic records #2741

Conversation

jogold commented Jun 4, 2019 • edited Loading

Pull Request Checklist

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

rix0rrr Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

jogold Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Jun 5, 2019 • 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

rix0rrr left a comment

Choose a reason for hiding this comment

jogold commented Jun 4, 2019 •

edited

Loading

rix0rrr Jun 5, 2019 •

edited

Loading

jogold Jun 5, 2019 •

edited

Loading

jogold Jun 5, 2019 •

edited

Loading