feat(redshift): support default role for redshift clusters #23554

sean-beath · 2023-01-04T03:06:59Z

Redshift clusters have the ability to specify a default role for the cluster. This is not currently available as a feature in CloudFormation so requires a AwsCustomResource to be done at runtime.

This feature allows users to specify a default role at cluster creation or post-cluster creation.

closes #22551

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime 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

…d integ test

gitpod-io · 2023-01-04T03:07:08Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

HBobertz

Overall seems pretty good, but given we would be adding a custom resource to this construct, I'd like to see unit testing, as well as integ testing. If we did execute on the default role list I mention in the comments instead of default role, I'd like to see error checking to ensure it doesn't go over 10. May need to do this anyway but I'm not sure how this works with previously provisioned clusters.

Unit tests could just be added here https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-redshift/lib/cluster.ts

HBobertz · 2023-01-04T19:08:18Z

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

+      }),
+    });
+
+    defaultIamRole.grantPassRole(defaultRoleCustomeResource.grantPrincipal);


I don't see why this wouldn't be a valid use case for using a custom resource, and I like the functionality of the design, but just to be pedantic gonna verify with another team member.

I would have expected not to require this line, but was facing an error when it wasn't included.

HBobertz · 2023-01-04T19:09:14Z

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

+   * @param defaultIamRole the IAM role to be set as the default role
+   */
+  public addDefaultIamRole(defaultIamRole: iam.IRole): void {
+    const defaultRoleCustomeResource = new AwsCustomResource(this, 'default-role', {


why is there an e in custom here? Can we change this to be just "Custom"?

Fixed spelling mistake

HBobertz · 2023-01-04T19:11:20Z

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

+   *
+   * @default - No default role is attached to the cluster.
+   */
+  readonly defaultRole?: iam.IRole;


I'm absolutely fine with keeping this as a singleton, but given the roles prop is a list could this not also be a list? Is there a reason why a default singleton role would be preferable over a potential list of default roles?

Redshift only allows the one default role per cluster.
Assume your cluster has two IAM roles (role1 and role2). If role1 is default and then you modify the cluster to make role2 the default, role1 will still be a role for the cluster but no longer the default role.

HBobertz · 2023-01-04T19:18:57Z

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

+});
+```
+
+A default role can also be added to a cluster using the `addDefaultIamRole` method.


Might be misunderstanding but can this be added to an already existing cluster? If so is there a possibility this default role could attempt to append an 11th role to an existing cluster and then go over 10?

The way this is set up it could be added to an already existing cluster which would push the number of IAM roles to more than 10. It's interesting that this limit has been imposed as for most regions the role limit for a cluster is 50. Is there a way to make this limit region specific?

I'll try to find a way to check the total number of roles before adding the default role.

It's also interesting that there doesn't seem to be a check for the IAM role list to make sure there's less than 10 - should this be added?

I am thinking of changing the functionality so that the default role needs to be defined in the roles parameter on cluster creation, or already attached to the cluster if adding post-creation. This would remove the need to check for the total number of IAM roles and I think would reflect better practice.
What are your thoughts?

I've now changed the behaviour so that the method will check to see if the role is already associated to the cluster, if it is not then an error will be thrown. This will stop the max roles being exceeded.

awesome thanks

HBobertz · 2023-01-04T19:24:07Z

Also this is more a question for myself as well. How should we note that this custom resource functionality should be replaced when the cfn redshift cluster adds support for this. Another issue? Will sync with another engineer on that as well

Pull request has been modified.

HBobertz

looks good to me 👍

mergify · 2023-01-12T14:46:13Z

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

…hift-clusters

Pull request has been modified.

sean-beath · 2023-01-12T23:35:46Z

@HBobertz - the build has failed due to a change in the integ test that is being created with this PR (integ.cluster-defaultiamrole) - is there something specific that needs to be done here?

HBobertz · 2023-01-13T18:05:17Z

@sean-beath run yarn integ --update-on-failed and update the cr

sean-beath · 2023-01-30T00:29:33Z

@HBobertz - any movement on this PR?

HBobertz · 2023-01-30T02:42:21Z

If I remember correctly it will need to be merged from live again. I'll double check it and approve tommorow. I pulled it down and I think an integ test was failing due to a recent change, I don't think the pr will need to be changed though. Either way I'll let you know

…hift-clusters

HBobertz · 2023-01-20T18:26:23Z

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

+});
+```
+
+A default role can also be added to a cluster using the `addDefaultIamRole` method.


awesome thanks

Pull request has been modified.

…hift-clusters

HBobertz

I'm gonna babysit this today so I know it merges

aws-cdk-automation · 2023-01-31T16:06:46Z

AWS CodeBuild CI Report

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

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

PR was merged manually without squash

sean-beath added 3 commits January 4, 2023 01:27

updated cluster to create custom resource for default iam role + adde…

76b0248

…d integ test

added grantpassrole to custom resource

db57523

added test for adding default iam role post cluster creation

8bad9cb

github-actions bot added p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Jan 4, 2023

aws-cdk-automation requested a review from a team January 4, 2023 03:07

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Jan 4, 2023

aws-cdk-automation previously requested changes Jan 4, 2023

View reviewed changes

sean-beath changed the title ~~feat(redshift):support default role for redshift clusters~~ feat(redshift): support default role for redshift clusters Jan 4, 2023

updated README

33df50d

sean-beath added 3 commits January 4, 2023 03:39

added trailing newline to README

0df0d5f

added newlines around headers

d89ab34

updated README

184e8fc

HBobertz self-assigned this Jan 4, 2023

HBobertz previously requested changes Jan 4, 2023

View reviewed changes

fixed spelling mistake

54ebce1

sean-beath added 6 commits January 4, 2023 22:59

added lookup for cluster IAM roles before adding default role

a2147f0

added delete policy for custom resource to remove the default iam role

a4b935c

updated default iam role behaviour

44c5a28

updated unit tests to show errors on incorrect default IAM role usage

b6275c2

updated wording of comments and errors

c5fafdb

updated README

b4f9de6

HBobertz previously approved these changes Jan 12, 2023

View reviewed changes

Merge branch 'main' into feat(redshift)/support-default-role-for-reds…

cd19a0d

…hift-clusters

reran integ teest

cffdcca

Merge branch 'main' into feat(redshift)/support-default-role-for-reds…

8857600

…hift-clusters

HBobertz previously approved these changes Jan 30, 2023

View reviewed changes

HBobertz enabled auto-merge January 30, 2023 16:52

Merge branch 'main' into feat(redshift)/support-default-role-for-reds…

e5d6589

…hift-clusters

HBobertz approved these changes Jan 31, 2023

View reviewed changes

HBobertz merged commit 7945fa6 into aws:main Jan 31, 2023

corymhall added a commit that referenced this pull request Jan 31, 2023

chore: adding changelog entry for PR #23554

16e698f

PR was merged manually without squash

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(redshift): support default role for redshift clusters #23554

feat(redshift): support default role for redshift clusters #23554

sean-beath commented Jan 4, 2023 •

edited

Loading

gitpod-io bot commented Jan 4, 2023

aws-cdk-automation left a comment

HBobertz left a comment

HBobertz Jan 4, 2023

sean-beath Jan 4, 2023

HBobertz Jan 4, 2023

sean-beath Jan 4, 2023

HBobertz Jan 4, 2023

sean-beath Jan 4, 2023

HBobertz Jan 4, 2023

sean-beath Jan 4, 2023

sean-beath Jan 4, 2023

sean-beath Jan 5, 2023

sean-beath Jan 5, 2023

HBobertz Jan 20, 2023

HBobertz commented Jan 4, 2023 •

edited

Loading

HBobertz left a comment

mergify bot commented Jan 12, 2023

sean-beath commented Jan 12, 2023

HBobertz commented Jan 13, 2023 •

edited

Loading

sean-beath commented Jan 30, 2023

HBobertz commented Jan 30, 2023 •

edited

Loading

HBobertz Jan 20, 2023

HBobertz left a comment

aws-cdk-automation commented Jan 31, 2023

feat(redshift): support default role for redshift clusters #23554

feat(redshift): support default role for redshift clusters #23554

Conversation

sean-beath commented Jan 4, 2023 • edited Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Jan 4, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

HBobertz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HBobertz commented Jan 4, 2023 • edited Loading

HBobertz left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 12, 2023

sean-beath commented Jan 12, 2023

HBobertz commented Jan 13, 2023 • edited Loading

sean-beath commented Jan 30, 2023

HBobertz commented Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

HBobertz left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 31, 2023

AWS CodeBuild CI Report

sean-beath commented Jan 4, 2023 •

edited

Loading

HBobertz commented Jan 4, 2023 •

edited

Loading

HBobertz commented Jan 13, 2023 •

edited

Loading

HBobertz commented Jan 30, 2023 •

edited

Loading