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

shivlaks · 2020-09-02T16:25:26Z

When the s3ImportBuckets or s3ExportBuckets properties are set, we also need
to include the name of the feature for the DB instance that the IAM role is to be associated with.

Excluding the feature name causes a deploy-time failure as follows:

The feature-name parameter must be provided with the current operation ...

Added an EngineFeatures struct to specify the feature name for s3Import and s3Export

Closes #4419
Closes #8201

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

skinny85

Looks really good! Some comments / ideas.

packages/@aws-cdk/aws-rds/lib/cluster-engine.ts

skinny85 · 2020-09-03T02:33:55Z

Also, can you verify whether the feature names are needed for the Postgres Aurora engine without a version?

skinny85 · 2020-09-14T17:31:07Z

@shivlaks can you resolve the conflicts with the current master? Thanks!

skinny85

Overall looks great!

Couple of minor questions before I give my 'ShipIt' 🙂

packages/@aws-cdk/aws-rds/test/test.cluster.ts

packages/@aws-cdk/aws-rds/lib/cluster-engine.ts

packages/@aws-cdk/aws-rds/test/test.cluster.ts

packages/@aws-cdk/aws-rds/lib/cluster-engine.ts

skinny85 · 2020-09-14T22:33:51Z

packages/@aws-cdk/aws-rds/lib/cluster-engine.ts

+    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.`);
+    }
+    if (options.s3ExportRole && !(config.features?.s3Export)) {


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

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

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.

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

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.

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?

This change introduces S3 import/export for DatabaseInstances, the same as what currently exists today for DatabaseClusters. This change was heavily influenced by #10132 (the work to introduce feature names for DatabaseCluster), and steals patterns and names heavily from it. **Implementation Notes:** * Unlike for clsuters, for instances, the feature names are required; if the feature name doesn't exist, we shouldn't be creating the role. * For both Oracle and SQL Server, all current/active versions support the same feature names. This simplified the implementation quite a bit. * I opted **not** to support features for the deprecated Oracle versions. * I moved the `setupS3ImportExport` helper function into a utils class. One quirk of the SQL Server requirement is that you must create an OptionGroup with only one role (for both import & export). Oracle, likewise, has a single feature for both import and export. So I opted to default to creating a single role (if necessary) for both import and export. Open to challenges on this. * The `OptionGroup` class needed some rework to be able to make the list of configurations dynamic. I then had to do some light tweaking to ensure backwards compatibility with the connections property. fixes #4419

… s3Export buckets

skinny85

Looks great! A few last notes if you want to hear them 🙂

packages/@aws-cdk/aws-rds/lib/cluster-engine.ts

mergify · 2020-09-17T09:15:25Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2020-09-17T09:49:42Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-09-17T09:50:24Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9f42b00
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

* feat(rds): S3 import and export for DatabaseInstances This change introduces S3 import/export for DatabaseInstances, the same as what currently exists today for DatabaseClusters. This change was heavily influenced by #10132 (the work to introduce feature names for DatabaseCluster), and steals patterns and names heavily from it. **Implementation Notes:** * Unlike for clusters, for instances, the feature names are required; if the feature name doesn't exist, we shouldn't be creating the role. * For both Oracle and SQL Server, all current/active versions support the same feature names. This simplified the implementation quite a bit. * I opted **not** to support features for the deprecated Oracle versions. * I moved the `setupS3ImportExport` helper function into a utils class. One quirk of the SQL Server requirement is that you must create an OptionGroup with only one role (for both import & export). Oracle, likewise, has a single feature for both import and export. So I opted to default to creating a single role (if necessary) for both import and export. Open to challenges on this. * The `OptionGroup` class needed some rework to be able to make the list of configurations dynamic. I then had to do some light tweaking to ensure backwards compatibility with the connections property. fixes #4419

shivlaks added 2 commits September 2, 2020 09:03

add engine features for s3Import and s3Export

34c0997

add test

2699247

shivlaks added the @aws-cdk/aws-rds Related to Amazon Relational Database label Sep 2, 2020

shivlaks requested review from skinny85 and njlynch September 2, 2020 16:25

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 2, 2020

skinny85 suggested changes Sep 3, 2020

View reviewed changes

SomayaB assigned skinny85 Sep 3, 2020

shivlaks added 4 commits September 3, 2020 15:47

wip - addressed some of the PR feedback

9774ddf

add empty line

ca46702

more PR feedback

fd761df

add validation on supported features, move logic into bindToCluster

4d00cfd

shivlaks marked this pull request as ready for review September 14, 2020 14:58

shivlaks requested a review from skinny85 September 14, 2020 14:58

shivlaks added 2 commits September 14, 2020 13:50

Merge branch 'master' into shivlaks/rds-engine-version-features

d22901f

missed a change from the merge

50e3f58

skinny85 reviewed Sep 14, 2020

View reviewed changes

PR feedback

4620ac1

shivlaks requested a review from skinny85 September 15, 2020 16:24

njlynch mentioned this pull request Sep 15, 2020

feat(rds): S3 import and export for DatabaseInstances #10370

Merged

add support for unversioned aurora postgresql to specify s3Import and…

3a2fa03

… s3Export buckets

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Sep 17, 2020

skinny85 approved these changes Sep 17, 2020

View reviewed changes

final tweaks

cf2c014

shivlaks removed the pr/do-not-merge This PR should not be merged at this time. label Sep 17, 2020

Merge branch 'master' into shivlaks/rds-engine-version-features

9f42b00

mergify bot merged commit cb6fef8 into master Sep 17, 2020

mergify bot deleted the shivlaks/rds-engine-version-features branch September 17, 2020 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shivlaks commented Sep 2, 2020 •

edited

Loading

skinny85 left a comment

skinny85 commented Sep 3, 2020

skinny85 commented Sep 14, 2020

skinny85 left a comment

skinny85 Sep 14, 2020

shivlaks Sep 15, 2020

skinny85 Sep 15, 2020

shivlaks Sep 16, 2020

skinny85 Sep 16, 2020

skinny85 Sep 16, 2020 •

edited

Loading

skinny85 left a comment

mergify bot commented Sep 17, 2020

mergify bot commented Sep 17, 2020

aws-cdk-automation commented Sep 17, 2020

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

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

Conversation

shivlaks commented Sep 2, 2020 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 commented Sep 3, 2020

skinny85 commented Sep 14, 2020

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Sep 14, 2020

Choose a reason for hiding this comment

shivlaks Sep 15, 2020

Choose a reason for hiding this comment

skinny85 Sep 15, 2020

Choose a reason for hiding this comment

shivlaks Sep 16, 2020

Choose a reason for hiding this comment

skinny85 Sep 16, 2020

Choose a reason for hiding this comment

skinny85 Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 17, 2020

mergify bot commented Sep 17, 2020

aws-cdk-automation commented Sep 17, 2020

AWS CodeBuild CI Report

shivlaks commented Sep 2, 2020 •

edited

Loading

skinny85 Sep 16, 2020 •

edited

Loading