Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions packages/@aws-cdk/aws-redshift/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Duration, IResource, RemovalPolicy, Resource, SecretValue, Token } from '@aws-cdk/core';
import { AwsCustomResource, PhysicalResourceId, AwsCustomResourcePolicy } from '@aws-cdk/custom-resources';
import { Construct } from 'constructs';
import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
Expand Down Expand Up @@ -304,6 +305,13 @@ export interface ClusterProps {
*/
readonly roles?: iam.IRole[];

/**
* A single AWS Identity and Access Management (IAM) role to be used as the default role for the cluster.
*
* @default - No default role is attached to the cluster.
*/
readonly defaultRole?: iam.IRole;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Name of a database which is automatically created inside the cluster
*
Expand Down Expand Up @@ -575,6 +583,11 @@ export class Cluster extends ClusterBase {

const defaultPort = ec2.Port.tcp(this.clusterEndpoint.port);
this.connections = new ec2.Connections({ securityGroups, defaultPort });

// Add default role if specified
if (props.defaultRole) {
this.addDefaultIamRole(props.defaultRole);
}
}

/**
Expand Down Expand Up @@ -662,4 +675,31 @@ export class Cluster extends ClusterBase {
throw new Error('Cannot add a parameter to an imported parameter group.');
}
}

/**
* Adds default IAM role to cluster
*
* @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', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed spelling mistake

onUpdate: {
service: 'Redshift',
action: 'modifyClusterIamRoles',
parameters: {
ClusterIdentifier: this.cluster.ref,
AddIamRoles: [defaultIamRole.roleArn],
DefaultIamRoleArn: defaultIamRole.roleArn,
},
physicalResourceId: PhysicalResourceId.of(
`${defaultIamRole.roleArn}-${this.cluster.ref}`,
),
},
policy: AwsCustomResourcePolicy.fromSdkCalls({
resources: AwsCustomResourcePolicy.ANY_RESOURCE,
}),
});

defaultIamRole.grantPassRole(defaultRoleCustomeResource.grantPrincipal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "22.0.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
"path": "DefaultIamRoleIntegDefaultTestDeployAssert161B8D72.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"Parameters": {
"BootstrapVersion": {
"Type": "AWS::SSM::Parameter::Value<String>",
"Default": "/cdk-bootstrap/hnb659fds/version",
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
}
},
"Rules": {
"CheckBootstrapVersion": {
"Assertions": [
{
"Assert": {
"Fn::Not": [
{
"Fn::Contains": [
[
"1",
"2",
"3",
"4",
"5"
],
{
"Ref": "BootstrapVersion"
}
]
}
]
},
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
}
]
}
}
}

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"version":"22.0.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"version": "22.0.0",
"testCases": {
"DefaultIamRoleInteg/DefaultTest": {
"stacks": [
"redshift-defaultiamrole-integ"
],
"assertionStack": "DefaultIamRoleInteg/DefaultTest/DeployAssert",
"assertionStackName": "DefaultIamRoleIntegDefaultTestDeployAssert161B8D72"
}
}
}
Loading