aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.65k stars 3.91k forks source link

python docs are incoreect for aws_s3.CfnBucket "logging_configuration" #4900

Open eduardohki opened 4 years ago

eduardohki commented 4 years ago

Hi,

I'm trying to set the S3 Access Logging using the aws_s3.CfnBucket resource.

Based on the documentation found here and here, I'm adding the value { "DestinationBucketName" : "MyLoggingBucket", "LogFilePrefix" : "logging-"} to the logging_configuration parameter.

The problem is that when I generate the CloudFormation Template, I see always an empty LoggingConfiguration property, as the example below:

Resources:
  bucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: my_bucket
      LoggingConfiguration: {}
    Metadata:
      aws:cdk:path: logging/bucket
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Modules: aws-cdk=1.15.0,@aws-cdk/assets=1.15.0,@aws-cdk/aws-cloudwatch=1.15.0,@aws-cdk/aws-ec2=1.15.0,@aws-cdk/aws-events=1.15.0,@aws-cdk/aws-iam=1.15.0,@aws-cdk/aws-kms=1.15.0,@aws-cdk/aws-lambda=1.15.0,@aws
-cdk/aws-logs=1.15.0,@aws-cdk/aws-s3=1.15.0,@aws-cdk/aws-s3-assets=1.15.0,@aws-cdk/aws-s3-notifications=1.15.0,@aws-cdk/aws-sns=1.15.0,@aws-cdk/aws-sqs=1.15.0,@aws-cdk/aws-ssm=1.15.0,@aws-cdk/core=1.15.0,@aws-cdk/c
x-api=1.15.0,@aws-cdk/region-info=1.15.0,jsii-runtime=Python/3.7.5

I think this is a bug (at least in Python) that blocks me into getting further case.

Reproduction Steps

Create a new Stack using Python and add the following code snippet there:

bucket = s3.CfnBucket(
    self, "bucket",
    bucket_name="my_bucket",
    logging_configuration={
        "DestinationBucketName": "MyLoggingBucket",
        "LogFilePrefix": "logging-",
    }
)

Then run cdk synth to see the generated CloudFormation Template.

Error Log

N/A

Environment

Other


This is :bug: Bug Report

skinny85 commented 4 years ago

Hey @eduardohki ,

as can be seen in the docs, the logging_configuration property is of type LoggingConfigurationProperty, and your'e passing a dictionary instead. Your code should look something like:

bucket = s3.CfnBucket(
    self, "bucket",
    bucket_name="my_bucket",
    logging_configuration=s3.CfnBucket.LoggingConfigurationProperty(
        destinationBucketName="MyLoggingBucket",
        logFilePrefix="logging-",
    ),
)

Hope this helps!

Adam

eduardohki commented 4 years ago

Hi @skinny85, thanks for the hint!

I see, I was able to run it using the following syntax:

logging_configuration=s3.CfnBucket.LoggingConfigurationProperty(
    destination_bucket_name="MyLoggingBucket",
    log_file_prefix="logging-",
)

I noticed, however, that the LoggingConfigurationProperty interface is not documented on the Python's side. You can see it by yourself by clicking in the Python-specific link in any of the mentioned pages.

I see the same with BucketEncryptionProperty, for example (https://github.com/aws/aws-cdk/issues/4902)

skinny85 commented 4 years ago

Yep, you are correct @eduardohki - that's an error in the Python version of the docs.

eduardohki commented 4 years ago

I was wondering if there's space for improvements here... like throwing an explicit error when I try to use a Python dict as an input. I see that this can happen very often, based on that we rely strongly on TS code examples to apply the same logic in Python.

eladb commented 4 years ago

Assigning to @RomainMuller to see if we can improve the python errors.

RomainMuller commented 4 years ago

@eduardohki - I reckon the situation here has improved in 1.16 -- the dict should be handled correctly now. Can you please check with the latest CDK and let us know whether you are still affected?

eduardohki commented 4 years ago

Hi @RomainMuller,

I just tested again, with the following code:

bucket = s3.CfnBucket(
    self,
    "MyBucket",
    bucket_name="MyBucket",
    logging_configuration={
        "DestinationBucketName": "MyLoggingBucket",
        "LogFilePrefix": "logging-",
    },
)

And I receive no warnings or errors, in fact I receive the same cdk synth output as before:

Resources:
  MyBucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: MyBucket
      LoggingConfiguration: {}  # <-- HERE
    Metadata:
      aws:cdk:path: S3Logs/MyBucket
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Modules: aws-cdk=1.16.3,@aws-cdk/assets=1.16.2,@aws-cdk/aws-athena=1.15.0,@aws-cdk/aws-cloudwatch=1.16.2,@aws-cdk/aws-config=1.16.2,@aws-cdk/aws-ec2=1.16.2,@aws-cdk/aws-events=1.16.2,@aws-cdk/aws-glue=1.15.0,@aws-cdk/aws-iam=1.16.2,@aws-cdk/aws-kms=1.16.
2,@aws-cdk/aws-lambda=1.16.2,@aws-cdk/aws-logs=1.16.2,@aws-cdk/aws-s3=1.16.2,@aws-cdk/aws-s3-assets=1.16.2,@aws-cdk/aws-s3-notifications=1.15.0,@aws-cdk/aws-sns=1.16.2,@aws-cdk/aws-sqs=1.16.2,@aws-cdk/aws-ssm=1.16.2,@aws-cdk/core=1.16.2,@aws-cdk/cx-api=1.16.2,
@aws-cdk/region-info=1.16.2,jsii-runtime=Python/3.7.5
RomainMuller commented 4 years ago

@eduardohki - thanks for the confirmation.

RomainMuller commented 4 years ago

So - it appears that the reason why the dict is not correctly processed is essentially because logging_configuration is typed as a union of IResolvable and CfnBucket.LoggingConfigurationProperty, which evades the code that'll automatically hydrate a CfnBucket.LoggingConfigurationProperty from the provided dict.

In this particular case, the only safe mode of operation is to "manually" instantiate the CfnBucket.LoggingConfigurationProperty instance as @skinny85 had suggested. There was definitely a change of behavior here, but the previous version would have had other issues in other contexts.

The fact that the Python documentation shows it as ForwardRef is however very definitely not something that was expected... And we have to look into that.

eduardohki commented 4 years ago

Thanks for your reply @RomainMuller

I understand about the limitation in the Python implementation, and my wish is to use CfnBucket.LoggingConfigurationProperty (I did so).

The issue here IMO is the lack of this proper information in the Python Docs, as you stated.

RomainMuller commented 4 years ago

@eduardohki absolutely!