feat(rds): support certificate autority certificate #26883

lpizzinidev · 2023-08-25T08:38:47Z

Exposes the caCertificate property for an RDS instance to allow specifying a custom CA identifier using the CaCertificate class.

Usage:

new DatabaseInstance(stack, 'Instance', {
      engine:DatabaseInstanceEngine.mysql({ version: rds.MysqlEngineVersion.VER_8_0_30 }),
      instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE3, ec2.InstanceSize.SMALL),
      vpc,
      caCertificate: CaCertificate.RDS_CA_RSA2048_G1,
});

Closes #26865.

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

mrgrain

Nice one, thank you. Got just a couple requests.

mrgrain · 2023-08-25T10:41:54Z

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

+   *
+   * @default - rds-ca-2019
+   */
+  readonly caCertificateIdentifier?: CertificateIdentifier;


Suggested change

readonly caCertificateIdentifier?: CertificateIdentifier;

readonly caCertificate?: CertificateIdentifier;

Can you drop the Identifier everywhere please. It's not really relevant to the user intent.
Also in the docstring, e.g. The the CA certificate used for this DB instance.

mrgrain · 2023-08-25T10:42:05Z

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

+/**
+ * The CA identifier of the DB instance
+ */
+export enum CertificateIdentifier {


Suggested change

export enum CertificateIdentifier {

export enum CaCertificate {

mrgrain · 2023-08-25T10:43:56Z

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

+/**
+ * The CA identifier of the DB instance
+ */
+export enum CertificateIdentifier {


Can you turn the enum into an enum-like class instead, with the ability to create a CaCertificate from a custom string: new CaCertificate('future-identifier')

class CaCertificate { static public RDS_CA_2019() { return new CaCertificate('rds-ca-2019') } public constructor(identifier: string) { } }

Reason is that this property strikes me as one that will likely have new values added in future.
With an enum-like class, users can always use whatever value they want and don't need to wait for us to add the new value and/or can use new values without having to upgrade the CDK version.

I'm more in favor of enums vs enum-like classes. How often do CaCertificate change that ability to add new values is needed?

With an enum-like class, users can always use whatever value they want and don't need to wait for us to add the new value and/or can use new values without having to upgrade the CDK version.

Does this mean enum-like classes will be used all across other parts of CDK instead of enums?

Pull request has been modified.

robertd · 2023-08-26T17:14:53Z

@mrgrain @lpizzinidev I'm warming up to enum-like class approach 😄 . Would this approach be better than getters & setters? In the code we'd use CaCertificate.RDS_CA_2019 compared to CaCertificate.rdsCaRsa2019() which feels more like invoking an enum. Thoughts?

export class CaCertificate {
  /**
   * rds-ca-2019 certificate authority
   */
  public static readonly RDS_CA_2019 = CaCertificate.of('rds-ca-2019');

  /**
   * rds-ca-ecc384-g1 certificate authority
   */
  public static readonly RDS_CA_ECC384_G1 = CaCertificate.of('rds-ca-ecc384-g1');

  /**
   * rds-ca-rsa2048-g1 certificate authority
   */
  public static readonly RDS_CA_RDS2048_G1 = CaCertificate.of('rds-ca-rsa2048-g1');

  /**
   * rds-ca-rsa4096-g1 certificate authority
   */
  public static readonly RDS_CA_RDS4096_G1 = CaCertificate.of('rds-ca-rsa4096-g1');

  /**
   * Custom certificate identifier
   * @param custom certificate authority identifier
   */
  public static of(identifier: string) { return new CaCertificate(identifier); }

  /**
   *
   * @param certificate authority identifier
   */
  private constructor(public readonly identifier: string) { }
}

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

robertd · 2023-08-25T21:42:25Z

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

+/**
+ * The CA identifier of the DB instance
+ */
+export enum CertificateIdentifier {


I'm more in favor of enums vs enum-like classes. How often do CaCertificate change that ability to add new values is needed?

With an enum-like class, users can always use whatever value they want and don't need to wait for us to add the new value and/or can use new values without having to upgrade the CDK version.

Does this mean enum-like classes will be used all across other parts of CDK instead of enums?

mrgrain · 2023-08-27T11:10:14Z

@robertd Thanks for the review! Yes this is exactly what I had in mind. Apologies for not conveying this in my review.

I think in general it's the better pattern because it enables us to easily extend features in future. E.g. what if (theoretically) you could upload your own CA Certificate in future. It's more strongly typed and unlike enums can be extended on by userland code. In the wider TS community, there's some talk at the moment about how enums are bad in general.

So yes, we probably should use enum-like classes everywhere instead.

In practice, it might more relevant for some situations than others. I picked it up here, because historically with certificates there has ALWAYS be a newer one. Whereas other choices often are fundamentally more limited.

robertd · 2023-08-27T15:08:36Z

@mrgrain Yep, makes total sense in this use case. If @lpizzinidev can change enum-like class to use CaCertificate.RDS_CA_2019 approach instead of CaCertificate.rdsCaRsa2019() one (and update stuff in other places accordingly), then I think we’re good to ship this. 👍

export class CaCertificate {
  /**
   * rds-ca-2019 certificate authority
   */
  public static readonly RDS_CA_2019 = CaCertificate.of('rds-ca-2019');

  /**
   * rds-ca-ecc384-g1 certificate authority
   */
  public static readonly RDS_CA_ECC384_G1 = CaCertificate.of('rds-ca-ecc384-g1');

  /**
   * rds-ca-rsa2048-g1 certificate authority
   */
  public static readonly RDS_CA_RDS2048_G1 = CaCertificate.of('rds-ca-rsa2048-g1');

  /**
   * rds-ca-rsa4096-g1 certificate authority
   */
  public static readonly RDS_CA_RDS4096_G1 = CaCertificate.of('rds-ca-rsa4096-g1');

  /**
   * Custom certificate identifier
   * @param custom certificate authority identifier
   */
  public static of(identifier: string) { return new CaCertificate(identifier); }

  /**
   *
   * @param certificate authority identifier
   */
  private constructor(public readonly identifier: string) { }
}

robertd · 2023-08-27T15:13:03Z

@lpizzinidev is on the top of the game with this PR with updating it to the latest changes we discussed. LGTM. 👍

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

lpizzinidev · 2023-08-28T06:29:32Z

@robertd Thanks for the feedback.
I missed the blueprint used for enum-like classes in the rest of the project.
Now the code is much cleaner 👍

mrgrain

One last question regarding the default.

mrgrain · 2023-08-28T10:43:10Z

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

+   * For Aurora DB engines:
+   * @see https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/UsingWithRDS.SSL-certificate-rotation.html
+   *
+   * @default - CaCertificate.RDS_CA_2019


This default is unexpected and seems like a potentially breaking change to me. Can you explain or remove it?

Trying to create an instance from the console, it suggests rds-ca-2019 as default (this is the reason for the @default).

Anyway, looking at the CFN documentation, it does not specify any default, so I guess I should remove it.

I see. It probably doesn't change much, but I think we want to end up in this case:
"If you don't select a certificate authority, RDS chooses one for you."

Suggested change

* @default - CaCertificate.RDS_CA_2019

* @default - RDS will choose a certificate authority

Defaulting to ‘rds-ca-2019’ will definitely be a breaking change. We also have a year before this becomes an issue. :)… until then… hopefully, anyone creating new instances/clusters will move to the newer CAs ahead of time anyway. :)

I see. It probably doesn't change much, but I think we want to end up in this case: "If you don't select a certificate authority, RDS chooses one for you."

AWS will pick the CA flavored soup of the day lol… I like it :)

mrgrain · 2023-08-28T10:44:58Z

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

@@ -865,6 +920,7 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
      domain: this.domainId,
      domainIamRoleName: this.domainRole?.roleName,
      networkType: props.networkType,
+      caCertificateIdentifier: props.caCertificate ? props.caCertificate.identifier : CaCertificate.RDS_CA_2019.identifier,


Default question, see above.

Suggested change

caCertificateIdentifier: props.caCertificate ? props.caCertificate.identifier : CaCertificate.RDS_CA_2019.identifier,

caCertificateIdentifier: props.caCertificate ? props.caCertificate.identifier : undefined,

Btw, we also could implement a toString() method on CaCertificate. This is works as well though.

Pull request has been modified.

mergify · 2023-08-28T14:37:06Z

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

robertd

LGTM. Many thanks @lpizzinidev 🚀

Pull request has been modified.

StefanPrintezis · 2023-08-28T17:21:44Z

static .of() usually refers to a known given, like an enum. Why would the constructor be private and then publish the of( method to be a constructor wrapper, it's a bit inverse right now. If you want a custom CA i'd think: new PrivateCa('custom-ca')

I think I'd still prefer to go with the enums for consistency. Feels more light weight than having the instances be instantiated in properties of a static class.

export declare enum CertificateIdentifier {
    RDS_CA_2019 = 'rds-ca-2019',
    RDS_CA_ECC384_G1 = 'rds-ca-ecc384-g1',
    RDS_CA_RDS2048_G1 = 'rds-ca-rsa2048-g1',
    RDS_CA_RDS4096_G1 = 'rds-ca-rsa4096-g1',
}

export declare class CaCertificate {
    static of(identifier: CertificateIdentifier): CaCertificate {
        return new CaCertificate(identifier);
    }

    constructor(private readonly identifier: string) {};

    toString(): string {
        return this.identifier;
    }
}

mergify · 2023-08-28T17:50:23Z

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

mrgrain · 2023-08-28T18:09:32Z

static .of() usually refers to a known given, like an enum. Why would the constructor be private and then publish the of( method to be a constructor wrapper, it's a bit inverse right now. If you want a custom CA i'd think: new PrivateCa('custom-ca')

I think I'd still prefer to go with the enums for consistency. Feels more light weight than having the instances be instantiated in properties of a static class.

But using it would still be typed to CaCertificate, so as a user you will have to do:

new CaCertificate(CertificateIdentifier.RDS_CA_2019)

and instantiate an object anyway. I don't think the practical memory savings from not having all variations pre-instantiated are worth the worse UX.

static of(identifier: string) is the pattern we use right now everywhere else.

mergify · 2023-08-28T18:16:11Z

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

mrgrain · 2023-08-28T18:21:13Z

@Mergifyio requeue

mergify · 2023-08-28T18:21:17Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

aws-cdk-automation · 2023-08-28T18:57:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 6e1f71e
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-08-28T18:57:28Z

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

baguilarq · 2023-09-04T17:55:15Z

do we need another implementation to support this for DatabaseCluster, isn't it?

feat(rds): support certificate autority

8c0d287

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Aug 25, 2023

aws-cdk-automation requested a review from a team August 25, 2023 08:39

github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Aug 25, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 25, 2023

mrgrain previously requested changes Aug 25, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 25, 2023

changed names and CaCertificate structure

e022856

fix integration tests

1ac08d2

lpizzinidev force-pushed the gh-26865 branch from 2e7db34 to 1ac08d2 Compare August 26, 2023 13:06

lpizzinidev added 2 commits August 26, 2023 16:08

Merge branch 'main' into awsgh-26865

cd60da7

fixed README

2e6328e

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 26, 2023

robertd reviewed Aug 26, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 26, 2023

updated CaCertificate implementation

7002d6b

robertd reviewed Aug 27, 2023

View reviewed changes

packages/aws-cdk-lib/aws-rds/lib/instance.ts Show resolved Hide resolved

updated jsdoc reference

45a169e

mrgrain previously requested changes Aug 28, 2023

View reviewed changes

fixed caCertificate default and added toString class method

290d36e

mrgrain changed the title ~~feat(rds): support certificate autority~~ feat(rds): support certificate autority certificate Aug 28, 2023

mrgrain previously approved these changes Aug 28, 2023

View reviewed changes

robertd approved these changes Aug 28, 2023

View reviewed changes

Merge branch 'main' into awsgh-26865

3275317

mrgrain approved these changes Aug 28, 2023

View reviewed changes

Merge branch 'main' into awsgh-26865

8dcc5e4

mrgrain added feature-request A feature should be added or improved. and removed feature-request A feature should be added or improved. labels Aug 28, 2023

Merge branch 'main' into awsgh-26865

6e1f71e

mergify bot merged commit 4fd510e into aws:main Aug 28, 2023
9 checks passed

github-actions bot mentioned this pull request Sep 1, 2023

Monthly PRs metrics report - Aug 2023 #26965

Closed

github-actions bot added effort/small Small work item – less than a day of effort p1 and removed effort/medium Medium work item – several days of effort p2 labels Sep 14, 2023

badmintoncryer mentioned this pull request Jan 27, 2024

feat(cognito): specifying the trigger event version in UserPool.addTrigger() #28899

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): support certificate autority certificate #26883

feat(rds): support certificate autority certificate #26883

lpizzinidev commented Aug 25, 2023 •

edited

Loading

mrgrain left a comment

mrgrain Aug 25, 2023

mrgrain Aug 25, 2023

mrgrain Aug 25, 2023 •

edited

Loading

robertd Aug 25, 2023

robertd commented Aug 26, 2023 •

edited

Loading

robertd Aug 25, 2023

mrgrain commented Aug 27, 2023 •

edited

Loading

robertd commented Aug 27, 2023 •

edited

Loading

robertd commented Aug 27, 2023

lpizzinidev commented Aug 28, 2023

mrgrain left a comment

mrgrain Aug 28, 2023

lpizzinidev Aug 28, 2023

mrgrain Aug 28, 2023 •

edited

Loading

robertd Aug 28, 2023

robertd Aug 28, 2023

mrgrain Aug 28, 2023

mergify bot commented Aug 28, 2023

robertd left a comment •

edited

Loading

StefanPrintezis commented Aug 28, 2023 •

edited

Loading

mergify bot commented Aug 28, 2023

mrgrain commented Aug 28, 2023 •

edited

Loading

mergify bot commented Aug 28, 2023

mrgrain commented Aug 28, 2023

mergify bot commented Aug 28, 2023

aws-cdk-automation commented Aug 28, 2023

mergify bot commented Aug 28, 2023

baguilarq commented Sep 4, 2023

	readonly caCertificateIdentifier?: CertificateIdentifier;
	readonly caCertificate?: CertificateIdentifier;

	export enum CertificateIdentifier {
	export enum CaCertificate {

	* @default - CaCertificate.RDS_CA_2019
	* @default - RDS will choose a certificate authority

	caCertificateIdentifier: props.caCertificate ? props.caCertificate.identifier : CaCertificate.RDS_CA_2019.identifier,
	caCertificateIdentifier: props.caCertificate ? props.caCertificate.identifier : undefined,

feat(rds): support certificate autority certificate #26883

feat(rds): support certificate autority certificate #26883

Conversation

lpizzinidev commented Aug 25, 2023 • edited Loading

mrgrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertd commented Aug 26, 2023 • edited Loading

Choose a reason for hiding this comment

mrgrain commented Aug 27, 2023 • edited Loading

robertd commented Aug 27, 2023 • edited Loading

robertd commented Aug 27, 2023

lpizzinidev commented Aug 28, 2023

mrgrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 28, 2023

robertd left a comment • edited Loading

Choose a reason for hiding this comment

StefanPrintezis commented Aug 28, 2023 • edited Loading

mergify bot commented Aug 28, 2023

mrgrain commented Aug 28, 2023 • edited Loading

mergify bot commented Aug 28, 2023

mrgrain commented Aug 28, 2023

mergify bot commented Aug 28, 2023

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

aws-cdk-automation commented Aug 28, 2023

AWS CodeBuild CI Report

mergify bot commented Aug 28, 2023

baguilarq commented Sep 4, 2023

lpizzinidev commented Aug 25, 2023 •

edited

Loading

mrgrain Aug 25, 2023 •

edited

Loading

robertd commented Aug 26, 2023 •

edited

Loading

mrgrain commented Aug 27, 2023 •

edited

Loading

robertd commented Aug 27, 2023 •

edited

Loading

mrgrain Aug 28, 2023 •

edited

Loading

robertd left a comment •

edited

Loading

StefanPrintezis commented Aug 28, 2023 •

edited

Loading

mrgrain commented Aug 28, 2023 •

edited

Loading