fix(aws-cdk-lib): validation namespaces for annotations are inconsistent (#38256)

Validation rules are always prefixed with their plugin name, and Annotations are converted to Validations with a plugin named `"Construct Annotations"`.

This means that added via `Annotations.of(...).addWarningV2` would get reported with a suppression string of `Construct-Annotations::<warning-id>`.

It's a bit ugly because the `Construct-Annotations` string is quite large, but so far so good.

However, `Validations.of(...).addWarning()` would require its IDs to be namespaced, and add a `annotations::` prefix in front if the ID was not namespaced yet. It would then use the Annotations system to publish that marker.

The end result is that warnings added via `Validations.addWarning` would get reported as

```
Construct-Annotations::annotation::<warning-id>
```

This PR is taking the opportunity to polish that up a little bit before we fully release this new feature:

- The *plugin name* needs to remain `"Construct Annotations"`, because the CLI will look for the annotations report in the output by its plugin name
- But we added an metadata field so that the CLI can use structured information in the future to find this report.
- We add the convention that if a rule name is already namespaced `<prefix>::<rule-name>`, then that embedded prefix takes precedence over the plugin name in acknowledgements.
- Rename `annotation::` to `Annotation::` to be more consistent in casing with other plugins which all use camelcased names (keep supporting suppressions under the lowercased name).
- We effectively renamed all suppression keys for construct annotations, so we continue to support both `Construct-Annotations::<rule>` and `Annotation::<rule>` as a suppression key for annotations.

Also:

* Move `collectAnnotationReport` to the same file as the `AnnotationPlugin` that it is doing its work for.
 
----

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

Author: rix0rrr

packages/aws-cdk-lib/core/lib/private/annotation-plugin.ts

@@ -1,17 +1,123 @@
+import * as path from 'path';
+import type { IConstruct } from 'constructs';
 import type { IPolicyValidationPlugin, IPolicyValidationContext } from '../validation';
+import { iterateDfsPreorder } from './construct-iteration';
+import * as cxschema from '../../../cloud-assembly-schema';
+import { Stack } from '../stack';
 import type { NamedValidationPluginReport } from '../validation/private/report';
-import type { PolicyValidationPluginReport } from '../validation/report';
+import type { PolicyValidationPluginReport, PolicyViolation, PolicyViolatingResource } from '../validation/report';
 
 /**
  * Wraps the annotation collection logic as an IPolicyValidationPlugin
  * so it can be run through the same unified plugin loop.
+ *
+ * Because the released version of this logic had a version where this plugin
+ * was called 'Construct Annotations', and the CLI looked for the report issued
+ * by this plugin by name, we need to keep the name stable for backwards
+ * compatibility.
+ *
+ * However, the display title and acknowledgement title of annotations issued
+ * by this plugin is now `Annotation::<rule-name>`, *unless* the annotation ID already
+ * has a prefix in which case the prefix is preserved.
  */
 export class AnnotationPlugin implements IPolicyValidationPlugin {
-  public readonly name = 'Construct Annotations';
+  public static RULE_PREFIX = 'Annotation';
+  public static NAME = 'Construct Annotations';
+  public readonly name = AnnotationPlugin.NAME;
 
   constructor(private readonly report: NamedValidationPluginReport) {}
 
   public validate(_context: IPolicyValidationContext): PolicyValidationPluginReport {
     return this.report;
   }
 }
+
+/**
+ * Collect annotation metadata (warnings and errors) from the construct tree
+ * and convert them into a NamedValidationPluginReport that can be merged
+ * into the same report pipeline as plugin violations.
+ */
+export function collectAnnotationReport(root: IConstruct, outdir: string): IPolicyValidationPlugin | undefined {
+  const violationMap = new Map<string, PolicyViolation & { violatingResources: PolicyViolatingResource[] }>();
+
+  for (const construct of iterateDfsPreorder(root)) {
+    for (const entry of construct.node.metadata) {
+      if (entry.type !== cxschema.ArtifactMetadataEntryType.WARN && entry.type !== cxschema.ArtifactMetadataEntryType.ERROR) {
+        continue;
+      }
+
+      const severity = entry.type === cxschema.ArtifactMetadataEntryType.ERROR ? 'error' : 'warning';
+      let { message, ruleName } = splitDescriptionAndId(String(entry.data));
+
+      if (ruleName && !ruleName.includes('::')) {
+        ruleName = `${AnnotationPlugin.RULE_PREFIX}::${ruleName}`;
+      }
+
+      let templatePath: string | undefined;
+      try {
+        templatePath = path.join(outdir, Stack.of(construct).templateFile);
+      } catch {
+        // Construct is not inside a Stack
+      }
+
+      const violatingResource: PolicyViolatingResource = {
+        constructPath: construct.node.path,
+        templatePath,
+        locations: [],
+      };
+
+      const key = `${ruleName}|${severity}|${message}`;
+      const existing = violationMap.get(key);
+      if (existing) {
+        existing.violatingResources.push(violatingResource);
+      } else {
+        violationMap.set(key, {
+          ruleName: ruleName ?? `${AnnotationPlugin.RULE_PREFIX}::${severity}-annotation`,
+          description: message,
+          severity,
+          violatingResources: [violatingResource],
+          ruleMetadata: {
+            'cdk:annotation': 'true',
+          },
+        });
+      }
+    }
+  }
+
+  const violations = Array.from(violationMap.values());
+  if (violations.length === 0) {
+    return undefined;
+  }
+
+  const hasErrors = violations.some(v => v.severity === 'error');
+  return new AnnotationPlugin({
+    pluginName: AnnotationPlugin.NAME,
+    success: !hasErrors,
+    violations,
+    metadata: {
+      'cdk:annotations': 'true',
+    },
+  });
+}
+
+/**
+ * Annotations have IDs in two places:
+ *
+ * - Warnings have `[ack:<id>]` in the message.
+ * - Errors have `(<namespace>::<id>)` in the message.
+ *
+ * Separate the rule name from the rest of the description.
+ */
+function splitDescriptionAndId(message: string): { message: string; ruleName?: string } {
+  const ackMatch = message.match(/\[ack: ([^\]]+)\]/);
+  if (ackMatch) {
+    return { message: message.replace(ackMatch[0], '').trim(), ruleName: ackMatch[1] };
+  }
+
+  const idMatch = message.match(/\(([^()]+::[^()]+)\)$/);
+  if (idMatch) {
+    return { message: message.replace(idMatch[0], '').trim(), ruleName: idMatch[1] };
+  }
+
+  return { message };
+}

packages/aws-cdk-lib/core/lib/private/collect-annotation-report.ts

@@ -3,9 +3,8 @@ import * as fs from 'fs';
 import * as path from 'path';
 import type * as private_cxapi from '@aws-cdk/cloud-assembly-api';
 import type { IConstruct } from 'constructs';
-import { AnnotationPlugin } from './annotation-plugin';
+import { AnnotationPlugin, collectAnnotationReport } from './annotation-plugin';
 import { collectAcknowledgedRuleIds } from './collect-acknowledged-rule-ids';
-import { collectAnnotationReport } from './collect-annotation-report';
 import { lit } from './literal-string';
 import * as cxapi from '../../../cx-api';
 import { _convertCloudAssemblyBuilder } from '../../../cx-api/lib/legacy-moved';
@@ -213,9 +212,9 @@ function pluginsToEvaluate(root: IConstruct, stage: Stage, stageAssembly: privat
 
   // 3. Construct annotations (as a plugin, only if there are annotations to report)
   if (FeatureFlags.of(root).isEnabled(cxapi.ANNOTATIONS_IN_VALIDATION_REPORT)) {
-    const annotationReport = collectAnnotationReport(stage, stageAssembly.directory);
-    if (annotationReport) {
-      ret.push(new AnnotationPlugin(annotationReport));
+    const annotationsPlugin = collectAnnotationReport(stage, stageAssembly.directory);
+    if (annotationsPlugin) {
+      ret.push(annotationsPlugin);
     }
   }
 
@@ -254,23 +253,39 @@ function collectSuppressions(root: App, reports: NamedValidationPluginReport[])
 
   if (acknowledgedRules.size > 0) {
     for (let i = 0; i < reports.length; i++) {
-      const pluginName = reports[i].pluginName.replace(/ /g, '-');
+      const pluginName = reports[i].pluginName;
       const active: typeof reports[0]['violations'] = [];
       const suppressed: SuppressedViolation[] = [];
       for (const v of reports[i].violations) {
         if (!isSuppressibleViolation(v)) {
           active.push(v);
           continue;
         }
-        const ruleId = `${pluginName}::${v.ruleName.replace(/ /g, '-')}`;
-        const ack = acknowledgedRules.get(ruleId);
+
+        const ackIds: string[] = [];
+        if (v.ruleName.includes('::')) {
+          ackIds.push(v.ruleName);
+
+          // Annotations are special; we renamed the suppression namespace at one point
+          // from "Construct-Annotations" to just "Annotation", and we also want
+          // to support the naked-rule-name form for backwards compatibility.
+          if (pluginName === AnnotationPlugin.NAME) {
+            const unnamespacedPart = v.ruleName.split('::').slice(1).join('::');
+            ackIds.push(`${pluginName}::${unnamespacedPart}`);
+          }
+        } else {
+          ackIds.push(`${pluginName}::${v.ruleName}`);
+        }
+
+        const ack = firstThat(ackIds.map(hyphenify), id => acknowledgedRules.get(id));
+
         if (ack) {
           suppressed.push({
             ...v,
-            acknowledgedId: ruleId,
-            reason: ack.reason,
-            acknowledgedAt: ack.constructPath,
-            acknowledgedStackTrace: ack.stackTrace,
+            acknowledgedId: ack.key,
+            reason: ack.value.reason,
+            acknowledgedAt: ack.value.constructPath,
+            acknowledgedStackTrace: ack.value.stackTrace,
           });
         } else {
           active.push(v);
@@ -507,3 +522,17 @@ function stripAnsi(x: string) {
   const re = new RegExp(pattern, 'g');
   return x.replaceAll(re, '');
 }
+
+function firstThat<A, B>(xs: A[], predicate: (x: A) => B | undefined): { key: A; value: B } | undefined {
+  for (const x of xs) {
+    const value = predicate(x);
+    if (value !== undefined) {
+      return { key: x, value };
+    }
+  }
+  return undefined;
+}
+
+function hyphenify(x: string) {
+  return x.replace(/ /g, '-');
+}

packages/aws-cdk-lib/core/lib/private/synthesis-validation.ts

@@ -66,7 +66,7 @@ function formatViolationBlock(fileRoot: string, v: FlattenedViolation): string {
   lines.push([
     Colorize.bold(getSeverityColor(v.severity)(sanitize(v.severity))),
     Colorize.bold(stripAckTag(sanitize(v.description))),
-    Colorize.grey(`(${sanitize(v.pluginName)})`),
+    Colorize.grey(`(${namespace(v)})`),
   ].join(' '));
 
   const constructInfo = formatConstructInfo(fileRoot, v.construct);
@@ -77,8 +77,7 @@ function formatViolationBlock(fileRoot: string, v: FlattenedViolation): string {
   }
 
   if (isSuppressibleViolation(v)) {
-    const ackId = `${sanitize(v.pluginName)}::${sanitize(v.ruleName)}`.replace(/ /g, '-');
-    lines.push(`   ${Colorize.grey(`Acknowledge with '${ackId}'`)}`);
+    lines.push(`   ${Colorize.grey(`Acknowledge with '${ackId(v)}'`)}`);
   } else {
     // If not acknowledgeable, we should still show the rule name for reference.
     lines.push(`   ${Colorize.grey(`Rule ${sanitize(v.ruleName)}`)}`);
@@ -174,3 +173,11 @@ function isPluginFailure(r: PluginReportJson): PluginError | undefined {
   }
   return { error: r.metadata.error };
 }
+
+function namespace(v: FlattenedViolation): string {
+  return v.ruleName.includes('::') ? sanitize(v.ruleName.split('::')[0]) : sanitize(v.pluginName);
+}
+
+function ackId(v: FlattenedViolation): string {
+  return (v.ruleName.includes('::') ? sanitize(v.ruleName) : `${sanitize(v.pluginName)}::${sanitize(v.ruleName)}`).replace(/ /g, '-');
+}

packages/aws-cdk-lib/core/lib/validation/private/modern-formatter.ts

@@ -2,6 +2,7 @@ import type { IConstruct } from 'constructs';
 import type { IPolicyValidationPlugin } from './validation';
 import { Annotations } from '../annotations';
 import { UnscopedValidationError } from '../errors';
+import { AnnotationPlugin } from '../private/annotation-plugin';
 import { lit } from '../private/literal-string';
 import { Stage } from '../stage';
 
@@ -47,16 +48,6 @@ export class Validations {
     return new Validations(scope);
   }
 
-  /**
-   * Well-known prefix for annotation-based validation rules.
-   *
-   * Every validation source identifies itself via a prefix so that
-   * `acknowledge()` can route suppressions to the correct handler.
-   * The `::` delimiter is reserved for separating the prefix from the
-   * rule name (e.g. `annotation::MyWarning`).
-   */
-  private static readonly ANNOTATION_PREFIX = 'annotation';
-
   private constructor(private readonly scope: IConstruct) {}
 
   /**
@@ -146,9 +137,16 @@ export class Validations {
     if (parts.length > 2 || (parts.length === 2 && parts[0].length === 0)) {
       throw new UnscopedValidationError(lit`InvalidValidationId`, `Invalid validation rule ID '${id}'. The '::' delimiter is reserved for separating the prefix from the rule name (e.g. 'prefix::RuleName').`);
     }
-    if (parts.length === 2) {
-      return id;
+
+    if (parts.length === 1) {
+      return `${AnnotationPlugin.RULE_PREFIX}::${id}`;
+    }
+
+    if (parts[0] === 'annotation') {
+      // Uppercase this
+      return `${AnnotationPlugin.RULE_PREFIX}::${parts[1]}`;
     }
-    return `${Validations.ANNOTATION_PREFIX}::${id}`;
+
+    return id;
   }
 }