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.

Sign up for GitHub

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

Jump to bottom

fix(rds): cannot use s3ImportBuckets or s3ExportBuckets with aurora postgres #10132

Merged
merged 12 commits into from
Sep 17, 2020
Merged

fix(rds): cannot use s3ImportBuckets or s3ExportBuckets with aurora postgres #10132

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
34c0997
add engine features for s3Import and s3Export
shivlaks Sep 2, 2020
2699247
add test
shivlaks Sep 2, 2020
9774ddf
wip - addressed some of the PR feedback
shivlaks Sep 3, 2020
ca46702
add empty line
shivlaks Sep 4, 2020
fd761df
more PR feedback
shivlaks Sep 4, 2020
4d00cfd
add validation on supported features, move logic into bindToCluster
shivlaks Sep 14, 2020
d22901f
Merge branch 'master' into shivlaks/rds-engine-version-features
shivlaks Sep 14, 2020
50e3f58
missed a change from the merge
shivlaks Sep 14, 2020
4620ac1
PR feedback
shivlaks Sep 15, 2020
3a2fa03
add support for unversioned aurora postgresql to specify s3Import and…
shivlaks Sep 16, 2020
cf2c014
final tweaks
shivlaks Sep 17, 2020
9f42b00
Merge branch 'master' into shivlaks/rds-engine-version-features
mergify[bot] Sep 17, 2020
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
97 changes: 88 additions & 9 deletions packages/@aws-cdk/aws-rds/lib/cluster-engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,36 @@ export interface ClusterEngineConfig {
* @default - use the default port for clusters (3306)
*/
readonly port?: number;

/**
* Features supported by the database engine.
*
* @see https://docs.aws.amazon.com/AmazonRDS/latest/APIReference/API_DBEngineVersion.html
*
* @default - no features
*/
readonly features?: ClusterEngineFeatures;
}

/**
* Represents Database Engine features
*/
export interface ClusterEngineFeatures {
/**
* Feature name for the DB instance that the IAM role to access the S3 bucket for import
* is to be associated with.
*
* @default - no s3Import feature name
*/
readonly s3Import?: string;

/**
* Feature name for the DB instance that the IAM role to export to S3 bucket is to be
* associated with.
*
* @default - no s3Export feature name
*/
readonly s3Export?: string;
}

/**
Expand Down Expand Up @@ -76,6 +106,7 @@ interface ClusterEngineBaseProps {
readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication;
readonly defaultPort?: number;
readonly engineVersion?: EngineVersion;
readonly features?: ClusterEngineFeatures;
}

abstract class ClusterEngineBase implements IClusterEngine {
Expand All @@ -87,9 +118,11 @@ abstract class ClusterEngineBase implements IClusterEngine {
public abstract readonly supportedLogTypes: string[];

private readonly defaultPort?: number;
private readonly features?: ClusterEngineFeatures;

constructor(props: ClusterEngineBaseProps) {
this.engineType = props.engineType;
this.features = props.features;
this.singleUserRotationApplication = props.singleUserRotationApplication;
this.multiUserRotationApplication = props.multiUserRotationApplication;
this.defaultPort = props.defaultPort;
Expand All @@ -102,6 +135,7 @@ abstract class ClusterEngineBase implements IClusterEngine {
return {
parameterGroup,
port: this.defaultPort,
features: this.features,
};
}

Expand Down Expand Up @@ -345,6 +379,25 @@ class AuroraMysqlClusterEngine extends MySqlClusterEngineBase {
}
}

/**
* Features supported by Aurora Postgres database engine
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
*/
export interface AuroraPostgresEngineFeatures {
/**
* Whether the IAM role can access the S3 bucket for importing data
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
*
* @default false
*/
readonly s3Import?: boolean;

/**
* Whether the IAM role can access the S3 bucket for exporting data
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
*
* @default false
*/
readonly s3Export?: boolean;
}

/**
* The versions for the Aurora PostgreSQL cluster engine
* (those returned by {@link DatabaseClusterEngine.auroraPostgres}).
Expand All @@ -369,17 +422,17 @@ export class AuroraPostgresEngineVersion {
/** Version "10.6". */
public static readonly VER_10_6 = AuroraPostgresEngineVersion.of('10.6', '10');
/** Version "10.7". */
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
public static readonly VER_10_7 = AuroraPostgresEngineVersion.of('10.7', '10');
public static readonly VER_10_7 = AuroraPostgresEngineVersion.of('10.7', '10', { s3Import: true });
/** Version "10.11". */
public static readonly VER_10_11 = AuroraPostgresEngineVersion.of('10.11', '10');
public static readonly VER_10_11 = AuroraPostgresEngineVersion.of('10.11', '10', { s3Import: true, s3Export: true });
/** Version "10.12". */
public static readonly VER_10_12 = AuroraPostgresEngineVersion.of('10.12', '10');
public static readonly VER_10_12 = AuroraPostgresEngineVersion.of('10.12', '10', { s3Import: true, s3Export: true });
/** Version "11.4". */
public static readonly VER_11_4 = AuroraPostgresEngineVersion.of('11.4', '11');
public static readonly VER_11_4 = AuroraPostgresEngineVersion.of('11.4', '11', { s3Import: true });
/** Version "11.6". */
public static readonly VER_11_6 = AuroraPostgresEngineVersion.of('11.6', '11');
public static readonly VER_11_6 = AuroraPostgresEngineVersion.of('11.6', '11', { s3Import: true, s3Export: true });
/** Version "11.7". */
public static readonly VER_11_7 = AuroraPostgresEngineVersion.of('11.7', '11');
public static readonly VER_11_7 = AuroraPostgresEngineVersion.of('11.7', '11', { s3Import: true, s3Export: true });

/**
* Create a new AuroraPostgresEngineVersion with an arbitrary version.
Expand All @@ -389,18 +442,30 @@ export class AuroraPostgresEngineVersion {
* @param auroraPostgresMajorVersion the major version of the engine,
* for example "9.6"
*/
public static of(auroraPostgresFullVersion: string, auroraPostgresMajorVersion: string): AuroraPostgresEngineVersion {
return new AuroraPostgresEngineVersion(auroraPostgresFullVersion, auroraPostgresMajorVersion);
public static of(auroraPostgresFullVersion: string, auroraPostgresMajorVersion: string,
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
auroraPostgresFeatures?: AuroraPostgresEngineFeatures): AuroraPostgresEngineVersion {

return new AuroraPostgresEngineVersion(auroraPostgresFullVersion, auroraPostgresMajorVersion, auroraPostgresFeatures);
}

/** The full version string, for example, "9.6.25.1". */
public readonly auroraPostgresFullVersion: string;
/** The major version of the engine, for example, "9.6". */
public readonly auroraPostgresMajorVersion: string;
/**
* The supported features for the DB engine
*
* @internal
*/
public readonly _features?: ClusterEngineFeatures;
shivlaks marked this conversation as resolved.
Show resolved Hide resolved

private constructor(auroraPostgresFullVersion: string, auroraPostgresMajorVersion: string) {
private constructor(auroraPostgresFullVersion: string, auroraPostgresMajorVersion: string, auroraPostgresFeatures?: AuroraPostgresEngineFeatures) {
this.auroraPostgresFullVersion = auroraPostgresFullVersion;
this.auroraPostgresMajorVersion = auroraPostgresMajorVersion;
this._features = {
s3Import: auroraPostgresFeatures?.s3Import ? 's3Import' : undefined,
s3Export: auroraPostgresFeatures?.s3Export ? 's3Export' : undefined,
};
}
}

Expand Down Expand Up @@ -428,9 +493,23 @@ class AuroraPostgresClusterEngine extends ClusterEngineBase {
majorVersion: version.auroraPostgresMajorVersion,
}
: undefined,
features: version?._features,
});
}

public bindToCluster(scope: core.Construct, options: ClusterEngineBindOptions): ClusterEngineConfig {
const config = super.bindToCluster(scope, options);
if (options.s3ImportRole && !(config.features?.s3Import)) {
throw new Error (`s3Import is not supported for engine: ${this.engineVersion?.fullVersion}. Use an engine version that supports the s3Import feature.`);
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
}
if (options.s3ExportRole && !(config.features?.s3Export)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These ifs here mean that the unversioned Postgres engine doesn't support S3 import/export at all. Is that true? Have you verified it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right that this code precludes supporting these features for the unversioned Postgres version.
The only reason I added these conditions are because it's a latent failure and it felt like adding validation would help users in getting ahead of it.

The reason I left out the unversioned feature support is because we've already marked them deprecated and the feature is not supported today. I didn't think it was a good idea to try and support it and keep that usage going and was hoping to nudge users towards the guidance we're providing in our doc string.

What do you think? is this validation worth it? should we drop it altogether?
This may also affect the default message for the error (should probably drop the ?? <unversioned>).

Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion is the following: if the RDS service supports it, we should support it as well.

Note that we might also have to "un-deprecate" these fields at some point before CDK v2.0 hits - see the discussion here, which makes fully supporting them even more relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, I didn't realize we weren't fully convinced that we wanted to keep it marked as @deprecated
added support for this case and skipped validation if the featurename is supported.

When unversioned is specified, both features are marked as supported. This is true today, but may not be if the version is changed out from underneath. I'm not sure the constant can support it both ways

Copy link
Contributor

Choose a reason for hiding this comment

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

I would mark is as supported for both, always (otherwise, this feature has no chance of working with the unversioned engine at all).

Worst comes to worst, it will fail at deploy time, which is fine by me.

Copy link
Contributor

@skinny85 skinny85 Sep 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, here's a thought.

Today, you always return the features names in the features property of ClusterEngineConfig. I wonder whether a better strategy would be to only return them if s3ImportRole / s3ExportRole was passed in ClusterEngineBindOptions...?

Yes, I understand the code in Cluster today uses the feature names only if s3ImportRole / s3ExportRole are != undefined... but I wonder whether that will be obvious when we implement other feature names, and whether it's actually better to not return them if they were not requested...

Thoughts on this change?

throw new Error (`s3Export is not supported for engine: ${this.engineVersion?.fullVersion}. Use an engine version that supports the s3Export feature.`);
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
}
return {
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
...config,
};
}

protected defaultParameterGroup(scope: core.Construct): IParameterGroup | undefined {
if (!this.parameterGroupFamily) {
throw new Error('Could not create a new ParameterGroup for an unversioned aurora-postgresql cluster engine. ' +
Expand Down
17 changes: 9 additions & 8 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,21 +305,22 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
}),
];

const clusterAssociatedRoles: CfnDBCluster.DBClusterRoleProperty[] = [];
let { s3ImportRole, s3ExportRole } = this.setupS3ImportExport(props);
if (s3ImportRole) {
clusterAssociatedRoles.push({ roleArn: s3ImportRole.roleArn });
}
if (s3ExportRole) {
clusterAssociatedRoles.push({ roleArn: s3ExportRole.roleArn });
}

// bind the engine to the Cluster
const clusterEngineBindConfig = props.engine.bindToCluster(this, {
s3ImportRole,
s3ExportRole,
parameterGroup: props.parameterGroup,
});

const clusterAssociatedRoles: CfnDBCluster.DBClusterRoleProperty[] = [];
if (s3ImportRole) {
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
clusterAssociatedRoles.push({ roleArn: s3ImportRole.roleArn, featureName: clusterEngineBindConfig.features?.s3Import });
}
if (s3ExportRole) {
clusterAssociatedRoles.push({ roleArn: s3ExportRole.roleArn, featureName: clusterEngineBindConfig.features?.s3Export });
}

const clusterParameterGroup = props.parameterGroup ?? clusterEngineBindConfig.parameterGroup;
const clusterParameterGroupConfig = clusterParameterGroup?.bindToCluster({});

Expand Down
136 changes: 135 additions & 1 deletion packages/@aws-cdk/aws-rds/test/test.cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,140 @@ export = {
test.done();
},

'cluster with s3 import bucket adds supported feature name to IAM role'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const bucket = new s3.Bucket(stack, 'Bucket');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.auroraPostgres({
version: AuroraPostgresEngineVersion.VER_10_12,
}),
instances: 1,
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
vpc,
},
s3ImportBuckets: [bucket],
});

// THEN
expect(stack).to(haveResource('AWS::RDS::DBCluster', {
AssociatedRoles: [{
RoleArn: {
'Fn::GetAtt': [
'DatabaseS3ImportRole377BC9C0',
'Arn',
],
},
FeatureName: 's3Import',
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
}],
}));

test.done();
},

'throws when s3 import bucket is supplied and database engine does not support it'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const bucket = new s3.Bucket(stack, 'Bucket');

// WHEN / THEN
test.throws(() => {
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.auroraPostgres({
version: AuroraPostgresEngineVersion.VER_10_4,
}),
instances: 1,
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
vpc,
},
s3ImportBuckets: [bucket],
});
}), /s3Import is not supported for engine version 10.4. Use an engine version that supports the s3Import feature./;

test.done();
},

'throws when s3 export bucket is supplied and database engine does not support it'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const bucket = new s3.Bucket(stack, 'Bucket');

// WHEN / THEN
test.throws(() => {
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.auroraPostgres({
version: AuroraPostgresEngineVersion.VER_10_4,
}),
instances: 1,
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
vpc,
},
s3ExportBuckets: [bucket],
});
}), /s3Import is not supported for engine version 10.4. Use an engine version that supports the s3Import feature./;

test.done();
},

'cluster with s3 export bucket adds supported feature name to IAM role'(test: Test) {
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const bucket = new s3.Bucket(stack, 'Bucket');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.auroraPostgres({
version: AuroraPostgresEngineVersion.VER_10_12,
}),
instances: 1,
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
vpc,
},
s3ExportBuckets: [bucket],
});

// THEN
expect(stack).to(haveResource('AWS::RDS::DBCluster', {
AssociatedRoles: [{
RoleArn: {
'Fn::GetAtt': [
'DatabaseS3ExportRole9E328562',
'Arn',
],
},
FeatureName: 's3Export',
}],
}));

test.done();
},

'create a cluster with s3 export role'(test: Test) {
// GIVEN
const stack = testStack();
Expand Down Expand Up @@ -1125,7 +1259,7 @@ export = {
// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.auroraPostgres({
version: AuroraPostgresEngineVersion.VER_11_4,
version: AuroraPostgresEngineVersion.VER_11_6,
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
}),
instances: 1,
masterUser: {
Expand Down