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

Jump to bottom

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

Merged
merged 1 commit into from
Jan 15, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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).
*
* @default Inferred from bucket name
*/
bucketWebsiteUrl?: string;
}

/**
Expand Down Expand Up @@ -571,6 +578,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 @@ -599,6 +607,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 @@ -621,6 +630,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()
};
}

Expand Down Expand Up @@ -956,6 +966,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 @@ -972,6 +983,7 @@ class ImportedBucket extends BucketBase {
this.bucketArn = parseBucketArn(this, props);
this.bucketName = bucketName;
this.domainName = props.bucketDomainName || this.generateDomainName();
this.bucketWebsiteUrl = props.bucketWebsiteUrl || this.generateBucketWebsiteUrl();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we ALWAYS want to do this? Is there a case in which you wouldn't want the bucketWebsiteUrl set?

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 think we do. I was basing this off the current CloudFormation functionality; it will always allow you to output (and therefore read) the website URL even if web hosting is not configured.

this.autoCreatePolicy = false;
this.policy = undefined;
}
Expand All @@ -986,4 +998,8 @@ class ImportedBucket extends BucketBase {
private generateDomainName() {
return `${this.bucketName}.s3.amazonaws.com`;
}

private generateBucketWebsiteUrl() {
return `${this.bucketName}.s3-website-${new cdk.Aws().region}.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.

The region should be ${cdk.Stack.find(this).region}

And I think we need ${cdk.Stack.find(this).urlSuffix} instead of amazonaws.com.

Just found out is no generalized form of this. It's either:

bucket-name.s3-website-region.amazonaws.com
OR
bucket-name.s3-website.region.amazonaws.com

depending on the region :/ Grrr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Region change seems like a simple change 😄

The URL suffix seems less so. Do you know which one it is depending on which region? It's a bit ugly but we could have something like:

const suffix = cdk.Stack.find(this).urlSuffix
const region = cdk.Stack.find(this).region
const regionToSuffix = {
  'eu-west-1': `s3-website-eu-west-1.${suffix}`,
  'us-west-1': `s3-website.us-west-1.${suffix}`
}

return `${this.bucketName}.${regionToSuffix[region]}`;

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, this problem is intractable for you right now (since we may be deploying without the region being available at all), so don't worry about it. This ties into #1282, we need some kind of region information to solve this.

Leave it as is and ignore it, I was just making a mental note of this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a switch on the Props to pick between the two formats?

Looking at the list: https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_website_region_endpoints I'm tempted to call it something like bucketWebsiteNewUrlFormat?: boolean, and in the documentation mention it should be true for regions launched since 2014.

}
}
36 changes: 34 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(bucket.node.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(bucket.node.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": {
"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 Expand Up @@ -1190,6 +1214,14 @@ export = {
}
}));
test.done();
},
'exports the WebsiteURL'(test: Test) {
const stack = new cdk.Stack();
const bucket = new s3.Bucket(stack, 'Website', {
websiteIndexDocument: 'index.html'
});
test.deepEqual(bucket.node.resolve(bucket.bucketWebsiteUrl), { 'Fn::GetAtt': [ 'Website32962D0B', 'WebsiteURL' ] });
test.done();
}
}
};