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

FIS: Experiment Template Log Configuration Class Camel Cases LogGroupArn #29761

Closed
hakenmt opened this issue Apr 8, 2024 · 9 comments
Closed
Labels
@aws-cdk/aws-fis Related to the @aws-cdk/aws-fis package bug This issue is a bug. cli Issues related to the CDK CLI closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@hakenmt
Copy link

hakenmt commented Apr 8, 2024

Describe the bug

Using the CfnExperimentTemplate construct in .net. Using the below log group configuration.

LogConfiguration = new ExperimentTemplateLogConfigurationProperty() {
                    CloudWatchLogsConfiguration = new CloudWatchLogsConfigurationProperty() {
                        LogGroupArn = this.LogGroup.LogGroupArn
                    }
                }

This produces output like the following:

"LogConfiguration": {
     "CloudWatchLogsConfiguration": {
      "logGroupArn": {
       "Fn::GetAtt": [
        "logGroup68A52FBE",
        "Arn"
       ]
      }
     },
     "LogSchemaVersion": 0
    },

The property logGroupArn isn't accepted by CloudFormation, it must be LogGroupArn.

Expected Behavior

Produces this output:

"LogConfiguration": {
     "CloudWatchLogsConfiguration": {
      "LogGroupArn": {
       "Fn::GetAtt": [
        "logGroup68A52FBE",
        "Arn"
       ]
      }
     },
     "LogSchemaVersion": 0
    },

Current Behavior

Produces camel case name.

Reproduction Steps

LogConfiguration = new ExperimentTemplateLogConfigurationProperty() {
                    CloudWatchLogsConfiguration = new CloudWatchLogsConfigurationProperty() {
                        LogGroupArn = this.LogGroup.LogGroupArn
                    }
                }

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.133.0 (build dcc1e75)

Framework Version

No response

Node.js Version

v20.9.0

OS

darwin

Language

.NET

Language Version

No response

Other information

No response

@hakenmt hakenmt added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 8, 2024
@github-actions github-actions bot added the @aws-cdk/aws-fis Related to the @aws-cdk/aws-fis package label Apr 8, 2024
@nmussy
Copy link
Contributor

nmussy commented Apr 8, 2024

Confirmed in TypeScript and with v2.136.0. See also the AWS::FIS::ExperimentTemplate CloudWatchLogsConfiguration CloudFormation docs

import { CfnExperimentTemplate } from 'aws-cdk-lib/aws-fis';

// ...
new CfnExperimentTemplate(this, "ExperimentTemplate", {
  description: "description",
  roleArn: "roleArn",
  logConfiguration: {
    cloudWatchLogsConfiguration: {
      logGroupArn: "logArn",
    } satisfies CfnExperimentTemplate.CloudWatchLogsConfigurationProperty,
    logSchemaVersion: 0,
  },
  stopConditions: [
    {
      source: "aws:cloudwatch:alarm",
      value: "alarmArn",
    },
  ],
  targets: {
    name: {
      resourceType: "aws:ec2:instance",
      resourceArns: ["resourceArn"],
      selectionMode: "64",
    },
  },
});
$ npm run cdk synth

Resources:
  ExperimentTemplate:
    Type: AWS::FIS::ExperimentTemplate
    Properties:
      Description: description
      LogConfiguration:
        CloudWatchLogsConfiguration:
          logGroupArn: logArn
        LogSchemaVersion: 0
      RoleArn: roleArn
      StopConditions:
        - Source: aws:cloudwatch:alarm
          Value: alarmArn
      Targets:
        name:
          ResourceArns:
            - resourceArn
          ResourceType: aws:ec2:instance
          SelectionMode: "64"

EDIT: Updated the repro, previous example didn't prove anything

@nmussy
Copy link
Contributor

nmussy commented Apr 8, 2024

Not sure how the L1 generated constructs handle this, but I'm going to assume that this is happening because the cloudWatchLogsConfiguration property is not properly typed, see docs. Because it's just any object, I think that the CDK will not translate the property key because it does not expect it to be a CloudFormation object.

I'm not great in C# @hakenmt, but what you can do in the meantime is just not type the cloudWatchLogsConfiguration value, something like this:

LogConfiguration = new ExperimentTemplateLogConfigurationProperty() {
    CloudWatchLogsConfiguration = new {
        LogGroupArn = this.LogGroup.LogGroupArn
    }
}

@hakenmt
Copy link
Author

hakenmt commented Apr 8, 2024

Thanks for the workaround, I actually ended up doing this:

this.PacketLossExperiment.AddOverride("Properties.LogConfiguration.CloudWatchLogsConfiguration.LogGroupArn", this.LogGroup.LogGroupArn);
this.PacketLossExperiment.AddOverride("Properties.LogConfiguration.CloudWatchLogsConfiguration.logGroupArn", null);

I think both work, but obviously ideally would like to use the concrete generated type for the property if possible. The entire ExperimentTemplate construct is fairly hard to navigate, the Action and Target properties are both just object? objects (like any), instead of being something like Dictionary<string, ExperimentTemplateTargetProperty>.

@nmussy
Copy link
Contributor

nmussy commented Apr 8, 2024

Yeah, your solution at least keeps some type safety 👍

As far as the issues with the generated L1, I don't have a ton of insight into that process. There was an issue raised about creating an L2 construct for ExperimentTemplate (#13744), but it never got a PR

@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 8, 2024
@khushail khushail self-assigned this Apr 8, 2024
@khushail
Copy link
Contributor

khushail commented Apr 8, 2024

Hi @hakenmt , thanks for reaching out with this bug info.
Thanks @nmussy for sharing the repro code in Typescript. I also see the casing issue in generated synth output template.
Marking this issue as appropriate. Also Please feel free to submit a PR for L2 construct if you are willing.

@khushail khushail added cli Issues related to the CDK CLI effort/medium Medium work item – several days of effort p2 and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Apr 8, 2024
@khushail khushail removed their assignment Apr 8, 2024
@pahud
Copy link
Contributor

pahud commented Apr 9, 2024

@nmussy
Copy link
Contributor

nmussy commented Apr 9, 2024

@pahud From what I gather, https://github.com/cdklabs/awscdk-service-spec needs to get a patch PR to override the currently generated L1?

@pahud
Copy link
Contributor

pahud commented Apr 9, 2024

@nmussy No, from my take, it's because it’s originally set to Json in cfnspec but at some point was added to an actual type by service team. To avoid breaking changes in CDK, we added a patch from awscdk-service-spec to make it always JSON type even it’s now typed in CFN. We have some other similar cases mentioned in #21767

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 9, 2024
Copy link

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 Apr 12, 2024
@github-actions github-actions bot added closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-fis Related to the @aws-cdk/aws-fis package bug This issue is a bug. cli Issues related to the CDK CLI closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

4 participants