(ec2): 2.31.0 breaks deployments to vpcflow logs that don't specify optional DestinationOptions

[cmoore1776]

Describe the bug

After CDK 2.31.0 aws_cdk.aws_ec2.FlowLogOptions synthesize with a new parameter DestinationOptions regardless of whether or not that parameter is defined in CDK code.

For example, you can implement aws_cdk.aws_ec2.FlowLogDestination.to_s3 without specifying destination options, resulting in a synthesized value of DestinationOptions set to {}.

This appears valid at first, an a CDK diff:

Stack prod-vpc-ap-northeast-1
--
Resources
[~] AWS::EC2::FlowLog primary/vpcFlowLogs/FlowLog primaryvpcFlowLogsFlowLogA2C669F8 replace
└─ [+] DestinationOptions (requires replacement)
└─ {}

But then fails CFN validation during deployment:

Failed resources:
--
prod-vpc-ap-northeast-1 | 3:19:15 PM | UPDATE_FAILED        | AWS::EC2::FlowLog
Resource handler returned message: "Model validation failed (#/DestinationOptions: 3 schema violations found)
#/DestinationOptions: required key [FileFormat] not found (#/DestinationOptions)
#/DestinationOptions: required key [HiveCompatiblePartitions] not found (#/DestinationOptions)
#/DestinationOptions: required key [PerHourPartition] not found (#/DestinationOptions)" (RequestToken: 3d6360da-a3f1-b912-c6c1-63fca4576878, HandlerErrorCode: InvalidRequest)

Looking at the CFN User Guide, DestinationOptions is not required, but apparently if specified, all three keys become required.

Expected Behavior

I expect the ec2.Vpc.add_flow_log method to synthesize without an empty DestinationOptions parameter.

e.g. this CDK python code:

self.prodVpc.add_flow_log(
  id= "prodVpcFlowLog",
  destination= ec2.FlowLogDestination.to_s3(
    bucket= self.vpcFlowLogsBucket,
    key_prefix= self.prodVpc.vpc_id
  ),
  traffic_type= ec2.FlowLogTrafficType.ALL
)

should synthesize as such:

[..]
{
  "Type": "AWS::EC2::FlowLog",
  "Properties": {
  "ResourceId": "vpc-xxx",
  "ResourceType": "VPC",
  "TrafficType": "ALL",
  "LogDestination": { "s3://xxxx" },
  "LogDestinationType": "s3"
 }
[..]

Current Behavior

Current behavior is ec2.Vpc.add_flow_log method synthesizes with an empty DestinationOptions parameter.

e.g. this CDK python code:

self.prodVpc.add_flow_log(
  id= "prodVpcFlowLog",
  destination= ec2.FlowLogDestination.to_s3(
    bucket= self.vpcFlowLogsBucket,
    key_prefix= self.prodVpc.vpc_id
  ),
  traffic_type= ec2.FlowLogTrafficType.ALL
)

synthesizes as invalid CFN:

[..]
{
  "Type": "AWS::EC2::FlowLog",
  "Properties": {
  "ResourceId": "vpc-xxx",
  "ResourceType": "VPC",
  "TrafficType": "ALL",
  "DestinationOptions": {},
  "LogDestination": { "s3://xxxx" },
  "LogDestinationType": "s3"
 }
[..]

Reproduction Steps

See "Expected Behavior"

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.31.0

Framework Version

No response

Node.js Version

16.15.1

OS

macOS 12.4

Language

Python

Language Version

Python 3.10.5

Other information

No response

[corymhall]

I can reproduce this on a python project, but not in a typescript project. I'm not sure why python is handling this differently. @rix0rrr @RomainMuller any ideas?

[peterwoodworth]

I am also finding the same behavior here - and can confirm that this isn't seen on Python CDK version 2.30.0

[corymhall]

I am also finding the same behavior here - and can confirm that this isn't seen on Python CDK version 2.30.0

It was introduced in #20765

[RomainMuller]

This is because ec2.FlowLogDestination.toS3 accepts an optional S3DestinationOptions object at

aws-cdk/packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

Line 178 in 68f09b7

public static toS3(bucket?: s3.IBucket, keyPrefix?: string, options?: S3DestinationOptions): FlowLogDestination {

.

As far as Python is concerned, this terminal position property bag is a set of keyword arguments, and there is no way to explicitly pass None here, making Python treat those as ALWAYS empty object.

The issue actually is that this place assumes no S3DestinationOptions has different semantics than an empty S3DestinationOptions value, which is an anti-pattern (not passing any should mean "use defaults", which is effectively what happens when {} is passed).

[corymhall]

I think this might be fixed in #21042 since I am explicitly setting destinationOptions to undefined in the bind method.

[macdrorepo]

Update to 2.33.0 is the cure...

[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.