-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from 3 commits
76b0248
db57523
8bad9cb
33df50d
0df0d5f
d89ab34
184e8fc
54ebce1
a2147f0
a4b935c
44c5a28
b6275c2
c5fafdb
b4f9de6
cd19a0d
cffdcca
8857600
e5d6589
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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; | ||
|
||
/** | ||
* Name of a database which is automatically created inside the cluster | ||
* | ||
|
@@ -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); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -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', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.