fix(s3): infer bucketWebsiteUrl and bucketDomainName suffixes from bucket region (#23919)

fynnfluegge · web-flow · commit 252f052d4239 · 2023-02-07T16:25:36.000Z
The current implementations of determining various bucket URLs was incorrectly relying on the stack's region.
In practice this did not matter a lot, since the suffix depends on the partition and a cross over from one partition to another is not likely or even impossible.

We also required users to determine the correct bucketWebsiteUrl format for a bucket.
However this information can reliably be inferred from a bucket's region.
Since bucket ARNs do not contain region information, it is best to provide the bucket region whenever bucketWebsiteUrl will be used.
As a fallback we assume a bucket is placed in the region of its stack.

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts
@@ -23,6 +23,7 @@ import {
 } from '@aws-cdk/core';
 import { CfnReference } from '@aws-cdk/core/lib/private/cfn-reference';
 import * as cxapi from '@aws-cdk/cx-api';
+import * as regionInformation from '@aws-cdk/region-info';
 import { Construct } from 'constructs';
 import { BucketPolicy } from './bucket-policy';
 import { IBucketNotificationDestination } from './destination';
@@ -408,14 +409,14 @@ export interface BucketAttributes {
   /**
    * The domain name of the bucket.
    *
-   * @default Inferred from bucket name
+   * @default - Inferred from bucket name
    */
   readonly bucketDomainName?: string;
 
   /**
    * The website URL of the bucket (if static web hosting is enabled).
    *
-   * @default Inferred from bucket name
+   * @default - Inferred from bucket name and region
    */
   readonly bucketWebsiteUrl?: string;
 
@@ -430,13 +431,22 @@ export interface BucketAttributes {
   readonly bucketDualStackDomainName?: string;
 
   /**
-   * The format of the website URL of the bucket. This should be true for
+   * Force the format of the website URL of the bucket. This should be true for
    * regions launched since 2014.
    *
-   * @default false
+   * @default - inferred from available region information, `false` otherwise
+   *
+   * @deprecated The correct website url format can be inferred automatically from the bucket `region`.
+   * Always provide the bucket region if the `bucketWebsiteUrl` will be used.
+   * Alternatively provide the full `bucketWebsiteUrl` manually.
    */
   readonly bucketWebsiteNewUrlFormat?: boolean;
 
+  /**
+   * KMS encryption key associated with this bucket.
+   *
+   * @default - no encryption key
+   */
   readonly encryptionKey?: kms.IKey;
 
   /**
@@ -455,6 +465,8 @@ export interface BucketAttributes {
 
   /**
    * The region this existing bucket is in.
+   * Features that require the region (e.g. `bucketWebsiteUrl`) won't fully work
+   * if the region cannot be correctly inferred.
    *
    * @default - it's assumed the bucket is in the same region as the scope it's being imported into
    */
@@ -1626,21 +1638,27 @@ export class Bucket extends BucketBase {
   public static fromBucketAttributes(scope: Construct, id: string, attrs: BucketAttributes): IBucket {
     const stack = Stack.of(scope);
     const region = attrs.region ?? stack.region;
-    const urlSuffix = stack.urlSuffix;
+    const regionInfo = regionInformation.RegionInfo.get(region);
+    const urlSuffix = regionInfo.domainSuffix ?? stack.urlSuffix;
 
     const bucketName = parseBucketName(scope, attrs);
     if (!bucketName) {
       throw new Error('Bucket name is required');
     }
     Bucket.validateBucketName(bucketName);
 
-    const newUrlFormat = attrs.bucketWebsiteNewUrlFormat === undefined
-      ? false
-      : attrs.bucketWebsiteNewUrlFormat;
+    const oldEndpoint = `s3-website-${region}.${urlSuffix}`;
+    const newEndpoint = `s3-website.${region}.${urlSuffix}`;
+
+    let staticDomainEndpoint = regionInfo.s3StaticWebsiteEndpoint
+      ?? Lazy.string({ produce: () => stack.regionalFact(regionInformation.FactName.S3_STATIC_WEBSITE_ENDPOINT, newEndpoint) });
+
+    // Deprecated use of bucketWebsiteNewUrlFormat
+    if (attrs.bucketWebsiteNewUrlFormat !== undefined) {
+      staticDomainEndpoint = attrs.bucketWebsiteNewUrlFormat ? newEndpoint : oldEndpoint;
+    }
 
-    const websiteDomain = newUrlFormat
-      ? `${bucketName}.s3-website.${region}.${urlSuffix}`
-      : `${bucketName}.s3-website-${region}.${urlSuffix}`;
+    const websiteDomain = `${bucketName}.${staticDomainEndpoint}`;
 
     class Import extends BucketBase {
       public readonly bucketName = bucketName!;
@@ -1650,7 +1668,7 @@ export class Bucket extends BucketBase {
       public readonly bucketWebsiteDomainName = attrs.bucketWebsiteUrl ? Fn.select(2, Fn.split('/', attrs.bucketWebsiteUrl)) : websiteDomain;
       public readonly bucketRegionalDomainName = attrs.bucketRegionalDomainName || `${bucketName}.s3.${region}.${urlSuffix}`;
       public readonly bucketDualStackDomainName = attrs.bucketDualStackDomainName || `${bucketName}.s3.dualstack.${region}.${urlSuffix}`;
-      public readonly bucketWebsiteNewUrlFormat = newUrlFormat;
+      public readonly bucketWebsiteNewUrlFormat = attrs.bucketWebsiteNewUrlFormat ?? false;
       public readonly encryptionKey = attrs.encryptionKey;
       public readonly isWebsite = attrs.isWebsite ?? false;
       public policy?: BucketPolicy = undefined;
diff --git a/packages/@aws-cdk/aws-s3/package.json b/packages/@aws-cdk/aws-s3/package.json
@@ -82,9 +82,9 @@
   "devDependencies": {
     "@aws-cdk/assertions": "0.0.0",
     "@aws-cdk/cdk-build-tools": "0.0.0",
+    "@aws-cdk/cfn2ts": "0.0.0",
     "@aws-cdk/integ-runner": "0.0.0",
     "@aws-cdk/integ-tests": "0.0.0",
-    "@aws-cdk/cfn2ts": "0.0.0",
     "@aws-cdk/pkglint": "0.0.0",
     "@types/aws-lambda": "^8.10.110",
     "@types/jest": "^27.5.2",
@@ -96,6 +96,7 @@
     "@aws-cdk/aws-kms": "0.0.0",
     "@aws-cdk/core": "0.0.0",
     "@aws-cdk/cx-api": "0.0.0",
+    "@aws-cdk/region-info": "0.0.0",
     "constructs": "^10.0.0"
   },
   "homepage": "https://github.com/aws/aws-cdk",
@@ -105,6 +106,7 @@
     "@aws-cdk/aws-kms": "0.0.0",
     "@aws-cdk/core": "0.0.0",
     "@aws-cdk/cx-api": "0.0.0",
+    "@aws-cdk/region-info": "0.0.0",
     "constructs": "^10.0.0"
   },
   "engines": {
diff --git a/packages/@aws-cdk/aws-s3/test/bucket.test.ts b/packages/@aws-cdk/aws-s3/test/bucket.test.ts
@@ -2,6 +2,7 @@ import { EOL } from 'os';
 import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import * as iam from '@aws-cdk/aws-iam';
 import * as kms from '@aws-cdk/aws-kms';
+import { testDeprecated } from '@aws-cdk/cdk-build-tools';
 import * as cdk from '@aws-cdk/core';
 import * as s3 from '../lib';
 
@@ -805,18 +806,69 @@ describe('bucket', () => {
       });
     });
 
-    test('import can explicitly set bucket region', () => {
+    test('import can explicitly set bucket region with different suffix than stack', () => {
       const stack = new cdk.Stack(undefined, undefined, {
-        env: { region: 'us-east-1' },
+        env: { region: 'cn-north-1' },
       });
 
       const bucket = s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', {
         bucketName: 'mybucket',
         region: 'eu-west-1',
       });
 
-      expect(bucket.bucketRegionalDomainName).toEqual(`mybucket.s3.eu-west-1.${stack.urlSuffix}`);
-      expect(bucket.bucketWebsiteDomainName).toEqual(`mybucket.s3-website-eu-west-1.${stack.urlSuffix}`);
+      expect(bucket.bucketRegionalDomainName).toEqual('mybucket.s3.eu-west-1.amazonaws.com');
+      expect(bucket.bucketWebsiteDomainName).toEqual('mybucket.s3-website-eu-west-1.amazonaws.com');
+    });
+
+    test('new bucketWebsiteUrl format for specific region', () => {
+      const stack = new cdk.Stack(undefined, undefined, {
+        env: { region: 'us-east-2' },
+      });
+
+      const bucket = s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', {
+        bucketName: 'mybucket',
+      });
+
+      expect(bucket.bucketWebsiteUrl).toEqual('http://mybucket.s3-website.us-east-2.amazonaws.com');
+    });
+
+    test('new bucketWebsiteUrl format for specific region with cn suffix', () => {
+      const stack = new cdk.Stack(undefined, undefined, {
+        env: { region: 'cn-north-1' },
+      });
+
+      const bucket = s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', {
+        bucketName: 'mybucket',
+      });
+
+      expect(bucket.bucketWebsiteUrl).toEqual('http://mybucket.s3-website.cn-north-1.amazonaws.com.cn');
+    });
+
+
+    testDeprecated('new bucketWebsiteUrl format with explicit bucketWebsiteNewUrlFormat', () => {
+      const stack = new cdk.Stack(undefined, undefined, {
+        env: { region: 'us-east-1' },
+      });
+
+      const bucket = s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', {
+        bucketName: 'mybucket',
+        bucketWebsiteNewUrlFormat: true,
+      });
+
+      expect(bucket.bucketWebsiteUrl).toEqual('http://mybucket.s3-website.us-east-1.amazonaws.com');
+    });
+
+    testDeprecated('old bucketWebsiteUrl format with explicit bucketWebsiteNewUrlFormat', () => {
+      const stack = new cdk.Stack(undefined, undefined, {
+        env: { region: 'us-east-2' },
+      });
+
+      const bucket = s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', {
+        bucketName: 'mybucket',
+        bucketWebsiteNewUrlFormat: false,
+      });
+
+      expect(bucket.bucketWebsiteUrl).toEqual('http://mybucket.s3-website-us-east-2.amazonaws.com');
     });
 
     test('import needs to specify a valid bucket name', () => {
@@ -2026,25 +2078,32 @@ describe('bucket', () => {
         'Fn::Join': [
           '',
           [
-            'http://my-test-bucket.s3-website-',
-            { Ref: 'AWS::Region' },
-            '.',
-            { Ref: 'AWS::URLSuffix' },
+            'http://my-test-bucket.',
+            {
+              'Fn::FindInMap': [
+                'S3staticwebsiteMap',
+                { Ref: 'AWS::Region' },
+                'endpoint',
+              ],
+            },
           ],
         ],
       });
       expect(stack.resolve(bucket.bucketWebsiteDomainName)).toEqual({
         'Fn::Join': [
           '',
           [
-            'my-test-bucket.s3-website-',
-            { Ref: 'AWS::Region' },
-            '.',
-            { Ref: 'AWS::URLSuffix' },
+            'my-test-bucket.',
+            {
+              'Fn::FindInMap': [
+                'S3staticwebsiteMap',
+                { Ref: 'AWS::Region' },
+                'endpoint',
+              ],
+            },
           ],
         ],
       });
-
     });
     test('exports the WebsiteURL for imported buckets with url', () => {
       const stack = new cdk.Stack();
