New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(aws-cdk): Detect presence of EC2 credentials #724

Merged

rix0rrr merged 1 commit into master from huijbers/toolkit-credentials

Sep 17, 2018

Contributor

rix0rrr commented Sep 17, 2018

Automatically detect whether we're on an EC2 instance and only add
looking up metadata credentials if that appears to be true. Add
--instance, --no-instance command-line arguments to override
the guessing if it happens to be wrong.

This will fix long hangs for people that happen to be on machines
where the metadata service address happens to be routable or blackholed,
such as observed in #702.

Fixes #130.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

rix0rrr requested review from RomainMuller and eladb

September 17, 2018 09:59

eladb approved these changes

View reviewed changes

packages/aws-cdk/bin/cdk.ts

@@ @@ -30,16 +30,14 @@ const DEFAULT_TOOLKIT_STACK_NAME = 'CDKToolkit'; @@
               const DEFAULTS = 'cdk.json';
               const PER_USER_DEFAULTS = '~/.cdk.json';
-              // tslint:disable:no-shadowed-variable
+              // tslint:disable:no-shadowed-variable max-line-length

Contributor

eladb Sep 17, 2018

Thank you!

packages/aws-cdk/lib/api/util/sdk.ts Outdated

+                      // run on EC2 boxes but the creds represent something different. Same behavior as
+                      // upstream code.
+                      if (instance === undefined) { instance = await hasEc2Credentials(); }

Contributor

eladb Sep 17, 2018

rename instance to ec2creds or something like that

packages/aws-cdk/lib/api/util/sdk.ts


		/**
		* Return whether we're on an EC2 instance

Contributor

eladb Sep 17, 2018

Is there a reference somewhere for an AWS tool that does this?

Contributor

RomainMuller Sep 17, 2018

The EC2 documentation mentions those!

RomainMuller approved these changes

View reviewed changes

packages/aws-cdk/lib/api/util/sdk.ts

+               */
+              function hasEcsCredentials() {
+                  return (AWS.ECSCredentials.prototype as any).isConfiguredForEcsCredentials();

Contributor

RomainMuller Sep 17, 2018

Sweet, some more esoteric code of unholy nature doing something that the JS SDK should expose more directly \o/

packages/aws-cdk/lib/api/util/sdk.ts


		/**
		* Return whether we're on an EC2 instance

Contributor

RomainMuller Sep 17, 2018

The EC2 documentation mentions those!

packages/aws-cdk/lib/api/util/sdk.ts

+               * Return whether we're on an EC2 instance
+               */
+              async function hasEc2Credentials() {

Contributor

RomainMuller Sep 17, 2018

Surely, this is not something that could be included in all the SDKs, right? 🤨

Contributor Author

rix0rrr Sep 17, 2018

Probably most other ones have semi-sane behavior in this regard already.


          feat(aws-cdk): Detect presence of EC2 credentials

8565b64

Automatically detect whether we're on an EC2 instance and only add
looking up metadata credentials if that appears to be true. Add
`--instance`, `--no-instance` command-line arguments to override
the guessing if it happens to be wrong.

This will fix long hangs for people that happen to be on machines
where the metadata service address happens to be routable or blackholed,
such as observed in #702.

Fixes #130.

rix0rrr force-pushed the huijbers/toolkit-credentials branch from 2acb0a9 to 8565b64 Compare

September 17, 2018 12:03

rix0rrr merged commit 8e8c295 into master

rix0rrr deleted the huijbers/toolkit-credentials branch

September 17, 2018 12:40

eladb pushed a commit that referenced this pull request


          v0.9.2

__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)

eladb pushed a commit that referenced this pull request


          v0.9.2 (#749)

d68b6c3

* v0.9.2

__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)

NGL321 added the contribution/core label

SNYKabbott mentioned this pull request

[Snyk] Security upgrade archiver from 3.0.0 to 7.0.0 SNYKabbott/aws-cdk#39

Open

sumodgeorge mentioned this pull request

[Snyk] Security upgrade archiver from 2.1.1 to 7.0.0 sumodgeorge/aws-cdk#27

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution/core