[Auditbeat] System module: Update and re-enable package dataset #10225

cwurm · 2019-01-21T17:35:15Z

Re-enables the disabled package dataset and brings it up to date with the other, soon-to-be released datasets.

High-level changes:

Renamed to package (singular)
Scheduled state reporting based on state.period and package.state.period
Common fields: event.kind, event.action, event.id, message
Save/Restore package information to disk

Unfortunately, the changes to package.go are extensive enough that the Github diff view presents it as a new file. A lot of lines have indeed changed, though none of the concepts are net new, they either exist in the other datasets or in the disabled implementation of the dataset.

Follow-ups, already listed in #10103:

Improve Homebrew package collection: parse INSTALL_RECEIPT.json
RPM support (Add RPM packaging #9092)
Dashboard
More and better tests

elasticmachine · 2019-01-21T17:35:17Z

Pinging @elastic/secops

x-pack/auditbeat/module/system/package/package.go

webmat

I shouldn't be the only one reviewing this Go code, but it looks good to me.

Field naming looks good as well. Took a note to eventually add package support in ECS.

Test failure unrelated, metricbeat

webmat · 2019-01-22T14:07:09Z

x-pack/auditbeat/module/system/package/package.go

+		"installtime": pkg.InstallTime,
+	}
+
+	if pkg.Arch != "" {


Nit: Couldn't you use putIfNotEmpty here as well?

I could have indeed, and I briefly thought about it but shied away from copying the function. Looking at more of this type of code throughout the Beats codebase I think putIfNotEmpty should be added to common.MapStr so we can use it everywhere. I'll make a note to propose adding it when I have a minute.

webmat · 2019-01-22T14:07:49Z

x-pack/auditbeat/module/system/package/package.go

+	ms := &MetricSet{
+		BaseMetricSet: base,
+		config:        config,
+		log:           logp.NewLogger(metricsetName),


A what? A "metricset"? ;-)

Indeed :) We might never be able to get it out of the whole codebase. For now at least everything that is user facing calls it "dataset".

tsg · 2019-01-24T10:33:31Z

x-pack/auditbeat/module/system/packages/packages.go

-// or more contributor license agreements. Licensed under the Elastic License;
-// you may not use this file except in compliance with the Elastic License.
-
-package packages


How come this file was deleted? Does it exists somewhere else now?

Oops, nevermind, saw it now. It was renamed to package/package.go.

tsg · 2019-01-24T10:52:45Z

x-pack/auditbeat/module/system/package/package.go

+	// Load from disk: Packages
+	packages, err := ms.restorePackagesFromDisk()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to restore packages from disk")


I think that returning an error here is going to cause Auditbeat to stop, right? Not sure if we discussed this before, but I wonder if recovering from this condition is not something that we want, to protect from corrupted gob.

We could print a stacktrace to the logs with a clear "please report this" message, but still continue the process.

I'm not convinced that we should recovered from the error, but I think it's worth discussing.

I think that returning an error here is going to cause Auditbeat to stop, right?

Yes. If the beat.db file is corrupted, the user will have to delete it to start.

My first instinct is to be conservative and surface the error. Something unexpected happened - maybe innocent, maybe not, we don't know - and the user should address it before continuing.

One alternative would be to print a warning but continue, e.g. treating the file as non existent which would trigger a whole state update to be sent. But that would miss any changes made while Auditbeat was down.

I think my opinion at this time is that unless we have a somewhat common case in which this error can be triggered and there is a good reason to not stop in that case I would be in favor of stopping as we do now.

Ok, I agree. Let's start with stopping Auditbeat in this case.

My concern is that a break in compatibility in beat.db is going to escalate itself to blocker bugs (Auditbeat doesn't start at all). In order to protect against this, we should have tests with some beat.db golden files from older versions.

tsg · 2019-01-24T11:02:23Z

x-pack/auditbeat/module/system/package/package.go

+			formulaPath := path.Join(homebrewCellarPath, pkg.Name, pkg.Version, ".brew", pkg.Name+".rb")
+			file, err := os.Open(formulaPath)
+			if err != nil {
+				//fmt.Printf("WARNING: Can't get formula for package %s-%s\n", pkg.Name, pkg.Version)


Log a warning or debug message?

Ah yes, good point. We don't have a logger in hand there at this time unfortunately. But I'll open a follow-up PR to improve Homebrew package collection that is going to address this as well. I have the code mostly ready.

tsg

I'm good to merge this as is. We should follow up with tests, though.

Please also add a CHANGELOG line if you didn't already in another PR.

x-pack/auditbeat/auditbeat.reference.yml

x-pack/auditbeat/module/system/_meta/config.yml.tmpl

cwurm · 2019-01-28T14:14:15Z

jenkins, test this

…tic#10225) Re-enables the disabled `package` dataset and brings it up to date with the other, soon-to-be released datasets. High-level changes: - Renamed to `package` (singular) - Scheduled state reporting based on `state.period` and `package.state.period` - Common fields: `event.kind`, `event.action`, `event.id`, `message` - Save/Restore package information to disk (cherry picked from commit 1e2c30a)

…nable package dataset (#10400) Cherry-pick of PR #10225 to 6.x branch. Original message: Re-enables the disabled `package` dataset and brings it up to date with the other, soon-to-be released datasets. High-level changes: - Renamed to `package` (singular) - Scheduled state reporting based on `state.period` and `package.state.period` - Common fields: `event.kind`, `event.action`, `event.id`, `message` - Save/Restore package information to disk

cwurm added review needs_backport PR is waiting to be backported to other branches. Auditbeat SecOps labels Jan 21, 2019

cwurm requested review from a team as code owners January 21, 2019 17:35

houndci-bot reviewed Jan 21, 2019

View reviewed changes

x-pack/auditbeat/module/system/package/package.go Outdated Show resolved Hide resolved

webmat approved these changes Jan 22, 2019

View reviewed changes

tsg reviewed Jan 24, 2019

View reviewed changes

tsg approved these changes Jan 24, 2019

View reviewed changes

andrewkroh approved these changes Jan 24, 2019

View reviewed changes

x-pack/auditbeat/auditbeat.reference.yml Outdated Show resolved Hide resolved

x-pack/auditbeat/module/system/_meta/config.yml.tmpl Outdated Show resolved Hide resolved

cwurm force-pushed the auditbeat_update_packages branch 2 times, most recently from 22a7c35 to ff07123 Compare January 28, 2019 12:26

Christoph Wurm added 13 commits January 29, 2019 12:56

Rename to package.

ce5be3a

Re-enable.

347287e

Remove report_changes.

eb21dd7

reportState

3d91523

Rename .go files.

abf148c

reportChanges, save/restore from disk

376eb2d

Tests and fields

f140de8

Docs

5af6677

Check for files when creating metricset

7421f9d

fmt

3d923f6

Make Hound happy

6d38d22

Exclude from build on Windows.

eb90ab8

Add package.state.period and package to some places in the docs.

3a3294c

Changelog

73da47f

cwurm force-pushed the auditbeat_update_packages branch from ff07123 to 73da47f Compare January 29, 2019 12:58

cwurm merged commit 1e2c30a into elastic:master Jan 29, 2019

cwurm deleted the auditbeat_update_packages branch January 29, 2019 14:36

cwurm mentioned this pull request Jan 29, 2019

Cherry-pick #10225 to 6.x: [Auditbeat] System module: Update and re-enable package dataset #10399

Closed

4 tasks

cwurm removed the needs_backport PR is waiting to be backported to other branches. label Jan 29, 2019

cwurm mentioned this pull request Jan 29, 2019

[Auditbeat] Cherry-pick #10225 to 6.x: System module: Update and re-enable package dataset #10400

Merged

4 tasks

cwurm added the v6.7.0 label Jan 29, 2019

This was referenced Jan 29, 2019

[Auditbeat] System module 6.7 / 7.0 #10103

Closed

Add RPM packaging #9092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Auditbeat] System module: Update and re-enable package dataset #10225

[Auditbeat] System module: Update and re-enable package dataset #10225

cwurm commented Jan 21, 2019 •

edited

Loading

elasticmachine commented Jan 21, 2019

webmat left a comment

webmat Jan 22, 2019

cwurm Jan 24, 2019

webmat Jan 22, 2019

cwurm Jan 24, 2019

tsg Jan 24, 2019

tsg Jan 24, 2019

tsg Jan 24, 2019

cwurm Jan 24, 2019

tsg Jan 24, 2019

tsg Jan 24, 2019

cwurm Jan 25, 2019

tsg left a comment

cwurm commented Jan 28, 2019

[Auditbeat] System module: Update and re-enable package dataset #10225

[Auditbeat] System module: Update and re-enable package dataset #10225

Conversation

cwurm commented Jan 21, 2019 • edited Loading

elasticmachine commented Jan 21, 2019

webmat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg left a comment

Choose a reason for hiding this comment

cwurm commented Jan 28, 2019

cwurm commented Jan 21, 2019 •

edited

Loading