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

(aws_docdb): addRotationSingleUser() not consistent with aws_rds.Cluster.addRotationSingleUser() #17347

Closed
ahammond opened this issue Nov 5, 2021 · 9 comments · Fixed by #17609
Labels
@aws-cdk/aws-docdb Related to Amazon DocumentDB effort/small Small work item – less than a day of effort feature/coverage-gap Gaps in CloudFormation coverage by L2 constructs feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1

Comments

@ahammond
Copy link
Contributor

ahammond commented Nov 5, 2021

What is the problem?

https://docs.aws.amazon.com/cdk/api/latest/docs/aws-rds-readme.html#rotating-credentials describes the aws_rds.Cluster.addRotationSinglerUser(), as implemented by aws_docdb there is no support for excludeCharacters. And sure enough, I went to update a stack containing a

const cluster = aws_docdb.DatabaseCluster(...);
cluster.addRotationSingleUser();

and got

The parameter MasterUserPassword is not a valid password. Only printable ASCII characters besides '/', '@', '"', ' ' may be used. (Service: AmazonRDS; Status Code:
400; Error Code: InvalidParameterValue; Request ID: 38bc6735-286e-41f2-89d4-3d2c8cb78ef9; Proxy: null)

Reproduction Steps

I'm not sure how to reliably repro this one. The problem, I think, is that the secret rotator generates an illegal password. The deeper problem is that the secret rotator doesn't follow the good example in aws_rds and allow for configurable excludeCharacters, so there's no simple way to fix this. This might be the root cause of #17288 (I'm imagining if the rotator generates an illegal password and DocDB refuses to accept it).

What did you expect to happen?

I expect to never have to think about the (unfortunately named) masterUser's password. The rotation should Just Work.

What actually happened?

First try: (pulled from the CloudFormation console)

The parameter MasterUserPassword is not a valid password. Only printable ASCII characters besides '/', '@', '"', ' ' may be used. (Service: AmazonRDS; Status Code: 400; Error Code: InvalidParameterValue; Request ID: 38bc6735-286e-41f2-89d4-3d2c8cb78ef9; Proxy: null)

Second try:

❯ npx cdk deploy --require-approval never Whiteboard
Deploying to "euDev" in "eu-central-1".
defaultRegion: "eu-central-1"
Whiteboard: deploying...
[0%] start: Publishing 5cc75ebf91260fff71aff7bfd82508a3d015ff54fefac979088541de57bb66ad:414375883647-eu-central-1
[100%] success: Published 5cc75ebf91260fff71aff7bfd82508a3d015ff54fefac979088541de57bb66ad:414375883647-eu-central-1
Whiteboard: creating CloudFormation changeset...

 ❌  Whiteboard failed: Error [ValidationError]: Stack:arn:aws:cloudformation:eu-central-1:414375883647:stack/Whiteboard/ee1bf3f0-3d9a-11ec-9fae-021733604472 is in UPDATE_ROLLBACK_FAILED state and can not be updated.
    at Request.extractError (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/protocol/query.js:50:29)
    at Request.callListeners (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/sequential_executor.js:106:20)
    at Request.emit (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/sequential_executor.js:78:10)
    at Request.emit (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/request.js:686:14)
    at Request.transition (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/request.js:22:10)
    at AcceptorStateMachine.runTo (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/state_machine.js:14:12)
    at /Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/state_machine.js:26:10
    at Request.<anonymous> (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/request.js:38:9)
    at Request.<anonymous> (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/request.js:688:12)
    at Request.callListeners (/Users/ahammond/Documents/ClickUp/whiteboard-cdk/node_modules/aws-sdk/lib/sequential_executor.js:116:18) {
  code: 'ValidationError',
  time: 2021-11-05T01:17:48.814Z,
  requestId: '8ec0b437-4a02-41df-8637-5e698c28e3b5',
  statusCode: 400,
  retryable: false,
  retryDelay: 30.455530850638013
}
Stack:arn:aws:cloudformation:eu-central-1:414375883647:stack/Whiteboard/ee1bf3f0-3d9a-11ec-9fae-021733604472 is in UPDATE_ROLLBACK_FAILED state and can not be updated.

CDK CLI Version

2.0.0-rc.27 (build 435e6f6)

Framework Version

2.0.0-rc.27 (build 435e6f6)

Node.js Version

v14.17.5

OS

MacOS

Language

Typescript

Language Version

4.4.4

Other information

No response

@ahammond ahammond added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 5, 2021
@github-actions github-actions bot added the @aws-cdk/aws-docdb Related to Amazon DocumentDB label Nov 5, 2021
@skinny85
Copy link
Contributor

skinny85 commented Nov 5, 2021

Hey @ahammond,

this was literally just added 4 days ago, in #17262.

Thanks,
Adam

@skinny85 skinny85 added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. guidance Question that needs advice or information. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 5, 2021
@skinny85 skinny85 removed their assignment Nov 5, 2021
@github-actions
Copy link

github-actions bot commented Nov 7, 2021

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 7, 2021
@ahammond
Copy link
Contributor Author

ahammond commented Nov 8, 2021

@skinny85 dude, fixed it before I even reported it? That's pretty next level. Thanks!!!

@ahammond ahammond closed this as completed Nov 8, 2021
@github-actions
Copy link

github-actions bot commented Nov 8, 2021

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@h1008
Copy link

h1008 commented Nov 19, 2021

@skinny85 I think this issue is not fixed yet. The merge request you linked provides the ability to specify the excluded chars for the secret itself but not for the secret rotation lambda function.
In rds you can pass the exclusion characters to the function addRotationSingleUser (see https://docs.aws.amazon.com/cdk/api/latest/docs/aws-rds-readme.html#rotating-credentials) and also to addRotationMultiUser. But that's not possible with docdb. If you have a look at the docdb rotation lambda in the console, you'll see that the environment variable EXCLUDE_CHARACTERS is always empty and the lambda will generate invalid passwords.

@skinny85
Copy link
Contributor

Hmm, you're right @h1008! Let me re-open this issue.

Would you consider picking this one up where the previous contributor left off in #17262? Our "Contributing" guide: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md.

Thanks,
Adam

@skinny85 skinny85 reopened this Nov 19, 2021
@skinny85 skinny85 added effort/small Small work item – less than a day of effort feature/coverage-gap Gaps in CloudFormation coverage by L2 constructs p1 feature-request A feature should be added or improved. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Nov 19, 2021
@h1008
Copy link

h1008 commented Nov 21, 2021

Thanks, I'll have a look if I can prepare a PR.

@h1008
Copy link

h1008 commented Nov 22, 2021

@skinny85 my PR is ready. I think this should fix it!

@skinny85 skinny85 added the in-progress This issue is being actively worked on. label Nov 22, 2021
@mergify mergify bot closed this as completed in #17609 Nov 25, 2021
mergify bot pushed a commit that referenced this issue Nov 25, 2021
…17609)

We need to pass whatever `excludeCharacters` were passed to the generated Secret to the application responsible for the rotation.

Fixes #17347
Fixes #17575 

------

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

beezly pushed a commit to beezly/aws-cdk that referenced this issue Nov 29, 2021
…ws#17609)

We need to pass whatever `excludeCharacters` were passed to the generated Secret to the application responsible for the rotation.

Fixes aws#17347
Fixes aws#17575 

------

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…ws#17609)

We need to pass whatever `excludeCharacters` were passed to the generated Secret to the application responsible for the rotation.

Fixes aws#17347
Fixes aws#17575 

------

*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-docdb Related to Amazon DocumentDB effort/small Small work item – less than a day of effort feature/coverage-gap Gaps in CloudFormation coverage by L2 constructs feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1
Projects
None yet
3 participants