Skip to content
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

fix(cli): cross account asset upload no longer works #12155

Merged
merged 4 commits into from
Jan 6, 2021

Conversation

redbaron
Copy link
Contributor

@redbaron redbaron commented Dec 18, 2020

cdk_asset asset handlers use IAws to make calls to AWS APIs to discover
information about target environment: account id, region, partition.

Each asset is described by its manifest in a Cloud Assembly. This manifest
can contain placeholders to resolve by asset handlers when publishing
assets.

Previously ${Aws::Partition} placeholder was derived from a code path used
to resolve ${Aws::AccountId}, which was introducing a cyclic dependency for
cross account deployments:

  • to replace partition placeholder it was assuming role in a target account
    to discover partition
  • to assume role in a target account it needs to know full role ARN to assume
  • role ARN contains partition placeholder

It was working for same account deployments and for non environment aware deployments,
because SdkProvider was always using current default (ambient) credentials without making
AssumeRole call, thus it was able to replace placeholders in asset manifest without
introducing a cyclic dependency.

To fix cross account deployments we introduce IAWS.discoverPartition() method to return
partition of default (ambient) credentials cdk deploy is called with. This works, because
cross partition AssumeRole calls are not possible, therefore it's enough to know our
default credentials partition.

Fixes #12151


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

cdk_asset asset handlers use IAws to make calls to AWS APIs to discover
information about target environment: account id, region, partition.

Each asset is described by its manifest in a Cloud Assembly. This manifest
can contain placeholders to resolved by asset handlers when publishing
assets.

Previously `${Aws::Partition}` placeholder was derived from a code path used
to resolve `${Aws::AccountId}`, which was introducing a cyclic dependency for
cross account deployments:

- to replace partition placeholder it was assuming role in a target account
  to discover partition
- to assume role in a target account it needs to know full role ARN to assume
- role ARN contains partition placeholder

It was working for same account deployments and for non environment aware deployments,
because SdkProvider was always using current default (ambient) credentials without making
`AssumeRole` call, thus it was able to replace placeholders in asset manifest without
introducing a cyclic dependency.

To fix cross account deployments we introduce `IAWS.discoverPartition()` method to return
partition of default (ambient) credentials `cdk deploy` is called with. This works, because
cross partition `AssumeRole` calls are not possible, therefore it's enough to know our
default credentials partition.
@gitpod-io
Copy link

gitpod-io bot commented Dec 18, 2020

@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Dec 18, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 18, 2020

You're a saint!

❤️

rix0rrr
rix0rrr previously approved these changes Dec 18, 2020
@mergify mergify bot dismissed rix0rrr’s stale review December 18, 2020 15:01

Pull request has been modified.

rix0rrr
rix0rrr previously approved these changes Dec 18, 2020
@redbaron redbaron marked this pull request as draft December 18, 2020 15:11
@redbaron redbaron marked this pull request as ready for review December 18, 2020 17:44
@redbaron
Copy link
Contributor Author

@rix0rrr , validate-pr check is stuck :(

@redbaron
Copy link
Contributor Author

@rix0rrr , you approved this PR to be merged, but it is stuck on validate-pr step. It might be broken because I briefly switched it to draft mode :(

@polothy
Copy link
Contributor

polothy commented Jan 4, 2021

Maybe rebase the branch to re-run the PR checks?

@mergify mergify bot dismissed rix0rrr’s stale review January 4, 2021 19:46

Pull request has been modified.

@redbaron redbaron requested a review from rix0rrr January 4, 2021 19:46
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: dd1605f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@redbaron
Copy link
Contributor Author

redbaron commented Jan 5, 2021

@rix0rrr , would you mind to approve it again, only change is that I merged master to retrigger checks, because validate-pr was stuck

@mergify
Copy link
Contributor

mergify bot commented Jan 6, 2021

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
Copy link
Contributor

mergify bot commented Jan 6, 2021

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 mergify bot merged commit 1c8cb11 into aws:master Jan 6, 2021
@redbaron redbaron deleted the fix-cross-account-assets branch January 6, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(cli): cross account deployments still don't work
4 participants