fix(redshift): deploy fails when creating logging bucket without s3 key #21243

sean-beath · 2022-07-20T06:05:27Z

To send these Amazon Redshift logging information about connections and user activities in your database to an S3 bucket, specify an S3 bucket and prefix using an interface.

CFN documentation has S3KeyPrefix as optional, but testing has shown that key is required and CFN error (same as in below issue) will be thrown when key is not provided.

closes: #19514.

BREAKING CHANGE: The way to specify a logging bucket and prefix will change to use an interface.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-07-20T06:05:35Z

TheRealAmazonKendra · 2022-07-21T00:50:05Z

Can you clarify for me if the issue here is that he CFN documentation is incorrect and the prefix key is, indeed, required by them? Or is this our logic that makes it necessary despite it being an optional prop?

sean-beath · 2022-07-21T01:09:29Z

Can you clarify for me if the issue here is that he CFN documentation is incorrect and the prefix key is, indeed, required by them? Or is this our logic that makes it necessary despite it being an optional prop?

I believe this field is required when adding a logging bucket as I tested the following:

Deploy Redshift cluster without logging bucket or logging prefix
Attempt deploy of Redshift cluster with logging bucket and no logging prefix (failed deployment, as expected)
Attempt deploy of Redshift cluster with both logging bucket and logging prefix (successful deployment)
Compare cfn template from 2 and 3 - the only difference being the logging prefix removed with no JSON errors (see below)

Without logging prefix:

  Redshift8A119849:
    Type: AWS::Redshift::Cluster
    Properties:
      ClusterType: multi-node
      DBName: default_db
      MasterUsername:
        Fn::Join:
          - ""
          - - "{{resolve:secretsmanager:"
            - Ref: RedshiftSecretA08D42D6
            - :SecretString:username::}}
      MasterUserPassword:
        Fn::Join:
          - ""
          - - "{{resolve:secretsmanager:"
            - Ref: RedshiftSecretA08D42D6
            - :SecretString:password::}}
      NodeType: dc2.large
      AllowVersionUpgrade: true
      AutomatedSnapshotRetentionPeriod: 1
      ClusterSubnetGroupName:
        Ref: RedshiftSubnetsDFE70E0A
      Encrypted: true
      IamRoles:
        - Fn::GetAtt:
            - RedshiftRoleBD8C342F
            - Arn
      LoggingProperties:
        BucketName:
          Ref: RedshiftLogs2FBC9C20
      NumberOfNodes: 2
      PubliclyAccessible: false
      VpcSecurityGroupIds:
        - Fn::GetAtt:
            - RedshiftSecurityGroup796D74A7
            - GroupId
    UpdateReplacePolicy: Delete
    DeletionPolicy: Delete
    Metadata:
      aws:cdk:path: RedshiftLoggingBucketStack/Redshift/Resource
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/21SwY6CMBD9Fu+lrnDa27quMV42BIzXTSkjdIWWtFONafj3LRTFbDy9N2+m0/fSxjRZURcv2NVEvDxHjSioy5HxM/HSjwMeU3fsONmc5DHdkNQWjeC5LSTgoM0sUxbhwIoGZn3W1sYoLhgKJR/DA9nu0wG+Ge4YwpXdSKrFxdN58V4iaM/vA8HJVK3Re61bkEgmxZvdyrJTQuI0+ihz4FYLvO20st3o8lnoiWAtdZkKCUZMlU87XhlYT0xC3afl5+BtYgHm4ee6JxpKU4sTUudbm8YanyfEexiZVL8euAY0LZOsAj2eyEeJBDgwXQE+pX4M/O/0PcnAKKu5j+O3q3YufbrXrVSriyhBE/9egP4fVEJWo0ElSzG8Xk/iiDVdzejb4mP6NcsB7ymnrpsikReBvxiyghkIxr3R9Ia1ksuEvtNVsvg1QkTaShQt0CzgH/N0+BaoAgAA
    Metadata:
      aws:cdk:path: RedshiftLoggingBucketStack/CDKMetadata/Default
    Condition: CDKMetadataAvailable

With logging prefix:

  Redshift8A119849:
    Type: AWS::Redshift::Cluster
    Properties:
      ClusterType: multi-node
      DBName: default_db
      MasterUsername:
        Fn::Join:
          - ""
          - - "{{resolve:secretsmanager:"
            - Ref: RedshiftSecretA08D42D6
            - :SecretString:username::}}
      MasterUserPassword:
        Fn::Join:
          - ""
          - - "{{resolve:secretsmanager:"
            - Ref: RedshiftSecretA08D42D6
            - :SecretString:password::}}
      NodeType: dc2.large
      AllowVersionUpgrade: true
      AutomatedSnapshotRetentionPeriod: 1
      ClusterSubnetGroupName:
        Ref: RedshiftSubnetsDFE70E0A
      Encrypted: true
      IamRoles:
        - Fn::GetAtt:
            - RedshiftRoleBD8C342F
            - Arn
      LoggingProperties:
        BucketName:
          Ref: RedshiftLogs2FBC9C20
        S3KeyPrefix: AWSLogs
      NumberOfNodes: 2
      PubliclyAccessible: false
      VpcSecurityGroupIds:
        - Fn::GetAtt:
            - RedshiftSecurityGroup796D74A7
            - GroupId
    UpdateReplacePolicy: Delete
    DeletionPolicy: Delete
    Metadata:
      aws:cdk:path: RedshiftLoggingBucketStack/Redshift/Resource
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/21SwY6CMBD9Fu+lrnDa27quMV42BIzXTSkjdIWWtFONafj3LRTFbDy9N2+m0/fSxjRZURcv2NVEvDxHjSioy5HxM/HSjwMeU3fsONmc5DHdkNQWjeC5LSTgoM0sUxbhwIoGZn3W1sYoLhgKJR/DA9nu0wG+Ge4YwpXdSKrFxdN58V4iaM/vA8HJVK3Re61bkEgmxZvdyrJTQuI0+ihz4FYLvO20st3o8lnoiWAtdZkKCUZMlU87XhlYT0xC3afl5+BtYgHm4ee6JxpKU4sTUudbm8YanyfEexiZVL8euAY0LZOsAj2eyEeJBDgwXQE+pX4M/O/0PcnAKKu5j+O3q3YufbrXrVSriyhBE/9egP4fVEJWo0ElSzG8Xk/iiDVdzejb4mP6NcsB7ymnrpsikReBvxiyghkIxr3R9Ia1ksuEvtNVsvg1QkTaShQt0CzgH/N0+BaoAgAA
    Metadata:
      aws:cdk:path: RedshiftLoggingBucketStack/CDKMetadata/Default
    Condition: CDKMetadataAvailable

TheRealAmazonKendra

Thank you for your contribution! Please see my comment below. While this works, I think improving the contract is a better approach.

TheRealAmazonKendra · 2022-07-22T20:27:09Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

@@ -474,6 +474,10 @@ export class Cluster extends ClusterBase {
    this.singleUserRotationApplication = secretsmanager.SecretRotationApplication.REDSHIFT_ROTATION_SINGLE_USER;
    this.multiUserRotationApplication = secretsmanager.SecretRotationApplication.REDSHIFT_ROTATION_MULTI_USER;

+    if (props.loggingBucket && !props.loggingKeyPrefix) {


If these props need to be both or neither, I'd prefer we take loggingBucket and loggingKeyPrefix and make them into an interface that is optional, with both fields being required. I recognize that this is a breaking change, but I think it's the better contract overall. Since this is an experimental module, we can do this without a feature flag.

Pull request has been modified.

TheRealAmazonKendra

The fact that this change didn't break any integration tests tells me we have a testing coverage gap. Please add an integration test with these properties set.

sean-beath · 2022-08-02T22:10:57Z

The fact that this change didn't break any integration tests tells me we have a testing coverage gap. Please add an integration test with these properties set.

There was already an integration test with both the logging bucket and prefix set, so this change meant that this test had the syntax altered to use the new interface. Does this need another test setting the logging bucket and prefix?

TheRealAmazonKendra · 2022-08-04T19:57:13Z

The fact that this change didn't break any integration tests tells me we have a testing coverage gap. Please add an integration test with these properties set.

There was already an integration test with both the logging bucket and prefix set, so this change meant that this test had the syntax altered to use the new interface. Does this need another test setting the logging bucket and prefix?

Can you point me in the direction of this integ test? This change in contract should have broken it. Note that the integ test file names start with integ.<whatever> so if you're referring to a test where it's not named that way, it isn't an integ test. See our integration test for more information.

sean-beath · 2022-08-05T06:44:09Z

The fact that this change didn't break any integration tests tells me we have a testing coverage gap. Please add an integration test with these properties set.

There was already an integration test with both the logging bucket and prefix set, so this change meant that this test had the syntax altered to use the new interface. Does this need another test setting the logging bucket and prefix?

Can you point me in the direction of this integ test? This change in contract should have broken it. Note that the integ test file names start with integ.<whatever> so if you're referring to a test where it's not named that way, it isn't an integ test. See our integration test for more information.

You're absolutely right. I misunderstood the difference between integ tests and regular tests!

I've tried to create a new integ test but it fails on execution as to create a logging bucket Redshift needs to have a role that allows it to s3:GetBucketAcl and s3:PutObject.
To get around this locally I've included the following, however this L2 construct does not currently create an IAM role which would be needed to add these permissions to.

        logging_bucket.add_to_resource_policy(iam.PolicyStatement(
            principals=[iam.ServicePrincipal('redshift.amazonaws.com')],
            actions=[
                "s3:GetBucketAcl",
                "s3:PutObject"
            ],
            resources=[
                "arn:aws:s3:::{}".format(logging_bucket.bucket_name),
                "arn:aws:s3:::{}/*".format(logging_bucket.bucket_name)
            ]
        ))

When creating a cluster with a logging bucket should we also create a new IAM role and attach these permissions to it by default?

CFN error:

Resource handler returned message: "Cannot read ACLs of bucket redshift-loggingbucket-integ-s3486f821d-g1qlzniit57c. Please ensure that your IAM permissions are set up correctly. (Service: Redshift, Status Code: 400, Request ID: 71cf5447-ee6f-4cc5-bd1b-9d961c1dddff)" (RequestToken: de0aae14-636a-27d9-0f66-71e9505f1546, HandlerErrorCode: InvalidRequest)

Redshift documentation: https://docs.aws.amazon.com/redshift/latest/mgmt/db-auditing.html#db-auditing-manage-log-files

Pull request has been modified.

TheRealAmazonKendra · 2022-08-05T07:04:11Z

Yep, we definitely want to attach the role/policy that will allow this to deploy successfully. This is one of those perfect examples of what we only catch in integ tests and not in unit tests. Thanks for adding it so we could find the gap now instead of having a bug later!

TheRealAmazonKendra · 2022-08-05T07:06:04Z

This looks great, btw. Once those permissions get added and the tests are updated to include them, I think this is ready to go

TheRealAmazonKendra

Just putting this back in changes requested so I get the alert when new changes have been made.

Pull request has been modified.

TheRealAmazonKendra

Looks good! Thanks for all your work on this!

mergify · 2022-08-08T18:41:01Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-08-08T19:52:03Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 03c02a7
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-08-08T19:52:22Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ey (aws#21243) To send these Amazon Redshift logging information about connections and user activities in your database to an S3 bucket, specify an S3 bucket and prefix using an interface. CFN documentation has S3KeyPrefix as optional, but testing has shown that key is required and CFN error (same as in below issue) will be thrown when key is not provided. closes: aws#19514. BREAKING CHANGE: The way to specify a logging bucket and prefix will change to use an interface. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix/added error when creating logging bucket without s3 key

9c61c33

github-actions bot added the p2 label Jul 20, 2022

aws-cdk-automation requested a review from a team July 20, 2022 06:05

TheRealAmazonKendra previously requested changes Jul 22, 2022

View reviewed changes

updated logging bucket to use new interface

e572147

sean-beath changed the title ~~fix(redshift): added error when creating logging bucket without s3 key~~ feat(redshift): added error when creating logging bucket without s3 key Aug 2, 2022

updated README to show how to include logging

8033dac

sean-beath changed the title ~~feat(redshift): added error when creating logging bucket without s3 key~~ refactor(redshift): added error when creating logging bucket without s3 key Aug 2, 2022

TheRealAmazonKendra changed the title ~~refactor(redshift): added error when creating logging bucket without s3 key~~ fix(redshift): deploy fails when creating logging bucket without s3 key Aug 2, 2022

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

added integ test for logging bucket

939494e

TheRealAmazonKendra previously requested changes Aug 5, 2022

View reviewed changes

added s3 permissions when using logging bucket

7e8037f

TheRealAmazonKendra approved these changes Aug 8, 2022

View reviewed changes

Merge branch 'main' into fix/require_s3prefix_with_bucketname

03c02a7

mergify bot merged commit 220177f into aws:main Aug 8, 2022

sean-beath deleted the fix/require_s3prefix_with_bucketname branch August 24, 2022 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(redshift): deploy fails when creating logging bucket without s3 key #21243

fix(redshift): deploy fails when creating logging bucket without s3 key #21243

sean-beath commented Jul 20, 2022 •

edited

Loading

gitpod-io bot commented Jul 20, 2022

TheRealAmazonKendra commented Jul 21, 2022

sean-beath commented Jul 21, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jul 22, 2022

TheRealAmazonKendra left a comment

sean-beath commented Aug 2, 2022

TheRealAmazonKendra commented Aug 4, 2022

sean-beath commented Aug 5, 2022 •

edited

Loading

TheRealAmazonKendra commented Aug 5, 2022

TheRealAmazonKendra commented Aug 5, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra left a comment

mergify bot commented Aug 8, 2022

aws-cdk-automation commented Aug 8, 2022

mergify bot commented Aug 8, 2022

fix(redshift): deploy fails when creating logging bucket without s3 key #21243

fix(redshift): deploy fails when creating logging bucket without s3 key #21243

Conversation

sean-beath commented Jul 20, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 20, 2022

TheRealAmazonKendra commented Jul 21, 2022

sean-beath commented Jul 21, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 22, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

sean-beath commented Aug 2, 2022

TheRealAmazonKendra commented Aug 4, 2022

sean-beath commented Aug 5, 2022 • edited Loading

TheRealAmazonKendra commented Aug 5, 2022

TheRealAmazonKendra commented Aug 5, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 8, 2022

aws-cdk-automation commented Aug 8, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 8, 2022

sean-beath commented Jul 20, 2022 •

edited

Loading

sean-beath commented Aug 5, 2022 •

edited

Loading