wafv2: add_override does not obey Action key

[Rondineli]

Describe the bug

Trying to add a rule to an specific key breaks as Action is not added:

waf_acl = wafv2.CfnWebACL(self, 'id', **params)
# Assuming above we would have only one rule
new_rule = {
    "Action": {
        "Block": {}
    },
    "Name": "Deny_regex",
    "Priority": 1,
    "Statement": {
        "RegexPatternSetReferenceStatement": {
            "Arn": {
                "Fn::ImportValue": "my-regex-arn"
            },
            "FieldToMatch": {
                "SingleHeader": {
                    "Name": "my-header"
                }
            },
            "TextTransformations": [
                {
                    "Priority": 0,
                    "Type": "NONE"
                }
            ]
        }
    },
    "VisibilityConfig": {
        "CloudWatchMetricsEnabled": True,
        "MetricName": "Deny_regex",
        "SampledRequestsEnabled": True
    }
}
waf_acl.add_override('Properties.Rules.1', new_rule)

At the final template, the Action block is not present:

{
    "Name": "Deny_regex",
    "Priority": 1,
    "Statement": {
        "RegexPatternSetReferenceStatement": {
            "Arn": {
                "Fn::ImportValue": "my-regex-arn"
            },
            "FieldToMatch": {
                "SingleHeader": {
                    "Name": "my-header"
                }
            },
            "TextTransformations": [
                {
                    "Priority": 0,
                    "Type": "NONE"
                }
            ]
        }
    },
    "VisibilityConfig": {
        "CloudWatchMetricsEnabled": true,
        "MetricName": "Deny_regex",
        "SampledRequestsEnabled": true
    }
}

Expected Behavior

Expected the rule json output to have the Action block

Current Behavior

Somehow, the Action json block is not being added to the final template

Reproduction Steps

Create a waf, and try to use add_override with a plain json rule on wafv2

Possible Solution

Add Action to the jsii interface mappings? Somehow Action is missing.

Additional Information/Context

No response

CDK CLI Version

2.127.0 (build 6c90efc)

Framework Version

No response

Node.js Version

v18.17.0

OS

MacOs

Language

Python

Language Version

Python 3.9.6

Other information

No response

[pahud]

I guess you should just use Rules

This works for me in ts

export class DummyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    const new_rule = {
      "Action": {
          "Block": {}
      },
      "Name": "Deny_regex",
      "Priority": 1,
      "Statement": {
          "RegexPatternSetReferenceStatement": {
              "Arn": {
                  "Fn::ImportValue": "my-regex-arn"
              },
              "FieldToMatch": {
                  "SingleHeader": {
                      "Name": "my-header"
                  }
              },
              "TextTransformations": [
                  {
                      "Priority": 0,
                      "Type": "NONE"
                  }
              ]
          }
      },
      "VisibilityConfig": {
          "CloudWatchMetricsEnabled": "True",
          "MetricName": "Deny_regex",
          "SampledRequestsEnabled": "True"
      }
  }

    const cfnwebacl = new wafv2.CfnWebACL(this, 'WebACL', {
      defaultAction: {},
      scope: 'CLOUDFRONT',
      visibilityConfig: {
        cloudWatchMetricsEnabled: true,
        metricName: 'foo',
        sampledRequestsEnabled: true,
      }
    });
    cfnwebacl.addPropertyOverride('Rules', [ new_rule ])
  }
}

synth output

Resources:
  WebACL:
    Type: AWS::WAFv2::WebACL
    Properties:
      DefaultAction: {}
      Rules:
        - Action:
            Block: {}
          Name: Deny_regex
          Priority: 1
          Statement:
            RegexPatternSetReferenceStatement:
              Arn:
                Fn::ImportValue: my-regex-arn
              FieldToMatch:
                SingleHeader:
                  Name: my-header
              TextTransformations:
                - Priority: 0
                  Type: NONE
          VisibilityConfig:
            CloudWatchMetricsEnabled: "True"
            MetricName: Deny_regex
            SampledRequestsEnabled: "True"
      Scope: CLOUDFRONT
      VisibilityConfig:
        CloudWatchMetricsEnabled: true
        MetricName: foo
        SampledRequestsEnabled: true

[Rondineli]

Yes, it does, but in my case I would need to change only partial the rules. Not the entire Property unfortunately.

Part of the objects are made from cdk, and part from cfn (as for shield and other rules in plain json).

[Rondineli]

As part of my rules are cdk objects, I cant use it to replace the whole property, according to this:

The `value` argument to `addOverride` will not be processed or translated
   * in any way. Pass raw JSON values in here with the correct capitalization
   * for CloudFormation. If you pass CDK classes or structs, they will be
   * rendered with lowercased key names, and CloudFormation will reject the
   * template.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/core/lib/cfn-resource.ts#L224C6-L228C15

[Rondineli]

Tehe deletion is happening on this block: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/core/lib/cfn-resource.ts#L728

I added some logs locally and can see This deletion happening:

if (typeof(output) === 'object' && Object.keys(output).length === 0) {
     console.log(`Target deleting ====> ${JSON.stringify(target)}::::${key}`);
     delete target[key];
}

Then, getting this:

Target deleting ====> {"Block":{}}::::Block
....
Target deleting ====> {"Name":"..."}}]}},"Action":{}}::::Action
...

So, the deepMerge method is trying to delete objects from the add_override in a wrong way, going to try to push a PR for fixing it.

[paulhcsun]

As part of my rules are cdk objects, I cant use it to replace the whole property, according to this:

You can override the property using escape hatches: https://docs.aws.amazon.com/cdk/v2/guide/cfn_layer.html

I hesitate to accept the PR as there may be some backwards compatibility concerns, but an escape hatch is probably the best option here.

[Rondineli]

I tried the escape hatch by trying to override only the Action key as well, and it did not work, anything that maybe mix the cdk construct and cfn plain resource property wont work it seems. As the add_override will just delete it, only way to keep it is transforming it to a string (which will make the cfn fail for obvious reasons).

If you are ok, I can work out the PR and make it work or, any approach preferred for this.

[paulhcsun]

@Rondineli I see, let's continue with the PR then. I've left some comments but I think if we want to fix it with that approach then it should be added behind a feature flag to avoid breaking existing customers.

[paulhcsun]

@Rondineli Based off of @pahud's workaround of using addPropertyOverride on Rules, if you just changed the first parameter to Rules.1 it seems to work, and that way it shouldn't override the entire Rules property.

    const new_rule = {
      "Action": {
          "Block": {}
      },
      .
      .
      .
      "VisibilityConfig": {
          "CloudWatchMetricsEnabled": "True",
          "MetricName": "Deny_regex",
          "SampledRequestsEnabled": "True"
      }
  }

    const cfnwebacl = new wafv2.CfnWebACL(this, 'WebACL', {
      defaultAction: {},
      scope: 'CLOUDFRONT',
      visibilityConfig: {
        cloudWatchMetricsEnabled: true,
        metricName: 'foo',
        sampledRequestsEnabled: true,
      }
    });
    cfnwebacl.addPropertyOverride('Rules.1', [ new_rule ])

Output from cdk synth:

Resources:
  WebACL:
    Type: AWS::WAFv2::WebACL
    Properties:
      DefaultAction: {}
      Rules:
        "1":
          - Action:
              Block: {}
            Name: Deny_regex
            Priority: 1
            Statement:
              RegexPatternSetReferenceStatement:
                Arn:
                  Fn::ImportValue: my-regex-arn
                FieldToMatch:
                  SingleHeader:
                    Name: my-header
                TextTransformations:
                  - Priority: 0
                    Type: NONE
            VisibilityConfig:
              CloudWatchMetricsEnabled: "True"
              MetricName: Deny_regex
              SampledRequestsEnabled: "True"
      Scope: CLOUDFRONT
      VisibilityConfig:
        CloudWatchMetricsEnabled: true
        MetricName: foo
        SampledRequestsEnabled: true
    Metadata:
      aws:cdk:path: Wafv2Stack/WebACL

[TheRealAmazonKendra]

If what @paulhcsun posted above doesn't work, can you please elaborate more on your use case for having that empty block there? In its current state, I don't think that this is a change we can accept even with a feature flag. There is too much potential for unintended consequences. We do, however, understand that it's important to help find a solution and/or workaround here and we do not want to leave you blocked.

I'm not terribly familiar with this service so I want to make sure I thoroughly understand the problem you're working to solve with your proposed PR.

[TheRealAmazonKendra]

Oh, actually, I did some additional digging and I understand now. I don't think this is quite the right fix here, but you're absolutely right that this is an edge case we didn't take into account. Hopefully @paulhcsun's workaround gets you something workable in the short term, we do need to better handle this edge case. I'm looking into it.

[Rondineli]

@Rondineli Based off of @pahud's workaround of using addPropertyOverride on Rules, if you just changed the first parameter to Rules.1 it seems to work, and that way it shouldn't override the entire Rules property.

    const new_rule = {
      "Action": {
          "Block": {}
      },
      .
      .
      .
      "VisibilityConfig": {
          "CloudWatchMetricsEnabled": "True",
          "MetricName": "Deny_regex",
          "SampledRequestsEnabled": "True"
      }
  }

    const cfnwebacl = new wafv2.CfnWebACL(this, 'WebACL', {
      defaultAction: {},
      scope: 'CLOUDFRONT',
      visibilityConfig: {
        cloudWatchMetricsEnabled: true,
        metricName: 'foo',
        sampledRequestsEnabled: true,
      }
    });
    cfnwebacl.addPropertyOverride('Rules.1', [ new_rule ])

Output from cdk synth:

Resources:
  WebACL:
    Type: AWS::WAFv2::WebACL
    Properties:
      DefaultAction: {}
      Rules:
        "1":
          - Action:
              Block: {}
            Name: Deny_regex
            Priority: 1
            Statement:
              RegexPatternSetReferenceStatement:
                Arn:
                  Fn::ImportValue: my-regex-arn
                FieldToMatch:
                  SingleHeader:
                    Name: my-header
                TextTransformations:
                  - Priority: 0
                    Type: NONE
            VisibilityConfig:
              CloudWatchMetricsEnabled: "True"
              MetricName: Deny_regex
              SampledRequestsEnabled: "True"
      Scope: CLOUDFRONT
      VisibilityConfig:
        CloudWatchMetricsEnabled: true
        MetricName: foo
        SampledRequestsEnabled: true
    Metadata:
      aws:cdk:path: Wafv2Stack/WebACL

I tried this approach, and there 2 problems:

1* Can you see the Rules: "1": {parameters} ? It is not the correct syntax, right?
2* When you initiate your object with rules and cdk rule objects, it also breaks as it causes a mess with the rules and types on the final template.

I can try it again and post the output, but I have tried that and the final template gets really messed up.

[Rondineli]

@paulhcsun sorry the late reply here, but, as I said, when trying what you suggested, I get this on cloudformation:

....
"Model validation failed (#/Rules: expected type: JSONArray, found: JSONObject
...

The only way is either add it trough the Override and using deepMerge, wafv2 is too complex to that method imho.

I am planning to fix the reviews from my PR to make it work, but let me know if you need any more info on that.

[vinayak-kukreja]

Hey @Rondineli, thank you for giving us visibility into this issue and also sharing context.

You are correct that deepMerge is removing the empty object. We are trying to resolve resource definition and raw overrides, where we keep the empty objects because that is how deletion overrides are represented.

Now, since this has been the behavior till now, we cannot change it in-place since the blast radius could be huge and impact multiple users.

But, I also agree that this needs to be fixed and we should not be removing an empty object while doing a deep merge. We discussed this internally and one of the suggested approach is:

1. To Accumulate the addOverride call into a list without trying to interpret them in any way. For instance, if we have

   {
       "x": {
           "y": {
               "z": "someValue"
           }
       }
   }
   

   then, we can have in the list [ { path: 'x.y.z', value: someValue }, ... ].
2. Now, when we are rendering it, we can do the following:

   const regularProps = resolve(regularProps);
   
   for override of this.overrides {
     const finalValue = resolve(override.Value);
     if finalValue !== undefined {
       applyPath(regularProps, override.path, finalValue);
     }
   }
   

We will need to add this behind a feature flag and this will become the default behavior for anyone using the feature flag.

[Rondineli]

@vinayak-kukreja I got it!

That makes sense, as the Keys can be used in others resources and being removed. I can re-work my PR to fit your suggestions or if you guys want re-work it also would be fine for me.

[vinayak-kukreja]

Hey @Rondineli, please go ahead with re-working your PR and thank you for picking this up. :)