feat(rds): introduce type-safe engine versions #9016

skinny85 · 2020-07-11T01:59:48Z

Change engine versions, for boh cluster and instance engines,
from strings to strongly-typed classes with static constants representing common versions.

Fixes #6532

BREAKING CHANGE: the property 'version' has been changed from string to an engine-specific
version class; use VersionClass.of() if you need to create a specific version of an engine from a string

rds: the property ParameterGroupProps.family has been renamed to engine, and its type changed from string to IEngine
rds: the property engineVersion in IClusterEngine changed from a string to EngineVersion
rds: the property engineVersion in IInstanceEngine changed from a string to EngineVersion
rds: the property parameterGroupFamily in IClusterEngine changed from required to optional
rds: the property parameterGroupFamily in IInstanceEngine changed from required to optional

skinny85 · 2020-07-11T02:00:02Z

Still missing:

ReadMe changes

njlynch

With this pattern, is there any equivalent of specifying major version, but leaving the minor version to latest/recommended? For example, can I say: "create an Aurora cluster with v2" or "MySQL 5.7" and let the CDK figure out the details?

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

skinny85 · 2020-07-14T02:17:55Z

With this pattern, is there any equivalent of specifying major version, but leaving the minor version to latest/recommended? For example, can I say: "create an Aurora cluster with v2" or "MySQL 5.7" and let the CDK figure out the details?

Not currently, but I really like the idea!

What API would you see for this? Something like AuroraEngineVersion.latest(majorVersion: string)?

skinny85 · 2020-07-14T21:03:55Z

cc @jogold if you'd like to weigh in here, I would love to use your expertise!

skinny85 · 2020-07-15T01:56:26Z

With this pattern, is there any equivalent of specifying major version, but leaving the minor version to latest/recommended? For example, can I say: "create an Aurora cluster with v2" or "MySQL 5.7" and let the CDK figure out the details?

Not currently, but I really like the idea!

What API would you see for this? Something like AuroraEngineVersion.latest(majorVersion: string)?

Done for the instance engines (unfortunately, there is no such option for cluster engines, at least that I know of).

skinny85 · 2020-07-15T02:06:14Z

@njlynch @shivlaks @jogold this is ready for another round of reviews. I also have one open question, that I want your opinion on.

I wonder what should be the behavior for engines without a version specified, with respect to their parameterGroupFamily used in ParameterGroup and majorVersion used in OptionGroup. Today, for the engines that don't a version specified (so, the static constants from DatabaseClusterEngine and DatabaseInstanceEngine), they have a well-defined "default" majorVersion (and, by extension, parameterGroupFamily, as that is equal to <engine-type><major-version>). In this PR, I preserved this behavior, but I'm having second thoughts about this now.

For example, let's take the DatabaseInstanceEngine.POSTGRES constant. It uses 11 as its default major version. But there is already version 12 supported by RDS. Does it mean, at some point, that creating a postgres engine without a version will start using major version 12, instead of 11? At that point, will all of the ParameterGroups and OptionGroups created with 11 be invalid? It seems like a pretty risky thing to do.

I wonder if we should guard customers from this potential mistake, and simply say that engines without a version don't have a majorVersion defined, and thus cannot be used as arguments when creating ParameterGroup and OptionGroup. It would be a breaking change, of course, but maybe it's the right thing to do, considering how dangerous this functionality might be?

I would love to hear your thoughts!

jogold · 2020-07-15T08:29:46Z

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

+ */
+export class OracleEngineVersion {
+  /** Version "11.2" (only a major version, without a specific minor version). */
+  public static readonly VER_11_2 = OracleEngineVersion.of('11.2', '11.2');


https://forums.aws.amazon.com/ann.jspa?annID=7341

Amazon RDS for Oracle will end the support for Oracle Database version 11.2.0.4 Standard Edition 1 (SE1) for license-included (LI) on October 31, 2020. For bring-your-own-license (BYOL), RDS for Oracle will end the support for Oracle Database version 11.2.0.4 for all editions on December 31, 2020.

Starting on August 1, 2020, Amazon RDS for Oracle will disable the ability to create new DB instances using 11.2.0.4 on SE1 with LI.

You can drop all the VER_11_xxx (and the ORACLE_SE and ORACLE_SE1 because they only allow v11 instances).

This also means that for Oracle you maybe need specific versions lists per engine (ee, se2).

OK. The way I did it, is I split the existing one class into 2. OracleLegacyEngineVersion and OracleEngineVersion. The first one has all of the 11.2 versions, and is used in oracleSe and oracleSe1 methods. The other one has all version 12.1 and up, and is used for oracleSe2 and oracleEe methods.

Let me know if this makes sense @jogold !

Looks good.

packages/@aws-cdk/aws-rds/README.md

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

jogold · 2020-07-15T10:05:01Z

I wonder what should be the behavior for engines without a version specified

I personally think that the CDK should not let users create an instance/cluster without specifying a version. Making the version required will force users to think about it vs forget to specify it by mistake with potentially bad consequences (+ RDS deploy times are really long so this mistake can take hours to notice/fix).

A RDS instance/cluster is a costly piece of cloud infrastructure (and a complex one) and users (usually experienced ones) creating those instances/clusters should be forced to "design" before deploy.

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

packages/@aws-cdk/aws-rds/test/integ.instance.lit.ts

shivlaks · 2020-07-15T22:41:25Z

For example, let's take the DatabaseInstanceEngine.POSTGRES constant. It uses 11 as its default major version. But there is already version 12 supported by RDS. Does it mean, at some point, that creating a postgres engine without a version will start using major version 12, instead of 11? At that point, will all of the ParameterGroups and OptionGroups created with 11 be invalid? It seems like a pretty risky thing to do.

@skinny85 - I also share your concern for the versioning. In this example, if version 12 is supported, is it available in all regions at the same time? upgrading their CDK app would also switch out the version underneath the customer.

We had a similar problem in stepfunctions-tasks where Fargate has a "LATEST" which currently points to 1.3 and 1.4 has been out for a little bit now. It will presumably change to 1.4 at some point. To be safe, we ended up making the platform version required with the understanding that we can relax the condition and make it optional in the future if we decide there's a sensible default that doesn't hide a future problem.

I'd be inclined to go towards making it required for now and relaxing that condition at a later time (if it makes sense to).
@jogold also makes some good points around cost, deployment time, remediation time(s).

skinny85 · 2020-07-16T00:22:34Z

I wonder what should be the behavior for engines without a version specified

I personally think that the CDK should not let users create an instance/cluster without specifying a version. Making the version required will force users to think about it vs forget to specify it by mistake with potentially bad consequences (+ RDS deploy times are really long so this mistake can take hours to notice/fix).

A RDS instance/cluster is a costly piece of cloud infrastructure (and a complex one) and users (usually experienced ones) creating those instances/clusters should be forced to "design" before deploy.

For example, let's take the DatabaseInstanceEngine.POSTGRES constant. It uses 11 as its default major version. But there is already version 12 supported by RDS. Does it mean, at some point, that creating a postgres engine without a version will start using major version 12, instead of 11? At that point, will all of the ParameterGroups and OptionGroups created with 11 be invalid? It seems like a pretty risky thing to do.

@skinny85 - I also share your concern for the versioning. In this example, if version 12 is supported, is it available in all regions at the same time? upgrading their CDK app would also switch out the version underneath the customer.

We had a similar problem in stepfunctions-tasks where Fargate has a "LATEST" which currently points to 1.3 and 1.4 has been out for a little bit now. It will presumably change to 1.4 at some point. To be safe, we ended up making the platform version required with the understanding that we can relax the condition and make it optional in the future if we decide there's a sensible default that doesn't hide a future problem.

I'd be inclined to go towards making it required for now and relaxing that condition at a later time (if it makes sense to).
@jogold also makes some good points around cost, deployment time, remediation time(s).

OK. How about this?

Let's deprecate the current unversioned constants in DatabaseClusterEngine and InstanceClusterEngine, saying using them is not recommended. I feel like outright banning unversioned Clusters and Instances is a little too harsh.
Let's make version required in all static engine construction methods.
Let's remove the capability of the unversioned engines to have a default majorVersion (and thus a parameterGroupFamily). So, it will not be possible to use them for ParameterGroup and OptionGroup. EDIT: with the exception of Aurora and AuroraMysql, who always have majorVersion 5.6 and 5.7, respectively.

How does this sound @jogold @shivlaks ?

skinny85 · 2020-07-16T02:47:51Z

@njlynch @shivlaks @jogold I've implemented the discussed changes - this is ready for another round of reviews!

jogold · 2020-07-16T09:32:04Z

OK. How about this?

Let's deprecate the current unversioned constants in DatabaseClusterEngine and InstanceClusterEngine, saying using them is not recommended. I feel like outright banning unversioned Clusters and Instances is a little too harsh.

Let's make version required in all static engine construction methods.

Let's remove the capability of the unversioned engines to have a default majorVersion (and thus a parameterGroupFamily). So, it will not be possible to use them for ParameterGroup and OptionGroup. EDIT: with the exception of Aurora and AuroraMysql, who always have majorVersion 5.6 and 5.7, respectively.

How does this sound @jogold @shivlaks ?

Sounds good!

njlynch

Generally looks good to me - would want @shivlaks and/or @jogold to take another pass as well, since they've had more feedback than me.

njlynch · 2020-07-16T16:21:40Z

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

+ * The versions for the legacy Oracle instance engines
+ * (those returned by {@link DatabaseInstanceEngine.oracleSe}
+ * and {@link DatabaseInstanceEngine.oracleSe1}).
+ * Note: RDS will stop allowing creating new databases with this version in August 2020.


Is it worth pushing this only to deprecate it in one month?

I think so. People still might have CDK code with existing databases using it.

They can use the of() then?

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

shivlaks

I'm good with these changes. thanks for keeping with it and getting it here!!!

packages/@aws-cdk/aws-rds/README.md

jogold · 2020-07-16T19:37:33Z

packages/@aws-cdk/aws-rds/test/integ.instance.lit.ts

-      engine: rds.DatabaseInstanceEngine.oracleSe1({
-        version: '11.2',
-      }),
+      engine: rds.DatabaseInstanceEngine.ORACLE_SE1,


We should change to SE2 here also because this integ test will soon not deploy anymore, the instance should also be updated.

You're right. I'll do that in a follow-up PR (probably when we deprecate oracleSe and oracleSe1).

Change engine versions, for boh cluster and instance engines, from strings to strongly-typed classes with static constants representing common versions. Fixes aws#6532 BREAKING CHANGE: the property 'version' has been changed from string to an engine-specific version class; use VersionClass.of() if you need to create a specific version of an engine from a string * **rds**: the property engineVersion in IClusterEngine changed from a string to EngineVersion * **rds**: the property engineVersion in IInstanceEngine changed from a string to EngineVersion * **rds**: the property parameterGroupFamily in IClusterEngine changed from required to optional * **rds**: the property parameterGroupFamily in IInstanceEngine changed from required to optional

aws-cdk-automation · 2020-07-16T20:07:56Z

AWS CodeBuild CI Report

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

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

mergify · 2020-07-16T20:08:22Z

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

Change engine versions, for boh cluster and instance engines, from strings to strongly-typed classes with static constants representing common versions. Fixes aws#6532 BREAKING CHANGE: the property 'version' has been changed from string to an engine-specific version class; use VersionClass.of() if you need to create a specific version of an engine from a string * **rds**: the property ParameterGroupProps.family has been renamed to engine, and its type changed from string to IEngine * **rds**: the property engineVersion in IClusterEngine changed from a string to EngineVersion * **rds**: the property engineVersion in IInstanceEngine changed from a string to EngineVersion * **rds**: the property parameterGroupFamily in IClusterEngine changed from required to optional * **rds**: the property parameterGroupFamily in IInstanceEngine changed from required to optional

skinny85 requested review from njlynch and shivlaks July 11, 2020 01:59

skinny85 self-assigned this Jul 11, 2020

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

njlynch reviewed Jul 13, 2020

View reviewed changes

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

skinny85 force-pushed the feat/rds-engine-versions branch from bc561f1 to 5736be9 Compare July 15, 2020 01:53

skinny85 requested a review from njlynch July 15, 2020 02:07

jogold reviewed Jul 15, 2020

View reviewed changes

shivlaks reviewed Jul 15, 2020

View reviewed changes

packages/@aws-cdk/aws-rds/lib/engine-version.ts Show resolved Hide resolved

packages/@aws-cdk/aws-rds/test/integ.instance.lit.ts Show resolved Hide resolved

skinny85 force-pushed the feat/rds-engine-versions branch from 5736be9 to 07d38da Compare July 16, 2020 02:47

skinny85 requested a review from shivlaks July 16, 2020 02:47

njlynch mentioned this pull request Jul 16, 2020

feat(rds): unify ParameterGroup and ClusterParameterGroup #8959

Merged

njlynch reviewed Jul 16, 2020

View reviewed changes

shivlaks approved these changes Jul 16, 2020

View reviewed changes

skinny85 force-pushed the feat/rds-engine-versions branch from 07d38da to 2f04ec7 Compare July 16, 2020 19:31

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Jul 16, 2020

jogold reviewed Jul 16, 2020

View reviewed changes

packages/@aws-cdk/aws-rds/README.md Outdated Show resolved Hide resolved

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

jogold reviewed Jul 16, 2020

View reviewed changes

skinny85 force-pushed the feat/rds-engine-versions branch from 2f04ec7 to a6246b5 Compare July 16, 2020 19:41

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Jul 16, 2020

skinny85 force-pushed the feat/rds-engine-versions branch from a6246b5 to 9eddbca Compare July 16, 2020 19:48

mergify bot merged commit fab7e28 into aws:master Jul 16, 2020

skinny85 deleted the feat/rds-engine-versions branch July 16, 2020 20:08

iliapolo mentioned this pull request Jul 20, 2020

fix(appsync): strongly type expires prop in apiKeyConfig #9122

Merged

skinny85 mentioned this pull request Jul 24, 2020

Deprecate OracleSE and OracleSE1 engines after RDS blocks creating new instances (August 2020) #9249

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): introduce type-safe engine versions #9016

feat(rds): introduce type-safe engine versions #9016

skinny85 commented Jul 11, 2020 •

edited

skinny85 commented Jul 11, 2020 •

edited

njlynch left a comment

skinny85 commented Jul 14, 2020

skinny85 commented Jul 14, 2020

skinny85 commented Jul 15, 2020

skinny85 commented Jul 15, 2020 •

edited

jogold Jul 15, 2020

skinny85 Jul 16, 2020

jogold Jul 16, 2020

jogold commented Jul 15, 2020 •

edited

shivlaks commented Jul 15, 2020

skinny85 commented Jul 16, 2020 •

edited

skinny85 commented Jul 16, 2020

jogold commented Jul 16, 2020

njlynch left a comment

njlynch Jul 16, 2020

skinny85 Jul 16, 2020

jogold Jul 16, 2020

shivlaks left a comment

jogold Jul 16, 2020

skinny85 Jul 16, 2020 •

edited

aws-cdk-automation commented Jul 16, 2020

mergify bot commented Jul 16, 2020

feat(rds): introduce type-safe engine versions #9016

feat(rds): introduce type-safe engine versions #9016

Conversation

skinny85 commented Jul 11, 2020 • edited

skinny85 commented Jul 11, 2020 • edited

njlynch left a comment

Choose a reason for hiding this comment

skinny85 commented Jul 14, 2020

skinny85 commented Jul 14, 2020

skinny85 commented Jul 15, 2020

skinny85 commented Jul 15, 2020 • edited

jogold Jul 15, 2020

Choose a reason for hiding this comment

skinny85 Jul 16, 2020

Choose a reason for hiding this comment

jogold Jul 16, 2020

Choose a reason for hiding this comment

jogold commented Jul 15, 2020 • edited

shivlaks commented Jul 15, 2020

skinny85 commented Jul 16, 2020 • edited

skinny85 commented Jul 16, 2020

jogold commented Jul 16, 2020

njlynch left a comment

Choose a reason for hiding this comment

njlynch Jul 16, 2020

Choose a reason for hiding this comment

skinny85 Jul 16, 2020

Choose a reason for hiding this comment

jogold Jul 16, 2020

Choose a reason for hiding this comment

shivlaks left a comment

Choose a reason for hiding this comment

jogold Jul 16, 2020

Choose a reason for hiding this comment

skinny85 Jul 16, 2020 • edited

Choose a reason for hiding this comment

aws-cdk-automation commented Jul 16, 2020

AWS CodeBuild CI Report

mergify bot commented Jul 16, 2020

skinny85 commented Jul 11, 2020 •

edited

skinny85 commented Jul 11, 2020 •

edited

skinny85 commented Jul 15, 2020 •

edited

jogold commented Jul 15, 2020 •

edited

skinny85 commented Jul 16, 2020 •

edited

skinny85 Jul 16, 2020 •

edited