feat(cli): automatically determine region on EC2 instances #9313
Conversation
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
| headers: { 'x-aws-ec2-metadata-token-ttl-seconds': '60' }, | ||
| }, | ||
| (err: AWS.AWSError, token: string | undefined) => { | ||
| if (err || !token) { |
There was a problem hiding this comment.
Should be something like:
if (err) {
fail(err);
} else if (!token) {
fail(new Error('IMDS returned an empty token'));
} else {
resolve(token);
}There was a problem hiding this comment.
Changed to:
(err: AWS.AWSError, token: string | undefined) => {
if (err) {
reject(err);
} else if (!token) {
reject(new Error('IMDS did not return a token.'));
} else {
resolve(token);
}
});| httpOptions: { timeout: 1000, connectTimeout: 1000 }, maxRetries: 2, | ||
| }; | ||
| const metadataService = new AWS.MetadataService(imdsOptions); | ||
| const token = await getImdsV2Token(metadataService); |
There was a problem hiding this comment.
Please rewrite the helpers below to properly throw an error, and then try/catch and log any errors here.
There was a problem hiding this comment.
I rewrote it as such:
const token = await getImdsV2Token(metadataService)
.catch((error) => {
debug(`No token returned from IMDS; Error: ${error}`)
return undefined;
});
region = await getRegionFromImds(metadataService, token)
.catch((error) => {
debug(`No Instance Identity Document returned from IMDS; Error: ${error}`)
return undefined;
});
rix0rrr
changed the title
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
| httpOptions: { timeout: 1000, connectTimeout: 1000 }, maxRetries: 2, | ||
| }; | ||
| const metadataService = new AWS.MetadataService(imdsOptions); | ||
| const token = await getImdsV2Token(metadataService) |
There was a problem hiding this comment.
Kinda don't like the style this is written. It confuses me to combine async/await and thenables in this way.
Why not:
try {
const token = await getImdsV2Token(metadataService);
region = await getRegionFromImds(metadataService, token);
debug(`Retrieved AWS region "${region}" from the IMDS.`);
} catch (e) {
debug(`Unable to retrieve region from IMDS: ${e.message}`);
} There was a problem hiding this comment.
I was thinking about that, but was concerned about a case where IMDSv2 was not supported -- potentially in partition where it's not implemented. In that situation, getImdsV2Token would throw an error but IMDSv1 should still be queried to get the region, just without a token.
rix0rrr
added
the
pr-linter/exempt-readme
@rix0rrr -- In addition to your thoughts on continuing to query IMDSv1 if we don't get an IMDSv2 token, would you also mind pointing me in the right direction for writing a test? Maybe just a file that I should write the test in and where I could copy syntax from? Thanks! |
rix0rrr
added
pr-linter/exempt-test
nija-at
removed
the
pr-linter/exempt-test
mergify
bot
dismissed stale reviews from rix0rrr, nija-at, and njlynch
Pull request has been modified.
rix0rrr
removed
the
pr/requires-two-approvers
| async function isEc2Instance() { | ||
| if (isEc2InstanceCache === undefined) { | ||
| debug("Determining if we're on an EC2 instance."); | ||
| let instance = false; | ||
| if (process.platform === 'win32') { | ||
| // https://docs.aws.amazon.com/AWSEC2/latest/WindowsGuide/identify_ec2_instances.html | ||
| const result = await util.promisify(child_process.exec)('wmic path win32_computersystemproduct get uuid', { encoding: 'utf-8' }); | ||
| // output looks like | ||
| // UUID | ||
| // EC2AE145-D1DC-13B2-94ED-01234ABCDEF | ||
| const lines = result.stdout.toString().split('\n'); | ||
| instance = lines.some(x => matchesRegex(/^ec2/i, x)); | ||
| } else { | ||
| // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/identify_ec2_instances.html | ||
| const files: Array<[string, RegExp]> = [ |
There was a problem hiding this comment.
consider adding similar test cases for this method and its conditionals. Since this method already existed, I'll let you make the final call.
| .mockImplementationOnce((_1, _2, cb) => { cb(undefined as any, JSON.stringify({ region: 'some-region' })); }); | ||
|
|
||
| const region = await AwsCliCompatible.region({ ec2instance: true }); | ||
| expect(region).toEqual('some-region'); |
There was a problem hiding this comment.
Nit: add an expectation that the mocks were actually called
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
mergify
bot
dismissed stale reviews from rix0rrr and nija-at
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Enables EC2 instances to automatically determine their current region by querying the Instance Metadata Service (IMDS). Both IMDSv1 and v2 are supported.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license