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

backports for ActivityLog and Reporting 1.13.x #21140

Merged
merged 17 commits into from
Jun 14, 2023

Conversation

mpalmi
Copy link
Contributor

@mpalmi mpalmi commented Jun 12, 2023

This PR encapsulates the backports for Census and ActivityLog changes to get the release branches in sync and should resolve VAULT-14702 and VAULT-14765 along with the backports for the new client count testing library.

It was easier to backport the changes en masse, since they're all intertwined. Teasing apart feature-specific changes would have been more difficult.

Commits were prepared using (with fish shell, $() is just ()):

git cherry-pick -v -x (git log origin/release/1.13.x..origin/main --oneline --reverse -- "*activity*" | awk '{ print $1 }')

and editorialized/reworded using interactive rebase. The ACME-related changes (including testing of the clientType proto field via the client count testing library) were not backported.

Apologies for the large PR. It may be easiest to review by comparing each backport commit with the original. I can split this up if others think it's worthwhile.

Diff between backport and main

## NOTE: This diff does not include changes to core. 
git diff origin/activitylog-backports-1.13.x..origin/main -- "*activity*" "*census*" ':!ui/*'

https://gist.github.com/mpalmi/72d7f39c3d190ec660e0795419004545

@mpalmi mpalmi force-pushed the activitylog-backports-1.13.x branch 2 times, most recently from 1c2e47d to 09e7797 Compare June 12, 2023 18:29
@mpalmi mpalmi changed the title Activitylog backports 1.13.x backports for ActivityLog and Reporting 1.13.x Jun 12, 2023
@mpalmi mpalmi requested review from raskchanky, banks, miagilepner, hghaf099 and a team June 12, 2023 19:22
@mpalmi mpalmi marked this pull request as ready for review June 12, 2023 19:25
@mpalmi mpalmi requested a review from yhyakuna as a code owner June 12, 2023 19:25
@miagilepner
Copy link
Contributor

miagilepner commented Jun 13, 2023

I think you might be missing #20626. Otherwise looks good! You said this in the description, 🤦‍♀️

Makefile Outdated Show resolved Hide resolved
@raskchanky
Copy link
Contributor

When I ran your git log command myself, I got 27 commits. There are 18 here. I'm assuming the missing 9 commits were omitted on purpose, perhaps UI related or some such?

Also, the "*activity*" portion of your git command makes it look like it only applies to paths with activity in their name somewhere. Is that only for the purpose of selecting commits? e.g. If I commit something and any of the changed files in my commit has activity in its name, then it shows up via this command. Is that the case? If so, were there any changes made to files w/o activity in their name that need to be backported as well?

@mpalmi
Copy link
Contributor Author

mpalmi commented Jun 13, 2023

When I ran your git log command myself, I got 27 commits. There are 18 here. I'm assuming the missing 9 commits were omitted on purpose, perhaps UI related or some such?

While going through the backport, there were 4 classes of commits (marked below):

  • ❌ empty commits, which had already been backported but weren't attributed by git
  • ❌ superfluous changes which rejected the entire commit (these got the 'ol --skip during the cherry-pick)
  • ✅ changes that ported over (sometimes with amendment)
  • ✅ clean cherry-picks, which ported over with no modifications

It would probably be better if I had kept track of the 4 separate buckets so I could distinguish them, but for the purposes of this PR, separating by ❌ and ✅ seems sufficient.

Note: I will do something similar for the 1.11.x and 1.13.x PRs, if you think it's helpful.


a9e17c2 VAULT-13763 normalize activity log mount paths (#19343)
7ef7297 UI: Use specific date in clients activity test (#19419)
9f7f8d5 VAULT-13729 activity log test godocs (#19433)
e55c18e adding copyright header (#19555)
e3c5977 Add no-op CensusAgent (#19625)
e9d6dbc activitylog: Fix pq.Get trace logger output (#19650)
bf2f1a8 UI: Fix flaky time-related tests (#19521)~
b4fab6a VAULT-13191: OSS changes (#19891)
54904e4 VAULT-14733: Refactor processClientRecord in activity log (#19933)
d70c17f VAULT-14733: SegmentReader interface for reading activity log segments (#19934)
4b6ec40 Require activity log retention months at least the minimum (#20078)
05ba6bb api: Add reporting fields to activitylog config endpoint (#20086)
c95d4fb VAULT-14734: activity log write endpoint (#20019)
75c903f openapi: Add display attributes for /sys (p2) (#19707)
002a59a Add minimum_retention_months to config endpoint (#20150)
77f83d9 Refactor reporter for unseal setup (#20296)
730d0e2 VAULT-14733: Split logic of precomputedQueryWorker (#20073)
d234111 Start counting ACME certificate issuance as client activity (#20520)
35e2c16 VAULT-15703: Reload automated reporting (#20680)
6d95f8c Add client_type field to EntityRecord protobuf (#20626)
810d504 Add current_billing_period activity endpoint param (#20694)
5b23dd5 VAULT-14735: generate mock clients for activity log (#20252)
018ea84 VAULT-15395: Support mocking time functions in the activity log (#20720)
541f18e VAULT-14735: repeated and segmented activity log clients (#20699)
b4e2751 VAULT-14735: write mock activity log entity files (#20702)
dc5dd71 Deflake TestActivityLog_MultipleFragmentsAndSegments (#20930)
5002489 VAULT-15394: Generate activity log precomputed queries (#20778)


Also, the "*activity*" portion of your git command makes it look like it only applies to paths with activity in their name somewhere. Is that only for the purpose of selecting commits? e.g. If I commit something and any of the changed files in my commit has activity in its name, then it shows up via this command. Is that the case? If so, were there any changes made to files w/o activity in their name that need to be backported as well?

That's right, I used it as a general heuristic for selecting the appropriate commits (with some extra post-processing). I think the changes this PR addresses were all contained within *activity* files, but can't hurt to double-check -- good call!

Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

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

There's a lot here to go through, especially for a backport. I did my best to sort through it, and to my eyes, it looks correct.

@mpalmi mpalmi force-pushed the activitylog-backports-1.13.x branch from b75e69b to 627d3f9 Compare June 14, 2023 20:02
@mpalmi mpalmi added this to the 1.13.4 milestone Jun 14, 2023
@mpalmi mpalmi force-pushed the activitylog-backports-1.13.x branch from 627d3f9 to f2a62da Compare June 14, 2023 20:35
@mpalmi mpalmi merged commit fa4153d into release/1.13.x Jun 14, 2023
@mpalmi mpalmi deleted the activitylog-backports-1.13.x branch June 14, 2023 21:07
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