chore(kms): prefer new aliasArn to keyArn for getting arn of an alias

[rafaelrcamargo]

Motivation:

The current implementation of keyArn within the AWS CDK AWS KMS module returns the Key ARN for a key and an alias, which causes confusion for users expecting the Alias ARN. This PR aims to alleviate this confusion by providing clearer access to the Alias ARN.

Changes:

Introducing a new attribute aliasArn that mirrors the value from keyArn specifically for aliases to explicitly retrieve the Alias ARN.

/**
 * The ARN of the alias.
 *
 * @attribute
 * @deprecated use `aliasArn` instead
 */
public get keyArn(): string {
  return Stack.of(this).formatArn({
    service: 'kms',
    // aliasName already contains the '/'
    resource: this.aliasName,
  });
}

/**
 * The ARN of the alias.
 *
 * @attribute
 */
public get aliasArn(): string {
  return this.keyArn;
}

Query:

Should we deprecate the existing keyArn and mirror it in aliasArn or change the logic within keyArn to aliasArn and use the keyArn as the mirror?

Your feedback on the preferred approach would be greatly appreciated!

Closes #28105.

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[daschaa]

@rafaelrcamargo Thanks for opening up a pull request! I think it's good to add the deprecation warning.
One small remark: Could you also add tests for the aliasArn getter? The current test just checks if it is the same value as keyArn, but if in the future the keyArn logic is changed, we should still test that the aliasArn returns a valid ARN.

[rafaelrcamargo]

@daschaa for sure, makes sense. Are there any utils/helpers or recommended ways to test that in this project?

[daschaa]

@rafaelrcamargo You can use Arn.format(...) to build an ARN for the assertion.

[rafaelrcamargo]

Awesome, followed the current implementation in the L2 construct.

Should I worry about this linter failing? Seems like some change convention.

If there's anything else I can help with, let me know

[daschaa]

Thanks for the contribution. Looks good to me, so a maintainer can have a look :)

Can you add an exemption request for the integration test? Just add a comment with "Exemption request" and the reason why an integration test (and change to the README file) is not needed here.

[rafaelrcamargo]

Exemption request: Integration test and README update not necessary for this PR. No new features have been introduced; the modification solely involves an existing property, which is already well-documented and thoroughly tested with relevant tests.

[scanlonp]

Looks good, one nit and should be good to go.

I do not think this needs either integ tests or a readme update.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Pull request has been modified.

[scanlonp]

Thanks!

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 73b3902
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).