Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rds): support certificate autority certificate #26883

Merged
merged 11 commits into from
Aug 28, 2023

Conversation

lpizzinidev
Copy link
Contributor

@lpizzinidev lpizzinidev commented Aug 25, 2023

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

@github-actions 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 aws-cdk-automation requested a review from a team August 25, 2023 08:39
@github-actions github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Aug 25, 2023
@aws-cdk-automation 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
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @default - rds-ca-2019
*/
readonly caCertificateIdentifier?: CertificateIdentifier;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* The CA identifier of the DB instance
*/
export enum CertificateIdentifier {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export enum CertificateIdentifier {
export enum CaCertificate {

/**
* The CA identifier of the DB instance
*/
export enum CertificateIdentifier {
Copy link
Contributor

@mrgrain mrgrain Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@aws-cdk-automation 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
@mergify mergify bot dismissed mrgrain’s stale review August 25, 2023 14:57

Pull request has been modified.

@aws-cdk-automation 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
Copy link
Contributor

robertd commented Aug 26, 2023

@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 Show resolved Hide resolved
/**
* The CA identifier of the DB instance
*/
export enum CertificateIdentifier {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@aws-cdk-automation 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
@mrgrain
Copy link
Contributor

mrgrain commented Aug 27, 2023

@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
Copy link
Contributor

robertd commented Aug 27, 2023

@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
Copy link
Contributor

robertd commented Aug 27, 2023

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

@lpizzinidev
Copy link
Contributor Author

@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 👍

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last question regarding the default.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2023-08-28 at 15 53 43

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

Copy link
Contributor

@mrgrain mrgrain Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mergify mergify bot dismissed mrgrain’s stale review August 28, 2023 14:12

Pull request has been modified.

@mrgrain mrgrain changed the title feat(rds): support certificate autority feat(rds): support certificate autority certificate Aug 28, 2023
mrgrain
mrgrain previously approved these changes Aug 28, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2023

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

Copy link
Contributor

@robertd robertd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Many thanks @lpizzinidev 🚀

@mergify mergify bot dismissed mrgrain’s stale review August 28, 2023 16:18

Pull request has been modified.

@StefanPrintezis
Copy link

StefanPrintezis commented Aug 28, 2023

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
Copy link
Contributor

mergify bot commented Aug 28, 2023

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 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
@mrgrain
Copy link
Contributor

mrgrain commented Aug 28, 2023

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
Copy link
Contributor

mergify bot commented Aug 28, 2023

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
Copy link
Contributor

mrgrain commented Aug 28, 2023

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2023

requeue

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

@aws-cdk-automation
Copy link
Collaborator

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 mergify bot merged commit 4fd510e into aws:main Aug 28, 2023
8 checks passed
@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2023

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
Copy link

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

@github-actions 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(RDS): Support RDS certificate authority
6 participants