(aws-ecr): repository automatically adds imageScanningConfiguration even when imageScanOnPush is undefined

[tiefps]

Describe the bug

Hi CDK team,

A recent change to Repository started automatically adding ImageScanningConfiguration to the generated CloudFormation for AWS::ECR::Repository even when scanOnPush is undefined. This property is not supported in all regions (e.g. us-iso-east-1) and causes CloudFormation deployments to fail on the unrecognized property.

Line in question:

aws-cdk/packages/@aws-cdk/aws-ecr/lib/repository.ts

Line 537 in d0ace8f

imageScanningConfiguration: props.imageScanOnPush ? { scanOnPush: true } : { scanOnPush: false },

Suggested logic:

imageScanningConfiguration: props.imageScanOnPush === undefined ? undefined : { scanOnPush: props.imageScanOnPush },

---

The current workaround is to remove that property via escape hatch, however customers only know to do this after the deployment has already failed which is far from ideal.

const cfnRepo = somRepository.node.defaultChild as CfnRepository;
cfnRepo.addPropertyDeletionOverride('ImageScanningConfiguration');

Expected Behavior

ImageScanningConfiguration property is only added to AWS::ECR::Repository when scanOnPush is explicitly defined.

Current Behavior

ImageScanningConfiguration is always added even for unsupported regions.

Reproduction Steps

// Note: imageScanOnPush is not being set
const repo = new Repository(this, 'SomeRepo', {});

Possible Solution

(untested) Change the line to:

imageScanningConfiguration: props.imageScanOnPush === undefined ? undefined : { scanOnPush: props.imageScanOnPush },

Additional Information/Context

No response

CDK CLI Version

2.18.0 (build 75c90fa)

Framework Version

No response

Node.js Version

node-v14.19.1

OS

MacOS

Language

Typescript

Language Version

No response

Other information

No response

[peterwoodworth]

We actually added this line of code because of this bug where setting it to false can do nothing. I hadn't considered that regions might not support this property unfortunately... Or else your proposed fix would've been perfectly fine as well. (How it worked before is if you explicitly set it to false then the property would still be undefined)

I think changing this to your proposed solution would work, or maybe there's something we could do with the region-info package. If we go forward with your proposed solution I don't think there are any downsides, except that we will be removing a property that effectively does nothing from customer templates.

[tiefps]

I edited my comment to fix the possible solution. My original suggestion wasn't correct.

[peterwoodworth]

You're right, I'll fix that up and add a test to verify functionality works as expected

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.