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

tetragon: cache username lookups #2448

Merged
merged 2 commits into from
May 30, 2024
Merged

Conversation

tixxdz
Copy link
Member

@tixxdz tixxdz commented May 17, 2024

No description provided.

@tixxdz tixxdz requested a review from a team as a code owner May 17, 2024 17:40
@tixxdz tixxdz requested a review from olsajiri May 17, 2024 17:40
@tixxdz tixxdz added the release-note/minor This PR introduces a minor user-visible change label May 17, 2024
@tixxdz tixxdz force-pushed the pr/tixxdz/2024-05-cache-usernames branch from c4dc849 to 9b4446c Compare May 17, 2024 17:52
@tixxdz tixxdz requested a review from kkourt May 21, 2024 16:05
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks, overall looks good to me. I've left some minor comments.

pkg/reader/userdb/userdb.go Outdated Show resolved Hide resolved
if userInfo, err := user.LookupId(strconv.FormatUint(uint64(m.Unix.Process.UID), 10)); err == nil {
m.Unix.Process.User.Name = userInfo.Name
if username, err := userdb.UsersCache.LookupUser(m.Unix.Process.UID); err == nil {
m.Unix.Process.User.Name = username
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to have a metric here for errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do it separately, thanks

@olsajiri
Copy link
Contributor

@tixxdz lookgs good, few questions

could you please provide more details in changelogs on why we want to do that, I guess it's faster, can we measure that?
also why do we cache just part of the user ids?

thanks

tixxdz added 2 commits May 30, 2024 17:14
Cache UIDs/Usernames so we reduce:
- I/O on /etc/passwd for each execution especially for root user and
  other system user IDs as by default lot of software runs under those.
- Some users may watch /etc/passwd access, so caching those entries
  will reduce Tetragon generated events and improve performance overall.

We only cache UIDs from 0..999 as these are considered standard
system users, that are set by most distros packages. Distros maintainers
usually do great job to not clash such users. For anything above > 999
we do not cache it, we can that if users want such support later.

Reference:
https://github.com/systemd/systemd/blob/main/docs/UIDS-GIDS.md#summary

Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
@tixxdz
Copy link
Member Author

tixxdz commented May 30, 2024

@tixxdz lookgs good, few questions

could you please provide more details in changelogs on why we want to do that, I guess it's faster, can we measure that? also why do we cache just part of the user ids?

thanks

I added the following commit logs:

    Cache UIDs/Usernames so we reduce:
    - I/O on /etc/passwd for each execution especially for root user and
      other system user IDs as by default lot of software runs under those.
    - Some users may watch /etc/passwd access, so caching those entries
      will reduce Tetragon generated events and improve performance overall.
    
    We only cache UIDs from 0..999 as these are considered standard
    system users, that are set by most distros packages. Distros maintainers
    usually do great job to not clash such users. For anything above > 999
    we do not cache it, we can that if users want such support later.
    
    Reference:
    https://github.com/systemd/systemd/blob/main/docs/UIDS-GIDS.md#summary

Thank you

@tixxdz tixxdz force-pushed the pr/tixxdz/2024-05-cache-usernames branch from 9b4446c to 79aefef Compare May 30, 2024 16:25
@tixxdz tixxdz merged commit dac9959 into main May 30, 2024
39 checks passed
@tixxdz tixxdz deleted the pr/tixxdz/2024-05-cache-usernames branch May 30, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants