[Auditbeat] System module: Update and re-enable package dataset#10225
[Auditbeat] System module: Update and re-enable package dataset#10225cwurm merged 14 commits intoelastic:masterfrom
Conversation
cwurm
added
review
needs_backport
|
Pinging @elastic/secops |
webmat
left a comment
webmat
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Nit: Couldn't you use putIfNotEmpty here as well?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
How come this file was deleted? Does it exists somewhere else now?
There was a problem hiding this comment.
Oops, nevermind, saw it now. It was renamed to package/package.go.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Log a warning or debug message?
There was a problem hiding this comment.
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
left a comment
tsg
left a comment
There was a problem hiding this comment.
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.
cwurm
force-pushed
the
auditbeat_update_packages
branch
2 times, most recently
from
22a7c35 to
ff07123
Compare
|
jenkins, test this |
cwurm
force-pushed
the
auditbeat_update_packages
branch
from
ff07123 to
73da47f
Compare
…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)
cwurm
removed
the
needs_backport
…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
6 participants
Re-enables the disabled
packagedataset and brings it up to date with the other, soon-to-be released datasets.High-level changes:
package(singular)state.periodandpackage.state.periodevent.kind,event.action,event.id,messageUnfortunately, the changes to
package.goare 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:
INSTALL_RECEIPT.json