feat(redshift-alpha): directly add parameters to a parameter group or indirectly through a cluster by dontirun · Pull Request #20944 · aws/aws-cdk

dontirun · 2022-07-01T01:24:34Z

Closes #20656

This PR enables users to directly add parameters to a ClusterParameterGroup or indirectly through a Cluster. There are a few reasons why this would not succeed, such as the parameter already existing or trying to add parameters to an Imported Parameter Group and/or Cluster. With this in mind, the methods return a AddParameterResultStatus which let's developers handle failure cases more elegantly.

Ex. On SUCCESS or SAME_VALUE_FAILURE do nothing, but on CONFLICTING_VALUE_FAILURE or IMPORTED_RESOURCE_FAILURE throw some sort of error indicating what you need the developer to do in their application to resolve the issue.

This is very useful in the case of vending constructs that take in a Redshift Cluster as an input. See #20656 for more context.

I don't think this is significant enough to be called out in the README with an example, but happy to add one if necessary.

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

…ParameterGroup or indirectly through a Cluster

gitpod-io · 2022-07-01T01:24:41Z

…un/aws-cdk into dynamic-redshift-parameter-groups

comcalvi

This is a preliminary pass, as there are some things happening that I don't understand in this PR. Can you please add more detail to the PR description?

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

…o extend ClusterParameterGroupBase

Pull request has been modified.

dontirun · 2022-07-15T21:37:18Z

Going off of some of my previous comments I updated the Import class in fromClusterParameterGroupName to extend ClusterParameterGroupBase. It didn't affect the unit tests and I think it's a better implementation that is also more consistent with other implementations. I can revert that if needed

dontirun · 2022-08-01T21:17:58Z

@comcalvi Do you mind re-reviewing this PR?

TheRealAmazonKendra

New features require a README update. The act of writing the README often helps you assess your approach better and it gives us additional context when reviewing to understand the user experience. Additionally, we generate our documentation from there so it is needed.

I think there are some design issues here and need to be addressed. I also have a lot of questions about what's going on, which tells me that README is especially needed.

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

TheRealAmazonKendra · 2022-08-03T22:56:22Z

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

  }
+
+  public addToParameterGroup(_name: string, _value: string): AddParameterResult {
+    return { parameterAddedResult: AddParameterResultStatus.IMPORTED_RESOURCE_FAILURE };


Why does this just plain return a failure?

For addToParameterGroup in the Cluster construct, this method should fail for all imported resources since we can't add parameters to them. The L2 Cluster class is the only class that it wouldn't fail on. Any of the static import methods such as fromClusterAttributes should fail since they are imported resources. In the future there could potentially be more static methods that create imported resources from ClusterBase which inherit this method but I don't think there would be another instantiated Cluster class that extends ClusterBase; so I thought the default should be the failure.

Why can't we add parameters to imported Clusters?

In CloudFormation, the Cluster is has a property to specify a Parameter Group name . Parameter Groups don't have a property to tie them to a Cluster. That means that the association must be done on the side of the Cluster. I didn't think we could modify imports like that.

Additionally if we could modify imports, we could be potentially overriding an existing association since a Cluster can only have one parameter group associated with it at a time

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

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

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

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

comcalvi

There are some things about this approach that I still don't understand. Overall, I agree with @TheRealAmazonKendra. This is getting closer, but still needs some work.

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

comcalvi · 2022-08-04T00:24:31Z

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

  }
+
+  public addToParameterGroup(_name: string, _value: string): AddParameterResult {
+    return { parameterAddedResult: AddParameterResultStatus.IMPORTED_RESOURCE_FAILURE };


Why can't we add parameters to imported Clusters?

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

comcalvi · 2022-08-04T00:38:46Z

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

+    } else if (this.parameterGroup) {
+      return this.parameterGroup.addParameter(name, value);
+    } else {
+      return { parameterAddedResult: AddParameterResultStatus.IMPORTED_RESOURCE_FAILURE };


Please throw errors instead of returning failures. However, I still don't understand why these operations default to failures. What about the resource being imported prevents it from succeeding?

Regardless of why it fails for imported resources, the import should not be considered the default behavior anyway. Imports are the 'special-ish' case.

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

comcalvi · 2022-08-04T00:42:21Z

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

  public abstract readonly clusterParameterGroupName: string;
+
+  public addParameter(_name: string, _value: string): AddParameterResult {
+    return { parameterAddedResult: AddParameterResultStatus.IMPORTED_RESOURCE_FAILURE };


I agree. The add operation should not default to failure. Failure should be the exception.

comcalvi · 2022-08-04T00:44:47Z

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

   */
  public static fromClusterParameterGroupName(scope: Construct, id: string, clusterParameterGroupName: string): IClusterParameterGroup {
-    class Import extends Resource implements IClusterParameterGroup {
+    class Import extends ClusterParameterGroupBase {


The import may implement the abstract methods, but I don't think we need to change this at this time; this is out-of-scope.

Pull request has been modified.

packages/@aws-cdk/aws-redshift/README.md

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

TheRealAmazonKendra · 2022-08-05T17:38:35Z

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

+  /**
+   * Identifier of the cluster
+   */
+  protected clusterIdentifier: string;


Suggested change

protected clusterIdentifier: string;

protected clusterId: string;

Actually, is this necessary at all? I only see one place where this is used and this.clusterName could just as easily be used in that place. I'm not seeing the value add.

Probably not needed. I thought that the clusterIdentifier would make the description in the parameter group a a little nicer.

Given that the cluster is now a field, it could be taken from that as well

We should use clusterName instead of creating this new field.

That field was removed and replaced with this logic instead. clusterName could not be used since it caused a circular dependency. (Parameter group description field containing a ref to the Cluster, Cluster's group name field containing a ref to the Parameter Group).

TheRealAmazonKendra · 2022-08-05T17:45:42Z

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


    const defaultPort = ec2.Port.tcp(this.clusterEndpoint.port);
    this.connections = new ec2.Connections({ securityGroups, defaultPort });
+    this.clusterIdentifier = props.clusterName ?? Names.uniqueResourceName(this, {});


clusterName here would never be undefined because it's being set to the ref.

I think this is a case of poorly named variables. props.clusterName != this.clusterName. The clusterName prop is either a string or undefined and is being used as the CfnCluster's clusterIdentifier property

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

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Pull request has been modified.

TheRealAmazonKendra · 2022-08-06T05:45:27Z

@Mergifyio update

mergify · 2022-08-06T05:46:01Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra

Looks like the integ tests still need to be updated with the output from the suggested changes.

Pull request has been modified.

TheRealAmazonKendra

Some requested changes still not addressed.

dontirun · 2022-08-06T21:14:37Z

I believe they have been addressed, the comments look to be on outdated code

Pull request has been modified.

dontirun · 2022-08-31T11:26:27Z

@TheRealAmazonKendra Can you re-review this? I believe I've addressed the requested changes

TheRealAmazonKendra

Thanks for all your work on this! I have several comments below.

Apologies for not thinking about the ability to add multiple parameters at a time in a previous revision, but I'm thinking that if it's a use case that seem likely, going that direction would be significantly better.

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

TheRealAmazonKendra · 2022-09-03T03:55:35Z

packages/@aws-cdk/aws-redshift/README.md

+```ts fixture=cluster
+cluster.addToParameterGroup('enable_user_activity_logging', 'true');
+```


This isn't rendering correctly. I suspect that the issue is the fixture=cluster piece causing it.

I'll fix it for the example in this PR and raise an issue. Looking at the documentation it's also not rendering for the other examples using fixture=cluster

Actually I think none of the fixtures (even the default) are loading properly for the redshift alpha library

TheRealAmazonKendra · 2022-09-03T03:57:04Z

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

+      const param: { [name: string]: string } = {};
+      param[name] = value;
+      this.parameterGroup = new ClusterParameterGroup(this, 'ParameterGroup', {
+        description: `Parameter Group for the ${this.cluster.clusterIdentifier ?? Names.uniqueResourceName(this, {})} Redshift cluster`,


I think now that you are using this.cluster.clusterIdentifier, Names.uniqueResourceId is no longer needed in this context.

The clusterIdentifier can be undefined. It's being set to the optional clusterName prop

Right, that happens before this.clusterName is set to this.cluster.ref.

I think that I suggested using Names.uniqueResourceName instead of Names.uniqueResourceId somewhere in here, but since it wasn't used there, it doesn't make sense to use it here. It won't be named that and we'd be giving false information.

You can set

clusterIdentifier: props.clusterName ?? Names.uniqueResourceName(this), { maxLength: 63, separator: '-', allowedSpecialCharacters: '-', }

up on line 528. That would work.

Agreed with the false information.

It looks like the cluster Identifier needs to be unique account wide. So instead of setting the name to with Names.uniqueResourceName I think it might be better to fall back to the paramter group default description if the identifier isn't provided

Removed the Names.uniqueResourceName and falling back to the default

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

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

TheRealAmazonKendra · 2022-09-03T04:19:50Z

packages/@aws-cdk/aws-redshift/test/integ.database.ts

  encryptionKey: new kms.Key(stack, 'custom-kms-key'),
 });

+cluster.addToParameterGroup('enable_user_activity_logging', 'true');


I'd like to see more test cases. One where the parameters are added directly in the constructor and one where addParameter() is used. Or rather, addParameters() if you take my suggestion above.

I'll add some more tests to the cluster.test.ts for this. I don't think we need to update the integration test for this (unless we do change to addParameters())

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Pull request has been modified.

TheRealAmazonKendra · 2022-09-07T00:41:02Z

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

+      this.parameters[name] = value;
+      this.resource.parameters = this.parseParameters();
+    } else if (existingValue !== value) {
+      throw new Error(`The parameter group already contains the parameter "${name}", but with a different value (Given: ${value}, Existing: ${existingValue}).`);


Let's add the name of the parameter group in here in case the user has an app with multiple.

Parameter groups don't have names, but I agree that it makes sense to identify it somehow , so maybe it makes sense to use Names.uniqueResourceName here

throw new Error(`The parameter group id "${Names.uniqueResourceName(this, {})}" already contains the parameter "${name}", but with a different value (Given: ${value}, Existing: ${existingValue}).`);

See my most recent comment below.

In this case it's set to the Instrinsic Ref of CfnClusterParameterGroup, so the error message would contain a Token.

Considering that this is runtime error, I think using Names.uniqueResourceName (along with the stack trace) should be helpful enough to locate the problematic parameter group

This would be providing the customer with inaccurate information. Sure, it's an approximation, but it's not actually the name of the resource. I suppose the stack trace will have to provide enough information in conjunction with the key value pair they're trying to add.

Reverted back to this message in the latest commit

TheRealAmazonKendra · 2022-09-07T00:56:01Z

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

+      const param: { [name: string]: string } = {};
+      param[name] = value;
+      this.parameterGroup = new ClusterParameterGroup(this, 'ParameterGroup', {
+        description: `Parameter Group for the ${this.cluster.clusterIdentifier ?? Names.uniqueResourceName(this, {})} Redshift cluster`,


Right, that happens before this.clusterName is set to this.cluster.ref.

I think that I suggested using Names.uniqueResourceName instead of Names.uniqueResourceId somewhere in here, but since it wasn't used there, it doesn't make sense to use it here. It won't be named that and we'd be giving false information.

You can set

clusterIdentifier: props.clusterName ?? Names.uniqueResourceName(this), { maxLength: 63, separator: '-', allowedSpecialCharacters: '-', }

up on line 528. That would work.

TheRealAmazonKendra · 2022-09-07T04:29:33Z

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

@@ -62,17 +63,46 @@ export class ClusterParameterGroup extends ClusterParameterGroupBase {
   */
  public readonly clusterParameterGroupName: string;


This isn't a parameter group name?

Pull request has been modified.

mergify · 2022-09-09T06:36:05Z

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-09-09T07:13:57Z

AWS CodeBuild CI Report

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

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

mergify · 2022-09-09T07:14:17Z

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).

… indirectly through a cluster (aws#20944) Closes [aws#20656](aws#20656) This PR enables users to directly add parameters to a `ClusterParameterGroup` or indirectly through a `Cluster`. There are a few reasons why this would not succeed, such as the parameter already existing or trying to add parameters to an Imported Parameter Group and/or Cluster. With this in mind, the methods return a `AddParameterResultStatus` which let's developers handle failure cases more elegantly. Ex. On `SUCCESS` or `SAME_VALUE_FAILURE` do nothing, but on `CONFLICTING_VALUE_FAILURE` or `IMPORTED_RESOURCE_FAILURE` throw some sort of error indicating what you need the developer to do in their application to resolve the issue. This is very useful in the case of vending constructs that take in a Redshift Cluster as an input. See aws#20656 for more context. I don't think this is significant enough to be called out in the README with an example, but happy to add one if necessary. ---- ### 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 * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] 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*

dontirun added 2 commits June 30, 2022 20:38

feat(redshift-alpha): methods to directly add parameters to a Cluster…

a8aa2f8

…ParameterGroup or indirectly through a Cluster

test: integration test updates

8852e7b

aws-cdk-automation requested a review from a team July 1, 2022 01:24

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

dontirun added 3 commits June 30, 2022 21:28

Merge branch 'main' into dynamic-redshift-parameter-groups

05727b8

refactor: better description for generated Paramater Group

5c5f98b

Merge branch 'dynamic-redshift-parameter-groups' of github.com:dontir…

f2d5015

…un/aws-cdk into dynamic-redshift-parameter-groups

comcalvi previously requested changes Jul 15, 2022

View reviewed changes

refactor: spacing, naming, and change fromClusterParameterGroupName t…

1c8db26

…o extend ClusterParameterGroupBase

dontirun requested a review from comcalvi July 15, 2022 22:05

Merge branch 'main' into dynamic-redshift-parameter-groups

fe97d47

TheRealAmazonKendra previously requested changes Aug 3, 2022

View reviewed changes

comcalvi previously requested changes Aug 4, 2022

View reviewed changes

refactor: updating with feedback from the review

c3e5e87

revert: Import base class change

8ba1e4b

TheRealAmazonKendra previously requested changes Aug 6, 2022

View reviewed changes

Apply suggestions from code review

37b1c9a

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

refactor: add changes from code review

1a7d327

Merge branch 'main' into dynamic-redshift-parameter-groups

271e3e2

TheRealAmazonKendra previously requested changes Aug 6, 2022

View reviewed changes

fix: circular reference with parameter group description

e007770

TheRealAmazonKendra previously requested changes Aug 6, 2022

View reviewed changes

dontirun requested a review from TheRealAmazonKendra August 11, 2022 15:42

Arun Donti added 2 commits August 25, 2022 10:43

Merge branch 'main' into dynamic-redshift-parameter-groups

ad8140b

chore: update integ tests

87bffb0

dontirun and others added 2 commits August 26, 2022 20:16

Merge branch 'main' into dynamic-redshift-parameter-groups

9c3b28b

chore: update snapshots

220f871

TheRealAmazonKendra previously requested changes Sep 3, 2022

View reviewed changes

TheRealAmazonKendra changed the title ~~feat(redshift-alpha): directly add parameters to a Parameter Group or indirectly through a Cluster~~ feat(redshift-alpha): directly add parameters to a parameter group or indirectly through a cluster Sep 3, 2022

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

2191f58

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

dontirun and others added 3 commits September 3, 2022 08:14

update docs and test

2a4fd32

Merge branch 'main' into dynamic-redshift-parameter-groups

b75471a

chore: update integ tests

3621733

TheRealAmazonKendra previously requested changes Sep 7, 2022

View reviewed changes

TheRealAmazonKendra reviewed Sep 7, 2022

View reviewed changes

refactor: suggested changes and snapshot updates

482e9ec

refactor: error message for parameter group

e36ab6b

TheRealAmazonKendra approved these changes Sep 9, 2022

View reviewed changes

Merge branch 'main' into dynamic-redshift-parameter-groups

fa4f365

mergify bot merged commit 0ad307b into aws:main Sep 9, 2022

dontirun deleted the dynamic-redshift-parameter-groups branch October 10, 2022 12:34

	protected clusterIdentifier: string;
	protected clusterId: string;

		@@ -62,17 +63,46 @@ export class ClusterParameterGroup extends ClusterParameterGroupBase {
		*/
		public readonly clusterParameterGroupName: string;

Conversation

dontirun commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jul 1, 2022

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dontirun commented Jul 15, 2022

Uh oh!

dontirun commented Aug 1, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dontirun Aug 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dontirun commented Jul 1, 2022 •

edited

Loading

dontirun Aug 6, 2022 •

edited

Loading