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

feat(cli): preview of cdk import #17666

Merged
merged 16 commits into from
Apr 5, 2022
Merged

Conversation

tomas-mazak
Copy link
Contributor

@tomas-mazak tomas-mazak commented Nov 23, 2021

An initial version of cdk import, bringing existing resources under the
management of CloudFormation.

To use:

  • Make sure your diff is clean (you've recently deployed)
  • Add constructs for the resource(s) you want to import. Make sure the CDK code configures them exactly as they are configured in reality.
    • You can provide resource names here but it's probably better if you don't.
  • Run cdk import. If you use v2, this requires updating the bootstrap stack.
  • Provide the actual resource names for each resource (if necessary).
  • An importing changeset will execute and the resources are imported.

This is an implementation of aws/aws-cdk-rfcs#52

@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Nov 23, 2021
@gitpod-io
Copy link

gitpod-io bot commented Nov 23, 2021

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ love it! Thanks for working on this!

Comment on lines 454 to 455
**no other changes must be done to the stack before the import is completed**
- run `cdk deploy` with `--import-resources` argument to instruct CDK to start the import
Copy link
Contributor

@rix0rrr rix0rrr Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this is a different enough workflow that it's worth its own command. How about cdk import-resources (cdk import for short) ?

Probably the -e flag should be implied?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I didn't spend too much thought on the interface TBH, I considered this more like a PoC, to try out how it would work ... I am happy to raise a RFC - actually I only read about the RFC process after raising this PR and so I started with it right away by raising it at #aws-cdk-rfcs Slack channel.

Happy to change the interface to your liking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated accordingly

identifier[idpart] = chg.newProperties[idpart];
} else {
const displayName : string = newTemplate.template?.Resources?.[id]?.Metadata?.['aws:cdk:path'] ?? id;
identifier[idpart] = await promptly.prompt(`Please enter ${idpart} of ${chg.newResourceType} to import as ${displayName.replace(/\/Resource$/, '')}: `);
Copy link
Contributor

@rix0rrr rix0rrr Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to be able to read these answers from a file as well (JSON file?) so people can use it in an unattended way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
}

resourcesToImport.push({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the interactive flow (which I super love!) I think it's probably worth printing a summary with an option to confirm (or re-enter the names), wdyt?

resourcesToImport.push({
LogicalResourceId: id,
ResourceType: chg.newResourceType,
ResourceIdentifier: identifier,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh god must we really always supply this, even if it's already in the template? Great job on automating that away!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the CFN API for importing is suboptimal ... :/ This structure can at least be automatically assembled, but what's worse, a changeset of type import cannot contain any non-import template changes apparently. It will be a challenge to support an import of higher-level constructs that synthesise into multiple CFN resources of which only a subset must be imported and the rest needs to be created.

Copy link
Contributor

@rix0rrr rix0rrr Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be a challenge to support an import of higher-level constructs that synthesise into multiple CFN resources of which only a subset must be imported and the rest needs to be created.

That is a good point. What we could be doing is:

  • From all the resources that are added, have the user pick which ones they want to import (exact UI for this TBD)
  • Cull the template to only selected resources
  • Run an import with only those resources
  • (Potentially: follow up with a regular deploy)

By necessity the resources that are imported cannot depend on other resources that aren't being imported as well (or exist already) so that's a bit limiting but best we can do for now until we build a more comprehensive multi-phase importing engine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a great idea, I will try to implement that!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the UI, it might be as simple as:

  • For resources we don't know a name for, if the user doesn't fill in a name we skip it.
  • For sources we DO know a name for, we pre-fill it and the user can backspace it if they don't want to import.

The latter may not be possible with promptly, I'm not quite sure. But that would seem simple and clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rix0rrr , I have implemented the above flow: pre-filling a prompt wasn't possible (or I didn't find out how) but it now works that:

  • if the identifier can be inferred from props, only ask whether to import the given resource or skip
  • if the identifier can't be inferred, ask for the identifier interactively, if left empty, skip import

All resources that don't support importing or were marked to be skipped by user will be removed from the template for the import operation. So the import succeeds and the subsequent deploy will create the remaining resources.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds great!

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 24, 2021

Haha most of the things I was asking for were already in your TODO list 😅

@tomas-mazak tomas-mazak changed the title feat(cli): Add an option to import existing resources (currently S3 buckets only) feat(cli): Add an option to import existing resources Nov 27, 2021
…uckets only)

This is an initial proposal to support existing resources import into
CDK stacks. As a PoC, this PR shows a working solution for S3 buckets.
This is achieved by introducing `-i` / `--import-resources` CLI option
to the `cdk deploy` command. If specified, the newly added resources
will not be created, but attempted to be imported (adopted) instead. If
the resource definition contains the full resource identifier, this
happens automatically. For resources that can't be identified (e.g. an
S3 bucket without an explicit `bucketName`), user will be prompted for
the necessary information.
@tgigsigniant
Copy link

Hi @tomas-mazak, I was just wondering if CDK import will natively support importing Global tables for DynamoDB as part of this PR (AWS Support directed me towards this PR)?

Additional details:
We currently had to hand roll our own solution to import a Global Table to a CDK app by using the CFN import tool. Our solution was based off of this stack overflow post . But, we ran into several errors specific to importing a Global Table in CDK. For example, we couldn't import the Table with the replicationRegions param specified given all the additional AssetParameters it generates.

An error occurred (ValidationError) when calling the CreateChangeSet operation: Parameters: [AssetParametersc691172cdeefa2c91b5a2907f9d81118e47597634943344795f1a844192dd49cS3BucketEAC9DD43, ...] must have value

To get around this, we didn't include replicationRegions for the import, and added it back for the next deployment after the import. However, the replicationRegions param doesn't support tables with existing replicas and requires deleting them first and then recreating them with CDK if we want them to be managed by CDK.

@tomas-mazak
Copy link
Contributor Author

Hi @tomas-mazak, I was just wondering if CDK import will natively support importing Global tables for DynamoDB as part of this PR (AWS Support directed me towards this PR)?

@tgigsigniant , this PR is a (thin) layer on top of the CloudFormation import feature. Instead of doing several manual steps, like in the stackoverflow post you referred to, you will be able to handle the import from CDK command line. All limitations of the CloudFormation import API will still apply. So if CFN doesn't support importing dynamo tables with certain configuration, this PR won't help you unfortunately.

@fitzoh
Copy link

fitzoh commented Jan 18, 2022

If/when this gets merged, this portion of the CDK docs should be updated to read "Referencing existing external resources" instead of "Importing existing external resources" You could probably argue that change should happen regardless of this PR/functionality.

@henrysachs
Copy link

Hey There, really interested in this feature to faster recover rds instances. Any way to support or push this? :)

@RichiCoder1
Copy link
Contributor

Also would love to see this (and even assist if necessary) as this would make Serverless and TF migrations to CDK much simpler.

@saltman424
Copy link
Contributor

Once this is implemented, what would be the recommended way to utilize it as part of @aws-cdk/pipelines or would that require a separate PR?

@rix0rrr rix0rrr changed the title feat(cli): Add an option to import existing resources feat(cli): preview of cdk import Mar 23, 2022
@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 23, 2022

@tomas-mazak I'm doing some work on this PR. No need to jump in.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 5, 2022

TODO

  • Informational message at the end about needing to do a drift detection, or cdk deploy if there were ignored non-addition changes

packages/aws-cdk/lib/cli.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/cli.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/import.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/import.ts Outdated Show resolved Hide resolved
Comment on lines +348 to +350
function fmtdict<A>(xs: Record<string, A>) {
return Object.entries(xs).map(([k, v]) => `${k}=${v}`).join(', ');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Optional/Opportunistic) Any reason not to use JSON.stringify here? Or YAML.stringify?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm. Those don't format things the same. I want it human readable.

await shell(['npm', 'install',
'--prefix', installDir,
'npm@7']);
await shell(['npm', 'install', 'npm@7'], { cwd: installDir });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're there...

Suggested change
await shell(['npm', 'install', 'npm@7'], { cwd: installDir });
await shell(['npm', 'install', 'npm@8'], { cwd: installDir });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's another can of worms I'd rather not get into in this PR.


highlight(stack.displayName);

if (!stack.environment) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment might be agnostic, and could still be resolved using current credentials.

Comment on lines +348 to +350
function fmtdict<A>(xs: Record<string, A>) {
return Object.entries(xs).map(([k, v]) => `${k}=${v}`).join(', ');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm. Those don't format things the same. I want it human readable.

await shell(['npm', 'install',
'--prefix', installDir,
'npm@7']);
await shell(['npm', 'install', 'npm@7'], { cwd: installDir });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's another can of worms I'd rather not get into in this PR.

rix0rrr
rix0rrr previously approved these changes Apr 5, 2022
@mergify mergify bot dismissed rix0rrr’s stale review April 5, 2022 12:39

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2022

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: a021ed6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 4f12209 into aws:master Apr 5, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2022

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).

@github-actions github-actions bot added the p2 label Apr 7, 2022
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 7, 2022

Took a while to get this in, but <3 thanks @tomas-mazak for the hard work!

This was referenced Apr 7, 2022
mergify bot added a commit that referenced this pull request Apr 7, 2022
See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/1.152.0/CHANGELOG.md)

For convenience, extracted the relevant CHANGELOG entry:

## [1.152.0](v1.151.0...v1.152.0) (2022-04-06)


### Features

* **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9))
* **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64))
* **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667)
* **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209))
* **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e))
* **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5))
* **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e))
* add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010))
* **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df))
* **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605)
* **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076)
* **synthetics:** new puppeteer 3.5 runtime ([#19673](#19673)) ([ce2b91b](ce2b91b)), closes [#19634](#19634)


### Bug Fixes

* **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969)
* **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160)
* **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421)
* **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2))
* **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042))
* **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399)
* **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad))
* **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550)
* **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3))
* **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648)
* **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259)
* **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751)
* **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)
mergify bot added a commit that referenced this pull request Apr 7, 2022
See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/2.20.0/CHANGELOG.md)

For convenience, extracted the relevant CHANGELOG entry:

## [2.20.0](v2.19.0...v2.20.0) (2022-04-07)


### Features

* **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9))
* **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64))
* **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667)
* **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209))
* **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e))
* **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5))
* **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e))
* add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010))
* **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df))
* **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605)
* **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076)


### Bug Fixes

* **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969)
* **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160)
* **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421)
* **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2))
* **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042))
* **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399)
* **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad))
* **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550)
* **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3))
* **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648)
* **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259)
* **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751)
* **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)
@tomas-mazak
Copy link
Contributor Author

Thanks a lot @rix0rrr for taking over! Glad it's now in!

@mmuller88
Copy link
Contributor

Seems useful thanks to everyone working on this and to @pahud for the image.

instructions

StevePotter pushed a commit to StevePotter/aws-cdk that referenced this pull request Apr 27, 2022
An initial version of `cdk import`, bringing existing resources under the
management of CloudFormation.

To use:

- Make sure your diff is clean (you've recently deployed)
- Add constructs for the resource(s) you want to import. **Make sure the CDK code configures them exactly as they are configured in reality**. 
  - You can provide resource names here but it's probably better if you don't.
- Run `cdk import`
- Provide the actual resource names for each resource (if necessary).
- An importing changeset will execute and the resources are imported.

This is an implementation of aws/aws-cdk-rfcs#52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants