Open Rondineli opened 8 months ago
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
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).
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.
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.
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.
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.
@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.
@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
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.
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 Based off of @pahud's workaround of using
addPropertyOverride
onRules
, if you just changed the first parameter toRules.1
it seems to work, and that way it shouldn't override the entireRules
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.
@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.
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:
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 }, ... ]
.
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.
@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.
Hey @Rondineli, please go ahead with re-working your PR and thank you for picking this up. :)
Describe the bug
Trying to add a rule to an specific key breaks as Action is not added:
At the final template, the
Action
block is not present:Expected Behavior
Expected the rule json output to have the
Action
blockCurrent 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