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-alpha): implement IGrantable with a default service IAM Role #28018

Closed
Closed
Show file tree
Hide file tree
Changes from 2 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
13 changes: 12 additions & 1 deletion packages/@aws-cdk/aws-redshift-alpha/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,18 @@ new Table(this, 'Table', {

### Granting Privileges

You can give a user privileges to perform certain actions on a table by using the
You can grant a *cluster* IAM permissions to perform actions on other AWS services.
For example, you may want to allow a cluster to read from a S3 bucket for Redshift Spectrum.

```ts fixture=cluster
const bucket = new s3.Bucket(stack, 'KmsBucket',{
encryptionKey: new kms.Key(stack,'Key')
})
bucket.grantRead(cluster)
dontirun marked this conversation as resolved.
Show resolved Hide resolved
```


You can give a *user* privileges to perform certain actions on a table by using the
dontirun marked this conversation as resolved.
Show resolved Hide resolved
`Table.grant()` method.

```ts fixture=cluster
Expand Down
35 changes: 29 additions & 6 deletions packages/@aws-cdk/aws-redshift-alpha/lib/cluster.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
import * as path from 'path';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
import * as iam from 'aws-cdk-lib/aws-iam';
import * as kms from 'aws-cdk-lib/aws-kms';
import * as lambda from 'aws-cdk-lib/aws-lambda';
import { CfnCluster } from 'aws-cdk-lib/aws-redshift';
import * as s3 from 'aws-cdk-lib/aws-s3';
import * as secretsmanager from 'aws-cdk-lib/aws-secretsmanager';
import { ArnFormat, CustomResource, Duration, IResource, Lazy, RemovalPolicy, Resource, SecretValue, Stack, Token } from 'aws-cdk-lib/core';
import { AwsCustomResource, AwsCustomResourcePolicy, PhysicalResourceId, Provider } from 'aws-cdk-lib/custom-resources';
import { Construct } from 'constructs';
import * as path from 'path';
import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
import { ClusterParameterGroup, IClusterParameterGroup } from './parameter-group';
import { CfnCluster } from 'aws-cdk-lib/aws-redshift';
import { ClusterSubnetGroup, IClusterSubnetGroup } from './subnet-group';
/**
* Possible Node Types to use in the cluster
Expand Down Expand Up @@ -299,10 +299,17 @@ export interface ClusterProps {
readonly masterUser: Login;

/**
* A list of AWS Identity and Access Management (IAM) role that can be used by the cluster to access other AWS services.
* A default AWS Identity and Access Management (IAM) role to be used by the cluster to access other AWS services.
*
* @default - Create a new role
*/
dontirun marked this conversation as resolved.
Show resolved Hide resolved
readonly serviceRole?: iam.IRole;

/**
* A list of additional AWS Identity and Access Management (IAM) roles that can be used by the cluster to access other AWS services.
* The maximum number of roles to attach to a cluster is subject to a quota.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somewhat conflicted on whether this should even be a prop, but I am leaning towards having it since other services follow a similar pattern.

*
dontirun marked this conversation as resolved.
Show resolved Hide resolved
* @default - No role is attached to the cluster.
* @default - Only the serviceRole is attached to the cluster.
*/
readonly roles?: iam.IRole[];

Expand Down Expand Up @@ -417,7 +424,7 @@ abstract class ClusterBase extends Resource implements ICluster {
*
* @resource AWS::Redshift::Cluster
*/
export class Cluster extends ClusterBase {
export class Cluster extends ClusterBase implements iam.IGrantable {
/**
* Import an existing DatabaseCluster from properties
*/
Expand Down Expand Up @@ -454,6 +461,16 @@ export class Cluster extends ClusterBase {
*/
public readonly secret?: secretsmanager.ISecret;

/**
* A default AWS Identity and Access Management (IAM) role to be used by the cluster to access other AWS services.
*/
public readonly serviceRole: iam.IRole;

Comment on lines +466 to +469
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* A default AWS Identity and Access Management (IAM) role to be used by the cluster to access other AWS services.
*/
public readonly serviceRole: iam.IRole;
/**
* A default AWS Identity and Access Management (IAM) role to be used by the cluster to access other AWS services.
*/
private readonly serviceRole: iam.IRole;

No need to make this public.

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 think it's useful to either make this public or add additional methods to allow users to add permissions to the role directly.

It's convenient for adding additional permissions beyond what grants provide. For example if I wanted to allow my cluster to read from a Glue Data Catalog.

/**
* The principal this cluster is using.
*/
public readonly grantPrincipal: iam.IPrincipal;

private readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;
private readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication;

Expand Down Expand Up @@ -492,7 +509,13 @@ export class Cluster extends ClusterBase {
subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS,
};
this.parameterGroup = props.parameterGroup;
this.roles = props?.roles ? [...props.roles] : [];

this.serviceRole = props.serviceRole ?? new iam.Role(this, 'ServiceRole', {
assumedBy: new iam.ServicePrincipal('redshift.amazonaws.com'),
});
this.grantPrincipal = this.serviceRole;

this.roles = props?.roles ? [this.serviceRole, ...props.roles] : [this.serviceRole];

dontirun marked this conversation as resolved.
Show resolved Hide resolved
const removalPolicy = props.removalPolicy ?? RemovalPolicy.RETAIN;

Expand Down
32 changes: 30 additions & 2 deletions packages/@aws-cdk/aws-redshift-alpha/test/cluster.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import * as cdk from 'aws-cdk-lib';
import { Match, Template } from 'aws-cdk-lib/assertions';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
import * as iam from 'aws-cdk-lib/aws-iam';
import * as kms from 'aws-cdk-lib/aws-kms';
import { CfnCluster } from 'aws-cdk-lib/aws-redshift';
import * as s3 from 'aws-cdk-lib/aws-s3';
import * as cdk from 'aws-cdk-lib';
import { Cluster, ClusterParameterGroup, ClusterSubnetGroup, ClusterType } from '../lib';
import { CfnCluster } from 'aws-cdk-lib/aws-redshift';

let stack: cdk.Stack;
let vpc: ec2.IVpc;
Expand Down Expand Up @@ -779,6 +779,31 @@ describe('default IAM role', () => {
});

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

test('cluster instantiated with a default grantable IAM Role', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need unit tests for:

  • A Cluster with serviceRole specified via props
  • A Cluster with serviceRole and roles specified via props

// GIVEN
const cluster = new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
});

const bucket = new s3.Bucket(stack, 'Bucket');
bucket.grantRead(cluster);
// THEN
const template = Template.fromStack(stack);

template.hasResourceProperties('AWS::Redshift::Cluster', {
IamRoles: Match.arrayEquals([
{ 'Fn::GetAtt': [Match.stringLikeRegexp('ServiceRole*'), 'Arn'] },
]),
});

template.hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: Match.objectLike({ Statement: Match.arrayEquals([Match.objectLike({ Resource: Match.arrayWith([Match.objectLike({ 'Fn::GetAtt': [stack.getLogicalId(bucket.node.defaultChild as s3.CfnBucket), 'Arn'] })]) })]) }),
});
});
test('roles can be directly attached to cluster during declaration', () => {
// GIVEN
const role = new iam.Role(stack, 'Role', {
Expand All @@ -796,6 +821,7 @@ describe('IAM role', () => {
Template.fromStack(stack).hasResource('AWS::Redshift::Cluster', {
Properties: {
IamRoles: Match.arrayEquals([
{ 'Fn::GetAtt': [Match.stringLikeRegexp('ServiceRole*'), 'Arn'] },
{ 'Fn::GetAtt': [Match.stringLikeRegexp('Role*'), 'Arn'] },
]),
},
Expand All @@ -821,6 +847,7 @@ describe('IAM role', () => {
Template.fromStack(stack).hasResource('AWS::Redshift::Cluster', {
Properties: {
IamRoles: Match.arrayEquals([
{ 'Fn::GetAtt': [Match.stringLikeRegexp('ServiceRole*'), 'Arn'] },
{ 'Fn::GetAtt': [Match.stringLikeRegexp('Role*'), 'Arn'] },
]),
},
Expand Down Expand Up @@ -848,6 +875,7 @@ describe('IAM role', () => {
Template.fromStack(stack).hasResource('AWS::Redshift::Cluster', {
Properties: {
IamRoles: Match.arrayEquals([
{ 'Fn::GetAtt': [Match.stringLikeRegexp('ServiceRole*'), 'Arn'] },
{ 'Fn::ImportValue': Match.stringLikeRegexp('NewTestStack:ExportsOutputFnGetAttRole*') },
]),
},
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Large diffs are not rendered by default.

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,23 @@
}
}
},
"Cluster1ServiceRole62B18E46": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "redshift.amazonaws.com"
}
}
],
"Version": "2012-10-17"
}
}
},
"Cluster1Subnets31ACF7AC": {
"Type": "AWS::Redshift::ClusterSubnetGroup",
"Properties": {
Expand Down Expand Up @@ -477,6 +494,12 @@
"DBName": "default_db",
"Encrypted": true,
"IamRoles": [
{
"Fn::GetAtt": [
"Cluster1ServiceRole62B18E46",
"Arn"
]
},
{
"Fn::GetAtt": [
"IAMF8C44BCA",
Expand Down Expand Up @@ -708,7 +731,7 @@
"S3Bucket": {
"Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
},
"S3Key": "e5178afc49b7c6a85127a67856ce958e4f0879ce6aad5e974cac2a088bf939db.zip"
"S3Key": "b0de6372e1c756b7afa1a8a1d0a21ee4cd330150290abffc9bc163f285150514.zip"
},
"Handler": "index.handler",
"Role": {
Expand All @@ -725,6 +748,23 @@
"AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2"
]
},
"Cluster2ServiceRole7B955676": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "redshift.amazonaws.com"
}
}
],
"Version": "2012-10-17"
}
}
},
"Cluster2SubnetsB040B959": {
"Type": "AWS::Redshift::ClusterSubnetGroup",
"Properties": {
Expand Down Expand Up @@ -794,6 +834,12 @@
"DBName": "default_db",
"Encrypted": true,
"IamRoles": [
{
"Fn::GetAtt": [
"Cluster2ServiceRole7B955676",
"Arn"
]
},
{
"Fn::GetAtt": [
"IAMF8C44BCA",
Expand Down
Loading
Loading