-
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 14 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,14 @@ 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. | ||
* The default role must be included in the roles list. | ||
* | ||
* @default - No default role is specified for the cluster. | ||
*/ | ||
readonly defaultRole?: iam.IRole; | ||
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'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 commentThe reason will be displayed to describe this comment to others. Learn more. Redshift only allows the one default role per cluster. |
||
|
||
/** | ||
* Name of a database which is automatically created inside the cluster | ||
* | ||
|
@@ -575,6 +584,15 @@ 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 and also available in the roles list | ||
if (props.defaultRole) { | ||
if (props.roles?.some(x => x === props.defaultRole)) { | ||
this.addDefaultIamRole(props.defaultRole); | ||
} else { | ||
throw new Error('Default role must be included in role list.'); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -662,4 +680,57 @@ export class Cluster extends ClusterBase { | |
throw new Error('Cannot add a parameter to an imported parameter group.'); | ||
} | ||
} | ||
|
||
/** | ||
* Adds default IAM role to cluster. The default IAM role must be already associated to the cluster to be added as the default role. | ||
* | ||
* @param defaultIamRole the IAM role to be set as the default role | ||
*/ | ||
public addDefaultIamRole(defaultIamRole: iam.IRole): void { | ||
// Get list of IAM roles attached to cluster | ||
const clusterRoleList = this.cluster.iamRoles ?? []; | ||
|
||
// Check to see if default role is included in list of cluster IAM roles | ||
var roleAlreadyOnCluster = false; | ||
for (var i = 0; i < clusterRoleList.length; i++) { | ||
if (clusterRoleList[i] == defaultIamRole.roleArn) { | ||
roleAlreadyOnCluster = true; | ||
break; | ||
} | ||
} | ||
if (!roleAlreadyOnCluster) { | ||
throw new Error('Default role must be associated to the Redshift cluster to be set as the default role.'); | ||
} | ||
|
||
// On UPDATE or CREATE define the default IAM role. On DELETE, remove the default IAM role | ||
const defaultRoleCustomResource = new AwsCustomResource(this, 'default-role', { | ||
onUpdate: { | ||
service: 'Redshift', | ||
action: 'modifyClusterIamRoles', | ||
parameters: { | ||
ClusterIdentifier: this.cluster.ref, | ||
DefaultIamRoleArn: defaultIamRole.roleArn, | ||
}, | ||
physicalResourceId: PhysicalResourceId.of( | ||
`${defaultIamRole.roleArn}-${this.cluster.ref}`, | ||
), | ||
}, | ||
onDelete: { | ||
service: 'Redshift', | ||
action: 'modifyClusterIamRoles', | ||
parameters: { | ||
ClusterIdentifier: this.cluster.ref, | ||
DefaultIamRoleArn: '', | ||
}, | ||
physicalResourceId: PhysicalResourceId.of( | ||
`${defaultIamRole.roleArn}-${this.cluster.ref}`, | ||
), | ||
}, | ||
policy: AwsCustomResourcePolicy.fromSdkCalls({ | ||
resources: AwsCustomResourcePolicy.ANY_RESOURCE, | ||
}), | ||
}); | ||
|
||
defaultIamRole.grantPassRole(defaultRoleCustomResource.grantPrincipal); | ||
} | ||
} |
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." | ||
} | ||
] | ||
} | ||
} | ||
} |
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.
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?
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.
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.
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.
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?
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 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?
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'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.
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.
awesome thanks