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 14 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
45 changes: 45 additions & 0 deletions packages/@aws-cdk/aws-redshift/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -386,3 +386,48 @@ new Cluster(stack, 'Redshift', {
```

If enhanced VPC routing is not enabled, Amazon Redshift routes traffic through the internet, including traffic to other services within the AWS network.

## Default IAM role

Some Amazon Redshift features require Amazon Redshift to access other AWS services on your behalf. For your Amazon Redshift clusters to act on your behalf, you supply security credentials to your clusters. The preferred method to supply security credentials is to specify an AWS Identity and Access Management (IAM) role.

When you create an IAM role and set it as the default for the cluster using console, you don't have to provide the IAM role's Amazon Resource Name (ARN) to perform authentication and authorization.

```ts
declare const vpc: ec2.Vpc;

const defaultRole = new iam.Role(this, 'DefaultRole', {
assumedBy: new iam.ServicePrincipal('redshift.amazonaws.com'),
},
);

new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
roles: [defaultRole],
defaultRole: defaultRole,
});
```

A default role can also be added to a cluster using the `addDefaultIamRole` method.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

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 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?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome thanks


```ts
declare const vpc: ec2.Vpc;

const defaultRole = new iam.Role(this, 'DefaultRole', {
assumedBy: new iam.ServicePrincipal('redshift.amazonaws.com'),
},
);

const redshiftCluster = new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
roles: [defaultRole],
});

redshiftCluster.addDefaultIamRole(defaultRole);
```
71 changes: 71 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,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;
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 +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.');
}
}
}

/**
Expand Down Expand Up @@ -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);
}
}
35 changes: 35 additions & 0 deletions packages/@aws-cdk/aws-redshift/test/cluster.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Match, Template } from '@aws-cdk/assertions';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import * as cdk from '@aws-cdk/core';
Expand Down Expand Up @@ -613,6 +614,40 @@ test('elastic ip address', () => {
});
});

describe('default IAM role', () => {

test('Default role not in role list', () => {
// GIVEN
const clusterRole1 = new iam.Role(stack, 'clusterRole1', { assumedBy: new iam.ServicePrincipal('redshift.amazonaws.com') } );
const defaultRole1 = new iam.Role(stack, 'defaultRole1', { assumedBy: new iam.ServicePrincipal('redshift.amazonaws.com') } );

expect(() => {
new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
roles: [clusterRole1],
defaultRole: defaultRole1,
});
}).toThrow(/Default role must be included in role list./);
});

test('throws error when default role not attached to cluster when adding default role post creation', () => {
const defaultRole1 = new iam.Role(stack, 'defaultRole1', { assumedBy: new iam.ServicePrincipal('redshift.amazonaws.com') } );
const cluster = new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
});

expect(() => {
cluster.addDefaultIamRole(defaultRole1);
}).toThrow(/Default role must be associated to the Redshift cluster to be set as the default role./);
});
});

function testStack() {
const newTestStack = new cdk.Stack(undefined, undefined, { env: { account: '12345', region: 'us-test-1' } });
newTestStack.node.setContext('availability-zones:12345:us-test-1', ['us-test-1a', 'us-test-1b']);
Expand Down
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."
}
]
}
}
}
Loading