feat(cloudformation-diff): use awscdk-service-spec as data source (#2…

…7813)

We already changed the datasource for generated resource to use `@aws-cdk/awscdk-service-spec`.
However `cloudformation-diff` was still using the old data source. 

This PR swaps out the source, with minimal further changes.
Hence no additional tests are required.

Note that this new implementation does not apply `temporary-schemas` defined in `spec2cdk`.
This is because `temporary-schemas` is a mechanism to unblock development of unreleased features, and not a mechanism to publish schema changes.
For the latter, changes must be made in [cdklabs/awscdk-service-spec](https://github.com/cdklabs/awscdk-service-spec).

I ran a local benchmark and `cdk diff` is now running ~1.03 slower than before.
I investigated why and this appears to be mostly due to the gzip compression of the database file.
If we load the DB from an uncompressed source, the new code path performs ~1.07 times faster than the old code path. This trade-off is acceptable.

Compressed package size of `aws-cdk` is reducing with this change from `11.7MB` to `8.9MB`

----

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

aws-cdk.code-workspace

@@ -10,6 +10,10 @@
         "name": "cli-lib-alpha",
         "rootPath": "packages/@aws-cdk/cli-lib-alpha"
       },
+      {
+        "name": "cloudformation-diff",
+        "rootPath": "packages/@aws-cdk/cloudformation-diff"
+      },
       {
         "name": "custom-resource-handlers",
         "rootPath": "packages/@aws-cdk/custom-resource-handlers"

packages/@aws-cdk/cloudformation-diff/jest.config.js

@@ -1,10 +1,18 @@
 const baseConfig = require('@aws-cdk/cdk-build-tools/config/jest.config');
+
+/** @type {import('ts-jest').JestConfigWithTsJest} */
 module.exports = {
-    ...baseConfig,
-    coverageThreshold: {
-        global: {
-            statements: 60,
-            branches: 55,
-        },
+  ...baseConfig,
+
+  // Different than usual
+  testMatch: [
+    '<rootDir>/**/test/**/?(*.)+(test).ts',
+  ],
+
+  coverageThreshold: {
+    global: {
+      branches: 60,
+      statements: 55,
     },
+  },
 };

packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts

@@ -1,6 +1,6 @@
-import * as cfnspec from '@aws-cdk/cfnspec';
+import { Resource } from '@aws-cdk/service-spec-types';
 import * as types from './types';
-import { deepEqual, diffKeyedEntities } from './util';
+import { deepEqual, diffKeyedEntities, loadResourceModel } from './util';
 
 export function diffAttribute(oldValue: any, newValue: any): types.Difference<string> {
   return new types.Difference<string>(_asString(oldValue), _asString(newValue));
@@ -36,8 +36,7 @@ export function diffResource(oldValue?: types.Resource, newValue?: types.Resourc
 
   if (resourceType.oldType !== undefined && resourceType.oldType === resourceType.newType) {
     // Only makes sense to inspect deeper if the types stayed the same
-    const typeSpec = cfnspec.filteredSpecification(resourceType.oldType);
-    const impl = typeSpec.ResourceTypes[resourceType.oldType];
+    const impl = loadResourceModel(resourceType.oldType);
     propertyDiffs = diffKeyedEntities(oldValue!.Properties,
       newValue!.Properties,
       (oldVal, newVal, key) => _diffProperty(oldVal, newVal, key, impl));
@@ -50,16 +49,16 @@ export function diffResource(oldValue?: types.Resource, newValue?: types.Resourc
     resourceType, propertyDiffs, otherDiffs,
   });
 
-  function _diffProperty(oldV: any, newV: any, key: string, resourceSpec?: cfnspec.schema.ResourceType) {
+  function _diffProperty(oldV: any, newV: any, key: string, resourceSpec?: Resource) {
     let changeImpact = types.ResourceImpact.NO_CHANGE;
 
-    const spec = resourceSpec && resourceSpec.Properties && resourceSpec.Properties[key];
+    const spec = resourceSpec?.properties?.[key];
     if (spec && !deepEqual(oldV, newV)) {
-      switch (spec.UpdateType) {
-        case cfnspec.schema.UpdateType.Immutable:
+      switch (spec.causesReplacement) {
+        case 'yes':
           changeImpact = types.ResourceImpact.WILL_REPLACE;
           break;
-        case cfnspec.schema.UpdateType.Conditional:
+        case 'maybe':
           changeImpact = types.ResourceImpact.MAY_REPLACE;
           break;
         default:

packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts

@@ -1,6 +1,6 @@
 import { AssertionError } from 'assert';
-import * as cfnspec from '@aws-cdk/cfnspec';
-import { deepEqual } from './util';
+import { PropertyScrutinyType, ResourceScrutinyType, Resource as ResourceModel } from '@aws-cdk/service-spec-types';
+import { deepEqual, loadResourceModel } from './util';
 import { IamChanges } from '../iam/iam-changes';
 import { SecurityGroupChanges } from '../network/security-group-changes';
 
@@ -55,10 +55,10 @@ export class TemplateDiff implements ITemplateDiff {
     });
 
     this.securityGroupChanges = new SecurityGroupChanges({
-      egressRulePropertyChanges: this.scrutinizablePropertyChanges([cfnspec.schema.PropertyScrutinyType.EgressRules]),
-      ingressRulePropertyChanges: this.scrutinizablePropertyChanges([cfnspec.schema.PropertyScrutinyType.IngressRules]),
-      egressRuleResourceChanges: this.scrutinizableResourceChanges([cfnspec.schema.ResourceScrutinyType.EgressRuleResource]),
-      ingressRuleResourceChanges: this.scrutinizableResourceChanges([cfnspec.schema.ResourceScrutinyType.IngressRuleResource]),
+      egressRulePropertyChanges: this.scrutinizablePropertyChanges([PropertyScrutinyType.EgressRules]),
+      ingressRulePropertyChanges: this.scrutinizablePropertyChanges([PropertyScrutinyType.IngressRules]),
+      egressRuleResourceChanges: this.scrutinizableResourceChanges([ResourceScrutinyType.EgressRuleResource]),
+      ingressRuleResourceChanges: this.scrutinizableResourceChanges([ResourceScrutinyType.IngressRuleResource]),
     });
   }
 
@@ -110,7 +110,7 @@ export class TemplateDiff implements ITemplateDiff {
    * We don't just look at property updates; we also look at resource additions and deletions (in which
    * case there is no further detail on property values), and resource type changes.
    */
-  private scrutinizablePropertyChanges(scrutinyTypes: cfnspec.schema.PropertyScrutinyType[]): PropertyChange[] {
+  private scrutinizablePropertyChanges(scrutinyTypes: PropertyScrutinyType[]): PropertyChange[] {
     const ret = new Array<PropertyChange>();
 
     for (const [resourceLogicalId, resourceChange] of Object.entries(this.resources.changes)) {
@@ -119,16 +119,23 @@ export class TemplateDiff implements ITemplateDiff {
         continue;
       }
 
-      const props = cfnspec.scrutinizablePropertyNames(resourceChange.newResourceType!, scrutinyTypes);
-      for (const propertyName of props) {
-        ret.push({
-          resourceLogicalId,
-          propertyName,
-          resourceType: resourceChange.resourceType,
-          scrutinyType: cfnspec.propertySpecification(resourceChange.resourceType, propertyName).ScrutinyType!,
-          oldValue: resourceChange.oldProperties && resourceChange.oldProperties[propertyName],
-          newValue: resourceChange.newProperties && resourceChange.newProperties[propertyName],
-        });
+      if (!resourceChange.newResourceType) {
+        continue;
+      }
+
+      const newTypeProps = loadResourceModel(resourceChange.newResourceType)?.properties || {};
+      for (const [propertyName, prop] of Object.entries(newTypeProps)) {
+        const propScrutinyType = prop.scrutinizable || PropertyScrutinyType.None;
+        if (scrutinyTypes.includes(propScrutinyType)) {
+          ret.push({
+            resourceLogicalId,
+            propertyName,
+            resourceType: resourceChange.resourceType,
+            scrutinyType: propScrutinyType,
+            oldValue: resourceChange.oldProperties?.[propertyName],
+            newValue: resourceChange.newProperties?.[propertyName],
+          });
+        }
       }
     }
 
@@ -141,11 +148,9 @@ export class TemplateDiff implements ITemplateDiff {
    * We don't just look at resource updates; we also look at resource additions and deletions (in which
    * case there is no further detail on property values), and resource type changes.
    */
-  private scrutinizableResourceChanges(scrutinyTypes: cfnspec.schema.ResourceScrutinyType[]): ResourceChange[] {
+  private scrutinizableResourceChanges(scrutinyTypes: ResourceScrutinyType[]): ResourceChange[] {
     const ret = new Array<ResourceChange>();
 
-    const scrutinizableTypes = new Set(cfnspec.scrutinizableResourceTypes(scrutinyTypes));
-
     for (const [resourceLogicalId, resourceChange] of Object.entries(this.resources.changes)) {
       if (!resourceChange) { continue; }
 
@@ -158,35 +163,47 @@ export class TemplateDiff implements ITemplateDiff {
       // changes to the Type of resources can happen when migrating from CFN templates that use Transforms
       if (resourceChange.resourceTypeChanged) {
         // Treat as DELETE+ADD
-        if (scrutinizableTypes.has(resourceChange.oldResourceType!)) {
-          ret.push({
-            ...commonProps,
-            newProperties: undefined,
-            resourceType: resourceChange.oldResourceType!,
-            scrutinyType: cfnspec.resourceSpecification(resourceChange.oldResourceType!).ScrutinyType!,
-          });
+        if (resourceChange.oldResourceType) {
+          const oldResourceModel = loadResourceModel(resourceChange.oldResourceType);
+          if (oldResourceModel && this.resourceIsScrutinizable(oldResourceModel, scrutinyTypes)) {
+            ret.push({
+              ...commonProps,
+              newProperties: undefined,
+              resourceType: resourceChange.oldResourceType!,
+              scrutinyType: oldResourceModel.scrutinizable!,
+            });
+          }
         }
-        if (scrutinizableTypes.has(resourceChange.newResourceType!)) {
-          ret.push({
-            ...commonProps,
-            oldProperties: undefined,
-            resourceType: resourceChange.newResourceType!,
-            scrutinyType: cfnspec.resourceSpecification(resourceChange.newResourceType!).ScrutinyType!,
-          });
+
+        if (resourceChange.newResourceType) {
+          const newResourceModel = loadResourceModel(resourceChange.newResourceType);
+          if (newResourceModel && this.resourceIsScrutinizable(newResourceModel, scrutinyTypes)) {
+            ret.push({
+              ...commonProps,
+              oldProperties: undefined,
+              resourceType: resourceChange.newResourceType!,
+              scrutinyType: newResourceModel.scrutinizable!,
+            });
+          }
         }
       } else {
-        if (scrutinizableTypes.has(resourceChange.resourceType)) {
+        const resourceModel = loadResourceModel(resourceChange.resourceType);
+        if (resourceModel && this.resourceIsScrutinizable(resourceModel, scrutinyTypes)) {
           ret.push({
             ...commonProps,
             resourceType: resourceChange.resourceType,
-            scrutinyType: cfnspec.resourceSpecification(resourceChange.resourceType).ScrutinyType!,
+            scrutinyType: resourceModel.scrutinizable!,
           });
         }
       }
     }
 
     return ret;
   }
+
+  private resourceIsScrutinizable(res: ResourceModel, scrutinyTypes: Array<ResourceScrutinyType>): boolean {
+    return scrutinyTypes.includes(res.scrutinizable || ResourceScrutinyType.None);
+  }
 }
 
 /**
@@ -211,7 +228,7 @@ export interface PropertyChange {
   /**
    * Scrutiny type for this property change
    */
-  scrutinyType: cfnspec.schema.PropertyScrutinyType;
+  scrutinyType: PropertyScrutinyType;
 
   /**
    * Name of the property that is changing
@@ -243,7 +260,7 @@ export interface ResourceChange {
   /**
    * Scrutiny type for this resource change
    */
-  scrutinyType: cfnspec.schema.ResourceScrutinyType;
+  scrutinyType: ResourceScrutinyType;
 
   /**
    * The type of the resource

packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts

@@ -1,3 +1,6 @@
+import { loadAwsServiceSpecSync } from '@aws-cdk/aws-service-spec';
+import { Resource, SpecDatabase } from '@aws-cdk/service-spec-types';
+
 /**
  * Compares two objects for equality, deeply. The function handles arguments that are
  * +null+, +undefined+, arrays and objects. For objects, the function will not take the
@@ -163,3 +166,23 @@ export function mangleLikeCloudFormation(payload: string) {
 function safeParseFloat(str: string): number {
   return Number(str);
 }
+
+/**
+ * Lazily load the service spec database and cache the loaded db
+*/
+let DATABASE: SpecDatabase | undefined;
+function database(): SpecDatabase {
+  if (!DATABASE) {
+    DATABASE = loadAwsServiceSpecSync();
+  }
+  return DATABASE;
+}
+
+/**
+ * Load a Resource model from the Service Spec Database
+ *
+ * The database is loaded lazily and cached across multiple calls to `loadResourceModel`.
+ */
+export function loadResourceModel(type: string): Resource | undefined {
+  return database().lookup('resource', 'cloudFormationType', 'equals', type).at(0);
+}

packages/@aws-cdk/cloudformation-diff/lib/format.ts

@@ -305,12 +305,12 @@ class Formatter {
     for (const [logicalId, resourceDiff] of Object.entries(templateDiff.resources)) {
       if (!resourceDiff) { continue; }
 
-      const oldPathMetadata = resourceDiff.oldValue && resourceDiff.oldValue.Metadata && resourceDiff.oldValue.Metadata[PATH_METADATA_KEY];
+      const oldPathMetadata = resourceDiff.oldValue?.Metadata?.[PATH_METADATA_KEY];
       if (oldPathMetadata && !(logicalId in this.logicalToPathMap)) {
         this.logicalToPathMap[logicalId] = oldPathMetadata;
       }
 
-      const newPathMetadata = resourceDiff.newValue && resourceDiff.newValue.Metadata && resourceDiff.newValue.Metadata[PATH_METADATA_KEY];
+      const newPathMetadata = resourceDiff.newValue?.Metadata?.[PATH_METADATA_KEY];
       if (newPathMetadata && !(logicalId in this.logicalToPathMap)) {
         this.logicalToPathMap[logicalId] = newPathMetadata;
       }

packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts

@@ -1,4 +1,4 @@
-import * as cfnspec from '@aws-cdk/cfnspec';
+import { PropertyScrutinyType, ResourceScrutinyType } from '@aws-cdk/service-spec-types';
 import * as chalk from 'chalk';
 import { ManagedPolicyAttachment, ManagedPolicyJson } from './managed-policy';
 import { parseLambdaPermission, parseStatements, Statement, StatementJson } from './statement';
@@ -18,15 +18,15 @@ export interface IamChangesProps {
  */
 export class IamChanges {
   public static IamPropertyScrutinies = [
-    cfnspec.schema.PropertyScrutinyType.InlineIdentityPolicies,
-    cfnspec.schema.PropertyScrutinyType.InlineResourcePolicy,
-    cfnspec.schema.PropertyScrutinyType.ManagedPolicies,
+    PropertyScrutinyType.InlineIdentityPolicies,
+    PropertyScrutinyType.InlineResourcePolicy,
+    PropertyScrutinyType.ManagedPolicies,
   ];
 
   public static IamResourceScrutinies = [
-    cfnspec.schema.ResourceScrutinyType.ResourcePolicyResource,
-    cfnspec.schema.ResourceScrutinyType.IdentityPolicyResource,
-    cfnspec.schema.ResourceScrutinyType.LambdaPermission,
+    ResourceScrutinyType.ResourcePolicyResource,
+    ResourceScrutinyType.IdentityPolicyResource,
+    ResourceScrutinyType.LambdaPermission,
   ];
 
   public readonly statements = new DiffableCollection<Statement>();
@@ -144,17 +144,17 @@ export class IamChanges {
 
   private readPropertyChange(propertyChange: PropertyChange) {
     switch (propertyChange.scrutinyType) {
-      case cfnspec.schema.PropertyScrutinyType.InlineIdentityPolicies:
+      case PropertyScrutinyType.InlineIdentityPolicies:
         // AWS::IAM::{ Role | User | Group }.Policies
         this.statements.addOld(...this.readIdentityPolicies(propertyChange.oldValue, propertyChange.resourceLogicalId));
         this.statements.addNew(...this.readIdentityPolicies(propertyChange.newValue, propertyChange.resourceLogicalId));
         break;
-      case cfnspec.schema.PropertyScrutinyType.InlineResourcePolicy:
+      case PropertyScrutinyType.InlineResourcePolicy:
         // Any PolicyDocument on a resource (including AssumeRolePolicyDocument)
         this.statements.addOld(...this.readResourceStatements(propertyChange.oldValue, propertyChange.resourceLogicalId));
         this.statements.addNew(...this.readResourceStatements(propertyChange.newValue, propertyChange.resourceLogicalId));
         break;
-      case cfnspec.schema.PropertyScrutinyType.ManagedPolicies:
+      case PropertyScrutinyType.ManagedPolicies:
         // Just a list of managed policies
         this.managedPolicies.addOld(...this.readManagedPolicies(propertyChange.oldValue, propertyChange.resourceLogicalId));
         this.managedPolicies.addNew(...this.readManagedPolicies(propertyChange.newValue, propertyChange.resourceLogicalId));
@@ -164,17 +164,17 @@ export class IamChanges {
 
   private readResourceChange(resourceChange: ResourceChange) {
     switch (resourceChange.scrutinyType) {
-      case cfnspec.schema.ResourceScrutinyType.IdentityPolicyResource:
+      case ResourceScrutinyType.IdentityPolicyResource:
         // AWS::IAM::Policy
         this.statements.addOld(...this.readIdentityPolicyResource(resourceChange.oldProperties));
         this.statements.addNew(...this.readIdentityPolicyResource(resourceChange.newProperties));
         break;
-      case cfnspec.schema.ResourceScrutinyType.ResourcePolicyResource:
+      case ResourceScrutinyType.ResourcePolicyResource:
         // AWS::*::{Bucket,Queue,Topic}Policy
         this.statements.addOld(...this.readResourcePolicyResource(resourceChange.oldProperties));
         this.statements.addNew(...this.readResourcePolicyResource(resourceChange.newProperties));
         break;
-      case cfnspec.schema.ResourceScrutinyType.LambdaPermission:
+      case ResourceScrutinyType.LambdaPermission:
         this.statements.addOld(...this.readLambdaStatements(resourceChange.oldProperties));
         this.statements.addNew(...this.readLambdaStatements(resourceChange.newProperties));
         break;

packages/@aws-cdk/cloudformation-diff/lib/util.ts

@@ -46,4 +46,4 @@ export function flatMap<T, U>(xs: T[], f: (x: T) => U[]): U[] {
     ret.push(...f(x));
   }
   return ret;
-}
+}

packages/@aws-cdk/cloudformation-diff/package.json

@@ -23,7 +23,8 @@
   },
   "license": "Apache-2.0",
   "dependencies": {
-    "@aws-cdk/cfnspec": "0.0.0",
+    "@aws-cdk/aws-service-spec": "^0.0.26",
+    "@aws-cdk/service-spec-types": "^0.0.26",
     "chalk": "^4",
     "diff": "^5.1.0",
     "fast-deep-equal": "^3.1.3",
@@ -56,5 +57,10 @@
   "maturity": "stable",
   "publishConfig": {
     "tag": "latest"
+  },
+  "pkglint": {
+    "exclude": [
+      "dependencies/cdk-point-dependencies"
+    ]
   }
 }

packages/@aws-cdk/integ-runner/package.json

@@ -44,7 +44,7 @@
         "BSD-2-Clause",
         "0BSD"
       ],
-      "dontAttribute": "^@aws-cdk/|^cdk-assets$",
+      "dontAttribute": "^@aws-cdk/|^@cdklabs/|^cdk-assets$",
       "test": "bin/integ-runner --version"
     }
   },

packages/aws-cdk/.gitignore

@@ -41,3 +41,4 @@ test/integ/cli/*.d.ts
 junit.xml
 
 lib/**/*.wasm
+db.json.gz