-
Notifications
You must be signed in to change notification settings - Fork 17
Major: rework environment configuration of CDK #109
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.
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
Conversation
* sanitise environment * handle endpoint url configuration misconfiguration * add testing
8095e03
to
90051e7
Compare
We don't track the lockfile
90051e7
to
e30c850
Compare
By default, we strip the values so we are overwriting but if the user specifies the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY in their AWS_ENVAR_ALLOWLIST then they can read these from the environment
5b3f352
to
c8597b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I hope this finally fixes that weird issue. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the thorough documentation around sanitizing the environment variables. 🚀
Had a few minor nits but nothing blocking us from getting this out
.github/workflows/unit.yml
Outdated
unit-tests: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: could use the newest version (v4
) if there's no specific reason to stay on a lower one
- name: Use Node.js ${{ inputs.node-version }} | ||
uses: actions/setup-node@v2 | ||
with: | ||
node-version: ${{ inputs.node-version }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: same as above, newest version for that action is v4
README.md
Outdated
|
||
For these CDK versions, we remove AWS configuration environment variables like `AWS_PROFILE` from the shell environment before invoking the `cdk` command, and explicitly set `AWS_ENDPOINT_URL` and `AWS_ENDPOINT_URL_S3` to target LocalStack. | ||
|
||
1. We do this because other environment variables may lead to a conflicting set of configuration options, where the wrong region is used to target LocalStack, or `cdklocal` tries to deploy into upstream AWS by mistake. If individual configuration variables are needed for the deploy process (e.g. `AwS_REGION`) these configuration variables can be propagated to the `cdk` command by configuring `AWS_ENVAR_ALLOWLIST`, for example: `AWS_ENVAR_ALLOWLIST=AWS_REGION,AWS_DEFAULT_REGION AWS_REGION=eu-central-1 cdklocal ...`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. We do this because other environment variables may lead to a conflicting set of configuration options, where the wrong region is used to target LocalStack, or `cdklocal` tries to deploy into upstream AWS by mistake. If individual configuration variables are needed for the deploy process (e.g. `AwS_REGION`) these configuration variables can be propagated to the `cdk` command by configuring `AWS_ENVAR_ALLOWLIST`, for example: `AWS_ENVAR_ALLOWLIST=AWS_REGION,AWS_DEFAULT_REGION AWS_REGION=eu-central-1 cdklocal ...`. | |
1. We do this because other environment variables may lead to a conflicting set of configuration options, where the wrong region is used to target LocalStack, or `cdklocal` tries to deploy into upstream AWS by mistake. If individual configuration variables are needed for the deploy process (e.g. `AWS_REGION`) these configuration variables can be propagated to the `cdk` command by configuring `AWS_ENVAR_ALLOWLIST`, for example: `AWS_ENVAR_ALLOWLIST=AWS_REGION,AWS_DEFAULT_REGION AWS_REGION=eu-central-1 cdklocal ...`. |
also: Isn't the AWS_DEFAULT_REGION
in the allowlist example a bit confusing since (contrary to AWS_REGION
) we don't provide it in the following env arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't understand - what do you mean by "don't provide it in the following env arguments"? Do you mean in the README.md
Configurations section? Or in the code? We do allow overriding the AWS_DEFAULT_REGION
in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
AWS_ENVAR_ALLOWLIST=AWS_REGION,AWS_DEFAULT_REGION AWS_REGION=eu-central-1 cdklocal ...
vs
AWS_ENVAR_ALLOWLIST=AWS_REGION,AWS_DEFAULT_REGION AWS_DEFAULT_REGION=eu-central-1 AWS_REGION=eu-central-1 cdklocal ...
README.md
Outdated
For these CDK versions, we remove AWS configuration environment variables like `AWS_PROFILE` from the shell environment before invoking the `cdk` command, and explicitly set `AWS_ENDPOINT_URL` and `AWS_ENDPOINT_URL_S3` to target LocalStack. | ||
|
||
1. We do this because other environment variables may lead to a conflicting set of configuration options, where the wrong region is used to target LocalStack, or `cdklocal` tries to deploy into upstream AWS by mistake. If individual configuration variables are needed for the deploy process (e.g. `AwS_REGION`) these configuration variables can be propagated to the `cdk` command by configuring `AWS_ENVAR_ALLOWLIST`, for example: `AWS_ENVAR_ALLOWLIST=AWS_REGION,AWS_DEFAULT_REGION AWS_REGION=eu-central-1 cdklocal ...`. | ||
2. If you are manually setting `AWS_ENDPOINT_URL`, the new value will continue to be read from the environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing me a bit as it seems to conflict with the PR description:
Raise an exception if the user specifies AWS_ENDPOINT_URL without AWS_ENDPOINT_URL_S3 as we cannot automatically determine a configuration that will work. We need to ensure that .s3. is in the domain for S3 requests. If the user customises the endpoint then it may not support subdomains (so we could not just inject .s3. in their URL, and we cannot use the same URL as our S3 detection won't work.
Motivation
We have had numerous reports of users trying to use
cdklocal
and receiving error messages like:or
My hypothesis is when
cdk
switched to using the JavaScript AWS SDK v3 our patching/envar overriding incdklocal
reads valid AWS configuration from the environment, and either does not execute against LocalStack correctly, or tries to deploy to AWS but does not have enough context to connect correctly.In #107 we updated
cdklocal
forcdk >= 2.177.0
by configuring justAWS_ENDPOINT_URL
andAWS_ENDPOINT_URL_S3
. However we still read e.g. theAWS_PROFILE
envar, orAWS_ACCESS_KEY_ID
/AWS_SECRET_ACCESS_KEY
envars. This confusion of environment variables and configuration should be removed - we should sanitise the environment incdklocal
before executing commands.Future work
We are starting to evaluate deprecating our "*local" tools and using the first party tooling where possible. In this issue comment I outline a working configuration for using the upstream
cdk
with LocalStack. In summary, creating an AWS profile (in~/.aws/config
) with the following contents:Changes
This PR includes a few changes (#srynotsry)
cdklocal
primarily so we can unit testconfigureEnvironment
to the utilities library for testingconfigureEnvironment
AWS_ENDPOINT_URL
withoutAWS_ENDPOINT_URL_S3
as we cannot automatically determine a configuration that will work. We need to ensure that.s3.
is in the domain for S3 requests. If the user customises the endpoint then it may not support subdomains (so we could not just inject.s3.
in their URL, and we cannot use the same URL as our S3 detection won't work.AWS_*
envars from the environment before executing the CDK, except:AWS_ENDPOINT_URL
AWS_ENDPOINT_URL_S3
AWS_REGION
AWS_DEFAULT_REGION
AWS_ENVAR_ALLOWLIST
) to allow specific envars past this sanitsation, for example if the user wants to deploy to a different regionAWS_ACCESS_KEY_ID
,AWS_SECRET_ACCESS_KEY
,AWS_REGION
andAWS_DEFAULT_REGION
if customised3.0.0
as this change requires changes from the user to maintain the current (i.e. potentially broken) behaviourTesting
aws sts get-caller-identity
returns your AWS credentials) and runcdklocal bootstrap && cdklocal deploy --require-approval never
to deploy your CDK stack. The stack should be deployed successfully to LocalStack.