feat(rds): support setting database master users from existing secrets

[njlynch]

See #7927 (comment) for motivation and design.

The current way of specifying master user logins for DatabaseInstance and
DatabaseCluster is inconsistent between the two and introduces some awkward
usage when creating a login from an existing Secret.

This change converts the existing Login interface (used by the DatabaseCluster)
into a class with factory methods for username/password or secret-based logins.
This also then re-uses that same interface for DatabaseInstance.

The one exception now will be DatabaseInstanceFromSnapshot, which has specific
requirements that deserved its own interface (SnapshotLogin).

As a side effect of this approach, existing DatabaseCluster users -- in
Typescript at least -- will not be broken. For example, the following are
equivalent:

new rds.DatabaseCluster(this, 'Cluster1', {
  // Existing usage
  masterUser: {
    username: 'admin',
  },
  // New usage
  masterUser: Login.fromUsername('admin'),
});

Lastly, this change makes the whole masterUser prop optional, as there's no good reason why we can't default a username.

fixes #7927

BREAKING CHANGE: DatabaseInstanceProps and DatabaseInstanceFromSnapshotProps -
masterUsername, masterUserPassword and masterUserPasswordEncryptionKey moved
to credentials as a new Credentials class.

• rds: Login renamed to Credentials. Use Credentials.fromUsername to replace existing usage.
• rds: DatabaseClusterProps masterUser renamed to credentials.

---

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

[skinny85]

Some initial comments.

I'm wondering what's the correct abstraction for Login. The edge case I'm thinking is when a customer needs to provide a custom implementation of Login, because ours doesn't cater to some edge-case. I guess with an abstract class, it might be tricky to subclass it from other languages, while a struct (+ class with static methods) should be easy... something to think about perhaps?

[njlynch]

I'm wondering what's the correct abstraction for Login.

I was wondering about this a bit myself. As-is, it's interesting that changing Login from an interface to abstract class was effectively a no-op for clusters (in Typescript). I like that the factory methods mean that you know that the combination of properties is always valid, but it's so easy to just anonymously subclass that there's nothing stopping anyone from continuing to use Login as it was already being used before (at least in some languages).

I guess with an abstract class, it might be tricky to subclass it from other languages

Why would it be tricky to subclass it?

Login login = new Login() {
  public String getUsername { return "Adam"; }
  public SecretValue getPassword { return myPassword; }
  // ...
}

... while a struct (+ class with static methods) should be easy.

Are you arguing for a LoginFactory? I don't know that I can bring myself to do that.

[skinny85]

I guess with an abstract class, it might be tricky to subclass it from other languages

Why would it be tricky to subclass it?

Login login = new Login() {
  public String getUsername { return "Adam"; }
  public SecretValue getPassword { return myPassword; }
  // ...
}

I actually wasn't sure that even worked in Java (because of JSII), but I tried it, and it indeed does work.

... while a struct (+ class with static methods) should be easy.

Are you arguing for a LoginFactory? I don't know that I can bring myself to do that.

The name could be cooler than that 🙂 For example, Logins sounds good to me.

I'm just saying that if Login was a struct, you could provide one like so:

  Login login = Login.builder()
    .username(myUsername)
    .password(myPassword)
    .build();

Which I think is much more idiomatic in Java than an anonymous class.

But if you really really hate the idea of having 2 classes for Login, I won't block the PR on it. Just something to think about 🙂.

[jogold]

I also prefer a concrete class here... with the static methods returning a new Login()... there's something similar here

aws-cdk/packages/@aws-cdk/aws-amplify/lib/basic-auth.ts

Lines 50 to 53 in 94bbabf

	/**
	* Basic Auth configuration
	*/
	export class BasicAuth {

I also wonder if MasterCredentials or just Credentials is a better name...

[njlynch]

I also prefer a concrete class here...

Out of curiosity, why? Just stylistic preference, or is there something else I'm missing?

I'm not strongly opposed; however, it's worth noting that changing Login from an interface (today) to a concrete class will be a breaking change for all active Cluster users, meaning this PR will break all current RDS users (both Instance and Cluster setups). As an abstract class, that's not the case. If we're going to break, it does need to be now, but I'm not sure the minor aesthetics of the class being concrete are worth it.

[jogold]

If you go from this

interface Login {
  readonly username: string;
  readonly password?: string;
  readonly encryptionKey?: kms.IKey;
}

to this:

class Login {
  public static fromUsername(username: string, options: FromUsernameOptions = {}): Login {
    return new Login(username, options.password, options.encryptionKey);
  }

  public static fromSecret(secret: secretsmanager.Secret): Login {
    return new Login(
      secret.secretValueFromJson('username').toString(),
      secret.secretValueFromJson('username'),
      secret.encryptionKey,
      secret,
    );
  }

  public readonly username: string;
  public readonly password?: SecretValue;
  public readonly encryptionKey?: kms.IKey;
  public readonly secret?: secretsmanager.Secret;

  constructor(username: string, password?: SecretValue, encryptionKey?: kms.IKey, secret?: secretsmanager.Secret) {
    this.username = username;
    this.password = password;
    this.encryptionKey = encryptionKey;
    this.secret = secret;
  }
}

it is not breaking, at least in TS, no?

[njlynch]

Ah, you're right. I had done a slightly different implementation (getters) that didn't work; this approach is fine (at least in Typescript).

I'm still curious if the perceived advantage of this approach is stylistic, or if there's a benefit I'm missing.

[skinny85]

+1 for credentials or masterCredentials as the name of the property, that's a great suggestion @jogold !

[njlynch]

... while a struct (+ class with static methods) should be easy.

I'm just saying that if Login was a struct, you could provide one like so:

  Login login = Login.builder()
    .username(myUsername)
    .password(myPassword)
    .build();

Which I think is much more idiomatic in Java than an anonymous class.

Sure, but you only need to subclass (anonymously or otherwise) if you want to do something not supported by the factory methods; the factory methods support everything possible with the constructs, so I'm not sure what use case you're trying to optimize for here, and what the above offers that the below doesn't.

Login login = Login.fromUsername(myUsername, myPassword);

If we want to optimize for the builder pattern, we could have chosen Option 2 from #7927 (comment), and skipped most of the breaking changes.

[skinny85]

Looks good to me!

One thing I would change is make the decision about concrete class vs. abstract once, and use it consistently between Login and SnapshotLogin - right now, Login is concrete, while SnapshotLogin is abstract.

I actually prefer the "struct + static factory" approach more than the concrete class (I don't like the 4 positional arguments in the constructor of the concrete class, and how it will translate to other languages; and if you make the constructor private, you're actually completely blocking the "escape hatch" of subclassing the class), so if the choice is between "concrete class" vs. "abstract class", I would go with "abstract class" here, but I'm deferring that decision to you, Nick 🙂.

[mergify]

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]

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]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 4ec1c28
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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