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

Move name_map/ids fields to root of user #7841

Closed
wants to merge 1 commit into from

Conversation

vjsamuel
Copy link
Contributor

@vjsamuel vjsamuel commented Aug 2, 2018

This PR moves user. name_map to user.name and top level ids to user.id for the auditd module

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@vjsamuel vjsamuel force-pushed the move_user_fields branch 4 times, most recently from 8fafc45 to 73e9422 Compare August 2, 2018 03:54
type: keyword
description: file system group ID
- name: name_map
- name: id
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this would conflict with ECS: https://github.com/elastic/ecs#user

@ruflin
Copy link
Member

ruflin commented Aug 6, 2018

Can you share a bit more background on the reasons behind this change?

@vjsamuel
Copy link
Contributor Author

vjsamuel commented Aug 6, 2018

@ruflin, the idea is to restructure it for better clarity. The feeling was:
ids in the root level and name mapping as a child isnt as clear as:
user.ids and user.names. thoughts?

@ruflin
Copy link
Member

ruflin commented Aug 6, 2018

Let's see what @andrewkroh thinks.

@andrewkroh
Copy link
Member

Like I said on Slack:

One thing we have to resolve is how this fits in with ECS. See user.id and user.name in https://github.com/elastic/ecs#-user-fields.

Not wanting to conflict with ECS is the reason for the current field naming. I like how you have changed the fields in this PR, but it does conflict.

One option would be to switch back to plurals like user.ids.* and user.names.* to avoid conflicting with the ECS fieldsuser.id: []string and user.name: []string. But the downside is that this could be confusing if/when Auditbeat is updated to also populate the user.id field with all the UID values contained in the event (same for user.name).

@ruflin
Copy link
Member

ruflin commented Aug 8, 2018

Completely different suggestion. It's kind of odd that for example the field suid exists twice, once it is described as sent user id, the other one as sent user name (because it has a name prefix). What if we turn it around to:

su.id
su.name

The advantage of this would be that id and name would be close together. Disadvantage is that suid is probably well know.

@ruflin
Copy link
Member

ruflin commented Aug 17, 2018

@vjsamuel I think the part I'm missing here is if the current data model causes some issues when querying / aggregating the data or this is mainly about making the data model nicer?

@vjsamuel
Copy link
Contributor Author

@ruflin the intent is to simplify the data model.

@cwurm
Copy link
Contributor

cwurm commented Feb 5, 2019

@vjsamuel With #10456 the Auditd module is no longer using name_map. Does this address your concern?

@vjsamuel
Copy link
Contributor Author

vjsamuel commented Feb 5, 2019

i was looking for something in similar lines. I can close this one out. Thanks!

@vjsamuel vjsamuel closed this Feb 5, 2019
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.

None yet

5 participants