fix(kms): allow multiple addAlias calls on single key
#3596
Conversation
| @@ -68,7 +68,7 @@ abstract class KeyBase extends Resource implements IKey { | |||
| * Defines a new alias for the key. | |||
| */ | |||
| public addAlias(alias: string): Alias { | |||
| return new Alias(this, 'Alias', { aliasName: alias, targetKey: this }); | |||
| return new Alias(this, `Alias${alias}`, { aliasName: alias, targetKey: this }); | |||
There was a problem hiding this comment.
Not a particularly clean solution.
Maybe adding an aliasCounter property to the Key and appending it instead of the name would be more preferable?
There was a problem hiding this comment.
This is good, because it's stable. Using counters will lead to problems if people change the order of a list of aliases, since all of their logical ids would change.
There was a problem hiding this comment.
Good point.
Could there be issues with valid aliasName values that would result in an invalid logical ID? I should probably slug the name if we're keeping it
There was a problem hiding this comment.
Don't worry about it, that happens automatically.
There was a problem hiding this comment.
Alright 👍
And thanks for the fixes!
| @@ -311,7 +311,7 @@ export = { | |||
| DeletionPolicy: "Retain", | |||
| UpdateReplacePolicy: "Retain", | |||
| }, | |||
| MyKeyAlias1B45D9DA: { | |||
| MyKeyAliasaliasxoo1B45D9DA: { | |||
There was a problem hiding this comment.
Testing it locally, the logical ID suffix kept changing, e.g. MyKeyAliasaliasxoo9464EB1C.
There was a problem hiding this comment.
@rix0rrr Out of curiosity, what was causing my IDs to be unstable?
There was a problem hiding this comment.
Honestly, I have no idea. I just changed them to the expectations. Might it have been copy/pasting between tests? I see some of them had similar but slightly different construct IDs.
There was a problem hiding this comment.
I ran the tests again on my original commit, and yeah they're stable. I must have misread the test output. Sorry about that!
rix0rrr
changed the title
addAlias calls on single key
|
Thanks again @rix0rrr ❤️ |
|
Hello @nmussy ! I have updated my project to CDK 1.4.0, and now my KMS aliases are failing. Basically, it shows now that aliases have to be destroyed and recreated with different logical names, but when I try to deploy, it fails saying that alias already exists. |
|
Hey @Visorgood. Thanks for letting us know, I was able to reproduce it. I'm opening a separate issue. Sorry for the inconvenience! |
We append alias name to logical ID to prevent collisions.
Also allow omitting the required
"alias/"prefix when adding an alias, it will be added automatically when necessary, and adding an initial alias through the construction props ofKey.Fixes #3594
Please read the contribution guidelines and follow the pull-request checklist.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license