Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove sdkv2 from cloudformation-diff #29730

Merged
merged 3 commits into from
Apr 8, 2024
Merged

chore: remove sdkv2 from cloudformation-diff #29730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9bb5b6e
chore: remove sdkv3 from cloudformation-diff
TheRealAmazonKendra Apr 4, 2024
f4af0e5
Merge branch 'main' into TheRealAmazonKendra/sdkv3
mergify[bot] Apr 8, 2024
70272d5
update new test
TheRealAmazonKendra Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
// The SDK is only used to reference `DescribeChangeSetOutput`, so the SDK is added as a devDependency.
// The SDK should not make network calls here
import type { CloudFormation } from 'aws-sdk';
import type { DescribeChangeSetOutput as DescribeChangeSet } from '@aws-sdk/client-cloudformation';
import * as impl from './diff';
import * as types from './diff/types';
import { deepEqual, diffKeyedEntities, unionOf } from './diff/util';

export * from './diff/types';

export type DescribeChangeSetOutput = DescribeChangeSet;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why both this and the import? The change on line 3 lets us reference this type as DescribeChangeSet, but without the as, you could achieve the same result we get from both lines 3 and 10.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is used extensively throughout aws-cdk (the cli, not the entire package). This re-export is a workaround In order to not cause a conflict of types in that package. You could argue that we should just migrate that whole package, but that's a much more complex task that we have not yet prioritized.


type DiffHandler = (diff: types.ITemplateDiff, oldValue: any, newValue: any) => void;
type HandlerRegistry = { [section: string]: DiffHandler };

Expand Down Expand Up @@ -45,7 +47,7 @@ const DIFF_HANDLERS: HandlerRegistry = {
export function fullDiff(
currentTemplate: { [key: string]: any },
newTemplate: { [key: string]: any },
changeSet?: CloudFormation.DescribeChangeSetOutput,
changeSet?: DescribeChangeSetOutput,
isImport?: boolean,
): types.TemplateDiff {

Expand Down Expand Up @@ -212,7 +214,7 @@ function deepCopy(x: any): any {
return x;
}

function addImportInformation(diff: types.TemplateDiff, changeSet: CloudFormation.DescribeChangeSetOutput) {
function addImportInformation(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
const imports = findResourceImports(changeSet);
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
if (imports.includes(logicalId)) {
Expand All @@ -227,7 +229,7 @@ function makeAllResourceChangesImports(diff: types.TemplateDiff) {
});
}

function filterFalsePositives(diff: types.TemplateDiff, changeSet: CloudFormation.DescribeChangeSetOutput) {
function filterFalsePositives(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
const replacements = findResourceReplacements(changeSet);
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
if (change.resourceType.includes('AWS::Serverless')) {
Expand Down Expand Up @@ -268,7 +270,7 @@ function filterFalsePositives(diff: types.TemplateDiff, changeSet: CloudFormatio
});
}

function findResourceImports(changeSet: CloudFormation.DescribeChangeSetOutput): string[] {
function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
const importedResourceLogicalIds = [];
for (const resourceChange of changeSet.Changes ?? []) {
if (resourceChange.ResourceChange?.Action === 'Import') {
Expand All @@ -279,7 +281,7 @@ function findResourceImports(changeSet: CloudFormation.DescribeChangeSetOutput):
return importedResourceLogicalIds;
}

function findResourceReplacements(changeSet: CloudFormation.DescribeChangeSetOutput): types.ResourceReplacements {
function findResourceReplacements(changeSet: DescribeChangeSetOutput): types.ResourceReplacements {
const replacements: types.ResourceReplacements = {};
for (const resourceChange of changeSet.Changes ?? []) {
const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {};
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/cloudformation-diff/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
"dependencies": {
"@aws-cdk/aws-service-spec": "^0.0.61",
"@aws-cdk/service-spec-types": "^0.0.61",
"aws-sdk": "2.1586.0",
"chalk": "^4",
"diff": "^5.2.0",
"fast-deep-equal": "^3.1.3",
Expand All @@ -35,6 +34,7 @@
"devDependencies": {
"@aws-cdk/cdk-build-tools": "0.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@aws-sdk/client-cloudformation": "^3.529.1",
"@types/jest": "^29.5.12",
"@types/string-width": "^4.0.1",
"fast-check": "^3.17.0",
Expand Down Expand Up @@ -64,4 +64,4 @@
"dependencies/cdk-point-dependencies"
]
}
}
}
4 changes: 2 additions & 2 deletions packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ describe('changeset', () => {
ResourceType: 'AWS::S3::Bucket',
Replacement: 'True',
Details: [{
Evaluation: 'Direct',
Evaluation: 'Static',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, do you mind explaining why we needed to change the Evaluation type from Direct to Static?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not a valid value before but in v2 it wasn't strongly typed enough for it to matter in our test case.

Target: {
Attribute: 'Properties',
Name: 'BucketName',
Expand Down Expand Up @@ -1153,7 +1153,7 @@ describe('changeset', () => {
ResourceType: 'AWS::Lambda::Function', // The SAM transform is applied before the changeset is created, so the changeset has a Lambda resource here!
Replacement: 'False',
Details: [{
Evaluation: 'Direct',
Evaluation: 'Static',
Target: {
Attribute: 'Properties',
Name: 'Code',
Expand Down
11 changes: 7 additions & 4 deletions packages/aws-cdk/lib/api/util/cloudformation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// Temporarily pull this in to avoid creating conflicts with the sdks in this package
import { DescribeChangeSetOutput } from '@aws-cdk/cloudformation-diff';
import { SSMPARAM_NO_INVALIDATE } from '@aws-cdk/cx-api';
import * as cxapi from '@aws-cdk/cx-api';
import { CloudFormation } from 'aws-sdk';
Expand Down Expand Up @@ -311,7 +313,7 @@ export type CreateChangeSetOptions = {
/**
* Create a changeset for a diff operation
*/
export async function createDiffChangeSet(options: PrepareChangeSetOptions): Promise<CloudFormation.DescribeChangeSetOutput | undefined> {
export async function createDiffChangeSet(options: PrepareChangeSetOptions): Promise<DescribeChangeSetOutput | undefined> {
// `options.stack` has been modified to include any nested stack templates directly inline with its own template, under a special `NestedTemplate` property.
// Thus the parent template's Resources section contains the nested template's CDK metadata check, which uses Fn::Equals.
// This causes CreateChangeSet to fail with `Template Error: Fn::Equals cannot be partially collapsed`.
Expand All @@ -327,7 +329,7 @@ export async function createDiffChangeSet(options: PrepareChangeSetOptions): Pro
return uploadBodyParameterAndCreateChangeSet(options);
}

async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOptions): Promise<CloudFormation.DescribeChangeSetOutput | undefined> {
async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOptions): Promise<DescribeChangeSetOutput | undefined> {
try {
const preparedSdk = (await options.deployments.prepareSdkWithDeployRole(options.stack));
const bodyParameter = await makeBodyParameterAndUpload(
Expand Down Expand Up @@ -363,7 +365,7 @@ async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOp
}
}

async function createChangeSet(options: CreateChangeSetOptions): Promise<CloudFormation.DescribeChangeSetOutput> {
async function createChangeSet(options: CreateChangeSetOptions): Promise<DescribeChangeSetOutput> {
await cleanupOldChangeset(options.changeSetName, options.stack.stackName, options.cfn);

debug(`Attempting to create ChangeSet with name ${options.changeSetName} for stack ${options.stack.stackName}`);
Expand All @@ -390,7 +392,8 @@ async function createChangeSet(options: CreateChangeSetOptions): Promise<CloudFo
const createdChangeSet = await waitForChangeSet(options.cfn, options.stack.stackName, options.changeSetName, { fetchAll: options.willExecute });
await cleanupOldChangeset(options.changeSetName, options.stack.stackName, options.cfn);

return createdChangeSet;
// TODO: Update this once we remove sdkv2 from the rest of this package
return createdChangeSet as DescribeChangeSetOutput;
}

export async function cleanupOldChangeset(changeSetName: string, stackName: string, cfn: CloudFormation) {
Expand Down
31 changes: 19 additions & 12 deletions packages/aws-cdk/lib/diff.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
import { format } from 'util';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import * as cfnDiff from '@aws-cdk/cloudformation-diff';
import {
type DescribeChangeSetOutput,
type FormatStream,
type TemplateDiff,
formatDifferences,
formatSecurityChanges,
fullDiff,
mangleLikeCloudFormation,
} from '@aws-cdk/cloudformation-diff';
import * as cxapi from '@aws-cdk/cx-api';
import { CloudFormation } from 'aws-sdk';
import * as chalk from 'chalk';
import { NestedStackTemplates } from './api/nested-stack-helpers';
import { print, warning } from './logging';
Expand All @@ -24,18 +31,18 @@ export function printStackDiff(
strict: boolean,
context: number,
quiet: boolean,
changeSet?: CloudFormation.DescribeChangeSetOutput,
changeSet?: DescribeChangeSetOutput,
isImport?: boolean,
stream: cfnDiff.FormatStream = process.stderr,
stream: FormatStream = process.stderr,
nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }): number {

let diff = cfnDiff.fullDiff(oldTemplate, newTemplate.template, changeSet, isImport);
let diff = fullDiff(oldTemplate, newTemplate.template, changeSet, isImport);

// detect and filter out mangled characters from the diff
let filteredChangesCount = 0;
if (diff.differenceCount && !strict) {
const mangledNewTemplate = JSON.parse(cfnDiff.mangleLikeCloudFormation(JSON.stringify(newTemplate.template)));
const mangledDiff = cfnDiff.fullDiff(oldTemplate, mangledNewTemplate, changeSet);
const mangledNewTemplate = JSON.parse(mangleLikeCloudFormation(JSON.stringify(newTemplate.template)));
const mangledDiff = fullDiff(oldTemplate, mangledNewTemplate, changeSet);
filteredChangesCount = Math.max(0, diff.differenceCount - mangledDiff.differenceCount);
if (filteredChangesCount > 0) {
diff = mangledDiff;
Expand All @@ -55,7 +62,7 @@ export function printStackDiff(
let stackDiffCount = 0;
if (!diff.isEmpty) {
stackDiffCount++;
cfnDiff.formatDifferences(stream, diff, {
formatDifferences(stream, diff, {
...logicalIdMapFromTemplate(oldTemplate),
...buildLogicalToPathMap(newTemplate),
}, context);
Expand Down Expand Up @@ -109,16 +116,16 @@ export function printSecurityDiff(
oldTemplate: any,
newTemplate: cxapi.CloudFormationStackArtifact,
requireApproval: RequireApproval,
changeSet?: CloudFormation.DescribeChangeSetOutput,
changeSet?: DescribeChangeSetOutput,
): boolean {
const diff = cfnDiff.fullDiff(oldTemplate, newTemplate.template, changeSet);
const diff = fullDiff(oldTemplate, newTemplate.template, changeSet);

if (difRequiresApproval(diff, requireApproval)) {
// eslint-disable-next-line max-len
warning(`This deployment will make potentially sensitive changes according to your current security approval level (--require-approval ${requireApproval}).`);
warning('Please confirm you intend to make the following modifications:\n');

cfnDiff.formatSecurityChanges(process.stdout, diff, buildLogicalToPathMap(newTemplate));
formatSecurityChanges(process.stdout, diff, buildLogicalToPathMap(newTemplate));
return true;
}
return false;
Expand All @@ -130,7 +137,7 @@ export function printSecurityDiff(
* TODO: Filter the security impact determination based off of an enum that allows
* us to pick minimum "severities" to alert on.
*/
function difRequiresApproval(diff: cfnDiff.TemplateDiff, requireApproval: RequireApproval) {
function difRequiresApproval(diff: TemplateDiff, requireApproval: RequireApproval) {
switch (requireApproval) {
case RequireApproval.Never: return false;
case RequireApproval.AnyChange: return diff.permissionsAnyChanges;
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/test/diff.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ describe('stack exists checks', () => {
Changes: [
{
ResourceChange: {
Action: 'Dummy',
Action: 'Add',
LogicalResourceId: 'Object',
},
},
Expand Down
Loading
Loading