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

wafv2: CfnWebACL.JsonMatchPatternProperty.all doesn't accept any value as documented #30666

Open
gravieure opened this issue Jun 25, 2024 · 10 comments
Labels
@aws-cdk/aws-wafv2 bug This issue is a bug. effort/small Small work item – less than a day of effort jsii This issue originates in jsii, or this feature must be implemented in jsii. p3

Comments

@gravieure
Copy link

gravieure commented Jun 25, 2024

Describe the bug

This pertains to the Python bindings, but I believe the issue affects TypeScript as well, though to a lesser degree.

The documentation for JsonMatchPatternProperty.all states:

all (Any) – Match all of the elements. See also MatchScope in the JsonBody FieldToMatch specification. You must specify either this setting or the IncludedPaths setting, but not both.

However, if I specify all=True, cdk synth fails:

RuntimeError: Error: Resolution error: Supplied properties not correct for "CfnWebACLProps"
  rules: element 2: supplied properties not correct for "RuleProperty"
    statement: supplied properties not correct for "StatementProperty"
      sqliMatchStatement: supplied properties not correct for "SqliMatchStatementProperty"
        fieldToMatch: supplied properties not correct for "FieldToMatchProperty"
          jsonBody: supplied properties not correct for "JsonBodyProperty"
            matchPattern: supplied properties not correct for "JsonMatchPatternProperty"
              all: true should be an 'object'.

So the type hint is incorrect; a value of Any type is not legal for the all argument.

The example code in the documentation is:

import aws_wafv2 as wafv2

# all: Any

json_match_pattern_property = wafv2.CfnWebACL.JsonMatchPatternProperty(
    all=all,
    included_paths=["includedPaths"]
)

This violates the immediately preceding text, "You must specify either this setting or the IncludedPaths setting, but not both." Specifying only all=all does not work; all is a built-in function in Python, which causes a JSII error:

jsii.errors.JSIIError: Cannot pass function as argument here (did you mean to call this function?): <built-in function all>

It appears that I have to pass some JSII-serializable value here to indicate truthy state, such as:

match_pattern=waf.CfnWebACL.JsonMatchPatternProperty(all={}),

This is a very unusual way to express a boolean, particularly because an empty dict is considered False in Python:

>>> "true" if {} else "false"
'false'
>>> "true" if {"some": "value"} else "false"
'true'
>>> 

Expected Behavior

  • I expected that a value conforming to the type hint of the all attribute would work.
  • I expected that the example code would work.
  • I expect fields for boolean options to accept boolean values.
  • I expect fields taking truthy/falsey values to conform to Python's notions of truthy and falsey.

Current Behavior

  • Example code does not work.
  • Boolean values are not accepted.
  • Values treated as falsey by Python are treated as truthy by CDK.

Reproduction Steps

  1. Synth a stack containing a WAF with JsonMatchPatternProperty whose all value is all, as the example code does.
  2. Observe that the synth fails

or

  1. Synth a stack containing a WAF with JsonMatchPatternProperty whose all value is True.
  2. Observe that the synth fails.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.147.1 (build d3695d4)

Framework Version

2.147.1

Node.js Version

v18.15.0

OS

macOS Sonoma 14.5 (23F79)

Language

Python

Language Version

Python 3.10.7

Other information

The TypeScript documentation also says that all? can accept any type, though this is not true.

@gravieure gravieure added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 25, 2024
@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 Jun 25, 2024
@khushail khushail self-assigned this Jun 25, 2024
@khushail
Copy link
Contributor

@gravieure , thanks for reaching out. I am not able to reproduce the issue. Could you please share a minnimal reproducible code snippet?

@khushail khushail added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jun 26, 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 Jun 28, 2024
@gravieure
Copy link
Author

@gravieure , thanks for reaching out. I am not able to reproduce the issue. Could you please share a minnimal reproducible code snippet?

The sample code in the documentation is a reproducer.

You can reproduce the other described behavior by changing the all=all in that code to all=True.

@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 Jun 28, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. effort/small Small work item – less than a day of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. labels Jun 28, 2024
@khushail
Copy link
Contributor

@gravieure , Thanks for clarification.

Since this is a CFN issue, please create an issue with the CFN team here on their Cloudformation coverage map and follow that one for updates/resolution.

Thanks.

@khushail khushail removed the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Jun 28, 2024
@khushail khushail removed their assignment Jun 28, 2024
@gravieure
Copy link
Author

@gravieure , Thanks for clarification.

Since this is a CFN issue, please create an issue with the CFN team here on their Cloudformation coverage map and follow that one for updates/resolution.

Thanks.

I apologize for not understanding, but could you explain why this is CFN issue? The type specified in CDK's code is incorrect -- the declared type is any, but values of any type are not accepted. The example code with all=all (which doesn't work) is in the CDK documentation.

Are the types in the CDK code, and the examples in the CDK documentation generated from CFN?

@khushail khushail added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Jun 28, 2024
@khushail khushail self-assigned this Jun 28, 2024
@khushail
Copy link
Contributor

Ah My Bad @gravieure , I missed comprehending the whole context. I will dive deep and get back to you

@khushail khushail removed the needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. label Jun 28, 2024
@khushail
Copy link
Contributor

khushail commented Jul 8, 2024

@gravieure , AFAIK, the doc generation is from jsii-docgen which generates the API.md and we tend to write the original implementation in TypeScript and have jsii-docgen to generate docs for different languages. I noticed in the Tyepscript as well as Python doc that all and includePaths should not have come together in the example code.

Also CfnWebACL is cloudformation resource (Doc) - all takes JSON object as mentioned in the doc . An example is also given which explains how values are passed. Types are also auto generated. This seems to be a JSII related issue and how docs are autogenerated. So marking it as appropriate.

@khushail khushail added the p3 label Jul 8, 2024
@khushail khushail added jsii This issue originates in jsii, or this feature must be implemented in jsii. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jul 8, 2024
@khushail khushail removed their assignment Jul 8, 2024
@gravieure
Copy link
Author

AFAIK , the doc generation is from jsii-docgen which generates the API.md and we tend to write the original implementation in TypeScript and have jsii-docgen to generate docs for different languages. I noticed in the Tyepscript as well as Python doc that all and includePaths should not have come together in the example code.

Also CfnWebACL is cloudformation resource (Doc) - all takes JSON object as mentioned in the doc . An example is also given which explains how values are passed. Types are also auto generated.

Thank you for the explanations.

I note that the same incorrect type and example exists in the TypeScript docs and example -- the field type is any, but I'm fairly sure that true wouldn't work in this instance, either.

It's extra confusing in Python, though, because an empty dict is treated as false -- so a false value in the host language is used as a true value by CDK.

Are there any plans to improve this API? I've found it much more difficult to work with than other CDK APIs, and since WAF is an important security control, I think the chances of getting something wrong in a way that negatively impacts security is high.

@khushail
Copy link
Contributor

khushail commented Jul 9, 2024

@gravieure , thanks for keeping patience!

I don't have any ETA but team surely has plans of improving the docs experience for the community.

@gravieure
Copy link
Author

@gravieure , thanks for keeping patience!

I don't have any ETA but team surely has plans of improving the docs experience for the community.

Thank you. Please note that there are two components to my report: code problems and documentation problems.

The code is buggy, because it declares that certain fields can accept values of Any type, when they cannot. This leads to the nonsensical situation where fields representing a boolean can't accept True as their value -- they need a value which is considered false by Python. This is very confusing and unintuitative.

The examples in the documentation are incomplete and incorrect, which I suspect is because it's some kind of auto-generated output based on the code, which is buggy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-wafv2 bug This issue is a bug. effort/small Small work item – less than a day of effort jsii This issue originates in jsii, or this feature must be implemented in jsii. p3
Projects
None yet
Development

No branches or pull requests

2 participants