-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Filebeat] Add ZooKeeper Module #25128
Conversation
fed9913
to
fb0205c
Compare
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
2acd2d8
to
d2e76c4
Compare
@matschaffer any thoughts on if more should be done for this? If not I'll take the PR out of draft. |
@legoguy1000 I haven't run it and not sure when I'll be able to to give it a proper review (starting on-call tomorrow). Just reading it through looks good though so taking out of draft gets 👍 from me. Maybe @jsoriano can help recommend a reviewer. I'll mention it to some other folks to get eyes on it. |
Oooh, on-call, been there. Sounds good, I'll take it out of draft. It's an initial release so can always be improved later. |
Pinging @elastic/integrations (Team:Integrations) |
This pull request is now in conflicts. Could you fix it? 🙏
|
d2e76c4
to
e6de063
Compare
I will take a look, thanks! |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, thanks @legoguy1000 for this!
I have added some suggestions, let me know what you think. Some CI jobs are also complaining about some outdated files, these can be probably solved by running mage update
from filebeat
and x-pack/filebeat
directories and committing the changes.
Other comment I have is that in principle new modules are only added to x-pack/filebeat/module
, would you mind to move the module there?
I can move it. My thought was zookeeper fit alongside Kafka and apache... more than all the security tools but doesn't matter to me. |
PR updated |
6ce4965
to
6d45f78
Compare
@jsoriano I've updated the PR to account for timestamps in the logs and set accordingly. If it looks good to you, can u run the tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty rad! I haven't tested the audit log but the app log seems to work nicely, especially given how non-parseable the logs are. I like that it also captures event.created an ingested so we can detect delays. Thanks for making this.
- convert: | ||
field: client.address | ||
target_field: client.ip | ||
type: ip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to future self; this needs 7.13 since it was added recently elastic/elasticsearch#71285
Do u have examples of real audit logs? I just used the example logs from the docs and then had to add additional data to match the log pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this is almost ready to go, added some small comments, but nothing serious.
x-pack/filebeat/module/zookeeper/audit/test/audit_with_dates.log
Outdated
Show resolved
Hide resolved
/test |
@jsoriano I've update the PR with your comments and added your provided sample logs. I think we should be good to test and if it passes merge. |
0a35fcd
to
00de4e9
Compare
@jsoriano good to retest. |
/test |
@jsoriano same issue with the |
I know why. its because the 1 set of example logs that doesn't have an timestamp field to parse so the time changes every time. I'm going to get rid of those since u provided logs from the default pattern and it has a timestamp. |
084a5ee
to
1b1807f
Compare
Oh, I also thought that tests ignored this field for logs without timestamp, but maybe there are no other cases of that. I am ok in any case with testing only with logs with timestamp as they seem to be the usual default. |
/test |
Ya there's a list in the python script to declare the filesets to ignore because they don't have a timestamp but decided to just remove them for the same reason. |
@jsoriano finally passed :) |
Adds a new module for ZooKeeper audit and service logs. (cherry picked from commit d09dfb0)
Merged and backport open, this will be likely released in 7.14.0. Thanks @legoguy1000 for the good work here! |
Adds a new module for ZooKeeper audit and service logs. (cherry picked from commit d09dfb0) Co-authored-by: Alex Resnick <[email protected]> Co-authored-by: DeDe Morton <[email protected]>
What does this PR do?
Adds a new module for ZooKeeper logs.
Why is it important?
Requested by Elastic Cloud team to ingest ZooKeeper logs.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs