fix: fix S3 object URL creation at AwsS3EntityProvider

Fix S3 object URL creation at AwsS3EntityProvider by - handle absence of region config, - handle regions with region-less URIs (us-east-1), - apply URI encoding, - and simplify the logic overall. Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
2022-04-20 06:06:28 +02:00
parent 44fd217f81
commit bffec1c96a
4 changed files with 158 additions and 125 deletions
@@ -0,0 +1,10 @@
+---
+'@backstage/plugin-catalog-backend-module-aws': patch
+---
+
+Fix S3 object URL creation at AwsS3EntityProvider by
+
+- handle absence of region config,
+- handle regions with region-less URIs (us-east-1),
+- apply URI encoding,
+- and simplify the logic overall.
@@ -339,6 +339,7 @@ untracked
 upsert
 upvote
 url
+URIs
 URLs
 utils
 validator
@@ -38,24 +38,8 @@ class PersistingTaskRunner implements TaskRunner {
 const logger = getVoidLogger();

 describe('AwsS3EntityProvider', () => {
-  const config = new ConfigReader({
-    catalog: {
-      providers: {
-        awsS3: {
-          anyProviderId: {
-            bucketName: 'bucket-1',
-            region: 'us-east-1',
-            prefix: 'sub/dir/',
-          },
-        },
-      },
-    },
-  });
-
-  const schedule = new PersistingTaskRunner();
-
  AWSMock.setSDKInstance(aws);
-  const createObjectList = (...keys: string[]): aws.S3.ObjectList => {
+  const createObjectList = (keys: string[]): aws.S3.ObjectList => {
    const objects = keys.map(key => {
      return {
        Key: key,
@@ -65,24 +49,55 @@ describe('AwsS3EntityProvider', () => {
    return objects as aws.S3.ObjectList;
  };

+  const keys = ['key1.yaml', 'key2.yaml', 'key3.yaml', 'key 4.yaml'];
+
  AWSMock.mock('S3', 'listObjectsV2', async req => {
    const prefix = req.Prefix ?? '';

    if (!req.ContinuationToken) {
      return {
-        Contents: createObjectList(`${prefix}key1.yaml`, `${prefix}key2.yaml`),
+        Contents: createObjectList(
+          keys
+            .slice(0, Math.ceil(keys.length / 2))
+            .map(key => `${prefix}${key}`),
+        ),
        NextContinuationToken: 'next-token',
      } as aws.S3.Types.ListObjectsV2Output;
    }

    return {
-      Contents: createObjectList(`${prefix}key3.yaml`, `${prefix}key4.yaml`),
+      Contents: createObjectList(
+        keys.slice(Math.ceil(keys.length / 2)).map(key => `${prefix}${key}`),
+      ),
    } as aws.S3.Types.ListObjectsV2Output;
  });

-  afterEach(() => jest.resetAllMocks());
+  afterEach(() => {
+    jest.resetAllMocks();
+    process.env.AWS_REGION = undefined;
+  });

-  it('apply full update on scheduled execution', async () => {
+  const expectMutation = async (
+    providerId: string,
+    providerConfig: object,
+    expectedBaseUrl: string,
+    names: Record<string, string>,
+    integrationConfig?: object,
+  ) => {
+    const config = new ConfigReader({
+      integrations: {
+        awsS3: integrationConfig ? [integrationConfig] : [],
+      },
+      catalog: {
+        providers: {
+          awsS3: {
+            [providerId]: providerConfig,
+          },
+        },
+      },
+    });
+
+    const schedule = new PersistingTaskRunner();
    const entityProviderConnection: EntityProviderConnection = {
      applyMutation: jest.fn(),
    };
@@ -91,106 +106,121 @@ describe('AwsS3EntityProvider', () => {
      logger,
      schedule,
    })[0];
-    expect(provider.getProviderName()).toEqual('awsS3-provider:anyProviderId');
+    expect(provider.getProviderName()).toEqual(`awsS3-provider:${providerId}`);

    await provider.connect(entityProviderConnection);

    const taskDef = schedule.getTasks()[0];
-    expect(taskDef.id).toEqual('awsS3-provider:anyProviderId:refresh');
+    expect(taskDef.id).toEqual(`awsS3-provider:${providerId}:refresh`);
    await (taskDef.fn as () => Promise<void>)();

+    const expectedEntities = keys.map(key => {
+      const url = encodeURI(`${expectedBaseUrl}${key}`);
+      return {
+        entity: {
+          apiVersion: 'backstage.io/v1alpha1',
+          kind: 'Location',
+          metadata: {
+            annotations: {
+              'backstage.io/managed-by-location': `url:${url}`,
+              'backstage.io/managed-by-origin-location': `url:${url}`,
+            },
+            name: names[key],
+          },
+          spec: {
+            presence: 'required',
+            target: `${url}`,
+            type: 'url',
+          },
+        },
+        locationKey: `awsS3-provider:${providerId}`,
+      };
+    });
+
    expect(entityProviderConnection.applyMutation).toBeCalledWith({
      type: 'full',
-      entities: [
-        {
-          entity: {
-            apiVersion: 'backstage.io/v1alpha1',
-            kind: 'Location',
-            metadata: {
-              annotations: {
-                'backstage.io/managed-by-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key1.yaml',
-                'backstage.io/managed-by-origin-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key1.yaml',
-              },
-              name: 'generated-980e6ad47fbfbfeead708a9c7c87331b7540296a',
-            },
-            spec: {
-              presence: 'required',
-              target:
-                'https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key1.yaml',
-              type: 'url',
-            },
-          },
-          locationKey: 'awsS3-provider:anyProviderId',
-        },
-        {
-          entity: {
-            apiVersion: 'backstage.io/v1alpha1',
-            kind: 'Location',
-            metadata: {
-              annotations: {
-                'backstage.io/managed-by-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key2.yaml',
-                'backstage.io/managed-by-origin-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key2.yaml',
-              },
-              name: 'generated-266794d8e789089dddba2b42cd79e70b149aa61c',
-            },
-            spec: {
-              presence: 'required',
-              target:
-                'https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key2.yaml',
-              type: 'url',
-            },
-          },
-          locationKey: 'awsS3-provider:anyProviderId',
-        },
-        {
-          entity: {
-            apiVersion: 'backstage.io/v1alpha1',
-            kind: 'Location',
-            metadata: {
-              annotations: {
-                'backstage.io/managed-by-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key3.yaml',
-                'backstage.io/managed-by-origin-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key3.yaml',
-              },
-              name: 'generated-96f0cdcd7e33aa687c19d160ec7d5b1975cb9ea1',
-            },
-            spec: {
-              presence: 'required',
-              target:
-                'https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key3.yaml',
-              type: 'url',
-            },
-          },
-          locationKey: 'awsS3-provider:anyProviderId',
-        },
-        {
-          entity: {
-            apiVersion: 'backstage.io/v1alpha1',
-            kind: 'Location',
-            metadata: {
-              annotations: {
-                'backstage.io/managed-by-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key4.yaml',
-                'backstage.io/managed-by-origin-location':
-                  'url:https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key4.yaml',
-              },
-              name: 'generated-cd1a799b5ecfc055a0c672654420af3afeb648d3',
-            },
-            spec: {
-              presence: 'required',
-              target:
-                'https://bucket-1.s3.us-east-1.amazonaws.com/sub/dir/key4.yaml',
-              type: 'url',
-            },
-          },
-          locationKey: 'awsS3-provider:anyProviderId',
-        },
-      ],
+      entities: expectedEntities,
    });
+  };
+
+  // eslint-disable-next-line jest/expect-expect
+  it('apply full update on scheduled execution', async () => {
+    return expectMutation(
+      'regionalStatic',
+      {
+        bucketName: 'bucket-1',
+        prefix: 'sub/dir/',
+        region: 'eu-west-1',
+      },
+      'https://s3.eu-west-1.amazonaws.com/bucket-1/sub/dir/',
+      {
+        'key1.yaml': 'generated-7f6d5861b0b3401a38b5fe62e6c7ca11da5fd6d8',
+        'key2.yaml': 'generated-a290be145586042af7d80715626399c9d661718d',
+        'key3.yaml': 'generated-8d75f78ed9fa618ce433b226dc24eeab441f3a2d',
+        'key 4.yaml': 'generated-1e0249dcb5805fc2ce6ac2d3c4d2a3ef4f1270c0',
+      },
+    );
+  });
+
+  // eslint-disable-next-line jest/expect-expect
+  it('us-east-1 has region-less URLs', async () => {
+    return expectMutation(
+      'usEast1',
+      {
+        bucketName: 'bucket-1',
+        prefix: 'sub/dir/',
+        region: 'us-east-1',
+      },
+      'https://s3.amazonaws.com/bucket-1/sub/dir/',
+      {
+        'key1.yaml': 'generated-f7e3f1c89f62cbf0d82db16452faaa7c040fc331',
+        'key2.yaml': 'generated-925173f7dba1acaa73cac5ef4d10c3ed7660aa25',
+        'key3.yaml': 'generated-d94cf017911ed7fb3be4a62a3dae1f5202879de3',
+        'key 4.yaml': 'generated-f917fca0cfacc2be478ca2a3cff94d00917a1cde',
+      },
+    );
+  });
+
+  // eslint-disable-next-line jest/expect-expect
+  it('fallback region if absent', async () => {
+    // logic will use region information provided from
+    // profile < AWS_REGION < argument to client/command
+    process.env.AWS_REGION = 'eu-central-1';
+
+    return expectMutation(
+      'absentRegion',
+      {
+        bucketName: 'bucket-1',
+        prefix: 'sub/dir/',
+      },
+      'https://s3.eu-central-1.amazonaws.com/bucket-1/sub/dir/',
+      {
+        'key1.yaml': 'generated-285d144b5c1e24e801b97f61f794d566dabc7236',
+        'key2.yaml': 'generated-36b78d7fa69690059797d4cc4f40c2a5eaed1d6d',
+        'key3.yaml': 'generated-a39ad804bf65993cbc0f3810ff128a3c91daf768',
+        'key 4.yaml': 'generated-bd7622ac86c313cb812d90e335b2f5f6bff14670',
+      },
+    );
+  });
+
+  // eslint-disable-next-line jest/expect-expect
+  it('custom endpoint', async () => {
+    return expectMutation(
+      'customEndpoint',
+      {
+        bucketName: 'bucket-1',
+        prefix: 'sub/dir/',
+      },
+      'http://localhost:1234/bucket-1/sub/dir/',
+      {
+        'key1.yaml': 'generated-e1f1dcfe44967b899a49d856cadcfa1ffc72a9f6',
+        'key2.yaml': 'generated-6b09503942fe41566339f2eccfbb3380a022494b',
+        'key3.yaml': 'generated-17274cdefac2feb3702f41605ca48aa4370d20a9',
+        'key 4.yaml': 'generated-659f17a2429f107db04850f9081e3e9c56084aeb',
+      },
+      {
+        endpoint: 'http://localhost:1234',
+      },
+    );
  });
 });
@@ -57,7 +57,7 @@ export class AwsS3EntityProvider implements EntityProvider {
    // Even though the awsS3 integration allows a config array
    // there is no *real* support for multiple configs.
    // Usually, there will be just the integration for the default host.
-    // In case, a config custom endpoint is used, the host from this endpoint
+    // In case, a custom endpoint is used, the host from this endpoint
    // will be extracted and used as host (e.g., localhost when used with LocalStack)
    // and the default integration will be added as second integration.
    // In this case, we still want the first one though, but have no means to select it
@@ -80,7 +80,7 @@ export class AwsS3EntityProvider implements EntityProvider {

  private constructor(
    private readonly config: AwsS3Config,
-    private readonly integration: AwsS3Integration,
+    integration: AwsS3Integration,
    logger: Logger,
    schedule: TaskRunner,
  ) {
@@ -196,16 +196,8 @@ export class AwsS3EntityProvider implements EntityProvider {

  private createObjectUrl(key: string): string {
    const bucketName = this.config.bucketName;
-    const endpoint = this.integration.config.endpoint;
+    const endpoint = this.s3.endpoint.href;

-    if (endpoint) {
-      if (endpoint.startsWith(`https://${bucketName}.`)) {
-        return `${endpoint}/${key}`;
-      }
-
-      return `${endpoint}/${bucketName}/${key}`;
-    }
-
-    return `https://${bucketName}.s3.${this.config.region}.amazonaws.com/${key}`;
+    return encodeURI(`${endpoint}${bucketName}/${key}`);
  }
 }