aws / aws/aws-cdk

(core): addUnacknowledgeableWarning() method for Annotations

Open
#26,914 6 comments 2 reactions 0 assignees View on GitHub
@aws-cdk/core bug effort/small p2
Dominant language
TypeScript
Stars
12.9k
Forks
4.6k
Avg merge
2d 3h
Merged PRs (30d)
83

Description

### Describe the feature

Exactly the [addWarning](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.Annotations.html#addwbrwarningmessagespan-classapi-icon-api-icon-deprecated-titlethis-api-element-is-deprecated-its-use-is-not-recommended%EF%B8%8Fspan) method, but not deprecated

### Use Case

[cdk-nag](https://github.com/cdklabs/cdk-nag) uses the cdk Annotation system to emit `Warnings` and `Errors`. Currently it uses the [addWarning](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.Annotations.html#addwbrwarningmessagespan-classapi-icon-api-icon-deprecated-titlethis-api-element-is-deprecated-its-use-is-not-recommended%EF%B8%8Fspan) method for `Warnings`. `addWarning` was deprecated with the [introduction of `addWarningV2`](https://github.com/aws/aws-cdk/pull/26144) in the latest cdk release. All `cdk-nag` Warnings now emit deprecation messages along with the warning, which adds a lot of clutter the the CLI output. While this could be fixed by switching to `addWarningV2`, I don't think that `addWarningV2` is the correct choice in this case, primarily due to how the `cdk-nag` warning/error suppression system is intended to function.

#### Primary Concern `acknowledgeWarning` vs `NagSuppression`

[`NagSuppression`s](https://github.com/cdklabs/cdk-nag#suppressing-a-rule) are `cdk-nag`'s way of acknowledging warnings and errors. The key difference between `acknowledgeWarning` and `NagSuppression`s are that `NagSuppresion`s write the suppression to the CloudFormation Metadata in a `cdk-nag` [specific format](https://github.com/cdklabs/cdk-nag#using-on-cloudformation-templates). That Cfn metadata is important when using SAST tooling to look over the generated CloudFormation templates (Anecdotally, I know of several use cases where the `cdk-nag` Metadata is reviewed in CI/CD pipelines). If `cdk-nag` were to switch from `addWarning` to `addWarningV2` users would be able to use both `acknowledgeWarning` and `NagSuppresion`s to silence warnings. While technically not a breaking change giving users the ability to use `acknowledgeWarning` for `cdk-nag` isn't a good choice.

#### Smaller Concerns/Annoyances with `addWarningV2` and `cdk-nag`

if `cdk-nag` were to switch to `addWarningV2` and we were to ignore the primary concern there would be a few other minor annoyances.

1. The CLI output of `warnings` and `errors` would be slightly different from one another. `Warnings` would have an additional `addWarningV2` identifier appended to the end of the message, while `errors` would not. While that would be the existing `RuleId` already used for `cdk-nag`, `errors` don't have a corresponding `addErrorV2` (and in my opinion it doesn't make sense to have this) so this adds a bit of clutter and confusion
2. Since there is no `addErrorV2` method there would a delta between the options to acknowledge/suppress `Warnings` and `Errors`

### Proposed Solution

I don't think that "undeprecating" `addWarning` is the correct solution, because in cdk internal uses cases`addWarningV2` is the correct choice and users should use `addWarningV2`. Having a clone of `addWarning` with a better name would add some "guardrails"(?) on usage

1. Add an `addUnacknowledgeableWarning()` method to `Annotations` which is a clone of of the current `addWarning` method
2. Put a disclaimer message message that users should likely be using `addWarningV2` unless they have a specific use case where the warnings should not be acknowledgeable and that unhandled warnings will prohibit users from deploying cdk applications with `strict` mode.

### Other Information

Related to https://github.com/cdklabs/cdk-nag/issues/1418

### Acknowledgements

- [X] I may be able to implement this feature request
- [ ] This feature might incur a breaking change

### CDK version used

2.93.0

### Environment details (OS name and version, etc.)

Not relevant, but macOs 13.4 (Ventura) 😄

Contributor guide

Open the contributing guide

Research direction

Start at the Annotations entry point and compare the existing addWarning and addWarningV2 APIs. Done means a non-deprecated addUnacknowledgeableWarning method mirrors addWarning, includes the proposed disclaimer, and preserves the stated acknowledgment behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
cloud, infrastructure
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.