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.

Sign up for GitHub

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

feat(s3): export bucket websiteURL (#1521) #1522

Closed
wants to merge 26 commits into from
Closed

feat(s3): export bucket websiteURL (#1521) #1522

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
497620d
feat(s3): export bucket websiteURL
AlexChesters Jan 11, 2019
f6a3bfa
fix
AlexChesters Jan 11, 2019
c2aeebd
rename bucketWebsiteURL to bucketWebsiteUrl
AlexChesters Jan 14, 2019
a36ac08
update implementation to return WebsiteURL value
AlexChesters Jan 14, 2019
7bafbff
add unit test
AlexChesters Jan 14, 2019
1724161
feat(apigateway): add tracingEnabled property to APIGW Stage (#1482)
workeitel Jan 6, 2019
a4645f2
feat(aws-ecs): add support Amazon Linux 2 (#1484)
cohalz Jan 6, 2019
b22cf0c
feat(aws-kms): allow tagging kms keys (#1485)
rsmogura Jan 7, 2019
19c9392
chore: Document return behavior of cdk diff (#1446)
RomainMuller Jan 7, 2019
5096201
fix(elbv2): unable to specify load balancer name (#1486)
Jan 7, 2019
9bae09b
feat(diff): Better diff of random objects (#1488)
RomainMuller Jan 7, 2019
be19cc5
feat(aws-codepipeline): Jenkins build and test Actions. (#1216)
skinny85 Jan 7, 2019
81a5a07
feat(route53): support CNAME records (#1487)
Jan 8, 2019
ed5a38b
feat: no more generated attribute types in CFN layer (L1) (#1489)
Jan 8, 2019
ccd01e2
feat: stop generating legacy cloudformation resources (#1493)
Jan 8, 2019
b1f5b2e
feat: cloudformation condition chaining (#1494)
Jan 8, 2019
9cb842f
feat(step-functions): support parameters option (#1492)
guisrx Jan 8, 2019
f393a26
feat(cdk): transparently use constructs from another stack
rix0rrr Jan 9, 2019
6ad8c5e
fix(lambda): use IRole instead of Role to allow imports (#1509)
Jan 10, 2019
0fb62c8
fix(core): automatic cross-stack refs for CFN resources (#1510)
Jan 10, 2019
3961ad7
feat(aws-cdk): better stack dependency handling (#1511)
rix0rrr Jan 10, 2019
01d72cf
v0.22.0 (#1518)
Jan 11, 2019
948b5df
add packages.txt to github pages (#1520)
Jan 11, 2019
4443d5c
chore(scripts): Add Jetbrains node_modules exclusion script (#1507)
costleya Jan 13, 2019
582c8b1
feat(ecs): environment variables for LoadBalancedXxxService (#1537)
PaulMaddox Jan 14, 2019
c79d234
chore(cdk): make `isInstance` static methods more concrete (#1519)
rix0rrr Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion packages/@aws-cdk/aws-s3/lib/bucket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export interface IBucket extends cdk.IConstruct {
*/
export interface BucketImportProps {
/**
* The ARN fo the bucket. At least one of bucketArn or bucketName must be
* The ARN of the bucket. At least one of bucketArn or bucketName must be
* defined in order to initialize a bucket ref.
*/
bucketArn?: string;
Expand All @@ -199,6 +199,13 @@ export interface BucketImportProps {
* @default Inferred from bucket name
*/
bucketDomainName?: string;

/**
* The website URL of the bucket (if static web hosting is enabled).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how useful this description is, I tried a couple of different variations but couldn't come up with anything better than this.

Suggestions welcome!

*
* @default Inferred from bucket name
*/
bucketWebsiteUrl?: string;
}

/**
Expand Down Expand Up @@ -570,6 +577,7 @@ export class Bucket extends BucketBase {
public readonly bucketArn: string;
public readonly bucketName: string;
public readonly domainName: string;
public readonly bucketWebsiteUrl: string;
public readonly dualstackDomainName: string;
public readonly encryptionKey?: kms.IEncryptionKey;
public policy?: BucketPolicy;
Expand Down Expand Up @@ -598,6 +606,7 @@ export class Bucket extends BucketBase {
this.bucketArn = resource.bucketArn;
this.bucketName = resource.bucketName;
this.domainName = resource.bucketDomainName;
this.bucketWebsiteUrl = resource.bucketWebsiteUrl;
this.dualstackDomainName = resource.bucketDualStackDomainName;

// Add all lifecycle rules
Expand All @@ -620,6 +629,7 @@ export class Bucket extends BucketBase {
bucketArn: new cdk.Output(this, 'BucketArn', { value: this.bucketArn }).makeImportValue().toString(),
bucketName: new cdk.Output(this, 'BucketName', { value: this.bucketName }).makeImportValue().toString(),
bucketDomainName: new cdk.Output(this, 'DomainName', { value: this.domainName }).makeImportValue().toString(),
bucketWebsiteUrl: new cdk.Output(this, 'WebsiteURL', { value: this.bucketWebsiteUrl }).makeImportValue().toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if bucketWebsiteUrl is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in from a consumer's point of view? Currently it would just result in the property being undefined as you said.

I've had a go at creating separate test cases (for cases where a bucket does and doesn't have a website configuration) but I'm not 100% sure how to proceed. Looking at the other cases it seems the usual way of testing this stuff is to generate the CloudFormation and then assert against that. In this case, however, the value is just an Fn::GetAtt snippet:

'exports the WebsiteURL is website configuration is enabled'(test: Test) {
  const stack = new cdk.Stack();
  const bucket = new s3.Bucket(stack, 'Website', {
    bucketName: 'my-bucket',
    websiteIndexDocument: 'index.html'
  });
  test.deepEqual(cdk.resolve(bucket.bucketWebsiteUrl), 'my-bucket-<REGION>.s3-website.amazonaws.com');
  test.done();
}

The are two problems I've ran into when attempting to create test cases like the above:

  1. AssertionError: { 'Fn::GetAtt': [ 'Website32962D0B', 'WebsiteURL' ] } deepEqual 'my-bucket-<REGION>.s3-website.amazonaws.com'
    1. How can I compare against the actual value as opposed to an Fn::GetAtt snippet? (I assumed calling cdk.resolve would have solved this
    2. How can I determine the region correctly? I can't see an example of how to do this in any of the other test cases

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently it would just result in the property being undefined as you said.

why? if the user sets up website hosting for the bucket, I'd expect this to return the URL, no?

How can I compare against the actual value as opposed to an Fn::GetAtt snippet? (I assumed calling cdk.resolve would have solved this

You can't. The "end result" of a CDK synth is a CloudFormation template. I'd expect the test to assert that the resulting template (or template part) is the right one (i.e. includes the GetAtt).

How can I determine the region correctly? I can't see an example of how to do this in any of the other test cases

What I usually do is print out the output of cdk.resolve, inspect it to make sure it's what I expect, and then use it as my test expectation:

Copy link
Contributor Author

@AlexChesters AlexChesters Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

why? if the user sets up website hosting for the bucket, I'd expect this to return the URL, no?

Ah, I thought you were asking what would happen if the user hadn't set up website hosting for the bucket. In the case where they have set up website hosting for the bucket, bucketWebsiteUrl should never be undefined. Is there something you've spotted that might result it in being undefined?

You can't. The "end result" of a CDK synth is a CloudFormation template. I'd expect the test to assert that the resulting template (or template part) is the right one (i.e. includes the GetAtt).
What I usually do is print out the output of cdk.resolve, inspect it to make sure it's what I expect, and then use it as my test expectation:

Gotcha, I'll give this a go. Thanks!

};
}

Expand Down Expand Up @@ -955,6 +965,7 @@ class ImportedBucket extends BucketBase {
public readonly bucketArn: string;
public readonly bucketName: string;
public readonly domainName: string;
public readonly bucketWebsiteUrl: string;
public readonly encryptionKey?: kms.EncryptionKey;

public policy?: BucketPolicy;
Expand All @@ -971,6 +982,7 @@ class ImportedBucket extends BucketBase {
this.bucketArn = parseBucketArn(props);
this.bucketName = bucketName;
this.domainName = props.bucketDomainName || this.generateDomainName();
this.bucketWebsiteUrl = props.bucketWebsiteUrl || this.generateBucketWebsiteUrl();
this.autoCreatePolicy = false;
this.policy = undefined;
}
Expand All @@ -985,4 +997,8 @@ class ImportedBucket extends BucketBase {
private generateDomainName() {
return `${this.bucketName}.s3.amazonaws.com`;
}

private generateBucketWebsiteUrl() {
return `${this.bucketName}.s3-website-${new cdk.AwsRegion()}.amazonaws.com`;
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this look in a different partition? (i.e. china)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a.. good question. I wasn't aware (until I found these docs) that the domain name was different in regions such as China.

Happy to take a look at making sure this works in regions such as China, would you want me to take a look at doing the same for the generateDomainName() function above (as that appears to have the same problem), or leave that for a separate issue and PR?

}
}
28 changes: 26 additions & 2 deletions packages/@aws-cdk/aws-s3/test/test.bucket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,8 @@ export = {
test.deepEqual(cdk.resolve(bucketRef), {
bucketArn: { 'Fn::ImportValue': 'MyStack:MyBucketBucketArnE260558C' },
bucketName: { 'Fn::ImportValue': 'MyStack:MyBucketBucketName8A027014' },
bucketDomainName: { 'Fn::ImportValue': 'MyStack:MyBucketDomainNameF76B9A7A' }
bucketDomainName: { 'Fn::ImportValue': 'MyStack:MyBucketDomainNameF76B9A7A' },
bucketWebsiteUrl: { 'Fn::ImportValue': 'MyStack:MyBucketWebsiteURL9C222788' }
});
test.done();
},
Expand All @@ -346,7 +347,8 @@ export = {
test.deepEqual(cdk.resolve(bucketRef), {
bucketArn: { 'Fn::ImportValue': 'MyStack:MyBucketBucketArnE260558C' },
bucketName: { 'Fn::ImportValue': 'MyStack:MyBucketBucketName8A027014' },
bucketDomainName: { 'Fn::ImportValue': 'MyStack:MyBucketDomainNameF76B9A7A' }
bucketDomainName: { 'Fn::ImportValue': 'MyStack:MyBucketDomainNameF76B9A7A' },
bucketWebsiteUrl: { 'Fn::ImportValue': 'MyStack:MyBucketWebsiteURL9C222788' }
});
test.done();
},
Expand Down Expand Up @@ -465,6 +467,17 @@ export = {
"Export": {
"Name": "S1:MyBucketDomainNameF76B9A7A"
}
},
"MyBucketWebsiteURL9C222788": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found the logical ID here (i.e. MyBucketWebsiteURL9C222788) from running the tests, watching them fail and observing the output.

I'm not sure if I was meant to have determined the output from somewhere else?

"Value": {
"Fn::GetAtt": [
"MyBucketF68F3FF0",
"WebsiteURL"
]
},
"Export": {
"Name": "S1:MyBucketWebsiteURL9C222788"
}
}
}
});
Expand Down Expand Up @@ -898,6 +911,17 @@ export = {
"Export": {
"Name": "MyBucketDomainNameF76B9A7A"
}
},
"MyBucketWebsiteURL9C222788": {
"Value": {
"Fn::GetAtt": [
"MyBucketF68F3FF0",
"WebsiteURL"
]
},
"Export": {
"Name": "MyBucketWebsiteURL9C222788"
}
}
}
});
Expand Down