Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,21 @@ export async function makeBodyParameter(
*
* Replaces environment placeholders (which this field may contain),
* and reformats s3://.../... urls into S3 REST URLs (which CloudFormation
* expects)
* expects).
*
* We need to return the official region- and partition-specific URL for AWS S3
* here, so we use the SDK's information about endpoints. At the same time, the
* SDK allows overriding this URL by setting an environment variable
* (specifically $AWS_ENDPOINT_URL_S3) but we want to *not* honor that, because
* there's a 99.9% chance this URL will not be routable from AWS CloudFormation.
*
* To allow for the off chance that someone is running this tool against a
* custom build of CloudFormation that does need a specific S3 endpoint passed
* to it, we'll introduce a new environment variable that we'll respect instead:
*
* AWS_ENDPOINT_URL_S3_FOR_CLOUDFORMATION
Comment on lines +110 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

(blocking): I'm leaning against the proliferation of introducing undocumented env vars doing things. I'm guessing this was added to preempt new issues? Let me reason this through:

  • Currently when AWS_ENDPOINT_URL_S3 is set it will break deployment
  • This PR fixes that
  • If someone deliberately used AWS_ENDPOINT_URL_S3 to do something unknown that we do not know about, they will be broken now.
  • How do they find the new env var? I say they won't and they will open an issue anyway.

I vote we only address this if someone has this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline. This might help use cases like OpenStack and LocalStack.

*/
async function restUrlFromManifest(url: string, environment: Environment): Promise<string> {
export async function restUrlFromManifest(url: string, environment: Environment): Promise<string> {
const doNotUseMarker = '**DONOTUSE**';
const region = environment.region;
// This URL may contain placeholders, so still substitute those.
Expand All @@ -127,13 +139,26 @@ async function restUrlFromManifest(url: string, environment: Environment): Promi
const bucketName = s3Url[1];
const objectKey = s3Url[2];

// SDK v3 no longer allows for getting endpoints from only region.
// A command and client config must now be provided.
const s3 = new S3Client({ region });
const endpoint = await getEndpointFromInstructions({}, HeadObjectCommand, {
...s3.config,
});
endpoint.url.hostname;
const originalOverrideS3Endpoint = process.env.AWS_ENDPOINT_URL_S3;
setEnv('AWS_ENDPOINT_URL_S3', process.env.AWS_ENDPOINT_URL_S3_FOR_CLOUDFORMATION);
try {
// SDK v3 no longer allows for getting endpoints from only region.
// A command and client config must now be provided.
const s3 = new S3Client({ region });
const endpoint = await getEndpointFromInstructions({}, HeadObjectCommand, {
...s3.config,
});

return `${endpoint.url.origin}/${bucketName}/${objectKey}`;
} finally {
setEnv('AWS_ENDPOINT_URL_S3', originalOverrideS3Endpoint);
}
}

return `${endpoint.url.origin}/${bucketName}/${objectKey}`;
function setEnv(name: string, value: string | undefined) {
if (value) {
process.env[name] = value;
} else {
delete process.env[name];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { restUrlFromManifest } from '../../../lib/api/cloudformation/template-body-parameter';

test('restUrlFromManifest ignores AWS_ENDPOINT_URL_S3', async () => {
process.env.AWS_ENDPOINT_URL_S3 = 'https://boop.com/';
try {
await expect(restUrlFromManifest('s3://my-bucket/object', {
account: '123456789012',
region: 'us-east-1',
name: 'env',
})).resolves.toEqual('https://s3.us-east-1.amazonaws.com/my-bucket/object');
} finally {
delete process.env.AWS_ENDPOINT_URL_S3;
}
});

test('restUrlFromManifest respects AWS_ENDPOINT_URL_S3_FOR_CLOUDFORMATION', async () => {
process.env.AWS_ENDPOINT_URL_S3_FOR_CLOUDFORMATION = 'https://boop.com/';
try {
await expect(restUrlFromManifest('s3://my-bucket/object', {
account: '123456789012',
region: 'us-east-1',
name: 'env',
})).resolves.toEqual('https://boop.com/my-bucket/object');
} finally {
delete process.env.AWS_ENDPOINT_URL_S3_FOR_CLOUDFORMATION;
}
});
Loading