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

fix(secretsmanager): allow templated string creation #2010

Merged
merged 7 commits into from
Mar 26, 2019

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Mar 13, 2019

The interface TemplatedSecretStringGenerator was exported but not
used in the generateSecretString prop.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

The interface `TemplatedSecretStringGenerator` was exported but not
used in the `generateSecretString` prop.
@jogold jogold requested a review from a team as a code owner March 13, 2019 22:19
@@ -75,7 +75,7 @@ export interface SecretProps {
* @default 32 characters with upper-case letters, lower-case letters, punctuation and numbers (at least one from each
* category), per the default values of ``SecretStringGenerator``.
*/
generateSecretString?: SecretStringGenerator;
generateSecretString?: SecretStringGenerator | TemplatedSecretStringGenerator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unions do not translate very nicely across programming languages since some languages don't support them, so we forbid unions at the L2 layer. The solution is usually to define an abstract class with a bunch of static factory methods for the various options.

Copy link
Contributor Author

@jogold jogold Mar 14, 2019

Choose a reason for hiding this comment

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

An easy solution here could be to integrate generateStringKey and secretStringTemplate in the SecretStringGenerator interface (which is a prop of SecretProps) and add validation (if one is specified the other must also be specified), what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

But... TemplatedSecretStringGenerator extends SecretStringGenerator, so you are able to pass an instance of that in and it'll be used just as you'd expect... The signature already enables that. I'll admit that discoverability isn't all so great, but I don't see why altering the signature is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work:

new secretsmanager.Secret(this, 'Secret', {
      generateSecretString: {
        secretStringTemplate: JSON.stringify({ username: 'username' }),
        generateStringKey: 'password',
        excludeCharacters: '"@/\\'
      }
    })
  Types of property 'generateSecretString' are incompatible.
    Type '{ passwordLength: number; secretStringTemplate: string; generateStringKey: string; excludeCharacters: string; }' is not assignable to type 'SecretStringGenerator'.
      Object literal may only specify known properties, and 'secretStringTemplate' does not exist in type 'SecretStringGenerator'.

You mean like this?

new secretsmanager.Secret(this, 'Resource', {
      generateSecretString: ({
        secretStringTemplate: JSON.stringify({ username: 'username' }),
        generateStringKey: 'password',
        excludeCharacters: '"@/\\'
      }) as secretsmanager.TemplatedSecretStringGenerator
    });

Copy link
Contributor

Choose a reason for hiding this comment

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

@RomainMuller it's not a comfortable interface for value-typed interfaces. TSC is not going to search the whole type space for a type that matches your literal object AND happens to be assignable to a SecretStringGenerator.

It's actually good that it doesn't because I'm not sure the semantics would be preserved when using it via JSII.

We can either make 2 arguments at the top-level:

secretStringGenerator?: SecretStringGenerator;
templatedSecretStringGenerator?: TemplatedSecretStringGenerator;

OR

Fold the 2 structures together and do some run-time analysis

OR

Define an interface and 2 classes and some factory functions.

Preferences?

Copy link
Contributor

Choose a reason for hiding this comment

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

If nobody expresses a preference by Monday I'm going to pick something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fold the 2 structures together and do some run-time analysis

I have a preference for this solution...

@@ -81,7 +81,7 @@ export interface SecretProps {
* @default 32 characters with upper-case letters, lower-case letters, punctuation and numbers (at least one from each
* category), per the default values of ``SecretStringGenerator``.
*/
generateSecretString?: SecretStringGenerator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why rid of the semicolon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants