-
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]Azure module - activity logs #13776
Conversation
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 looks mostly good and I will do a more thorough review after I return from vacation on Tuesday... a question in addition to the other comments: what specifically is restricted to x-pack here? It looks like ~all the code is in OSS and only module config is in x-pack. This would still leave the functionality in OSS, right? If there hasn't been a conversation about where the division lies, we should probably have one
filebeat/input/kafka/config.go
Outdated
@@ -50,6 +50,7 @@ type kafkaInputConfig struct { | |||
TLS *tlscommon.Config `config:"ssl"` | |||
Username string `config:"username"` | |||
Password string `config:"password"` | |||
AzureLogs string `config:"azure_logs"` |
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 field should be documented and validated -- consider following the pattern used for enums like kafka.Version
, which is validated by its Unpack
function (see libbeat/common/kafka/version.go
) and can thereafter be used as an enum constant rather than a string match.
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.
@faec , I have replaced the AzureLogs with YieldEventsFromField property. This property value, if entered will have the json object name that will contain multiple events.
Ex for azure:
{ "records": [ {event1}, {event2}]}
The YieldEventsFromField property value will be "records".
I have also removed any check for the azure type logs and added some validation inside the actual pipeline.
Let me know if you think this property should be handled the same way as "kafka.Version".
filebeat/input/kafka/input.go
Outdated
handler := &groupHandler{ | ||
version: input.config.Version, | ||
outlet: input.outlet, | ||
logType: input.config.AzureLogs, |
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.
I think I follow what's happening here (in generic kafka config there is no AzureLogs
field so this gets set to the empty string -> no special processing), but a comment would be nice to clarify that this is a no-op if there's no Azure-specific configuration
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.
AzureLogs have been replaced with a more generic option "YieldEventsFromField" and added some comments there
filebeat/input/kafka/input.go
Outdated
event := beat.Event{ | ||
Timestamp: timestamp, | ||
Fields: common.MapStr{ | ||
"message": msg, |
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.
Does this work ok? My recollection is that the explicit string conversion (string(message.Value)
in the old code) was necessary for a lot of things to work, since otherwise it got interpreted as raw bytes by the backend, which messed up the logs and the indexing.
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.
I had no issues with it so far, let me know if you know a more secure option, happy to change it
filebeat/input/kafka/input.go
Outdated
// if azure input, then a check for the message is done regarding the list of events | ||
|
||
var events []beat.Event | ||
messages := h.parseMultipleMessages(message.Value) |
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.
I understand that you need azure specific code in order to parse a message into multiple events. The rest could be done with a json
processor in a pipeline, isn't it?
If that's the case, perhaps it would be better to have a generic way to say: spawn events from list under this JSON field: "records"
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.
Unfortunately that is not possible, I could separate the json elements but they will still be generated as one event. The workaround would have been to create a new processor and a new interface for processors that can return multiple events.
"module" : "azure", | ||
"dataset" : "azure.activitylogs" | ||
}, | ||
"resultType" : "Failure", |
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.
I think some of these fields could be mapped into ECS like
resultType
->event.result
callerIpAddress
->source.ip
I'm sure they're are more and maybe you are already planning for some of these mappings.
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.
@andrewkroh , thanks, in the course of identifying the ecs fields. I could not find event.result in https://www.elastic.co/guide/en/beats/filebeat/master/exported-fields-ecs.html. Is it event.outcome or I am not looking in the right list.
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.
You're right. It's event.outcome
. I was working from memory on that example.
hi @faec , regarding:
I assumed the reason for adding the kafka input inside the oss is to be reused by other non x-pack modules as well. |
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.
Did a first pass, I like how this is looking!
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.
I think this is good to go if CI is happy. Thank you for implementing the changes!
Something that would be great to see here (in a different PR) is cloud.*
metadata matching the Metricbeat module. That should improve correlation opportunities
jenkins test this |
PR is a POC for the azure module in filebeat.
The azure module will contain:
activitylogs
fileset which will retrieve the Azure activity logs.Requirements are exporting these logs to an event hub (with Kafka feature enabled), steps here
signinlogs
fileset which will retrieve the Sign in logsauditlogs
fileset which will retrieve the AD audit logsRequirements are exporting these 2 AD reports to an event hub (with Kafka feature enabled), steps here
GH issue #13385