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

import: asks for multiple resource identifiers at once #20895

Closed
Rabadash8820 opened this issue Jun 28, 2022 · 4 comments · Fixed by #24439
Closed

import: asks for multiple resource identifiers at once #20895

Rabadash8820 opened this issue Jun 28, 2022 · 4 comments · Fixed by #24439
Assignees
Labels
@aws-cdk/aws-route53 Related to Amazon Route 53 bug This issue is a bug. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed.

Comments

@Rabadash8820
Copy link
Contributor

Rabadash8820 commented Jun 28, 2022

Describe the bug

I am trying to use the cdk import command to update a CDK-managed stack with existing AWS resources. The stack is for DNSSEC resources for a domain, and I'm trying to import an AWS::Route53::KeySigningKey. Unfortunately, the command seems to be asking for two resource identifiers at once, so I am unable to import the KeySigningKey.

Expected Behavior

Running cdk import on a stack with an AWS::Route53::KeySigningKey would ask for the HostedZoneId and Name identifiers of the KeySigningKey one at a time, then importing would succeed.

Current Behavior

cdk import instead gives me this prompt (where HostedZoneId,Name is all in blue, like a single identifier):

(AWS::Route53::KeySigningKey): enter HostedZoneId,Name to import (empty to skip):

I've tried providing just the KeySigningKey's HostedZoneId, just its Name, or both separated by a comma as the prompt suggests. In all cases, the import command returns the following error message (where <StackName> is the actual name of the CDK stack into which I'm importing):

❌  <StackName> failed: Error [ValidationError]: Invalid resource identifier for resource type AWS::Route53::KeySigningKey. Expected [HostedZoneId, Name]
    at Request.extractError (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/protocol/query.js:50:29)
    at Request.callListeners (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/sequential_executor.js:106:20)
    at Request.emit (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/sequential_executor.js:78:10)
    at Request.emit (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/request.js:686:14)
    at Request.transition (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/request.js:22:10)
    at AcceptorStateMachine.runTo (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/state_machine.js:14:12)
    at /usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/state_machine.js:26:10
    at Request.<anonymous> (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/request.js:38:9)
    at Request.<anonymous> (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/request.js:688:12)
    at Request.callListeners (/usr/local/share/nvm/versions/node/v16.15.1/lib/node_modules/cdk/node_modules/aws-sdk/lib/sequential_executor.js:116:18) {
  code: 'ValidationError',
  time: 2022-06-28T07:12:59.479Z,
  requestId: '6772576d-59db-4210-a6f9-af7eaf776d38',
  statusCode: 400,
  retryable: false,
  retryDelay: 645.4810660286106
}

Invalid resource identifier for resource type AWS::Route53::KeySigningKey. Expected [HostedZoneId, Name]

Adding the --debug flag does not provide any additional information.

Reproduction Steps

  1. Provision the following resources in an AWS account:
    • KMS Key set up for DNSSEC
    • Route53 HostedZone with KeySigningKey
  2. Define a CDK app with a single, empty stack.
  3. Deploy the app with cdk deploy
  4. Add a KeySigningKey resource to the stack as follows, filling in the <value>s:
    new route53.CfnKeySigningKey(this, "KeySigningKey", {
        hostedZoneId: "<hostedZoneId>",
        keyManagementServiceArn: "<keyArn>",
        name: "key_signing_key",
        status: "ACTIVE"
    });
  5. Run cdk import
  6. Follow the prompts and observe the messages from above

Possible Solution

No response

Additional Information/Context

The CDK logic to ask for resource identifiers for cdk import is in import.ts file. The resourceIdentifiers are read from some CloudFormation GetTemplateSummary response. Using another stack with KeySigningKeys in my account as an example, I found that the ResourceIdentifierSummaries array in the GetTemplateSummary response included this object:

{
    "ResourceType": "AWS::Route53::KeySigningKey",
    "LogicalResourceIds": [
        "KeySigningKey"
    ],
    "ResourceIdentifiers": [
        "HostedZoneId,Name"
    ]
},

Note the HostedZoneId,Name identifier. I'm not sure if this is a bug with Route53 or what, but it doesn't seem like any resource type should include a comma-delimited identifier in ResourceIdentifiers, since that field is already a string array. This may have something to do with the fact that the default Fn::Ref return value for a KeySigningKey is an identifier that has the format <HostedZoneId>|<Name>. I tried passing a string with this format to cdk import but still got the above error message.

CDK CLI Version

2.29.1 (build c42e961)

Framework Version

No response

Node.js Version

v16.15.1

OS

Debian bullseye

Language

Typescript

Language Version

3.9.10

Other information

Technically running Debian in a WSL container on Windows 11 Pro

@Rabadash8820 Rabadash8820 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 28, 2022
@github-actions github-actions bot added the @aws-cdk/aws-route53 Related to Amazon Route 53 label Jun 28, 2022
@Rabadash8820 Rabadash8820 changed the title import: requests multiple fields at once import: asks for multiple resource identifiers at once Jun 28, 2022
@saltman424
Copy link
Contributor

@Rabadash8820
Workaround until the prompting logic is fixed: you can use a resource mapping file with the correct values to address this problem.

Here are the steps to do so:

  1. Run import with --record-resource-mapping=cdk-import-resource-mapping.json
  2. Edit cdk-import-resource-mapping.json by splitting comma-separated resource identifiers into multiple fields. E.g.
    { "HostedZoneId,Name": "MyHostedZoneId,MyKeySigningKeyName" } becomes { "HostedZoneId": "MyHostedZoneId", "Name": "MyKeySigningKeyName" }
  3. Run import again but this time with --resource-mapping=cdk-import-resource-mapping.json

Minor note: the cdk-import-resource-mapping.json file can be named whatever you want

@Rabadash8820
Copy link
Contributor Author

Awesome, thanks for the workaround @saltman424!

@peterwoodworth
Copy link
Contributor

peterwoodworth commented Jul 13, 2022

I'm not sure if the bug that's been described here is related to CDK or CDK import - the resource that is getting created here is a Cfn resource, which means it's exactly the same resource that CloudFormation gives us. Since the API call to CloudFormation is what gives us this response, it's likely on CloudFormation's end

I can report this to CloudFormation internally - tracking P67786409

@peterwoodworth peterwoodworth added needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. and removed needs-triage This issue or PR still needs to be triaged. labels Jul 13, 2022
rix0rrr added a commit that referenced this issue Mar 3, 2023
Some resources that can be imported by CloudFormation allow multiple
identifiers. For example, `AWS::DynamoDB::GlobalTable` allows
`TableName`, `TableArn`, and `TableStreamArn`.

The CLI used to interpret multiple identifiers as "all must be present",
but the contract is actually "exactly one must be present":

* CloudFormation would fail the import if you supply all the fields;
* but CDK would skip the import if you left one out.

The effect is that it is impossible to import resources that allow
more than one identifier.

Fix this by being satisfied as soon as we have one identifier from
the set, instead of expecting all of them to be filled.

Fixes #20895.
@mergify mergify bot closed this as completed in #24439 Mar 6, 2023
mergify bot pushed a commit that referenced this issue Mar 6, 2023
…4439)

When CloudFormation tells us about identifiers for resources it can import, it returns a `string[]`.

Our CLI used to interpret this as a set of identifiers that all must be present. Instead, the contract is actually: each `string` is a comma-separated list of identifiers that must be present together, but from all `strings` exactly one key combination should be supplied (and not multiple).

So:

* `['BucketName']` -> Supply BucketName (easy)
* `['TableName', 'TableArn']` -> supply exactly one of TableName or TableArn (but not both)
* `['HostedZoneId,Name']` -> supply BOTH HostedZoneId and Name.

Because of our misinterpretations, both the cases of resources with multiple POSSIBLE identifiers as well as multiple REQUIRED identifiers would fail to import.

Make the code correctly model the expect types: identifiers are a `string[][]`, where the outer array indicates `OR` and the inner array indicates `AND`.

* For any of the combinations of properties we can lift from the template, prompt the user to confirm (typically 0 or 1, might be more). If the user rejected any of them, we don't do the resource at all.
* If we couldn't lift any full key from the template, ask the user for the properties of each compound key, lifting parts of it from the template if possible.

Fixes #20895.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Mar 6, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

homakk pushed a commit to homakk/aws-cdk that referenced this issue Mar 28, 2023
…s#24439)

When CloudFormation tells us about identifiers for resources it can import, it returns a `string[]`.

Our CLI used to interpret this as a set of identifiers that all must be present. Instead, the contract is actually: each `string` is a comma-separated list of identifiers that must be present together, but from all `strings` exactly one key combination should be supplied (and not multiple).

So:

* `['BucketName']` -> Supply BucketName (easy)
* `['TableName', 'TableArn']` -> supply exactly one of TableName or TableArn (but not both)
* `['HostedZoneId,Name']` -> supply BOTH HostedZoneId and Name.

Because of our misinterpretations, both the cases of resources with multiple POSSIBLE identifiers as well as multiple REQUIRED identifiers would fail to import.

Make the code correctly model the expect types: identifiers are a `string[][]`, where the outer array indicates `OR` and the inner array indicates `AND`.

* For any of the combinations of properties we can lift from the template, prompt the user to confirm (typically 0 or 1, might be more). If the user rejected any of them, we don't do the resource at all.
* If we couldn't lift any full key from the template, ask the user for the properties of each compound key, lifting parts of it from the template if possible.

Fixes aws#20895.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
rix0rrr added a commit that referenced this issue Oct 3, 2023
The `cdk import` feature has existed for a year and a half. Since then,
we've only had one major bug report around multiple identifiers (#20895)
which was fixed half a year ago.

This feature seems solid enough to remove the "Preview" label. There is
only one remaining limitation around Nested Stacks, but this feature
could be addressed without breaking backwards compatibility.
mergify bot pushed a commit that referenced this issue Oct 4, 2023
The `cdk import` feature has existed for a year and a half. Since then, we've only had one major bug report around multiple identifiers (#20895) which was fixed half a year ago.

This feature seems solid enough to remove the "Preview" label. There is only one remaining limitation around Nested Stacks, but this feature could be addressed without breaking backwards compatibility.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-route53 Related to Amazon Route 53 bug This issue is a bug. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants