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

VAULT-13061: Fix mount path discrepancy in activity log #18916

Merged
merged 3 commits into from
Feb 6, 2023

Conversation

miagilepner
Copy link
Contributor

@miagilepner miagilepner commented Jan 31, 2023

There are two problems this PR fixes:

  1. processClientRecord creates maps that key by mount accessor, but we later assume that the key is a mount path in transformActivityLogMounts
  2. Sometimes a deleted/empty mount record is thrown out (precomputedQueryWorker), while other times we keep it (transformMonthBreakdowns ). This change means that we will always keep the mount record.

Closes #18814, #18812

@miagilepner miagilepner added this to the 1.13.0-rc1 milestone Jan 31, 2023
valResp := a.core.router.ValidateMountByAccessor(mountAccessor)
if valResp == nil {
// Only persist valid mounts
continue
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 want to verify that this is the correct behavior - it seems wrong to me that these currently get thrown out, but I definitely might be missing 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.

and to clarify, deleted mounts were being excluded from the per-mount breakdown of counts. They were still present in the top-level totals.

@miagilepner miagilepner marked this pull request as ready for review February 1, 2023 14:49
Copy link
Contributor

@mpalmi mpalmi left a comment

Choose a reason for hiding this comment

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

Code looks great! Just a few nits on the tests, mainly relating to clarity. Thanks for including the test with this.

Worth noting: I tested this against your feature branch with the transformMonthBreakdowns changes reset and the test you have passes. Does this align with your expectations, or should we iron out that piece of testing a bit more?

vault/activity_log_test.go Show resolved Hide resolved
vault/activity_log_test.go Outdated Show resolved Hide resolved
vault/activity_log_test.go Outdated Show resolved Hide resolved
vault/activity_log_test.go Show resolved Hide resolved
vault/activity_log_test.go Outdated Show resolved Hide resolved
@miagilepner
Copy link
Contributor Author

Worth noting: I tested this against your feature branch with the transformMonthBreakdowns changes reset and the test you have passes. Does this align with your expectations, or should we iron out that piece of testing a bit more?

The relevant function to revert is transformActivityLogMounts. The test will fail when that function is reverted to what's on origin/main. The additional test comments should also hopefully make clearer what's going on here.

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