chore(rds): Refactor DatabaseCluster constructor (#9796)

The constructor was getting a bit long (~200 lines), so did a quick refactor to pull out the import/export and instance creation into their own methods. Also added some optional chaining and fixed a typo in the README. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Aug 19, 2020 · 4d044d0 · 4d044d0
1 parent 6edb6e6
commit 4d044d0
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 89 deletions.
diff --git a/packages/@aws-cdk/aws-rds/README.md b/packages/@aws-cdk/aws-rds/README.md
@@ -49,9 +49,9 @@ use the static factory methods on `DatabaseClusterEngine`:
 new rds.DatabaseCluster(this, 'Database', {
   engine: rds.DatabaseClusterEngine.aurora({
     version: rds.AuroraEngineVersion.VER_1_17_9, // different version class for each engine type
-  },
+  }),
   ...
-})
+});
 ```
 
 If there isn't a constant for the exact version you want to use,

diff --git a/packages/@aws-cdk/aws-rds/lib/cluster.ts b/packages/@aws-cdk/aws-rds/lib/cluster.ts
@@ -397,42 +397,13 @@ export class DatabaseCluster extends DatabaseClusterBase {
     this.singleUserRotationApplication = props.engine.singleUserRotationApplication;
     this.multiUserRotationApplication = props.engine.multiUserRotationApplication;
 
-    let s3ImportRole = props.s3ImportRole;
-    if (props.s3ImportBuckets && props.s3ImportBuckets.length > 0) {
-      if (props.s3ImportRole) {
-        throw new Error('Only one of s3ImportRole or s3ImportBuckets must be specified, not both.');
-      }
-
-      s3ImportRole = new Role(this, 'S3ImportRole', {
-        assumedBy: new ServicePrincipal('rds.amazonaws.com'),
-      });
-      for (const bucket of props.s3ImportBuckets) {
-        bucket.grantRead(s3ImportRole);
-      }
-    }
-
-    let s3ExportRole = props.s3ExportRole;
-    if (props.s3ExportBuckets && props.s3ExportBuckets.length > 0) {
-      if (props.s3ExportRole) {
-        throw new Error('Only one of s3ExportRole or s3ExportBuckets must be specified, not both.');
-      }
-
-      s3ExportRole = new Role(this, 'S3ExportRole', {
-        assumedBy: new ServicePrincipal('rds.amazonaws.com'),
-      });
-      for (const bucket of props.s3ExportBuckets) {
-        bucket.grantReadWrite(s3ExportRole);
-      }
-    }
-
     const clusterAssociatedRoles: CfnDBCluster.DBClusterRoleProperty[] = [];
-    if (s3ImportRole || s3ExportRole) {
-      if (s3ImportRole) {
-        clusterAssociatedRoles.push({ roleArn: s3ImportRole.roleArn });
-      }
-      if (s3ExportRole) {
-        clusterAssociatedRoles.push({ roleArn: s3ExportRole.roleArn });
-      }
+    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
@@ -461,13 +432,13 @@ export class DatabaseCluster extends DatabaseClusterBase {
         : (props.masterUser.password
           ? props.masterUser.password.toString()
           : undefined),
-      backupRetentionPeriod: props.backup && props.backup.retention && props.backup.retention.toDays(),
-      preferredBackupWindow: props.backup && props.backup.preferredWindow,
+      backupRetentionPeriod: props.backup?.retention?.toDays(),
+      preferredBackupWindow: props.backup?.preferredWindow,
       preferredMaintenanceWindow: props.preferredMaintenanceWindow,
       databaseName: props.defaultDatabaseName,
       enableCloudwatchLogsExports: props.cloudwatchLogsExports,
       // Encryption
-      kmsKeyId: props.storageEncryptionKey && props.storageEncryptionKey.keyArn,
+      kmsKeyId: props.storageEncryptionKey?.keyArn,
       storageEncrypted: props.storageEncryptionKey ? true : props.storageEncrypted,
     });
 
@@ -495,6 +466,91 @@ export class DatabaseCluster extends DatabaseClusterBase {
       this.secret = secret.attach(this);
     }
 
+    this.createInstances(props, cluster, subnetGroup, portAttribute);
+
+    const defaultPort = ec2.Port.tcp(this.clusterEndpoint.port);
+    this.connections = new ec2.Connections({ securityGroups, defaultPort });
+  }
+
+
+  /**
+   * Adds the single user rotation of the master password to this cluster.
+   *
+   * @param [automaticallyAfter=Duration.days(30)] Specifies the number of days after the previous rotation
+   * before Secrets Manager triggers the next automatic rotation.
+   */
+  public addRotationSingleUser(automaticallyAfter?: Duration): secretsmanager.SecretRotation {
+    if (!this.secret) {
+      throw new Error('Cannot add single user rotation for a cluster without secret.');
+    }
+
+    const id = 'RotationSingleUser';
+    const existing = this.node.tryFindChild(id);
+    if (existing) {
+      throw new Error('A single user rotation was already added to this cluster.');
+    }
+
+    return new secretsmanager.SecretRotation(this, id, {
+      secret: this.secret,
+      automaticallyAfter,
+      application: this.singleUserRotationApplication,
+      vpc: this.vpc,
+      vpcSubnets: this.vpcSubnets,
+      target: this,
+    });
+  }
+
+  /**
+   * Adds the multi user rotation to this cluster.
+   */
+  public addRotationMultiUser(id: string, options: RotationMultiUserOptions): secretsmanager.SecretRotation {
+    if (!this.secret) {
+      throw new Error('Cannot add multi user rotation for a cluster without secret.');
+    }
+    return new secretsmanager.SecretRotation(this, id, {
+      secret: options.secret,
+      masterSecret: this.secret,
+      automaticallyAfter: options.automaticallyAfter,
+      application: this.multiUserRotationApplication,
+      vpc: this.vpc,
+      vpcSubnets: this.vpcSubnets,
+      target: this,
+    });
+  }
+
+  private setupS3ImportExport(props: DatabaseClusterProps): { s3ImportRole?: IRole, s3ExportRole?: IRole } {
+    let s3ImportRole = props.s3ImportRole;
+    if (props.s3ImportBuckets && props.s3ImportBuckets.length > 0) {
+      if (props.s3ImportRole) {
+        throw new Error('Only one of s3ImportRole or s3ImportBuckets must be specified, not both.');
+      }
+
+      s3ImportRole = new Role(this, 'S3ImportRole', {
+        assumedBy: new ServicePrincipal('rds.amazonaws.com'),
+      });
+      for (const bucket of props.s3ImportBuckets) {
+        bucket.grantRead(s3ImportRole);
+      }
+    }
+
+    let s3ExportRole = props.s3ExportRole;
+    if (props.s3ExportBuckets && props.s3ExportBuckets.length > 0) {
+      if (props.s3ExportRole) {
+        throw new Error('Only one of s3ExportRole or s3ExportBuckets must be specified, not both.');
+      }
+
+      s3ExportRole = new Role(this, 'S3ExportRole', {
+        assumedBy: new ServicePrincipal('rds.amazonaws.com'),
+      });
+      for (const bucket of props.s3ExportBuckets) {
+        bucket.grantReadWrite(s3ExportRole);
+      }
+    }
+
+    return { s3ImportRole, s3ExportRole };
+  }
+
+  private createInstances(props: DatabaseClusterProps, cluster: CfnDBCluster, subnetGroup: CfnDBSubnetGroup, portAttribute: number) {
     const instanceCount = props.instances != null ? props.instances : 2;
     if (instanceCount < 1) {
       throw new Error('At least one instance is required');
@@ -517,7 +573,6 @@ export class DatabaseCluster extends DatabaseClusterBase {
     const instanceParameterGroupConfig = props.instanceProps.parameterGroup?.bindToInstance({});
     for (let i = 0; i < instanceCount; i++) {
       const instanceIndex = i + 1;
-
       const instanceIdentifier = props.instanceIdentifierBase != null ? `${props.instanceIdentifierBase}${instanceIndex}` :
         props.clusterIdentifier != null ? `${props.clusterIdentifier}instance${instanceIndex}` :
           undefined;
@@ -555,54 +610,6 @@ export class DatabaseCluster extends DatabaseClusterBase {
       this.instanceIdentifiers.push(instance.ref);
       this.instanceEndpoints.push(new Endpoint(instance.attrEndpointAddress, portAttribute));
     }
-
-    const defaultPort = ec2.Port.tcp(this.clusterEndpoint.port);
-    this.connections = new ec2.Connections({ securityGroups, defaultPort });
-  }
-
-  /**
-   * Adds the single user rotation of the master password to this cluster.
-   *
-   * @param [automaticallyAfter=Duration.days(30)] Specifies the number of days after the previous rotation
-   * before Secrets Manager triggers the next automatic rotation.
-   */
-  public addRotationSingleUser(automaticallyAfter?: Duration): secretsmanager.SecretRotation {
-    if (!this.secret) {
-      throw new Error('Cannot add single user rotation for a cluster without secret.');
-    }
-
-    const id = 'RotationSingleUser';
-    const existing = this.node.tryFindChild(id);
-    if (existing) {
-      throw new Error('A single user rotation was already added to this cluster.');
-    }
-
-    return new secretsmanager.SecretRotation(this, id, {
-      secret: this.secret,
-      automaticallyAfter,
-      application: this.singleUserRotationApplication,
-      vpc: this.vpc,
-      vpcSubnets: this.vpcSubnets,
-      target: this,
-    });
-  }
-
-  /**
-   * Adds the multi user rotation to this cluster.
-   */
-  public addRotationMultiUser(id: string, options: RotationMultiUserOptions): secretsmanager.SecretRotation {
-    if (!this.secret) {
-      throw new Error('Cannot add multi user rotation for a cluster without secret.');
-    }
-    return new secretsmanager.SecretRotation(this, id, {
-      secret: options.secret,
-      masterSecret: this.secret,
-      automaticallyAfter: options.automaticallyAfter,
-      application: this.multiUserRotationApplication,
-      vpc: this.vpc,
-      vpcSubnets: this.vpcSubnets,
-      target: this,
-    });
   }
 
   private setLogRetention(props: DatabaseClusterProps) {