Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(route53): return plain hosted zone id without /hostedzone/ prefix #5230

Merged
merged 9 commits into from
Dec 12, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ export = {
domainZone: {
hostedZoneId: 'fakeId',
zoneName: 'domain.com',
hostedZoneArn: 'arn:aws:route53:::hostedzone/fakeId',
stack,
node: stack.node,
},
Expand Down
7 changes: 7 additions & 0 deletions packages/@aws-cdk/aws-route53/lib/hosted-zone-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ export interface IHostedZone extends IResource {
*/
readonly zoneName: string;

/**
* ARN of this hosted zone, such as arn:${Partition}:route53:::hostedzone/${Id}
Copy link
Contributor

Choose a reason for hiding this comment

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

change arn:${partition}... to a concrete example of an ARN

*
* @attribute
*/
readonly hostedZoneArn: string;

/**
* Returns the set of name servers for the specific hosted zone. For example:
* ns1.example.com.
Expand Down
21 changes: 19 additions & 2 deletions packages/@aws-cdk/aws-route53/lib/hosted-zone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { HostedZoneProviderProps } from './hosted-zone-provider';
import { HostedZoneAttributes, IHostedZone } from './hosted-zone-ref';
import { CaaAmazonRecord, ZoneDelegationRecord } from './record-set';
import { CfnHostedZone } from './route53.generated';
import { validateZoneName } from './util';
import { makeHostedZoneArn, validateZoneName } from './util';

export interface CommonHostedZoneProps {
/**
Expand Down Expand Up @@ -45,13 +45,19 @@ export interface HostedZoneProps extends CommonHostedZoneProps {
}

export class HostedZone extends Resource implements IHostedZone {
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}

public static fromHostedZoneId(scope: Construct, id: string, hostedZoneId: string): IHostedZone {
class Import extends Resource implements IHostedZone {
public readonly hostedZoneId = hostedZoneId;
public get zoneName(): string {
throw new Error(`HostedZone.fromHostedZoneId doesn't support "zoneName"`);
}
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
}

return new Import(scope, id);
Expand All @@ -64,6 +70,9 @@ export class HostedZone extends Resource implements IHostedZone {
class Import extends Resource implements IHostedZone {
public readonly hostedZoneId = attrs.hostedZoneId;
public readonly zoneName = attrs.zoneName;
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
}

return new Import(scope, id);
Expand All @@ -74,7 +83,7 @@ export class HostedZone extends Resource implements IHostedZone {
*/
public static fromLookup(scope: Construct, id: string, query: HostedZoneProviderProps): IHostedZone {
const DEFAULT_HOSTED_ZONE: HostedZoneContextResponse = {
Id: '/hostedzone/DUMMY',
Id: 'DUMMY',
Name: query.domainName,
};

Expand All @@ -94,6 +103,8 @@ export class HostedZone extends Resource implements IHostedZone {
response.Name = response.Name.substring(0, response.Name.length - 1);
}

response.Id = response.Id.replace('/hostedzone/', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to regex so it matches only the beginning of the string: replace(/^\/hostedzone\//, '')


return HostedZone.fromHostedZoneAttributes(scope, id, {
hostedZoneId: response.Id,
zoneName: response.Name,
Expand Down Expand Up @@ -166,6 +177,9 @@ export class PublicHostedZone extends HostedZone implements IPublicHostedZone {
class Import extends Resource implements IPublicHostedZone {
public readonly hostedZoneId = publicHostedZoneId;
public get zoneName(): string { throw new Error(`cannot retrieve "zoneName" from an an imported hosted zone`); }
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
}
return new Import(scope, id);
}
Expand Down Expand Up @@ -246,6 +260,9 @@ export class PrivateHostedZone extends HostedZone implements IPrivateHostedZone
class Import extends Resource implements IPrivateHostedZone {
public readonly hostedZoneId = privateHostedZoneId;
public get zoneName(): string { throw new Error(`cannot retrieve "zoneName" from an an imported hosted zone`); }
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
}
return new Import(scope, id);
}
Expand Down
11 changes: 11 additions & 0 deletions packages/@aws-cdk/aws-route53/lib/util.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Construct, Stack } from '@aws-cdk/core';
import { IHostedZone } from './hosted-zone-ref';

/**
Expand Down Expand Up @@ -56,3 +57,13 @@ export function determineFullyQualifiedDomainName(providedName: string, hostedZo

return `${providedName}${suffix}.`;
}

export function makeHostedZoneArn(construct: Construct, hostedZoneId: string): string {
return Stack.of(construct).formatArn({
account: '',
region: '',
service: 'route53',
resource: 'hostedzone',
resourceName: hostedZoneId
});
}
70 changes: 54 additions & 16 deletions packages/@aws-cdk/aws-route53/test/test.hosted-zone-provider.ts
Original file line number Diff line number Diff line change
@@ -1,47 +1,85 @@
import { SynthUtils } from '@aws-cdk/assert';
import cdk = require('@aws-cdk/core');
import { Test } from 'nodeunit';
import { HostedZone, HostedZoneAttributes } from '../lib';
import { HostedZone } from '../lib';

export = {
'Hosted Zone Provider': {
'HostedZoneProvider will return context values if availble'(test: Test) {
'HostedZoneProvider will return context values if available'(test: Test) {
// GIVEN
const stack = new cdk.Stack(undefined, 'TestStack', { env: { account: '12345', region: 'us-east-1' } });
const filter = {domainName: 'test.com'};
const stack = new cdk.Stack(undefined, 'TestStack', {
env: { account: '12345', region: 'us-east-1' }
});
const filter = { domainName: 'test.com' };
Comment on lines -10 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

To improve code reviewer experience, try to avoid formatting changes (it's hard to ensure that there are no functional changes here)


HostedZone.fromLookup(stack, 'Ref', filter);

const missing = SynthUtils.synthesize(stack).assembly.manifest.missing!;
test.ok(missing && missing.length === 1);

const fakeZoneId = '11111111111111';
const fakeZone = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test which verifies that if users pass a hosted zone without the “/hostedzone” prefix, we still get the desired effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please see 667f65f

Id: "/hostedzone/11111111111111",
Name: "example.com.",
CallerReference: "TestLates-PublicZo-OESZPDFV7G6A",
Id: `/hostedzone/${fakeZoneId}`,
Name: 'example.com.',
CallerReference: 'TestLates-PublicZo-OESZPDFV7G6A',
Comment on lines -19 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer it if this code was completely reverted to demonstrate there is no regression.

Config: {
Comment: "CDK created",
Comment: 'CDK created',
PrivateZone: false
},
ResourceRecordSetCount: 3
};

const stack2 = new cdk.Stack(undefined, 'TestStack', { env: { account: '12345', region: 'us-east-1' } });
const stack2 = new cdk.Stack(undefined, 'TestStack', {
env: { account: '12345', region: 'us-east-1' }
});
stack2.node.setContext(missing[0].key, fakeZone);

const cdkZoneProps: HostedZoneAttributes = {
hostedZoneId: fakeZone.Id,
zoneName: 'example.com',
// WHEN
const zoneRef = HostedZone.fromLookup(stack2, 'MyZoneProvider', filter);

// THEN
test.deepEqual(zoneRef.hostedZoneId, fakeZoneId);
test.done();
},
'HostedZoneProvider will return context values if available when using plain hosted zone id'(
test: Test
) {
// GIVEN
const stack = new cdk.Stack(undefined, 'TestStack', {
env: { account: '12345', region: 'us-east-1' }
});
const filter = { domainName: 'test.com' };

HostedZone.fromLookup(stack, 'Ref', filter);

const missing = SynthUtils.synthesize(stack).assembly.manifest.missing!;
test.ok(missing && missing.length === 1);

const fakeZoneId = '11111111111111';
const fakeZone = {
Id: `/hostedzone/${fakeZoneId}`,
Name: 'example.com.',
CallerReference: 'TestLates-PublicZo-OESZPDFV7G6A',
Config: {
Comment: 'CDK created',
PrivateZone: false
},
ResourceRecordSetCount: 3
};

const cdkZone = HostedZone.fromHostedZoneAttributes(stack2, 'MyZone', cdkZoneProps);
const stack2 = new cdk.Stack(undefined, 'TestStack', {
env: { account: '12345', region: 'us-east-1' }
});
stack2.node.setContext(missing[0].key, fakeZone);

const zone = HostedZone.fromLookup(stack2, 'MyZoneProvider', filter);

// WHEN
const zoneRef = HostedZone.fromLookup(stack2, 'MyZoneProvider', filter);
const zoneId = zone.hostedZoneId;

// THEN
test.deepEqual(zoneRef.hostedZoneId, cdkZone.hostedZoneId);
test.deepEqual(fakeZoneId, zoneId);
test.done();
},
}
}
};
32 changes: 32 additions & 0 deletions packages/@aws-cdk/aws-route53/test/test.hosted-zone.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import cdk = require('@aws-cdk/core');
import { Test } from 'nodeunit';
import { HostedZone } from '../lib';

export = {
'Hosted Zone': {
'Hosted Zone constructs the ARN'(test: Test) {
// GIVEN
const stack = new cdk.Stack(undefined, 'TestStack', {
env: { account: '123456789012', region: 'us-east-1' }
});

const testZone = new HostedZone(stack, 'HostedZone', {
zoneName: 'testZone'
});

test.deepEqual(stack.resolve(testZone.hostedZoneArn), {
'Fn::Join': [
'',
[
'arn:',
{ Ref: 'AWS::Partition' },
':route53:::hostedzone/',
{ Ref: 'HostedZoneDB99F866' }
]
]
});

test.done();
}
}
};