Skip to content

Commit

Permalink
fix(rds): using both Instance imports & exports for Postgres fails de…
Browse files Browse the repository at this point in the history
…ployment (#17060)

Our CDK code was assuming that all instance engines that support S3 imports & exports
must use the same Role if both functions are enabled at the same time.
However, it turns out that's true only for Oracle and SQL Server,
but not for Postgres - in fact, Postgres has the exact opposite requirement,
and will fail deployment if the same Role is used for both.
Change our code to only use a single Role if the engine is Oracle or SQL Server.

Fixes #16757

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
skinny85 authored Oct 25, 2021
1 parent 924045d commit ab627c6
Show file tree
Hide file tree
Showing 5 changed files with 631 additions and 9 deletions.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
}),
];

let { s3ImportRole, s3ExportRole } = setupS3ImportExport(this, props);
let { s3ImportRole, s3ExportRole } = setupS3ImportExport(this, props, /* combineRoles */ false);
// bind the engine to the Cluster
const clusterEngineBindConfig = props.engine.bindToCluster(this, {
s3ImportRole,
Expand Down
13 changes: 7 additions & 6 deletions packages/@aws-cdk/aws-rds/lib/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,6 @@ export interface DatabaseInstanceNewProps {

/**
* Role that will be associated with this DB instance to enable S3 export.
* This feature is only supported by the Microsoft SQL Server and Oracle engines.
*
* This property must not be used if `s3ExportBuckets` is used.
*
Expand All @@ -591,7 +590,6 @@ export interface DatabaseInstanceNewProps {

/**
* S3 buckets that you want to load data into.
* This feature is only supported by the Microsoft SQL Server and Oracle engines.
*
* This property must not be used if `s3ExportRole` is used.
*
Expand Down Expand Up @@ -847,7 +845,10 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
this.multiUserRotationApplication = props.engine.multiUserRotationApplication;
this.engine = props.engine;

let { s3ImportRole, s3ExportRole } = setupS3ImportExport(this, props, true);
const engineType = props.engine.engineType;
// only Oracle and SQL Server require the import and export Roles to be the same
const combineRoles = engineType.startsWith('oracle-') || engineType.startsWith('sqlserver-');
let { s3ImportRole, s3ExportRole } = setupS3ImportExport(this, props, combineRoles);
const engineConfig = props.engine.bindToInstance(this, {
...props,
s3ImportRole,
Expand All @@ -866,8 +867,8 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
if (!engineFeatures?.s3Export) {
throw new Error(`Engine '${engineDescription(props.engine)}' does not support S3 export`);
}
// Only add the export role and feature if they are different from the import role & feature.
if (s3ImportRole !== s3ExportRole || engineFeatures.s3Import !== engineFeatures?.s3Export) {
// only add the export feature if it's different from the import feature
if (engineFeatures.s3Import !== engineFeatures?.s3Export) {
instanceAssociatedRoles.push({ roleArn: s3ExportRole.roleArn, featureName: engineFeatures?.s3Export });
}
}
Expand All @@ -883,7 +884,7 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
allowMajorVersionUpgrade: props.allowMajorVersionUpgrade,
dbName: props.databaseName,
dbParameterGroupName: instanceParameterGroupConfig?.parameterGroupName,
engine: props.engine.engineType,
engine: engineType,
engineVersion: props.engine.engineVersion?.fullVersion,
licenseModel: props.licenseModel,
timezone: props.timezone,
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-rds/lib/private/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ export interface DatabaseS3ImportExportProps {
* Validates the S3 import/export props and returns the import/export roles, if any.
* If `combineRoles` is true, will reuse the import role for export (or vice versa) if possible.
*
* Notably, `combineRoles` is (by default) set to true for instances, but false for clusters.
* Notably, `combineRoles` is set to true for instances, but false for clusters.
* This is because the `combineRoles` functionality is most applicable to instances and didn't exist
* for the initial clusters implementation. To maintain backwards compatibility, it is set to false for clusters.
*/
export function setupS3ImportExport(
scope: Construct,
props: DatabaseS3ImportExportProps,
combineRoles?: boolean): { s3ImportRole?: iam.IRole, s3ExportRole?: iam.IRole } {
combineRoles: boolean): { s3ImportRole?: iam.IRole, s3ExportRole?: iam.IRole } {

let s3ImportRole = props.s3ImportRole;
let s3ExportRole = props.s3ExportRole;
Expand Down
Loading

0 comments on commit ab627c6

Please sign in to comment.