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): make rds secret name configurable #13626

Merged
merged 13 commits into from Mar 19, 2021

Conversation

hedrall
Copy link
Contributor

@hedrall hedrall commented Mar 16, 2021

close #8984

As suggested by @njlynch in the first issue, I added the ability to set the secret name that RDS generates.


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

@gitpod-io
Copy link

gitpod-io bot commented Mar 16, 2021

@github-actions github-actions bot added the @aws-cdk/aws-rds Related to Amazon Relational Database label Mar 16, 2021
@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@hedrall hedrall changed the title [rds]: make rds secret name configurable feat(rds): make rds secret name configurable Mar 16, 2021
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @hedrall ! Despite being a small change, this is actually a really subtle PR. I've gone through it, and I think I understood everything finally (it definitely took me a while!).

I left some questions - can you confirm my understanding of the code is the same as yours?

Thanks,
Adam

Comment on lines 20 to 21
* For "owned" secrets, this will be the full resource name (secret name + suffix), unless the
* '@aws-cdk/aws-secetsmanager:parseOwnedSecretName' feature flag is set.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to say, I find this documentation super confusing 🤨.

This is an input property, right? So either you provide the name of the Secret, or it will be generated by CloudFormation.

What does this paragraph mean?

Copy link
Contributor Author

@hedrall hedrall Mar 18, 2021

Choose a reason for hiding this comment

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

I'm sorry. I did not consider this point very deeply, and used the comments for the properties of Secret itself.

https://github.com/hedrall/aws-cdk/blob/03fba9619d7f3ad4f5a27d8123ff826a0ed26bce/packages/%40aws-cdk/aws-secretsmanager/lib/secret.ts#L33-L39

I would like to look into this more, including how it relates to the Feature flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.

A name for the secret. Note that deleting secrets from SecretsManager does not happen immediately, but after a 7 to * 30 days blackout period. During that period, it is not possible to create another secret that shares the same name.

However, I just noticed that this seems to be an old specification.
I tried destroying the deployed secret and renaming the deployed secret, but in both cases the secret was deleted immediately.
I think that cloud formation use a flag like as cli 'force-delete-without-recovery’.

So, I change comment simply.
( Should I also change the comment of SecretProps? )

@@ -221,6 +228,7 @@ export abstract class Credentials {
return {
username: username ?? secret.secretValueFromJson('username').toString(),
password: secret.secretValueFromJson('password'),
secretName: secret.secretName,
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 99% sure this line is not actually needed.

@hedrall do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe Yes. I thought it may need to initialize and expose as a field like ‘encryptionKey’ below, so I wrote this line.

https://github.com/hedrall/aws-cdk/blob/7e5ee80ea28e4e8d123a7862f887f0f727fc011b/packages/%40aws-cdk/aws-rds/lib/props.ts#L227-L235

I not sure of any use case that uses the secretName of the object of Credentials created by fromSecret, so I will delete it now.

packages/@aws-cdk/aws-rds/lib/props.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/test/cluster.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/test/cluster.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/test/instance.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/test/instance.test.ts Outdated Show resolved Hide resolved
@skinny85
Copy link
Contributor

BTW, the build is failing because of

@aws-cdk/aws-rds: /codebuild/output/src361629757/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-rds/test/cluster.test.ts
@aws-cdk/aws-rds:   13:127  error  Missing trailing comma  comma-dangle
@aws-cdk/aws-rds: ✖ 1 problem (1 error, 0 warnings)

hedrall and others added 4 commits March 18, 2021 09:47
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@mergify mergify bot dismissed skinny85’s stale review March 18, 2021 00:47

Pull request has been modified.

hedrall and others added 2 commits March 18, 2021 09:55
hedrall and others added 4 commits March 19, 2021 06:36
Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
@hedrall
Copy link
Contributor Author

hedrall commented Mar 18, 2021

Thank you for all your suggestions. I’d fixed.
We hope you can confirm this again.

@hedrall hedrall requested a review from skinny85 March 18, 2021 22:45
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @hedrall, thanks so much for the contribution!

@mergify
Copy link
Contributor

mergify bot commented Mar 19, 2021

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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: ca2cc88
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Mar 19, 2021

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

eladb pushed a commit that referenced this pull request Mar 24, 2021
close #8984 

As suggested by @njlynch in the first issue, I added the ability to set the secret name that RDS generates.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
close aws#8984 

As suggested by @njlynch in the first issue, I added the ability to set the secret name that RDS generates.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rds] Ability to choose name for database secrets
5 participants