diff --git a/.changeset/odd-baboons-buy.md b/.changeset/odd-baboons-buy.md new file mode 100644 index 0000000000..6fe3826985 --- /dev/null +++ b/.changeset/odd-baboons-buy.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Fix a bug in the URL Reading towards AWS S3 where it would hang indefinitely. diff --git a/packages/backend-common/src/reading/AwsS3UrlReader.test.ts b/packages/backend-common/src/reading/AwsS3UrlReader.test.ts index ea969b43a9..0ae64e9404 100644 --- a/packages/backend-common/src/reading/AwsS3UrlReader.test.ts +++ b/packages/backend-common/src/reading/AwsS3UrlReader.test.ts @@ -18,6 +18,9 @@ import { ConfigReader } from '@backstage/config'; import { JsonObject } from '@backstage/types'; import { getVoidLogger } from '../logging'; import { DefaultReadTreeResponseFactory } from './tree'; +import { rest } from 'msw'; +import { setupServer } from 'msw/node'; +import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; import { AwsS3UrlReader, parseUrl } from './AwsS3UrlReader'; import { AwsS3Integration, @@ -111,6 +114,9 @@ describe('parseUrl', () => { }); describe('AwsS3UrlReader', () => { + const worker = setupServer(); + setupRequestMockHandlers(worker); + const createReader = (config: JsonObject): UrlReaderPredicateTuple[] => { return AwsS3UrlReader.factory({ config: new ConfigReader(config), @@ -119,10 +125,6 @@ describe('AwsS3UrlReader', () => { }); }; - afterEach(() => { - AWSMock.restore(); - }); - it('creates a dummy reader without the awsS3 field', () => { const entries = createReader({ integrations: {}, @@ -209,40 +211,34 @@ describe('AwsS3UrlReader', () => { }); describe('read', () => { - let awsS3UrlReader: AwsS3UrlReader; + const [{ reader }] = createReader({ + integrations: { + awsS3: [ + { + host: 'amazonaws.com', + accessKeyId: 'fake-access-key', + secretAccessKey: 'fake-secret-key', + }, + ], + }, + }); - beforeAll(() => { - AWSMock.setSDKInstance(aws); - AWSMock.mock( - 'S3', - 'getObject', - Buffer.from( - require('fs').readFileSync( - path.resolve( - __dirname, - '__fixtures__/awsS3/awsS3-mock-object.yaml', + beforeEach(() => { + worker.use( + rest.get( + 'https://test-bucket.s3.amazonaws.com/awsS3-mock-object.yaml', + (_, res, ctx) => + res( + ctx.status(200), + ctx.set('ETag', '123abc'), + ctx.body('site_name: Test'), ), - ), ), ); - - const s3 = new aws.S3(); - awsS3UrlReader = new AwsS3UrlReader( - new AwsS3Integration( - readAwsS3IntegrationConfig( - new ConfigReader({ - host: 'amazonaws.com', - accessKeyId: 'fake-access-key', - secretAccessKey: 'fake-secret-key', - }), - ), - ), - { s3, treeResponseFactory }, - ); }); it('returns contents of an object in a bucket', async () => { - const response = await awsS3UrlReader.read( + const response = await reader.read( 'https://test-bucket.s3.us-east-2.amazonaws.com/awsS3-mock-object.yaml', ); expect(response.toString().trim()).toBe('site_name: Test'); @@ -250,7 +246,7 @@ describe('AwsS3UrlReader', () => { it('rejects unknown targets', async () => { await expect( - awsS3UrlReader.read( + reader.read( 'https://test-bucket.s3.us-east-2.NOTamazonaws.com/file.yaml', ), ).rejects.toThrow( @@ -262,59 +258,53 @@ describe('AwsS3UrlReader', () => { }); describe('readUrl', () => { - let awsS3UrlReader: AwsS3UrlReader; + const [{ reader }] = createReader({ + integrations: { + awsS3: [ + { + host: 'amazonaws.com', + accessKeyId: 'fake-access-key', + secretAccessKey: 'fake-secret-key', + }, + ], + }, + }); beforeEach(() => { - AWSMock.setSDKInstance(aws); - - AWSMock.mock( - 'S3', - 'getObject', - Buffer.from( - require('fs').readFileSync( - path.resolve( - __dirname, - '__fixtures__/awsS3/awsS3-mock-object.yaml', + worker.use( + rest.get( + 'https://test-bucket.s3.amazonaws.com/awsS3-mock-object.yaml', + (_, res, ctx) => + res( + ctx.status(200), + ctx.set('ETag', '123abc'), + ctx.body('site_name: Test'), ), - ), ), ); - - const s3 = new aws.S3(); - - awsS3UrlReader = new AwsS3UrlReader( - new AwsS3Integration( - readAwsS3IntegrationConfig( - new ConfigReader({ - host: 'amazonaws.com', - accessKeyId: 'fake-access-key', - secretAccessKey: 'fake-secret-key', - }), - ), - ), - { s3, treeResponseFactory }, - ); }); it('returns contents of an object in a bucket via buffer', async () => { - const response = await awsS3UrlReader.readUrl( + const response = await reader.readUrl!( 'https://test-bucket.s3.us-east-2.amazonaws.com/awsS3-mock-object.yaml', ); + expect(response.etag).toBe('123abc'); const buffer = await response.buffer(); expect(buffer.toString().trim()).toBe('site_name: Test'); }); it('returns contents of an object in a bucket via stream', async () => { - const response = await awsS3UrlReader.readUrl( + const response = await reader.readUrl!( 'https://test-bucket.s3.us-east-2.amazonaws.com/awsS3-mock-object.yaml', ); + expect(response.etag).toBe('123abc'); const fromStream = await getRawBody(response.stream!()); expect(fromStream.toString().trim()).toBe('site_name: Test'); }); it('rejects unknown targets', async () => { await expect( - awsS3UrlReader.readUrl( + reader.readUrl!( 'https://test-bucket.s3.us-east-2.NOTamazonaws.com/file.yaml', ), ).rejects.toThrow( @@ -325,45 +315,73 @@ describe('AwsS3UrlReader', () => { }); }); - describe('readUrl with etag', () => { - let awsS3UrlReader: AwsS3UrlReader; + describe('readUrl towards custom host', () => { + const [{ reader }] = createReader({ + integrations: { + awsS3: [ + { + host: 'localhost:4566', + accessKeyId: 'fake-access-key', + secretAccessKey: 'fake-secret-key', + endpoint: 'http://localhost:4566', + s3ForcePathStyle: true, + }, + ], + }, + }); - beforeAll(() => { - AWSMock.setSDKInstance(aws); - - AWSMock.mock('S3', 'getObject', (_, callback) => { - const error: aws.AWSError = { - code: 'NotModified', - message: 'Not Modified', - statusCode: 304, - name: 'oops', - time: new Date('2019-01-01T00:00:00.000Z'), - }; - callback(error, undefined); - }); - - const s3 = new aws.S3(); - - awsS3UrlReader = new AwsS3UrlReader( - new AwsS3Integration( - readAwsS3IntegrationConfig( - new ConfigReader({ - host: 'amazonaws.com', - accessKeyId: 'fake-access-key', - secretAccessKey: 'fake-secret-key', - }), - ), + beforeEach(() => { + worker.use( + rest.get( + 'http://localhost:4566/test-bucket/awsS3-mock-object.yaml', + (_, res, ctx) => + res( + ctx.status(200), + ctx.set('ETag', '123abc'), + ctx.body('site_name: Test'), + ), + ), + ); + }); + + it('returns contents of an object in a bucket via buffer', async () => { + const response = await reader.readUrl!( + 'http://localhost:4566/test-bucket/awsS3-mock-object.yaml', + ); + expect(response.etag).toBe('123abc'); + const buffer = await response.buffer(); + expect(buffer.toString().trim()).toBe('site_name: Test'); + }); + }); + + describe('readUrl with etag', () => { + const [{ reader }] = createReader({ + integrations: { + awsS3: [ + { + host: 'amazonaws.com', + accessKeyId: 'fake-access-key', + secretAccessKey: 'fake-secret-key', + }, + ], + }, + }); + + beforeEach(() => { + worker.use( + rest.get( + 'https://test-bucket.s3.amazonaws.com/awsS3-mock-object.yaml', + (_, res, ctx) => res(ctx.status(304)), ), - { s3, treeResponseFactory }, ); }); it('returns contents of an object in a bucket', async () => { await expect( - awsS3UrlReader.readUrl( + reader.readUrl!( 'https://test-bucket.s3.us-east-2.amazonaws.com/awsS3-mock-object.yaml', { - etag: 'abc123', + etag: '123abc', }, ), ).rejects.toThrow(NotModifiedError); @@ -424,47 +442,4 @@ describe('AwsS3UrlReader', () => { expect(body.toString().trim()).toBe('site_name: Test'); }); }); - - describe('readNonAwsHost', () => { - let awsS3UrlReader: AwsS3UrlReader; - - beforeAll(() => { - AWSMock.setSDKInstance(aws); - AWSMock.mock( - 'S3', - 'getObject', - Buffer.from( - require('fs').readFileSync( - path.resolve( - __dirname, - '__fixtures__/awsS3/awsS3-mock-object.yaml', - ), - ), - ), - ); - - const s3 = new aws.S3(); - awsS3UrlReader = new AwsS3UrlReader( - new AwsS3Integration( - readAwsS3IntegrationConfig( - new ConfigReader({ - host: 'localhost:4566', - accessKeyId: 'fake-access-key', - secretAccessKey: 'fake-secret-key', - endpoint: 'http://localhost:4566', - s3ForcePathStyle: true, - }), - ), - ), - { s3, treeResponseFactory }, - ); - }); - - it('returns contents of an object in a bucket', async () => { - const response = await awsS3UrlReader.read( - 'http://localhost:4566/test-bucket/awsS3-mock-object.yaml', - ); - expect(response.toString().trim()).toBe('site_name: Test'); - }); - }); }); diff --git a/packages/backend-common/src/reading/AwsS3UrlReader.ts b/packages/backend-common/src/reading/AwsS3UrlReader.ts index dbd2c66a62..b0ac81676a 100644 --- a/packages/backend-common/src/reading/AwsS3UrlReader.ts +++ b/packages/backend-common/src/reading/AwsS3UrlReader.ts @@ -214,10 +214,38 @@ export class AwsS3UrlReader implements UrlReader { const request = this.deps.s3.getObject(params); options?.signal?.addEventListener('abort', () => request.abort()); - const etag = (await request.promise()).ETag; - return ReadUrlResponseFactory.fromReadable(request.createReadStream(), { - etag, + // Since we're consuming the read stream we need to consume headers and errors via events. + const etagPromise = new Promise((resolve, reject) => { + request.on('httpHeaders', (status, headers) => { + if (status < 400) { + if (status === 200) { + resolve(headers.etag); + } else if (status !== 304 /* not modified */) { + reject( + new Error( + `S3 readUrl request received unexpected status '${status}' in response`, + ), + ); + } + } + }); + request.on('error', error => reject(error)); + request.on('complete', () => + reject( + new Error('S3 readUrl request completed without receiving headers'), + ), + ); + }); + + const stream = request.createReadStream(); + stream.on('error', () => { + // The AWS SDK forwards request errors to the stream, so we need to + // ignore those errors here or the process will crash. + }); + + return ReadUrlResponseFactory.fromReadable(stream, { + etag: await etagPromise, }); } catch (e) { if (e.statusCode === 304) {