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.

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

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

Open
matthias-pichler opened this issue May 4, 2023 · 5 comments
Open

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

matthias-pichler opened this issue May 4, 2023 · 5 comments
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@matthias-pichler
Copy link
Contributor

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

@matthias-pichler matthias-pichler added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 4, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label May 4, 2023
@peterwoodworth
Copy link
Contributor

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

"AWS::EC2::FlowLog": {
"patch": {
"description": "This patch fixes all types that were previously typed as Json, and CfnSpec v101.0.0 added types to them, which is a breaking change.",
"operations": [
{
"op": "remove",
"path": "/Properties/DestinationOptions/Type"
},
{
"op": "add",
"path": "/Properties/DestinationOptions/PrimitiveType",
"value": "Json"
}
]
}
},

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)

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels May 4, 2023
@github-actions
Copy link

github-actions bot commented May 6, 2023

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.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 6, 2023
@matthias-pichler
Copy link
Contributor Author

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

destinationOptions: destinationConfig.destinationOptions,

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:

export interface S3DestinationOptions {
/**
* Use Hive-compatible prefixes for flow logs
* stored in Amazon S3
*
* @default false
*/
readonly hiveCompatiblePartitions?: boolean;
/**
* The format for the flow log
*
* @default FlowLogFileFormat.PLAIN_TEXT
*/
readonly fileFormat?: FlowLogFileFormat;
/**
* Partition the flow log per hour
*
* @default false
*/
readonly perHourPartition?: boolean;
}
.

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

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels May 7, 2023
@peterwoodworth
Copy link
Contributor

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
Copy link
Contributor

@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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

4 participants