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

Import user.group changes from ECS #10275

Merged
merged 5 commits into from
Jan 24, 2019
Merged

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Jan 23, 2019

The dependency on elastic/ecs#308 is resolved (see also elastic/ecs#304 for full context). The change is merged. This ECS change lets use nest both the group name and id at user.group.id and user.group.name.

This PR imports these changes in Beats, and will unblock #9963 and #10192.

Mapping Conflict Warning

If you have any index template from Beats 7.0 pre-release in your Elasticsearch, this change introduces a mapping conflict.

Before:

{ "user": { "group": "root" } }

After this PR:

{ "user": { "group": { "id": 0, "name": "root" } } }

You will have to overwrite your index templates, delete you indices and your Kibana index templates.

@webmat webmat requested review from a team as code owners January 23, 2019 05:52
@webmat webmat self-assigned this Jan 23, 2019
@webmat webmat added in progress Pull request is currently in progress. libbeat ecs labels Jan 23, 2019
@webmat webmat requested a review from ruflin January 23, 2019 05:53
@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

make check fails for legit reason: this PR tweaks some ECS fields, and make check detects they haven't made it to x-pack/auditbeat.

However running make update in x-pack/auditbeat/ or auditbeat/ doesn't update anything more than when I run it at the top of the Beats repo.

@cwurm or @andrewkroh what's the proper incantation, to update field definitions for x-pack/auditbeat?

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

However running make update in x-pack/auditbeat/ or auditbeat/ doesn't update anything more than when I run it at the top of the Beats repo.

There is nothing in x-pack/auditbeat to update w.r.t. common fields. The auditbeat/include/fields.go content is inherited by x-pack/auditbeat.

@@ -29,60 +29,26 @@ auditbeat.max_start_delay: 10s
#========================== Modules configuration =============================
auditbeat.modules:

# The auditd module collects events from the audit framework in the Linux
Copy link
Member

Choose a reason for hiding this comment

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

Did you have DEV_OS=darwin set when you updated? Looks like some thing that should be changed have been changed.

Copy link
Contributor Author

@webmat webmat Jan 23, 2019

Choose a reason for hiding this comment

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

My problem is that I had GOOS and GOARCH set manually, I think. I just resubmitted after generating without those, and it got rid of the config file weirdness.

@webmat webmat force-pushed the ecs-import-user-group branch 2 times, most recently from ce2a275 to c5cf850 Compare January 23, 2019 19:04
@webmat webmat requested a review from a team as a code owner January 23, 2019 19:04
@webmat webmat added review and removed in progress Pull request is currently in progress. labels Jan 23, 2019
@webmat webmat changed the title WIP Import user.group changes from ECS Import user.group changes from ECS Jan 23, 2019
@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

Figured out the make update problem mentioned above. First legit test run just started, so we need to wait for that.

But otherwise, this is now ready for official review.

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

@andrewkroh Part of this (or separate PR) would surely be to import the generated Go files?

Not sure how/where to integrate those. Can you show me how? Is it applicable here, or is it not yet being used in Beats?

@andrewkroh
Copy link
Member

We should try to always keep the fields yml and the generated code in sync. It should just be a matter of running

govendor fetch github.com/elastic/ecs/code/go/ecs from withing elastic/beats/.

Assuming that you have govendor installed.

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

@andrewkroh Alright, I'll run this here, thanks for the command.

Is this encapsulated in a make/mage command, and is govendor be part of the development dependencies?

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

@andrewkroh Ran the command. The old keyword field is no longer there, but the group field set is not being nested at user.group (it should). Is this a part I should do manually? How have the other reusable field sets been imported?

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

Jenkins failure limited to Metricbeat, and appears to be due to network flakiness (tar segfault when getting a Docker image, seems like)

@andrewkroh
Copy link
Member

Is this a part I should do manually? How have the other reusable field sets been imported?

It's the same problem we have with the generated fields.yml not including them. The reusable fields are not incorporated into generated code so don't worry about adding them. For now if someone wants to use the code it's easy enough to compose your own struct that embeds the reusable type.

@webmat webmat added the blocker label Jan 24, 2019
@webmat
Copy link
Contributor Author

webmat commented Jan 24, 2019

This is ready for final review. I would like to get this in Thursday, to unblock #9963 and #10192.

@andrewkroh Gotcha, thanks for confirming!

@webmat
Copy link
Contributor Author

webmat commented Jan 24, 2019

Just noticed I hadn't entered a changelog for a change that affects all beats and causes a mapping conflict. 🤦‍♂️

Added the changelog now, and will wait until tomorrow to merge :-)

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. You can ignore Jenkins.

@ruflin
Copy link
Contributor

ruflin commented Jan 24, 2019

jenkins, test this

@webmat webmat force-pushed the ecs-import-user-group branch from 53e23c3 to bcb5639 Compare January 24, 2019 14:12
@ruflin ruflin mentioned this pull request Jan 24, 2019
@webmat webmat force-pushed the ecs-import-user-group branch from bcb5639 to e71d1e5 Compare January 24, 2019 17:10
@webmat webmat force-pushed the ecs-import-user-group branch from e71d1e5 to d1f95c1 Compare January 24, 2019 18:16
@webmat webmat merged commit 2034fe4 into elastic:master Jan 24, 2019
@webmat webmat deleted the ecs-import-user-group branch January 24, 2019 19:00
@webmat
Copy link
Contributor Author

webmat commented Jan 24, 2019

@cwurm This has been merged 😃

@cwurm
Copy link
Contributor

cwurm commented Jan 24, 2019

Thanks a lot @webmat, I‘ve been eagerly awaiting it! :)

@ruflin
Copy link
Contributor

ruflin commented Jan 25, 2019

Not 100% sure but it seems this PR broke CI. Not sure why this was merged with a red build?

@ruflin
Copy link
Contributor

ruflin commented Jan 25, 2019

Seems like it was caused by a change in ES: #10336 We should still if possible not merge red PR's.

DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
This change enables us to nest the `group` field set at `user.group`, rather than being limited to only group name.

Imports the changes from ECS elastic/ecs#308, which solves elastic/ecs#304.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants