feat(redshift): add setLoggingProperties method for Cluster

[dontirun]

Closes #22034

---

All Submissions:

• [X] 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[bot]]

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[dontirun]

I'm not sure this is something we want to accept. This is essentially asking for also having a method for to set every property. I think this is something that you could handle in your application, i.e. create your own IMyCluster interface that gets passed and has a method for setting the logging properties.

I don't think this is the same thing as asking for a set method for every property. Specifically in this case There is a dependency on a different resource (the logging IBucket) to already be created prior to instantiation.

I listed a few reasons in [the issue] (https://github.com/aws/aws-cdk/issues/22034#issue-1372867641), but going more into depth about my specific use case we have a construct where users provide their own Redshift cluster. For security purposes certain configurations must be present on the cluster. It doesn't make sense for the cluster to be instantiated within the construct since the users keep the cluster for other purposes if they no longer want to use our construct.

[dontirun]

In addition to that, there are reasons why we can't determine the logging bucket configuration at synth time, and we rely on a custom resource to call an API to determine those details and provide it to the cluster.

It's something we can currently do with CloudFormation, but can't do with CDK without doing some really hacky property overrides

[corymhall]

In addition to that, there are reasons why we can't determine the logging bucket configuration at synth time, and we rely on a custom resource to call an API to determine those details and provide it to the cluster.

How does this PR help with that use case?

It's something we can currently do with CloudFormation, but can't do with CDK without doing some really hacky property overrides

How do you currently do it with CloudFormation?

For security purposes certain configurations must be present on the cluster. It doesn't make sense for the cluster to be instantiated within the construct since the users keep the cluster for other purposes if they no longer want to use our construct.

I think we would prefer to use aspects or cfn-guard to throw an error if certain configurations are not present. If they no longer want to use your construct then all of a sudden certain configurations are gone. What if one of those breaks the cluster?

This sounds like a much bigger ask around being able to apply common configurations to resources (similar to this rfc). I'm not saying that we are opposed to this, but I don't think we want to add these one off exceptions.

[dontirun]

In addition to that, there are reasons why we can't determine the logging bucket configuration at synth time, and we rely on a custom resource to call an API to determine those details and provide it to the cluster.

How does this PR help with that use case?

It's something we can currently do with CloudFormation, but can't do with CDK without doing some really hacky property overrides

How do you currently do it with CloudFormation?

CloudFormation doesn't depend on the resource ordering in templates, while the CDK does (before synthesizing to CloudFormation). I have S3 buckets configured later in my application (in the construct) which I would like to use as part of the logging configuration.

I think we would prefer to use aspects or cfn-guard to throw an error if certain configurations are not present.

Disregarding the point about resource ordering. This makes sense when we're talking about common security configurations across all resources of a specific type within an application, not individual resources.

If resource ordering wasn't a problem, I could technically apply an Aspect to throw an error or to alter the configuration, but both those cases are workarounds for the lack of a method to set the properties. I'd much rather use a method to set the property and throw an error during construction than messing with the Assembly and Template with Aspects and/or ovverrides

If they no longer want to use your construct then all of a sudden certain configurations are gone. What if one of those breaks the cluster?

I think needs to be taken on a case by case basis, adding then removing a logging configuration (and maybe even replacing) is not something that would break the cluster.

This sounds like a much bigger ask around being able to apply common configurations to resources (similar to this rfc). I'm not saying that we are opposed to this, but I don't think we want to add these one off exceptions.

While I have commented on that RFC and agree with the need for it. I don't think this is related since it's really just adding a property to a singular resource (which happens to be a security related property) versus application wide defaults.

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.