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

aws · Nov 6, 2023 · 97f9dc8 · 97f9dc8
1 parent 8fc3747
commit 97f9dc8
Show file tree

Hide file tree

Showing 9 changed files with 127 additions and 70 deletions.
diff --git a/aws-cdk.code-workspace b/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"

diff --git a/packages/@aws-cdk/cloudformation-diff/jest.config.js b/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,
     },
+  },
 };
diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts b/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:

diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/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

diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts b/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);
+}
diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/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;
       }

diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/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;

diff --git a/packages/@aws-cdk/cloudformation-diff/lib/util.ts b/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;
-}
+}
diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/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"
+    ]
   }
 }
-Original file line number
+Diff line change
@@ Expand Up @@
         ret.push(...f(x));
       }
       return ret;
-    }
+    }