aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.69k stars 3.93k forks source link

(aws-ec2): FlowLog DestinationOptions have wrong casing #25432

Open matthias-pichler opened 1 year ago

matthias-pichler commented 1 year ago

Describe the bug

When I create a FlowLog like so:

    const flowLog = new ec2.FlowLog(this, "FlowLog", {
      resourceType: {
        resourceType: "TransitGateway",
        resourceId: this.transitGateway.attrId,
      },
      destination: ec2.FlowLogDestination.toS3(vpcFlowLogBucket, undefined, {
        fileFormat: ec2.FlowLogFileFormat.PLAIN_TEXT, 
        hiveCompatiblePartitions: false,
        perHourPartition: false, 
      }),
    });

the destination options are not set correctly

Expected Behavior

When setting the options here they should be first transformed to pascal case

Current Behavior

During deploy time I get the following warnings:

transit-gateway-dev |   1 | 8:05:55 AM | UPDATE_IN_PROGRESS   | AWS::EC2::FlowLog        | FlowLog/FlowLog (FlowLog3CB084E9) Resource template validation failed for resource FlowLog3CB084E9 as the template has invalid properties. Please refer to the resource documentation to fix the template.
Properties validation failed for resource FlowLog3CB084E9 with message:
#/DestinationOptions: required key [FileFormat] not found
#/DestinationOptions: required key [HiveCompatiblePartitions] not found
#/DestinationOptions: required key [PerHourPartition] not found
#/DestinationOptions: extraneous key [hiveCompatiblePartitions] is not permitted
#/DestinationOptions: extraneous key [perHourPartition] is not permitted
#/DestinationOptions: extraneous key [fileFormat] is not permitted

Reproduction Steps

Deploy the above resource

Possible Solution

- destinationOptions: destinationConfig.destinationOptions,
+ destinationOptions: {
+   FileFormat: destinationConfig.destinationOptions.fileFormat,
+   HiveCompatiblePartitions: destinationConfig.destinationOptions.hiveCompatiblePartitions,
+   PerHourPartition: destinationConfig.destinationOptions.perHourPartition,
+ }

Additional Information/Context

No response

CDK CLI Version

2.77.0

Framework Version

2.77.0

Node.js Version

18

OS

MacOS

Language

Typescript

Language Version

TypeScript 5.0.4

Other information

No response

peterwoodworth commented 1 year ago

I can see that the wrong casing is getting generated. This has been the case ever since the feature was first introduced in 2.31.0 - except for versions 2.55.0 and 2.56.0 due to the breaking changes in the spec in 2.55.0 that we patched here in 2.56.1 https://github.com/aws/aws-cdk/blob/8189fbef2913ac13922146403480b7a8a4a1152d/packages/%40aws-cdk/cfnspec/spec-source/specification/000_cfn/500_Revert_To_Json_Types_patch.json#L92-L107

That said, I'm not running into any errors both on 2.31.0 and 2.78.0, my flow log is successfully deploying with the destination settings specified. I wonder if this has to do with you trying to specify a transit gateway instead of using one of our factory methods given that we don't support transit gateway here yet (it doesn't look like we have an open feature request for that either)

github-actions[bot] commented 1 year ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

matthias-pichler commented 1 year ago

So when I use the factory method the wrong options (lowercased) get synthesized

  "FlowLog3CB084E9": {
   "Type": "AWS::EC2::FlowLog",
   "Properties": {
    "ResourceId": {
     "Fn::GetAtt": [
      "TransitGateway",
      "Id"
     ]
    },
    "ResourceType": "TransitGateway",
    "DestinationOptions": {
     "fileFormat": "plain-text",
     "perHourPartition": false,
     "hiveCompatiblePartitions": false
    },
    "LogDestination": {
     "Fn::Join": [
      "",
      [
       "arn:",
       {
        "Ref": "AWS::Partition"
       },
       ":s3:::aws-tgw-flow-logs-**********-eu-west-1"
      ]
     ]
    },
    "LogDestinationType": "s3"
   },
   "UpdateReplacePolicy": "Retain",
   "DeletionPolicy": "Retain",
   "Metadata": {
    "aws:cdk:path": "transit-gateway-dev/FlowLog/FlowLog"
   }
  }

So either way the only way the CDK currently generates these options is in lowercase, which is also how I understand the code because here

https://github.com/aws/aws-cdk/blob/8ef0ba2bfe90cf6367f15ce48dd1f92f5fae2852/packages/aws-cdk-lib/aws-ec2/lib/vpc-flow-logs.ts#L747

destinationOptions is of type DestinationOptions

https://github.com/aws/aws-cdk/blob/8ef0ba2bfe90cf6367f15ce48dd1f92f5fae2852/packages/aws-cdk-lib/aws-ec2/lib/vpc-flow-logs.ts#LL152C18-L152C36

which only has the lowercase keys:

https://github.com/aws/aws-cdk/blob/8ef0ba2bfe90cf6367f15ce48dd1f92f5fae2852/packages/aws-cdk-lib/aws-ec2/lib/vpc-flow-logs.ts#L122-L144.

@peterwoodworth What exactly did you test to generate the correct options? Because the default is to omit destinationOptions.

peterwoodworth commented 1 year ago

I'm only generating the correct casing on the two specific versions I mentioned (2.55.0, 2.56.0). What I meant was that it successfully deploys for me anyway, for some reason, even with the wrong casing.

In the meantime while we decide how to best address this in our construct library, you can use escape hatches to override the output CDK creates.

sumupitchayan commented 1 year ago

@matthias-pichler-warrify thanks for reporting this. I was also able to replicate this issue. To implement @peterwoodworth's suggestion on escape hatches you can do the following as a temporary fix until we solve this issue:

const cfnFlowLog = flowLog.node.defaultChild as CfnFlowLog;

// Delete incorrect lower-cased properties
cfnFlowLog.addPropertyDeletionOverride('DestinationOptions.fileFormat');
cfnFlowLog.addPropertyDeletionOverride('DestinationOptions.hiveCompatiblePartitions');
cfnFlowLog.addPropertyDeletionOverride('DestinationOptions.perHourPartition');

// Add correct properties
cfnFlowLog.addPropertyOverride('DestinationOptions.FileFormat', FlowLogFileFormat.PLAIN_TEXT);
cfnFlowLog.addPropertyOverride('DestinationOptions.HiveCompatiblePartitions', false);
cfnFlowLog.addPropertyOverride('DestinationOptions.PerHourPartition', false);