Adopt SDK-standard behavior when no environment is specified #128

RomainMuller · 2018-06-19T12:31:11Z

Use environment variables in a way that is consistent with what the
various AWS SDKs and CLIs do:
- AWS_SHARED_CREDENTIALS_FILE can be used to override the location
  of the ~/.aws/credentials file
- AWS_CONFIG_FILE can be used to override the location of the
  .aws/config file
Ensure the environment variable instructing the JS AWS SDK to load up
data from the ~/.aws/config file gets set by the toolkit before
aws-sdk is loaded
Create ~/.aws/credentials if ~/.aws/config exists, since the JS
AWS SDK will start by loading configuration from there, and will fail
if it does not exist
Retire the default-account and default-region keys from the
context map, and replace them with aws:cdk:toolkit: keys that
cannot be user-provided
Obtain the default region from AWS.config.region instead of trying
to "manually" source from environment variables.

Fixes #59

* Use environment variables in a way that is consistent with what the various AWS SDKs and CLIs do: + `AWS_REGION` has precedence over `AWS_DEFAULT_REGION` + `AWS_SHARED_CREDENTIALS_FILE` can be used to override the location of the `~/.aws/credentials` file + `AWS_CONFIG_FILE` can be used to override the location of the `.aws/config` file * Ensure the environment variable instructing the JS AWS SDK to load up data from the `~/.aws/config` file gets set by the toolkit before `aws-sdk` is loaded * Create `~/.aws/credentials` if `~/.aws/config` exists, since the JS AWS SDK will start by loading configuration from there, and will fail if it does not exist * Retire the `default-account` and `default-region` keys from the `context` map, and replace them with `aws:cdk:toolkit:` keys that cannot be user-provided Fixes #59

eladb · 2018-06-19T12:37:19Z

packages/aws-cdk/bin/cdk.ts

@@ -401,7 +404,7 @@ async function initCommandLine() {
        for (const stack of synthesizedStacks) {
            if (stackIds.length !== 1) { highlight(stack.name); }
            if (!stack.environment) {
-                throw new Error(`Stack ${stack.name} has no environment`);
+                throw new Error(`Stack ${stack.name} does not define an environment, and no usable default environment was found.`);


Is this going to be the error we emit when there are no credentials defined? If it is, it should indicate that instead of talking about environments, which, in the simple "getting started" case, are not something users actually even aware of.

I was trying to come up with a phrasing to this effect, but nothing felt good. Will try again :)

eladb · 2018-06-19T12:38:16Z

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

+ * This ensures the correct environment is set-up so the AWS SDK properly
+ * loads up configruation stored in the shared credentials file (usually
+ * found at ~/.aws/credentials) and the aws config file (usually found at
+ * ~/.aws/config), if either is present.


Add the URL of this pull request here

eladb · 2018-06-19T12:53:07Z

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

+
+if (fs.existsSync(awsConfigFile) && !fs.existsSync(sharedCredentialsFile)) {
+    /*
+     * Write an empty credentials file if there's a config fil, otherwise the SDK will simply bail out,


"fil" => "file"

Is there some reference to that behavior that we can link to?

I don't believe so... The JS SDK documentation is extremely fuzzy about that. Short of linking to the particular line of code that fails to check for file existence...

eladb · 2018-06-19T12:55:57Z

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

+import * as os from 'os';
+import * as path from 'path';
+
+export const sharedCredentialsFile =


To ensure consistent behavior in the future in case the default of AWS_SHARED_CREDENTIALS_FILE changes in the SDK but we don't update here, why not just do:

process.env.AWS_SHARED_CREDENTIALS_FILE = process.env.AWS_SHARED_CREDENTIALS_FILE || path.join(...)

This way, if the SDK behavior will change for some reason and we don't update the toolkit (yet), the toolkit will "force" the SDK to read from the right file.

Also, this removes the need to export anything from this module and we won't need to require it twice.

All evidence (in particular, the reason why you have to set AWS_SDK_LOAD_CONFIG to have ~/.aws/config even considered by the JS SDK) indicates that SDKs are not going to change how those files are handled without a major version bump. That feels like unnecessary future-proofing.

And I would rather not use the process' environment as a fact way to manage global variables. I don't see why there's a problem in the double-require. Node's require will cache & only load once anyway.

Fair enough. For some reason I have a sense that since this is a "special" module, it would be better if it didn't export anything, so it's usage will strictly be reserved for the purpose of statically initializing the SDK.

It no longer exports anything - the main reason to export was to provide clear & effective debug messages to know "where did the region come from", but now using AWS.config.region, which is as SDK-esque as it gets.

eladb · 2018-06-19T12:58:30Z

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

@@ -72,12 +58,16 @@ export class SDK {
    }

    public defaultRegion() {


Since we need to read the account ID anyway by issuing a request to AWS, wouldn't it make more sense to read the region this way as well and avoid duplicating the SDKs behavior?

Which region to I configure in order to build the service endpoint URL that I will call in order to get the region? OH WAIT!

Obvsiouly you shouldn't configure a region. I believe the SDKs have a default region and we want the toolkit to behave exactly like them.

Ah right. It didn't work when I tested but that was because of initialization order. Now only using config.region.

eladb · 2018-06-19T12:59:05Z

packages/aws-cdk/lib/settings.ts

+        function prohibitContextKey(self: Settings, key: string) {
+            if (!self.settings.context) { return; }
+            if (key in self.settings.context) {
+                throw new Error(`The 'context.${key}' key was found in ${fs_path.resolve(fileName)}, but it is illegal. Please remove it.`);


Instead of "illegal" we should say "it is deprecated"

Deprecated implies it can still be used. Since this throws, it can no longer be used.

So maybe "no longer supported".

eladb · 2018-06-19T12:59:50Z

packages/aws-cdk/lib/settings.ts

+            for (const contextKey of Object.keys(self.settings.context)) {
+                if (contextKey.startsWith(prefix)) {
+                    // tslint:disable-next-line:max-line-length
+                    warning(`A key starting with '${prefix}' key was found in ${fs_path.resolve(fileName)} ('context.${prefix}'), it might cause surprising behavior and should be removed.`);


use the terminology "reserved context key"

RomainMuller requested review from eladb and rix0rrr June 19, 2018 12:36

eladb reviewed Jun 19, 2018

View reviewed changes

RomainMuller added 2 commits June 19, 2018 15:14

Tweaks from feedback.

1164d66

Further edits after feedback

ce2df4b

eladb approved these changes Jun 19, 2018

View reviewed changes

eladb merged commit 2263a5b into master Jun 19, 2018

eladb deleted the rmuller/default-environment branch June 19, 2018 13:59

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

snyk-bot mentioned this pull request Nov 10, 2021

[Snyk] Upgrade colors from 1.3.3 to 1.4.0 SNYKabbott/aws-cdk#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt SDK-standard behavior when no environment is specified #128

Adopt SDK-standard behavior when no environment is specified #128

RomainMuller commented Jun 19, 2018 •

edited

Loading

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

eladb Jun 19, 2018

RomainMuller Jun 19, 2018

		@@ -72,12 +58,16 @@ export class SDK {
		}

		public defaultRegion() {

Adopt SDK-standard behavior when no environment is specified #128

Adopt SDK-standard behavior when no environment is specified #128

Conversation

RomainMuller commented Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainMuller commented Jun 19, 2018 •

edited

Loading