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

core: CfnParameter not properly rendered #27064

Closed
emfrab opened this issue Sep 8, 2023 · 4 comments
Closed

core: CfnParameter not properly rendered #27064

emfrab opened this issue Sep 8, 2023 · 4 comments
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@emfrab
Copy link

emfrab commented Sep 8, 2023

Describe the bug

The expected value for a CfnParameter is not properly rendered when accessing it during cfn synth.

According to the guide the way to use CfnParameters is pretty straightforward:

log_retention_period = CfnParameter(self, "LogRetentionPeriod", type="Number", default=1, allowed_values=LOG_RETENTION_PERIOD_ALLOWED_VALUES,
                                    description=("This solution automatically logs events to Amazon CloudWatch. "
                                                 "Select the amount of time for CloudWatch logs from this solution to be retained (in days)."))

image_handler_log_group = logs.LogGroup(self, "LogGroup",
                                                log_group_name=f"/aws/lambda/{image_handler_lambda.function_name}",
                                                retention=logs.RetentionDays(log_retention_period.value_as_number))

Expected Behavior

The value to be referenced with the default (1) or the parameter sent via CLI, i.e cdk synth --parameters LogRetentionPeriod=2

Current Behavior

Fails with the following error:

Traceback (most recent call last):
  File "C:\serverless_image_handler\app.py", line 18, in <module>
    ServerlessImageHandlerStack(app, "serverless-image-handler-stack", environment
  File "C:\Program Files (x86)\Python39\lib\site-packages\jsii\_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
  File "C:\serverless_image_handler\serverless_image_handler\serverless_image_handler_stack.py", line 98, in __init__        
    retention=logs.RetentionDays(log_retention_period.value_as_number))
  File "C:\Program Files (x86)\Python39\lib\enum.py", line 315, in __call__
    return cls.__new__(cls, value)
  File "C:\Program Files (x86)\Python39\lib\enum.py", line 611, in __new__
    raise ve_exc
ValueError: -1.888154589708904e+289 is not a valid RetentionDays

Reproduction Steps

  1. mkdir error_reproduction
  2. cd error_reproduction
  3. cdk init -l python
  4. Paste in error_reproduction/error_reproduction_stack.py:
    from aws_cdk import (
        CfnParameter,
        Stack,
        aws_logs as logs
    )
    from constructs import Construct
    
    class ErrorReproductionStack(Stack):
    
        def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
            super().__init__(scope, construct_id, **kwargs)
    
            log_retention_period = CfnParameter(self, "LogRetentionPeriod", type="Number", default=1, allowed_values=["1", "2", "3"],
                                        description=("This solution automatically logs events to Amazon CloudWatch. "
                                                     "Select the amount of time for CloudWatch logs from this solution to be retained (in days)."))
    
            logs.LogGroup(self, "LogGroup",
                          log_group_name="/aws/test/log",
                          retention=logs.RetentionDays(log_retention_period.value_as_number))
  5. cdk synth

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.95.0 (build cfa7e88)

Framework Version

No response

Node.js Version

v18.17.1

OS

Windows 11

Language

Python

Language Version

Python 3.9.0

Other information

No response

@emfrab emfrab added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 8, 2023
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Sep 8, 2023
@khushail khushail added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Sep 8, 2023
@khushail
Copy link
Contributor

khushail commented Sep 8, 2023

Hi @emfrab , Thanks for reaching out. Yes, this seems to be an issue in rendering the value to ENUM thereby causing the error. I am marking this as P2.

@khushail khushail added p2 effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. needs-triage This issue or PR still needs to be triaged. labels Sep 8, 2023
@peterwoodworth
Copy link
Contributor

@khushail @emfrab this isn't supposed to work for a couple reasons.

First off, 2 isn't a valid member for the enum RetentionDays. You can view the values you can select here

More importantly, the value of a CfnParameter isn't known until deploy time. But the code for LogGroup requires that you are able to parse the enum at synth time. If you try to read the value of a token as a number, you'll end up with a number like -1.888154589708904e+289 because it's trying to parse a token, not an int.

If you need to set it through a CfnParameter, the following solution should work by working around the need to supply a valid enum for RetentionDays:

        param = cdk.CfnParameter(self, "param", type="Number", description="param")
        lg = logs.LogGroup(self, 'lg')
        lg.node.default_child.add_property_override("RetentionInDays", param.value_as_number)

Be sure to note the documentation for Log Groups, 2 doesn't seem to be a valid value accepted by the service in the first place

I supposed we could intake this as a feature request to have a property that accepts a Number, but at that point we would have to do that for every enum for every construct which I don't think is a good idea. Closing out, feel free to ping if there are any further clarifications I can make

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@emfrab
Copy link
Author

emfrab commented Sep 9, 2023

thanks for the detailed response, @peterwoodworth!
I did realize that after opening the issue, but I was having a bit of a hard time to understand why I couldn't retrieve the value of the CfnParameter on cdk synth. I have just started to use CDK, still learning :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality 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

3 participants