fix(toolkit-lib)!: diff message payloads are incomplete (#546)

Fixes #494

The messages emitted as part of diff were incomplete when multiple
stacks are diffed. Previously the code would overwrite the formatted
output with the last processed stack. We are also removing the already
deprecated `securityOnly` option.

To solve this properly, the diff messages have been restructured: For
each stack we emit a new message with code `CDK_TOOLKIT_I4002` that
contains all formatted diffs for the stack (including any nested
stacks).

The collated result message `CDK_TOOLKIT_I4001` does not contain
formatted diffs anymore, but instead has a count for stacks with changes
and the structured diff as returned by the action.

BREAKING CHANGE: The data emitted by message `CDK_TOOLKIT_I4001` has
changed. Instead of formatted diffs, it now contains a stack count and
the template diff as returned by the action. The formatted output is now
available on a new message `CDK_TOOLKIT_I4002`. Deprecated option
`securityOnly` removed from `DiffOptions` used by `Toolkit.diff()`.
Instead the action always prints and returns a security diff. Use a
custom `IoHost` implementation to customize the exact diff output.

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

Author: mrgrain

packages/@aws-cdk/toolkit-lib/docs/message-registry.md

@@ -71,7 +71,8 @@ Please let us know by [opening an issue](https://github.com/aws/aws-cdk-cli/issu
 | `CDK_TOOLKIT_I3110` | Additional information is needed to identify a resource | `info` | {@link ResourceIdentificationRequest} |
 | `CDK_TOOLKIT_E3900` | Resource import failed | `error` | {@link ErrorPayload} |
 | `CDK_TOOLKIT_I4000` | Diff stacks is starting | `trace` | {@link StackSelectionDetails} |
-| `CDK_TOOLKIT_I4001` | Output of the diff command | `info` | {@link DiffResult} |
+| `CDK_TOOLKIT_I4001` | Output of the diff command | `result` | {@link DiffResult} |
+| `CDK_TOOLKIT_I4002` | The diff for a single stack | `result` | {@link StackDiff} |
 | `CDK_TOOLKIT_I4590` | Results of the drift command | `result` | {@link DriftResultPayload} |
 | `CDK_TOOLKIT_I4591` | Missing drift result fort a stack. | `warn` | {@link SingleStack} |
 | `CDK_TOOLKIT_I5000` | Provides deployment times | `info` | {@link Duration} |

packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts

@@ -89,7 +89,7 @@ export class DiffMethod {
 }
 
 /**
- * Optins for the diff method
+ * Options for the diff method
  */
 export interface DiffOptions {
   /**
@@ -129,13 +129,4 @@ export interface DiffOptions {
    * @default 3
    */
   readonly contextLines?: number;
-
-  /**
-   * Only include broadened security changes in the diff
-   *
-   * @default false
-   *
-   * @deprecated implement in IoHost
-   */
-  readonly securityOnly?: boolean;
 }

packages/@aws-cdk/toolkit-lib/lib/api/diff/diff-formatter.ts

@@ -71,7 +71,7 @@ interface FormatStackDiffOptions {
    *
    * @default 3
    */
-  readonly context?: number;
+  readonly contextLines?: number;
 
   /**
    * silences \'There were no differences\' messages
@@ -157,7 +157,7 @@ export class DiffFormatter {
   /**
    * Get or creates the diff of a stack.
    * If it creates the diff, it stores the result in a map for
-   * easier retreval later.
+   * easier retrieval later.
    */
   private diff(stackName?: string, oldTemplate?: any) {
     const realStackName = stackName ?? this.stackName;
@@ -178,8 +178,8 @@ export class DiffFormatter {
    *
    * If no stackName is given, then the root stack name is used.
    */
-  private permissionType(stackName?: string): PermissionChangeType {
-    const diff = this.diff(stackName);
+  private permissionType(): PermissionChangeType {
+    const diff = this.diff();
 
     if (diff.permissionsBroadened) {
       return PermissionChangeType.BROADENING;
@@ -211,7 +211,7 @@ export class DiffFormatter {
     let diff = this.diff(stackName, oldTemplate);
 
     // The stack diff is formatted via `Formatter`, which takes in a stream
-    // and sends its output directly to that stream. To faciliate use of the
+    // and sends its output directly to that stream. To facilitate use of the
     // global CliIoHost, we create our own stream to capture the output of
     // `Formatter` and return the output as a string for the consumer of
     // `formatStackDiff` to decide what to do with it.
@@ -253,7 +253,7 @@ export class DiffFormatter {
         formatDifferences(stream, diff, {
           ...logicalIdMapFromTemplate(this.oldTemplate),
           ...buildLogicalToPathMap(this.newTemplate),
-        }, options.context);
+        }, options.contextLines);
       } else if (!options.quiet) {
         stream.write(chalk.green('There were no differences\n'));
       }

packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts

@@ -1,7 +1,7 @@
 import type * as cxapi from '@aws-cdk/cx-api';
 import * as make from './message-maker';
 import type { SpanDefinition } from './span';
-import type { DiffResult } from '../../../payloads';
+import type { StackDiff, DiffResult } from '../../../payloads';
 import type { BootstrapEnvironmentProgress } from '../../../payloads/bootstrap-environment-progress';
 import type { MissingContext, UpdatedContext } from '../../../payloads/context';
 import type { BuildAsset, DeployConfirmationRequest, PublishAsset, StackDeployProgress, SuccessfulDeployStackResult } from '../../../payloads/deploy';
@@ -85,11 +85,16 @@ export const IO = {
     description: 'Diff stacks is starting',
     interface: 'StackSelectionDetails',
   }),
-  CDK_TOOLKIT_I4001: make.info<DiffResult>({
+  CDK_TOOLKIT_I4001: make.result<DiffResult>({
     code: 'CDK_TOOLKIT_I4001',
     description: 'Output of the diff command',
     interface: 'DiffResult',
   }),
+  CDK_TOOLKIT_I4002: make.result<StackDiff>({
+    code: 'CDK_TOOLKIT_I4002',
+    description: 'The diff for a single stack',
+    interface: 'StackDiff',
+  }),
 
   // 4: Drift (45xx - 49xx)
   CDK_TOOLKIT_I4590: make.result<DriftResultPayload>({

packages/@aws-cdk/toolkit-lib/lib/payloads/diff.ts

@@ -1,4 +1,5 @@
-import type { Duration } from './types';
+import type { ITemplateDiff } from '@aws-cdk/cloudformation-diff';
+import type { Duration, SingleStack } from './types';
 
 /**
  * Different types of permission related changes in a diff
@@ -20,17 +21,57 @@ export enum PermissionChangeType {
   NON_BROADENING = 'non-broadening',
 }
 
+/**
+ * The diff formatted as different types of output
+ */
+export interface FormattedDiff {
+  /**
+   * The stack diff formatted as a string
+   */
+  readonly diff: string;
+  /**
+   * The security diff formatted as a string, if any
+   */
+  readonly security?: string;
+}
+
+/**
+ * Diff information for a single stack
+ */
+export interface StackDiff extends SingleStack {
+  /**
+   * Total number of stacks that have changes
+   * Can be higher than `1` if the stack has nested stacks.
+   */
+  readonly numStacksWithChanges: number;
+
+  /**
+   * Structural diff of the stack
+   * Can include more than a single diff if the stack has nested stacks.
+   */
+  readonly diffs: { [name: string]: ITemplateDiff };
+
+  /**
+   * The formatted diff
+   */
+  readonly formattedDiff: FormattedDiff;
+
+  /**
+   * Does the diff contain changes to permissions and what kind
+   */
+  readonly permissionChanges: PermissionChangeType;
+}
+
 /**
  * Output of the diff command
  */
 export interface DiffResult extends Duration {
   /**
-   * Stack diff formatted as a string
+   * Total number of stacks that have changes
    */
-  readonly formattedStackDiff: string;
-
+  readonly numStacksWithChanges: number;
   /**
-   * Security diff formatted as a string
+   * Structural diff of all selected stacks
    */
-  readonly formattedSecurityDiff: string;
+  readonly diffs: { [name: string]: ITemplateDiff };
 }

packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

@@ -339,39 +339,41 @@ export class Toolkit extends CloudAssemblySourceBuilder {
     const contextLines = options.contextLines || 3;
 
     let diffs = 0;
-    let formattedSecurityDiff = '';
-    let formattedStackDiff = '';
 
     const templateInfos = await prepareDiff(ioHelper, stacks, deployments, await this.sdkProvider('diff'), options);
     const templateDiffs: { [name: string]: TemplateDiff } = {};
     for (const templateInfo of templateInfos) {
-      const formatter = new DiffFormatter({
-        templateInfo,
-      });
+      const formatter = new DiffFormatter({ templateInfo });
+      const stackDiff = formatter.formatStackDiff({ strict, contextLines });
 
-      if (options.securityOnly) {
-        const securityDiff = formatter.formatSecurityDiff();
-        // In Diff, we only care about BROADENING security diffs
-        if (securityDiff.permissionChangeType == PermissionChangeType.BROADENING) {
-          const warningMessage = 'This deployment will make potentially sensitive changes according to your current security approval level.\nPlease confirm you intend to make the following modifications:\n';
-          await ioHelper.defaults.warn(warningMessage);
-          formattedSecurityDiff = securityDiff.formattedDiff;
-          diffs = securityDiff.formattedDiff ? diffs + 1 : diffs;
-        }
-      } else {
-        const diff = formatter.formatStackDiff({
-          strict,
-          context: contextLines,
-        });
-        formattedStackDiff = diff.formattedDiff;
-        diffs = diff.numStacksWithChanges;
+      // Security Diff
+      const securityDiff = formatter.formatSecurityDiff();
+      const formattedSecurityDiff = securityDiff.permissionChangeType !== PermissionChangeType.NONE ? stackDiff.formattedDiff : undefined;
+      // We only warn about BROADENING changes
+      if (securityDiff.permissionChangeType == PermissionChangeType.BROADENING) {
+        const warningMessage = 'This deployment will make potentially sensitive changes according to your current security approval level.\nPlease confirm you intend to make the following modifications:\n';
+        await diffSpan.notifyDefault('warn', warningMessage);
+        await diffSpan.notifyDefault('info', securityDiff.formattedDiff);
       }
+
+      // Stack Diff
+      diffs += stackDiff.numStacksWithChanges;
       appendObject(templateDiffs, formatter.diffs);
+      await diffSpan.notify(IO.CDK_TOOLKIT_I4002.msg(stackDiff.formattedDiff, {
+        stack: templateInfo.newTemplate,
+        diffs: formatter.diffs,
+        numStacksWithChanges: stackDiff.numStacksWithChanges,
+        permissionChanges: securityDiff.permissionChangeType,
+        formattedDiff: {
+          diff: stackDiff.formattedDiff,
+          security: formattedSecurityDiff,
+        },
+      }));
     }
 
     await diffSpan.end(`✨ Number of stacks with differences: ${diffs}`, {
-      formattedSecurityDiff,
-      formattedStackDiff,
+      numStacksWithChanges: diffs,
+      diffs: templateDiffs,
     });
 
     return templateDiffs;

packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts

@@ -51,11 +51,14 @@ describe('diff', () => {
     // THEN
     expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
       action: 'diff',
-      level: 'info',
+      level: 'result',
       code: 'CDK_TOOLKIT_I4001',
       message: expect.stringContaining('✨ Number of stacks with differences: 1'),
       data: expect.objectContaining({
-        formattedStackDiff: expect.stringContaining((chalk.bold('Stack1'))),
+        numStacksWithChanges: 1,
+        diffs: expect.objectContaining({
+          Stack1: expect.anything(),
+        }),
       }),
     }));
   });
@@ -131,12 +134,11 @@ describe('diff', () => {
     }));
   });
 
-  test('only security diff', async () => {
+  test('security diff', async () => {
     // WHEN
     const cx = await builderFixture(toolkit, 'stack-with-role');
     const result = await toolkit.diff(cx, {
       stacks: { strategy: StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE, patterns: ['Stack1'] },
-      securityOnly: true,
       method: DiffMethod.TemplateOnly({ compareAgainstProcessedTemplate: true }),
     });
 
@@ -148,11 +150,12 @@ describe('diff', () => {
     }));
     expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
       action: 'diff',
-      level: 'info',
-      code: 'CDK_TOOLKIT_I4001',
-      message: expect.stringContaining('✨ Number of stacks with differences: 1'),
+      level: 'result',
+      code: 'CDK_TOOLKIT_I4002',
       data: expect.objectContaining({
-        formattedSecurityDiff: expect.stringContaining((chalk.underline(chalk.bold('IAM Statement Changes')))),
+        formattedDiff: expect.objectContaining({
+          security: expect.stringContaining((chalk.underline(chalk.bold('IAM Statement Changes')))),
+        }),
       }),
     }));
 
@@ -182,17 +185,17 @@ describe('diff', () => {
     const cx = await builderFixture(toolkit, 'two-empty-stacks');
     await toolkit.diff(cx, {
       stacks: { strategy: StackSelectionStrategy.ALL_STACKS },
-      securityOnly: true,
     });
 
     // THEN
     expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
       action: 'diff',
-      level: 'info',
-      code: 'CDK_TOOLKIT_I4001',
-      message: expect.stringContaining('✨ Number of stacks with differences: 0'),
+      level: 'result',
+      code: 'CDK_TOOLKIT_I4002',
       data: expect.objectContaining({
-        formattedSecurityDiff: '',
+        formattedDiff: expect.objectContaining({
+          security: undefined,
+        }),
       }),
     }));
   });
@@ -207,19 +210,13 @@ describe('diff', () => {
 
     // THEN
     expect(ioHost.notifySpy).not.toHaveBeenCalledWith(expect.objectContaining({
-      action: 'diff',
-      level: 'info',
-      code: 'CDK_TOOLKIT_I0000',
       message: expect.stringContaining('Could not create a change set'),
     }));
     expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
       action: 'diff',
-      level: 'info',
+      level: 'result',
       code: 'CDK_TOOLKIT_I4001',
       message: expect.stringContaining('✨ Number of stacks with differences: 1'),
-      data: expect.objectContaining({
-        formattedStackDiff: expect.stringContaining(chalk.bold('Stack1')),
-      }),
     }));
   });
 
@@ -344,7 +341,6 @@ describe('diff', () => {
       const cx = await builderFixture(toolkit, 'stack-with-role');
       const result = await toolkit.diff(cx, {
         stacks: { strategy: StackSelectionStrategy.ALL_STACKS },
-        securityOnly: true,
         method: DiffMethod.LocalFile(path.join(__dirname, '..', '_fixtures', 'two-empty-stacks', 'cdk.out', 'Stack1.template.json')),
       });
 

packages/aws-cdk/lib/cli/cdk-toolkit.ts

@@ -241,7 +241,7 @@ export class CdkToolkit {
       } else {
         const diff = formatter.formatStackDiff({
           strict,
-          context: contextLines,
+          contextLines,
           quiet,
         });
         diffs = diff.numStacksWithChanges;
@@ -325,7 +325,7 @@ export class CdkToolkit {
         } else {
           const diff = formatter.formatStackDiff({
             strict,
-            context: contextLines,
+            contextLines,
             quiet,
           });
           info(diff.formattedDiff);