fix(cli): remove legacy stack tags behavior (#1053)

Here's some nasty legacy around Stack Tags: 

* Before CDK v1.33.0, we wrote stack tags to the construct metadata, and
they were written as a `{ Key, Value }` object that got directly passed
into the CloudFormation CreateChangeSet API.
* In CDK v1.33.0 (nearly 6 years ago), we formalized the cloud assembly
schema into a jsii package, so we had to type the tag object as `{ key,
value }` in memory (otherwise jsii would yell at us). For backwards
compatibility with the on-disk format and the (extracted) JSONSchema
Validation, we used to re-case the identifiers: just before they were
written to disk we would change `key → Key` and just after reading from
disk but before validation and we would change `Key → key`.
* In CDK v1.66.0 (5 and a half years ago), we added a `tags` property to
the CloudFormation Stack artifact in the CDK manifest, and the CLI and
other tools stopped using the stack metadata to read stack tags.

Since this is to support end-of-life CDKs, in this PR I'm taking some of
that complexity out; we stop converting `key → Key` when writing a cloud
assembly manifest -- but we do still have to convert `Key → key` in
order to pass JSONSchema validation as we read the manifest. Tags are
exclusively read from the artifact properties, there will be no fallback
to the metadata anymore.

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

---------

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: rix0rrr

.gitignore

@@ -293,6 +293,8 @@ new AdcPublishing(repoProject);
 new RecordPublishingTimestamp(repoProject);
 new BootstrapTemplateProtection(repoProject);
 
+repoProject.gitignore.addPatterns('.vscode/settings.json');
+
 // Eslint for projen config
 // @ts-ignore
 repoProject.eslint = new pj.javascript.Eslint(repoProject, {

.projenrc.ts

@@ -0,0 +1,3 @@
+{
+  "eslint.useFlatConfig": false,
+}

.vscode/settings.example.json

@@ -175,7 +175,7 @@ export class CloudFormationStackArtifact extends CloudArtifact {
 
     // We get the tags from 'properties' if available (cloud assembly format >= 6.0.0), otherwise
     // from the stack metadata
-    this.tags = properties.tags ?? this.tagsFromMetadata();
+    this.tags = properties.tags ?? {};
     this.notificationArns = properties.notificationArns;
     this.assumeRoleArn = properties.assumeRoleArn;
     this.assumeRoleExternalId = properties.assumeRoleExternalId;
@@ -215,16 +215,6 @@ export class CloudFormationStackArtifact extends CloudArtifact {
     }
     return this._template;
   }
-
-  private tagsFromMetadata() {
-    const ret: Record<string, string> = {};
-    for (const metadataEntry of this.findMetadataByType(cxschema.ArtifactMetadataEntryType.STACK_TAGS)) {
-      for (const tag of (metadataEntry.data ?? []) as cxschema.StackTagsMetadataEntry) {
-        ret[tag.key] = tag.value;
-      }
-    }
-    return ret;
-  }
 }
 
 /**

packages/@aws-cdk/cloud-assembly-api/lib/artifacts/cloudformation-artifact.ts

@@ -58,40 +58,6 @@ test('read tags from artifact properties', () => {
   expect(assembly.getStackByName('Stack').tags).toEqual({ foo: 'bar' });
 });
 
-test('stack tags get uppercased when written to Cloud Assembly', () => {
-  // Backwards compatibility test
-  // GIVEN
-  builder.addArtifact('Stack', {
-    ...stackBase,
-    metadata: {
-      '/Stack': [
-        {
-          type: 'aws:cdk:stack-tags',
-          data: [{ key: 'foo', value: 'bar' }],
-        },
-      ],
-    },
-  });
-
-  // WHEN
-  const assembly = builder.buildAssembly();
-
-  // THEN
-  const manifestStructure = JSON.parse(fs.readFileSync(path.join(assembly.directory, 'manifest.json'), { encoding: 'utf-8' }));
-  expect(manifestStructure.artifacts.Stack.metadata['/Stack']).toEqual([
-    {
-      type: 'aws:cdk:stack-tags',
-      data: [
-        {
-          // Note: uppercase due to historical accident
-          Key: 'foo',
-          Value: 'bar',
-        },
-      ],
-    },
-  ]);
-});
-
 test('already uppercased stack tags get left alone', () => {
   // Backwards compatibility test
   // GIVEN
@@ -126,7 +92,7 @@ test('already uppercased stack tags get left alone', () => {
   ]);
 });
 
-test('read tags from stack metadata', () => {
+test('tags are NO LONGER read from stack metadata', () => {
   // Backwards compatibility test
   // GIVEN
   builder.addArtifact('Stack', {
@@ -145,7 +111,7 @@ test('read tags from stack metadata', () => {
   const assembly = builder.buildAssembly();
 
   // THEN
-  expect(assembly.getStackByName('Stack').tags).toEqual({ foo: 'bar' });
+  expect(assembly.getStackByName('Stack').tags).toEqual({});
 });
 
 test('user friendly id is the assembly display name', () => {

packages/@aws-cdk/cloud-assembly-api/test/stack-artifact.test.ts

@@ -61,6 +61,12 @@ export interface AwsCloudFormationStackProperties {
   /**
    * Values for CloudFormation stack tags that should be passed when the stack is deployed.
    *
+   * N.B.: Tags are also written to stack metadata, under the path of the Stack
+   * construct. Only in CDK CLI v1 are those tags found in metadata used for
+   * actual deployments; in all stable versions of CDK only the stack tags
+   * directly found in the `tags` property of `AwsCloudFormationStack` artifact
+   * (i.e., this property) are used.
+   *
    * @default - No tags
    */
   readonly tags?: { [id: string]: string };

packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/artifact-schema.ts

@@ -72,15 +72,15 @@ export interface LoadManifestOptions {
 /**
  * Protocol utility class.
  */
-export class Manifest {
+export abstract class Manifest {
   /**
    * Validates and saves the cloud assembly manifest to file.
    *
    * @param manifest - manifest.
    * @param filePath - output file path.
    */
   public static saveAssemblyManifest(manifest: assembly.AssemblyManifest, filePath: string) {
-    Manifest.saveManifest(manifest, filePath, ASSEMBLY_SCHEMA, Manifest.patchStackTagsOnWrite);
+    Manifest.saveManifest(manifest, filePath, ASSEMBLY_SCHEMA);
   }
 
   /**
@@ -102,7 +102,7 @@ export class Manifest {
    * @param filePath - output file path.
    */
   public static saveAssetManifest(manifest: assets.AssetManifest, filePath: string) {
-    Manifest.saveManifest(manifest, filePath, ASSETS_SCHEMA, Manifest.patchStackTagsOnRead);
+    Manifest.saveManifest(manifest, filePath, ASSETS_SCHEMA);
   }
 
   /**
@@ -111,7 +111,7 @@ export class Manifest {
    * @param filePath - path to the manifest file.
    */
   public static loadAssetManifest(filePath: string): assets.AssetManifest {
-    return this.loadManifest(filePath, ASSETS_SCHEMA);
+    return Manifest.loadManifest(filePath, ASSETS_SCHEMA);
   }
 
   /**
@@ -266,27 +266,56 @@ export class Manifest {
   }
 
   /**
-   * This requires some explaining...
+   * Fix the casing of stack tags entries
    *
-   * We previously used `{ Key, Value }` for the object that represents a stack tag. (Notice the casing)
-   * @link https://github.com/aws/aws-cdk/blob/v1.27.0/packages/aws-cdk/lib/api/cxapp/stacks.ts#L427.
+   * At the very beginning of the CDK we used to emit stack tags as an object with
+   * `{ Key, Value }` keys; this had the "advantage" that we could stick those
+   * tags directly into the `CreateChangeSet` call.
    *
-   * When that object moved to this package, it had to be JSII compliant, which meant the property
-   * names must be `camelCased`, and not `PascalCased`. This meant it no longer matches the structure in the `manifest.json` file.
-   * In order to support current manifest files, we have to translate the `PascalCased` representation to the new `camelCased` one.
+   * Then we later on used jsii on the assembly schema and we were forced to type
+   * the in-memory objects as `{ key, value }` with lowercase letters. Now the
+   * objects have a different on-disk and in-memory format, and we need to convert
+   * between them.
    *
-   * Note that the serialization itself still writes `PascalCased` because it relates to how CloudFormation expects it.
+   * For backwards compatibility reasons, we used to convert lowercase in-memory
+   * to uppercase on-disk variant until very recently. This is now unnecessary,
+   * since no officially supported CDK tools read the stack tags from the
+   * metadata; the CLI and toolkit library read stack tags from the artifact
+   * properties.
    *
-   * Ideally, we would start writing the `camelCased` and translate to how CloudFormation expects it when needed. But this requires nasty
-   * backwards-compatibility code and it just doesn't seem to be worth the effort.
+   * So although we don't emit uppercase stack tag objects anymore, we might still read
+   * manifests that have them. Because the manifest we read must pass JSON Schema
+   * validation (which expects lowercase tag objects), we have to fix the casing
+   * of these objects after reading from disk and before validating.
+   *
+   * That's what this function does.
    */
   private static patchStackTagsOnRead(this: void, manifest: assembly.AssemblyManifest) {
-    return Manifest.replaceStackTags(manifest, (tags) =>
-      tags.map((diskTag: any) => ({
-        key: diskTag.Key,
-        value: diskTag.Value,
-      })),
-    );
+    const artifacts = Object.values(manifest.artifacts ?? {})
+      .filter(artifact => artifact.type === assembly.ArtifactType.AWS_CLOUDFORMATION_STACK);
+
+    for (const artifact of artifacts) {
+      const tagMetadata = Object.values(artifact.metadata ?? {})
+        .flatMap(x => x)
+        .filter(entry => entry.type === assembly.ArtifactMetadataEntryType.STACK_TAGS);
+
+      for (const entry of tagMetadata) {
+        const tags = entry.data as unknown as assembly.StackTagsMetadataEntry[] | undefined;
+        for (const tag of tags ?? []) {
+          const t: any = tag;
+          if ('Key' in t) {
+            t.key = t.Key;
+            delete t.Key;
+          }
+          if ('Value' in t) {
+            t.value = t.Value;
+            delete t.Value;
+          }
+        }
+      }
+    }
+
+    return manifest;
   }
 
   /**
@@ -317,86 +346,6 @@ export class Manifest {
       throw new Error(`ExternalId is not allowed inside '${key}'`);
     }
   }
-
-  /**
-   * See explanation on `patchStackTagsOnRead`
-   *
-   * Translate stack tags metadata if it has the "right" casing.
-   */
-  private static patchStackTagsOnWrite(this: void, manifest: assembly.AssemblyManifest) {
-    return Manifest.replaceStackTags(manifest, (tags) =>
-      tags.map(
-        (memTag) =>
-          // Might already be uppercased (because stack synthesis generates it in final form yet)
-          ('Key' in memTag ? memTag : { Key: memTag.key, Value: memTag.value }) as any,
-      ),
-    );
-  }
-
-  /**
-   * Recursively replace stack tags in the stack metadata
-   */
-  private static replaceStackTags(
-    manifest: assembly.AssemblyManifest,
-    fn: Endofunctor<assembly.StackTagsMetadataEntry>,
-  ): assembly.AssemblyManifest {
-    // Need to add in the `noUndefined`s because otherwise jest snapshot tests are going to freak out
-    // about the keys with values that are `undefined` (even though they would never be JSON.stringified)
-    return noUndefined({
-      ...manifest,
-      artifacts: mapValues(manifest.artifacts, (artifact) => {
-        if (artifact.type !== assembly.ArtifactType.AWS_CLOUDFORMATION_STACK) {
-          return artifact;
-        }
-        return noUndefined({
-          ...artifact,
-          metadata: mapValues(artifact.metadata, (metadataEntries) =>
-            metadataEntries.map((metadataEntry) => {
-              if (
-                metadataEntry.type !== assembly.ArtifactMetadataEntryType.STACK_TAGS ||
-                !metadataEntry.data
-              ) {
-                return metadataEntry;
-              }
-              return {
-                ...metadataEntry,
-                data: fn(metadataEntry.data as assembly.StackTagsMetadataEntry),
-              };
-            }),
-          ),
-        } as assembly.ArtifactManifest);
-      }),
-    });
-  }
-
-  private constructor() {
-  }
-}
-
-type Endofunctor<A> = (x: A) => A;
-
-function mapValues<A, B>(
-  xs: Record<string, A> | undefined,
-  fn: (x: A) => B,
-): Record<string, B> | undefined {
-  if (!xs) {
-    return undefined;
-  }
-  const ret: Record<string, B> | undefined = {};
-  for (const [k, v] of Object.entries(xs)) {
-    ret[k] = fn(v);
-  }
-  return ret;
-}
-
-function noUndefined<A extends object>(xs: A): A {
-  const ret: any = {};
-  for (const [k, v] of Object.entries(xs)) {
-    if (v !== undefined) {
-      ret[k] = v;
-    }
-  }
-  return ret;
 }
 
 function stripEnumErrors(errors: jsonschema.ValidationError[]) {

packages/@aws-cdk/cloud-assembly-schema/lib/manifest.ts

@@ -351,7 +351,7 @@
                     }
                 },
                 "tags": {
-                    "description": "Values for CloudFormation stack tags that should be passed when the stack is deployed. (Default - No tags)",
+                    "description": "Values for CloudFormation stack tags that should be passed when the stack is deployed.\n\nN.B.: Tags are also written to stack metadata, under the path of the Stack\nconstruct. Only in CDK CLI v1 are those tags found in metadata used for\nactual deployments; in all stable versions of CDK only the stack tags\ndirectly found in the `tags` property of `AwsCloudFormationStack` artifact\n(i.e., this property) are used. (Default - No tags)",
                     "type": "object",
                     "additionalProperties": {
                         "type": "string"

packages/@aws-cdk/cloud-assembly-schema/schema/cloud-assembly.schema.json

@@ -1,5 +1,5 @@
 {
-  "schemaHash": "c0092c55280aa03c568e1d591f2034783597e942a400d1d58d7f05a8215f51f4",
+  "schemaHash": "1b06659a117c44714e2e52854571bb1b45b765b277bb1c208bc4b7ea01f6a684",
   "$comment": "Do not hold back the version on additions: jsonschema validation of the manifest by the consumer will trigger errors on unexpected fields.",
   "revision": 50
 }

packages/@aws-cdk/cloud-assembly-schema/schema/version.json

@@ -496,6 +496,7 @@ const MOCK_STACK_A: TestStackArtifact = {
     '/Test-Stack-A': [
       {
         type: ArtifactMetadataEntryType.STACK_TAGS,
+        data: [],
       },
     ],
   },
@@ -508,6 +509,7 @@ const MOCK_STACK_B: TestStackArtifact = {
     '/Test-Stack-B': [
       {
         type: ArtifactMetadataEntryType.STACK_TAGS,
+        data: [],
       },
     ],
   },
@@ -530,6 +532,7 @@ const MOCK_STACK_C: TestStackArtifact = {
     '/Test-Stack-C': [
       {
         type: ArtifactMetadataEntryType.STACK_TAGS,
+        data: [],
       },
     ],
   },