Skip to content

Log DB Rotations #31402

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Aug 8, 2025
Merged

Log DB Rotations #31402

merged 13 commits into from
Aug 8, 2025
+17 −5

Conversation

kpcraig
Copy link
Contributor

@kpcraig kpcraig commented Jul 31, 2025 •
edited
Loading

Small update to log DB credential rotations at the caller level with a general Error/Info on failure/success.

Here's some sample logs for
Manually triggered root rotation:

2025-08-05T12:24:01.731-0400 [INFO]  secrets.database.database_9394a722: succesfully rotated root credential on user request: path=rotate-root/psql

failure:

2025-08-05T12:37:27.464-0400 [ERROR] secrets.database.database_9394a722: failed to rotate root credential on user request: path=rotate-root/psql
  err=
  | failed to update user: 1 error occurred:
  | \t* user does not appear to exist: failed to connect to `host=localhost user=alice database=`: dial error (dial tcp 127.0.0.1:5432: connect: connection refused)
  |

Manually triggered static rotation:

2025-08-05T12:25:33.643-0400 [INFO]  secrets.database.database_9394a722: rotated credential in rotate-role on user request: path=rotate-role/my-role TTL=60

failure:

2025-08-05T12:37:02.871-0400 [ERROR] secrets.database.database_9394a722: unable to rotate credentials in rotate-role on user request: path=rotate-role/my-role
  err=
  | error setting credentials: 1 error occurred:
  | \t* user does not appear to exist: failed to connect to `host=localhost user=alice database=`: dial error (dial tcp 127.0.0.1:5432: connect: connection refused)
  |

Static role periodic rotation:
(success)

2025-08-05T12:18:57.712-0400 [INFO]  secrets.database.database_9394a722: succesfully rotated static role: name=my-role ttl=60

(failure)

2025-08-05T12:20:57.705-0400 [ERROR] secrets.database.database_9394a722: unable to rotate credentials in periodic function: database=psql role=my-role name=my-role
  err=
  | error setting credentials: 1 error occurred:
  | \t* user does not appear to exist: failed to connect to `host=localhost user=alice database=`: dial error (dial tcp 127.0.0.1:5432: connect: connection refused)
  |

@kpcraig kpcraig requested a review from a team as a code owner July 31, 2025 18:25
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jul 31, 2025
@kpcraig kpcraig requested a review from a team as a code owner July 31, 2025 18:27
@kpcraig kpcraig requested a review from ldilalla-HC July 31, 2025 18:27
@vercel Vercel
Copy link

vercel bot commented Jul 31, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
vault-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2025 2:33pm

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 31, 2025 •
edited
Loading

CI Results:
All Go tests succeeded! ✅

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 31, 2025 •
edited
Loading

Build Results:
All builds succeeded! ✅

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 31, 2025
View reviewed changes
robmonte
robmonte reviewed Aug 1, 2025
View reviewed changes
fairclothjm
fairclothjm requested changes Aug 4, 2025
View reviewed changes
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

Can we see the before and after log snippets for the relevant plugins?

builtin/logical/database/path_rotate_credentials.go Outdated
return b.rotateRootCredentials(ctx, req, name)
resp, err = b.rotateRootCredentials(ctx, req, name)
if err != nil {
b.Logger().Error("failed to rotate root credential", "path", req.Path, "err", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be consistent and either use err or error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i decided to use "err" since it's generally more common for use to use it, particularly in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

per discussion, i updated this to error.

builtin/logical/database/path_rotate_credentials.go Outdated
@@ -236,7 +242,7 @@ func (b *databaseBackend) pathRotateRoleCredentialsUpdate() framework.OperationF
// this item back on the queue. The err should still be returned at the end
// of this method.
if err != nil {
b.logger.Warn("unable to rotate credentials in rotate-role", "error", err)
b.logger.Error("unable to rotate credentials in rotate-role", "rotationID", name, "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is name actually the rotationID? Should we use req.RotationID instead?

Copy link
Contributor Author

@kpcraig kpcraig Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So i was torn about using rotationID, since I don't think we actually have a rotationID if the request didn't come from the rotation manager.

I used rotationID here because the values will end up being the same or similar since they come from the same place, but it might be more accurate to use "path" or "name" instead, with the understanding that the log will become "rotationID" when the logging comes from the rotation manager in the future.

builtin/logical/database/path_rotate_credentials.go Outdated
item.Priority = role.StaticAccount.NextRotationTimeFromInput(resp.RotationTime).Unix()
nextRotationTime := role.StaticAccount.NextRotationTimeFromInput(resp.RotationTime).Unix()
ttl := nextRotationTime - time.Now().Unix()
b.Logger().Info("rotated credential in rotate-role", "rotationID", name, "TTL", ttl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is name actually the rotationID? Should we use req.RotationID instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i was torn about using rotationID, since I don't think we actually have a rotationID if the request didn't come from the rotation manager.

I used rotationID here because the values will end up being the same or similar since they come from the same place, but it might be more accurate to use "path" or "name" instead, with the understanding that the log will become "rotationID" when the logging comes from the rotation manager in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've swapped this text to "path" when we have access to that, "name" otherwise

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 4, 2025
View reviewed changes
fairclothjm
fairclothjm reviewed Aug 4, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 5, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 5, 2025
View reviewed changes
@kpcraig
Copy link
Contributor Author

kpcraig commented Aug 7, 2025

Switching the "err" statement to just use the string, i.e., err.Error() seems to have eliminated the static analysis tool's concern about data flow.

fairclothjm
fairclothjm approved these changes Aug 8, 2025
View reviewed changes
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

LGTM

@fairclothjm
Copy link
Contributor

Thanks for the log snippets! Could we see some for auto-rotations on root creds? Ideally, including all rotation manager logs too.

@kpcraig
Copy link
Contributor Author

kpcraig commented Aug 8, 2025

Sure, enterprise rotations (i.e., rotation manager rotations) are at https://github.com/hashicorp/vault-enterprise/pull/8412, but here's an example log (using my testing plugin):

rotation-job-manager: successfully rotated job: rotationID=testing/config time=2025-07-03T17:28:41-04:00

or in a failure

rotation-job-manager: rotation failed, attempting to re-queue: rotationID=testing/config error=didn't work retry_count=1

@fairclothjm
Copy link
Contributor

Thanks! I think that confirms that rotationID and req.Path can't be used interchangeably in all situations. LGTM

@kpcraig kpcraig merged commit 1fafe2f into main Aug 8, 2025
93 checks passed
@kpcraig kpcraig deleted the VAULT-38205/log-db-rotations branch August 8, 2025 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fairclothjm fairclothjm fairclothjm approved these changes

@robmonte robmonte robmonte approved these changes

@ldilalla-HC ldilalla-HC Awaiting requested review from ldilalla-HC ldilalla-HC is a code owner automatically assigned from hashicorp/vault

Assignees
No one assigned
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kpcraig @fairclothjm @robmonte