diff --git a/.github/workflows/bootstrap-template-protection.yml b/.github/workflows/bootstrap-template-protection.yml index 684aa43af..dba5b46ba 100644 --- a/.github/workflows/bootstrap-template-protection.yml +++ b/.github/workflows/bootstrap-template-protection.yml @@ -9,6 +9,7 @@ on: - reopened - labeled - unlabeled + merge_group: {} jobs: check-bootstrap-template: name: Check Bootstrap Template Changes @@ -16,6 +17,7 @@ jobs: permissions: contents: read pull-requests: write + if: (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') steps: - name: Checkout merge commit uses: actions/checkout@v4 diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/drift/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/drift/index.ts index 5404d1939..c519bc0e6 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/drift/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/drift/index.ts @@ -22,7 +22,7 @@ export interface FormattedDrift { readonly unchanged?: string; /** - * Resources that were not checked for drift + * Resources that were not checked for drift or have an UNKNOWN drift status */ readonly unchecked?: string; diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift-formatter.ts b/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift-formatter.ts index 046010723..e3d254c9e 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift-formatter.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift-formatter.ts @@ -41,7 +41,7 @@ interface DriftFormatterOutput { readonly unchanged?: string; /** - * Resources that were not checked for drift + * Resources that were not checked for drift or have an UNKNOWN drift status */ readonly unchecked?: string; @@ -98,12 +98,10 @@ export class DriftFormatter { public formatStackDrift(): DriftFormatterOutput { const formatterOutput = this.formatStackDriftChanges(this.buildLogicalToPathMap()); - // we are only interested in actual drifts and always ignore the metadata resource + // we are only interested in actual drifts (and ignore the metadata resource) const actualDrifts = this.resourceDriftResults.filter(d => - d.StackResourceDriftStatus === 'MODIFIED' || - d.StackResourceDriftStatus === 'DELETED' || - d.ResourceType === 'AWS::CDK::Metadata', - ); + (d.StackResourceDriftStatus === 'MODIFIED' || d.StackResourceDriftStatus === 'DELETED') + && d.ResourceType !== 'AWS::CDK::Metadata'); // must output the stack name if there are drifts const stackHeader = format(`Stack ${chalk.bold(this.stackName)}\n`); @@ -114,6 +112,7 @@ export class DriftFormatter { numResourcesWithDrift: 0, numResourcesUnchecked: this.allStackResources.size - this.resourceDriftResults.length, stackHeader, + unchecked: formatterOutput.unchecked, summary: finalResult, }; } @@ -140,11 +139,8 @@ export class DriftFormatter { } /** - * Renders stack drift information to the given stream + * Renders stack drift information * - * @param driftResults - The stack resource drifts from CloudFormation - * @param allStackResources - A map of all stack resources - * @param verbose - Whether to output more verbose text (include undrifted resources) * @param logicalToPathMap - A map from logical ID to construct path */ private formatStackDriftChanges( @@ -167,35 +163,35 @@ export class DriftFormatter { for (const drift of unchangedResources) { if (!drift.LogicalResourceId || !drift.ResourceType) continue; - unchanged += `${CONTEXT} ${this.formatValue(drift.ResourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; + unchanged += `${CONTEXT} ${chalk.cyan(drift.ResourceType)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; } unchanged += this.printSectionFooter(); } - // Process all unchecked resources - if (this.allStackResources) { - const uncheckedResources = Array.from(this.allStackResources.keys()).filter((logicalId) => { - return !drifts.find((drift) => drift.LogicalResourceId === logicalId); - }); - if (uncheckedResources.length > 0) { - unchecked = this.printSectionHeader('Unchecked Resources'); - for (const logicalId of uncheckedResources) { - const resourceType = this.allStackResources.get(logicalId); - unchecked += `${CONTEXT} ${this.formatValue(resourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, logicalId)}\n`; - } - unchecked += this.printSectionFooter(); + // Process all unchecked and unknown resources + const uncheckedResources = Array.from(this.allStackResources.keys()).filter((logicalId) => { + const drift = drifts.find((d) => d.LogicalResourceId === logicalId); + return !drift || drift.StackResourceDriftStatus === StackResourceDriftStatus.UNKNOWN; + }); + if (uncheckedResources.length > 0) { + unchecked = this.printSectionHeader('Unchecked Resources'); + for (const logicalId of uncheckedResources) { + const resourceType = this.allStackResources.get(logicalId); + unchecked += `${CONTEXT} ${chalk.cyan(resourceType)} ${this.formatLogicalId(logicalToPathMap, logicalId)}\n`; } + unchecked += this.printSectionFooter(); } - // Process modified resources - const modifiedResources = drifts.filter(d => d.StackResourceDriftStatus === StackResourceDriftStatus.MODIFIED); + // Process modified resources (exclude AWS::CDK::Metadata) + const modifiedResources = drifts.filter(d => + d.StackResourceDriftStatus === StackResourceDriftStatus.MODIFIED + && d.ResourceType !== 'AWS::CDK::Metadata'); if (modifiedResources.length > 0) { modified = this.printSectionHeader('Modified Resources'); for (const drift of modifiedResources) { if (!drift.LogicalResourceId || !drift.ResourceType) continue; - if (modified === undefined) modified = ''; - modified += `${UPDATE} ${this.formatValue(drift.ResourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; + modified += `${UPDATE} ${chalk.cyan(drift.ResourceType)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; if (drift.PropertyDifferences) { const propDiffs = drift.PropertyDifferences; for (let i = 0; i < propDiffs.length; i++) { @@ -209,13 +205,15 @@ export class DriftFormatter { modified += this.printSectionFooter(); } - // Process deleted resources - const deletedResources = drifts.filter(d => d.StackResourceDriftStatus === StackResourceDriftStatus.DELETED); + // Process deleted resources (exclude AWS::CDK::Metadata) + const deletedResources = drifts.filter(d => + d.StackResourceDriftStatus === StackResourceDriftStatus.DELETED + && d.ResourceType !== 'AWS::CDK::Metadata'); if (deletedResources.length > 0) { deleted = this.printSectionHeader('Deleted Resources'); for (const drift of deletedResources) { if (!drift.LogicalResourceId || !drift.ResourceType) continue; - deleted += `${REMOVAL} ${this.formatValue(drift.ResourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; + deleted += `${REMOVAL} ${chalk.cyan(drift.ResourceType)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; } deleted += this.printSectionFooter(); } @@ -250,16 +248,6 @@ export class DriftFormatter { return `${normalizedPath} ${chalk.gray(logicalId)}`; } - private formatValue(value: any, colorFn: (str: string) => string): string { - if (value == null) { - return ''; - } - if (typeof value === 'string') { - return colorFn(value); - } - return colorFn(JSON.stringify(value)); - } - private printSectionHeader(title: string): string { return `${chalk.underline(chalk.bold(title))}\n`; } @@ -268,7 +256,7 @@ export class DriftFormatter { return '\n'; } - private formatTreeDiff(propertyPath: string, difference: Difference, isLast: boolean): string { + private formatTreeDiff(propertyPath: string, difference: Difference, isLast: boolean): string { let result = format(' %s─ %s %s\n', isLast ? '└' : '├', difference.isAddition ? ADDITION : difference.isRemoval ? REMOVAL : @@ -276,8 +264,8 @@ export class DriftFormatter { propertyPath, ); if (difference.isUpdate) { - result += format(' ├─ %s %s\n', REMOVAL, this.formatValue(difference.oldValue, chalk.red)); - result += format(' └─ %s %s\n', ADDITION, this.formatValue(difference.newValue, chalk.green)); + result += format(' ├─ %s %s\n', REMOVAL, chalk.red(difference.oldValue)); + result += format(' └─ %s %s\n', ADDITION, chalk.green(difference.newValue)); } return result; } diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift.ts b/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift.ts index fa6a4ee54..78423b4e4 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/drift/drift.ts @@ -1,6 +1,7 @@ -import { format } from 'util'; +import { format } from 'node:util'; import type { DescribeStackDriftDetectionStatusCommandOutput, DescribeStackResourceDriftsCommandOutput } from '@aws-sdk/client-cloudformation'; import { ToolkitError } from '../../toolkit/toolkit-error'; +import { formatReason } from '../../util/string-manipulation'; import type { ICloudFormationClient } from '../aws-auth/private'; import type { IoHelper } from '../io/private'; @@ -29,20 +30,34 @@ export async function detectStackDrift( // Wait for drift detection to complete const driftStatus = await waitForDriftDetection(cfn, ioHelper, driftDetection.StackDriftDetectionId!); - if (!driftStatus) { - throw new ToolkitError('Drift detection took too long to complete. Aborting'); - } - - if (driftStatus?.DetectionStatus === 'DETECTION_FAILED') { - throw new ToolkitError( - `Failed to detect drift: ${driftStatus.DetectionStatusReason || 'No reason provided'}`, + // Handle UNKNOWN stack drift status + if (driftStatus?.StackDriftStatus === 'UNKNOWN') { + await ioHelper.defaults.trace( + 'Stack drift status is UNKNOWN. This may occur when CloudFormation is unable to detect drift for at least one resource and all other resources are IN_SYNC.\n' + + `Reason: ${formatReason(driftStatus.DetectionStatusReason)}`, ); } - // Get the drift results - return cfn.describeStackResourceDrifts({ + // Get the drift results, including resources with UNKNOWN status + const driftResults = await cfn.describeStackResourceDrifts({ StackName: stackName, }); + + // Log warning for any resources with UNKNOWN status + const unknownResources = driftResults.StackResourceDrifts?.filter( + drift => drift.StackResourceDriftStatus === 'UNKNOWN', + ); + + if (unknownResources && unknownResources.length > 0) { + await ioHelper.defaults.trace( + 'Some resources have UNKNOWN drift status. This may be due to insufficient permissions or throttling:\n' + + unknownResources.map(r => + ` - ${r.LogicalResourceId}: ${formatReason(r.DriftStatusReason)}`, + ).join('\n'), + ); + } + + return driftResults; } /** @@ -69,7 +84,7 @@ async function waitForDriftDetection( } if (response.DetectionStatus === 'DETECTION_FAILED') { - throw new ToolkitError(`Drift detection failed: ${response.DetectionStatusReason}`); + throw new ToolkitError(`Drift detection failed: ${formatReason(response.DetectionStatusReason)}`); } if (Date.now() > deadline) { diff --git a/packages/@aws-cdk/toolkit-lib/lib/util/string-manipulation.ts b/packages/@aws-cdk/toolkit-lib/lib/util/string-manipulation.ts index 0165abb3d..d1f95ac56 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/util/string-manipulation.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/util/string-manipulation.ts @@ -42,3 +42,11 @@ function millisecondsToSeconds(num: number): number { export function lowerCaseFirstCharacter(str: string): string { return str.length > 0 ? `${str[0].toLowerCase()}${str.slice(1)}` : str; } + +/** + * Returns the provided reason or a default fallback message if the reason is undefined, null, or empty. + * This is commonly used for AWS API responses that may not always provide a reason field. + */ +export function formatReason(reason: string | undefined | null, fallback: string = 'No reason provided'): string { + return reason?.trim() || fallback; +} diff --git a/packages/@aws-cdk/toolkit-lib/test/api/drift/drift.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/drift/drift.test.ts index 479a53265..84cd13bf7 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/drift/drift.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/drift/drift.test.ts @@ -295,11 +295,155 @@ describe('detectStackDrift', () => { level: 'trace', })); }); + + test('handles UNKNOWN stack drift status', async () => { + // GIVEN + const stackName = 'test-stack'; + const driftDetectionId = 'test-detection-id'; + const statusReason = 'Unable to check drift for some resources'; + const expectedDriftResults = { StackResourceDrifts: [], $metadata: {} }; + + mockCfn.detectStackDrift.mockResolvedValue({ + StackDriftDetectionId: driftDetectionId, + }); + + mockCfn.describeStackDriftDetectionStatus.mockResolvedValue({ + DetectionStatus: 'DETECTION_COMPLETE', + StackDriftStatus: 'UNKNOWN', + DetectionStatusReason: statusReason, + }); + + mockCfn.describeStackResourceDrifts.mockResolvedValue(expectedDriftResults); + + // WHEN + await detectStackDrift(mockCfn, ioHelper, stackName); + + // THEN + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining('Stack drift status is UNKNOWN'), + level: 'trace', + })); + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining(statusReason), + level: 'trace', + })); + }); + + test('handles resources with UNKNOWN drift status', async () => { + // GIVEN + const stackName = 'test-stack'; + const driftDetectionId = 'test-detection-id'; + const driftStatusReason = 'Insufficient permissions to check drift'; + const expectedDriftResults = { + StackResourceDrifts: [ + { + StackId: 'stack-id', + LogicalResourceId: 'MyResource1', + ResourceType: 'AWS::S3::Bucket', + StackResourceDriftStatus: 'UNKNOWN', + DriftStatusReason: driftStatusReason, + Timestamp: new Date(), + }, + ], + $metadata: {}, + }; + + mockCfn.detectStackDrift.mockResolvedValue({ + StackDriftDetectionId: driftDetectionId, + }); + + mockCfn.describeStackDriftDetectionStatus.mockResolvedValue({ + DetectionStatus: 'DETECTION_COMPLETE', + StackDriftStatus: 'UNKNOWN', + }); + + mockCfn.describeStackResourceDrifts.mockResolvedValue(expectedDriftResults); + + // WHEN + await detectStackDrift(mockCfn, ioHelper, stackName); + + // THEN + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining('Some resources have UNKNOWN drift status'), + level: 'trace', + })); + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining('MyResource1'), + level: 'trace', + })); + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining(driftStatusReason), + level: 'trace', + })); + }); + + test('sends periodic check-in message during drift detection wait', async () => { + // GIVEN + const stackName = 'test-stack'; + const driftDetectionId = 'drift-detection-id'; + const expectedDriftResults = { StackResourceDrifts: [], $metadata: {} }; + + mockCfn.detectStackDrift.mockResolvedValue({ StackDriftDetectionId: driftDetectionId }); + + // Mock Date.now to simulate time progression that triggers check-in message + const originalDateNow = Date.now; + const startTime = 1000; + const timeBetweenOutputs = 10_000; + + const mockDateNow = jest.fn() + .mockReturnValueOnce(startTime) // deadline calculation + .mockReturnValueOnce(startTime) // checkIn calculation + .mockReturnValueOnce(startTime + timeBetweenOutputs + 1000) // First status check - after checkIn + .mockReturnValueOnce(startTime + timeBetweenOutputs + 1000) // deadline check + .mockReturnValueOnce(startTime + timeBetweenOutputs + 1000) // checkIn comparison - triggers message + .mockReturnValue(startTime + timeBetweenOutputs + 2000); // All subsequent calls + + Date.now = mockDateNow; + + // First call returns IN_PROGRESS, second call returns COMPLETE + mockCfn.describeStackDriftDetectionStatus + .mockResolvedValueOnce({ DetectionStatus: 'DETECTION_IN_PROGRESS' }) + .mockResolvedValueOnce({ DetectionStatus: 'DETECTION_COMPLETE', StackDriftStatus: 'IN_SYNC' }); + + mockCfn.describeStackResourceDrifts.mockResolvedValue(expectedDriftResults); + + // WHEN + await detectStackDrift(mockCfn, ioHelper, stackName); + + // THEN + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining('Waiting for drift detection to complete...'), + level: 'trace', + })); + + // Restore original Date.now + Date.now = originalDateNow; + }); }); describe('formatStackDrift', () => { let mockNewTemplate: cxapi.CloudFormationStackArtifact; + // Helper function to create template with metadata + function createTemplateWithMetadata(resources: Record) { + return { + template: { + Resources: { + ...resources, + CDKMetadata: { + Type: 'AWS::CDK::Metadata', + Properties: { + Analytics: 'placeholder-string', + }, + }, + }, + }, + templateFile: 'template.json', + stackName: 'test-stack', + findMetadataByType: () => [], + } as any; + } + beforeEach(() => { mockNewTemplate = { template: { @@ -580,4 +724,406 @@ describe('formatStackDrift', () => { expect(result.deleted).toContain('Resource2'); expect(result.summary).toContain('2 resources have drifted'); }); + + test('formatting with UNKNOWN drift status', () => { + // GIVEN + const templateWithMultipleResources = { + template: { + Resources: { + Resource1: { + Type: 'AWS::S3::Bucket', + Properties: { + BucketName: 'expected-name', + }, + }, + Resource2: { + Type: 'AWS::IAM::Role', + Properties: { + RoleName: 'test-role', + }, + }, + Resource3: { + Type: 'AWS::Lambda::Function', + Properties: { + Handler: 'index.handler', + Runtime: 'nodejs20.x', + }, + }, + }, + }, + templateFile: 'template.json', + stackName: 'test-stack', + findMetadataByType: () => [], + } as any; + + const mockDriftedResources: StackResourceDrift[] = [ + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'MODIFIED', + LogicalResourceId: 'Resource1', + PhysicalResourceId: 'physical-id-1', + ResourceType: 'AWS::S3::Bucket', + PropertyDifferences: [{ + PropertyPath: '/BucketName', + ExpectedValue: 'expected-name', + ActualValue: 'actual-name', + DifferenceType: 'NOT_EQUAL', + }], + Timestamp: new Date(Date.now()), + }, + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'UNKNOWN', + LogicalResourceId: 'Resource2', + PhysicalResourceId: 'physical-id-2', + ResourceType: 'AWS::IAM::Role', + Timestamp: new Date(Date.now()), + }, + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'IN_SYNC', + LogicalResourceId: 'Resource3', + PhysicalResourceId: 'physical-id-3', + ResourceType: 'AWS::Lambda::Function', + Timestamp: new Date(Date.now()), + }, + ]; + + // WHEN + const formatter = new DriftFormatter({ + stack: templateWithMultipleResources, + resourceDrifts: mockDriftedResources, + }); + const result = formatter.formatStackDrift(); + + // THEN + expect(result.numResourcesWithDrift).toBe(1); // Only MODIFIED counts as drift, UNKNOWN does not + expect(result.modified).toContain('Modified Resources'); + expect(result.modified).toContain('AWS::S3::Bucket'); + expect(result.modified).toContain('Resource1'); + expect(result.summary).toContain('1 resource has drifted'); + + // UNKNOWN resources should be treated as unchecked, not as drift + expect(result.unchecked).toContain('AWS::IAM::Role'); + expect(result.unchecked).toContain('Resource2'); + }); + + test('formatting with only UNKNOWN drift status', () => { + // GIVEN + const templateWithUnknownResources = { + template: { + Resources: { + Resource1: { + Type: 'AWS::S3::Bucket', + Properties: { + BucketName: 'test-bucket', + }, + }, + Resource2: { + Type: 'AWS::IAM::Role', + Properties: { + RoleName: 'test-role', + }, + }, + }, + }, + templateFile: 'template.json', + stackName: 'test-stack', + findMetadataByType: () => [], + } as any; + + const mockDriftedResources: StackResourceDrift[] = [ + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'UNKNOWN', + LogicalResourceId: 'Resource1', + PhysicalResourceId: 'physical-id-1', + ResourceType: 'AWS::S3::Bucket', + Timestamp: new Date(Date.now()), + }, + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'UNKNOWN', + LogicalResourceId: 'Resource2', + PhysicalResourceId: 'physical-id-2', + ResourceType: 'AWS::IAM::Role', + Timestamp: new Date(Date.now()), + }, + ]; + + // WHEN + const formatter = new DriftFormatter({ + stack: templateWithUnknownResources, + resourceDrifts: mockDriftedResources, + }); + const result = formatter.formatStackDrift(); + + // THEN + expect(result.numResourcesWithDrift).toBe(0); // UNKNOWN resources do not count as drift + expect(result.summary).toContain('No drift detected'); + + // All UNKNOWN resources should be in the unchecked section + expect(result.unchecked).toBeDefined(); + expect(result.unchecked).toContain('AWS::S3::Bucket'); + expect(result.unchecked).toContain('Resource1'); + expect(result.unchecked).toContain('AWS::IAM::Role'); + expect(result.unchecked).toContain('Resource2'); + }); + + test('filters out AWS::CDK::Metadata resources from drift count', () => { + // GIVEN + const templateWithMetadata = createTemplateWithMetadata({ + MyBucket: { + Type: 'AWS::S3::Bucket', + Properties: { + BucketName: 'test-bucket', + }, + }, + }); + + const mockDriftedResources: StackResourceDrift[] = [ + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'MODIFIED', + LogicalResourceId: 'MyBucket', + PhysicalResourceId: 'physical-bucket-id', + ResourceType: 'AWS::S3::Bucket', + PropertyDifferences: [{ + PropertyPath: '/BucketName', + ExpectedValue: 'test-bucket', + ActualValue: 'actual-bucket-name', + DifferenceType: 'NOT_EQUAL', + }], + Timestamp: new Date(Date.now()), + }, + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'MODIFIED', + LogicalResourceId: 'CDKMetadata', + PhysicalResourceId: 'physical-metadata-id', + ResourceType: 'AWS::CDK::Metadata', + PropertyDifferences: [{ + PropertyPath: '/Analytics', + ExpectedValue: 'placeholder-string', + ActualValue: 'different-analytics-value', + DifferenceType: 'NOT_EQUAL', + }], + Timestamp: new Date(Date.now()), + }, + ]; + + // WHEN + const formatter = new DriftFormatter({ + stack: templateWithMetadata, + resourceDrifts: mockDriftedResources, + }); + const result = formatter.formatStackDrift(); + + // THEN + // Only the S3 bucket should count as drift, not the CDK metadata resource + expect(result.numResourcesWithDrift).toBe(1); + expect(result.summary).toContain('1 resource has drifted'); + + // The modified section should contain the S3 bucket but not the CDK metadata + expect(result.modified).toContain('AWS::S3::Bucket'); + expect(result.modified).toContain('MyBucket'); + expect(result.modified).not.toContain('AWS::CDK::Metadata'); + expect(result.modified).not.toContain('CDKMetadata'); + }); + + test('filters out AWS::CDK::Metadata resources from deleted resources section', () => { + // GIVEN + const templateWithMetadata = createTemplateWithMetadata({ + MyRole: { + Type: 'AWS::IAM::Role', + Properties: { + RoleName: 'test-role', + }, + }, + }); + + const mockDriftedResources: StackResourceDrift[] = [ + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'DELETED', + LogicalResourceId: 'MyRole', + PhysicalResourceId: 'physical-role-id', + ResourceType: 'AWS::IAM::Role', + Timestamp: new Date(Date.now()), + }, + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'DELETED', + LogicalResourceId: 'CDKMetadata', + PhysicalResourceId: 'physical-metadata-id', + ResourceType: 'AWS::CDK::Metadata', + Timestamp: new Date(Date.now()), + }, + ]; + + // WHEN + const formatter = new DriftFormatter({ + stack: templateWithMetadata, + resourceDrifts: mockDriftedResources, + }); + const result = formatter.formatStackDrift(); + + // THEN + // Only the IAM role should count as drift, not the CDK metadata resource + expect(result.numResourcesWithDrift).toBe(1); + expect(result.summary).toContain('1 resource has drifted'); + + // The deleted section should contain the IAM role but not the CDK metadata + expect(result.deleted).toContain('AWS::IAM::Role'); + expect(result.deleted).toContain('MyRole'); + expect(result.deleted).not.toContain('AWS::CDK::Metadata'); + expect(result.deleted).not.toContain('CDKMetadata'); + }); + + test('handles empty resource drift results', () => { + // GIVEN + const mockTemplate = { + template: { + Resources: { + MyBucket: { + Type: 'AWS::S3::Bucket', + Properties: { + BucketName: 'test-bucket', + }, + }, + }, + }, + templateFile: 'template.json', + stackName: 'test-stack', + findMetadataByType: () => [], + } as any; + + // WHEN + const formatter = new DriftFormatter({ + stack: mockTemplate, + resourceDrifts: [], // Empty array + }); + const result = formatter.formatStackDrift(); + + // THEN + expect(result.numResourcesWithDrift).toBe(0); + expect(result.summary).toContain('No drift detected'); + expect(result.numResourcesUnchecked).toBe(1); // MyBucket is unchecked + }); + + test('formatTreeDiff handles ADDITION and REMOVAL differences', () => { + // GIVEN + const mockTemplate = { + template: { + Resources: { + MyResource: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + }, + }, + templateFile: 'template.json', + stackName: 'test-stack', + findMetadataByType: () => [], + } as any; + + const mockDriftedResources: StackResourceDrift[] = [ + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'MODIFIED', + LogicalResourceId: 'MyResource', + PhysicalResourceId: 'physical-id', + ResourceType: 'AWS::S3::Bucket', + PropertyDifferences: [ + { + PropertyPath: '/AddedProperty', + ExpectedValue: undefined, + ActualValue: 'new-value', + DifferenceType: 'ADD', + }, + { + PropertyPath: '/RemovedProperty', + ExpectedValue: 'old-value', + ActualValue: undefined, + DifferenceType: 'REMOVE', + }, + ], + Timestamp: new Date(Date.now()), + }, + ]; + + // WHEN + const formatter = new DriftFormatter({ + stack: mockTemplate, + resourceDrifts: mockDriftedResources, + }); + const result = formatter.formatStackDrift(); + + // THEN + expect(result.numResourcesWithDrift).toBe(1); + expect(result.modified).toBeDefined(); + expect(result.modified).toContain('AddedProperty'); + expect(result.modified).toContain('RemovedProperty'); + }); + + test('formatLogicalId handles path normalization', () => { + // GIVEN + const mockTemplate = { + template: { + Resources: { + MyResource: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + }, + }, + templateFile: 'template.json', + stackName: 'test-stack', + findMetadataByType: () => [ + { + type: 'aws:cdk:logicalId', + data: 'MyResource', + path: '/MyStack/MyConstruct/Resource', + }, + { + type: 'aws:cdk:logicalId', + data: 'AnotherResource', + path: '/MyStack/AnotherConstruct/Default', + }, + ], + } as any; + + const mockDriftedResources: StackResourceDrift[] = [ + { + StackId: 'some:stack:arn', + StackResourceDriftStatus: 'MODIFIED', + LogicalResourceId: 'MyResource', + PhysicalResourceId: 'physical-id', + ResourceType: 'AWS::S3::Bucket', + PropertyDifferences: [ + { + PropertyPath: '/BucketName', + ExpectedValue: 'old-name', + ActualValue: 'new-name', + DifferenceType: 'NOT_EQUAL', + }, + ], + Timestamp: new Date(Date.now()), + }, + ]; + + // WHEN + const formatter = new DriftFormatter({ + stack: mockTemplate, + resourceDrifts: mockDriftedResources, + }); + const result = formatter.formatStackDrift(); + + // THEN + expect(result.numResourcesWithDrift).toBe(1); + expect(result.modified).toBeDefined(); + // Should normalize path by removing leading slash and "Resource"/"Default" suffixes + expect(result.modified).toContain('MyConstruct'); + expect(result.modified).toContain('MyResource'); // logical ID should still be present + }); }); diff --git a/packages/@aws-cdk/toolkit-lib/test/util/string-manipulation.test.ts b/packages/@aws-cdk/toolkit-lib/test/util/string-manipulation.test.ts index d9a311ff7..087257c7d 100644 --- a/packages/@aws-cdk/toolkit-lib/test/util/string-manipulation.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/util/string-manipulation.test.ts @@ -1,4 +1,4 @@ -import { padLeft, padRight, formatTime } from '../../lib/util/string-manipulation'; +import { padLeft, padRight, formatTime, formatReason } from '../../lib/util/string-manipulation'; describe('string-manipulation', () => { describe('padLeft', () => { @@ -70,4 +70,34 @@ describe('string-manipulation', () => { expect(formatTime(1500)).toBe(1.5); }); }); + + describe('formatReason', () => { + test.each([ + ['Something went wrong', undefined, 'Something went wrong'], + ['Error occurred', undefined, 'Error occurred'], + [' Valid reason ', undefined, 'Valid reason'], + ])('returns the reason when provided: %s', (reason, fallback, expected) => { + expect(formatReason(reason, fallback)).toBe(expected); + }); + + test.each([ + [undefined, undefined, 'No reason provided'], + [null, undefined, 'No reason provided'], + ['', undefined, 'No reason provided'], + [' ', undefined, 'No reason provided'], + ])('returns default fallback for invalid reasons: %s', (reason, fallback, expected) => { + expect(formatReason(reason, fallback)).toBe(expected); + }); + + test.each([ + [undefined, 'Custom fallback message', 'Custom fallback message'], + [null, 'Custom fallback message', 'Custom fallback message'], + ['', 'Custom fallback message', 'Custom fallback message'], + [' ', 'Custom fallback message', 'Custom fallback message'], + [undefined, 'no reason provided', 'no reason provided'], + [null, 'Unknown error', 'Unknown error'], + ])('returns custom fallback when provided: reason=%s, fallback=%s', (reason, fallback, expected) => { + expect(formatReason(reason, fallback)).toBe(expected); + }); + }); }); diff --git a/projenrc/bootstrap-template-protection.ts b/projenrc/bootstrap-template-protection.ts index 00df4c96e..4338c6556 100644 --- a/projenrc/bootstrap-template-protection.ts +++ b/projenrc/bootstrap-template-protection.ts @@ -25,11 +25,13 @@ export class BootstrapTemplateProtection extends Component { pullRequest: { types: ['opened', 'synchronize', 'reopened', 'labeled', 'unlabeled'], }, + mergeGroup: {}, }); workflow.addJob('check-bootstrap-template', { name: 'Check Bootstrap Template Changes', runsOn: ['ubuntu-latest'], + if: "(github.event_name == 'pull_request' || github.event_name == 'pull_request_target')", permissions: { contents: gh.workflows.JobPermission.READ, pullRequests: gh.workflows.JobPermission.WRITE,