-
Notifications
You must be signed in to change notification settings - Fork 230
Feature/windows event id filtering #1737
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
base: main
Are you sure you want to change the base?
Conversation
b758b9e
to
516df4e
Compare
516df4e
to
2276ab2
Compare
plugins/inputs/windows_event_log/wineventlog/wineventlog_test.go
Outdated
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go
Outdated
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist_test.go
Outdated
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist_test.go
Outdated
Show resolved
Hide resolved
9d99660
to
2667175
Compare
@@ -61,6 +61,7 @@ func (s *Plugin) SampleConfig() string { | |||
[[inputs.windows_event_log.event_config]] | |||
event_name = "System" | |||
event_levels = ["2", "3"] | |||
event_ids = [1001, 1002] |
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.
What are these event ids what are they representing, can we comment that here?
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.
They are generic event ids to show how the final sampleConfig of the Toml will look like. it serves as documentation and example config for users of the Windows event Log plugin. So I just add the event id since it is a feature now.
@@ -36,34 +25,7 @@ const ( | |||
UNKNOWN = "UNKNOWN" | |||
) | |||
|
|||
var NumberOfBytesPerCharacter = UnknownBytesPerCharacter | |||
|
|||
func RenderEventXML(eventHandle EvtHandle, renderBuf []byte) ([]byte, error) { |
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.
why are we deleting this function can you explain in your PR description?
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 just moved them to a different file due to the windows build tags. I wanted run some test on the linux environment so it doesn't skip those tests.
return utf16ToUTF8Bytes(renderBuf, bufferUsed) | ||
} | ||
|
||
func CreateBookmark(channel string, recordID uint64) (h EvtHandle, err error) { |
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.
why are we deleting this function can you explain in your PR description?
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.
Moved to a different file due to the windows build tags.
|
||
func WindowsEventLogLevelName(levelId int32) string { |
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.
why are we deleting this function can you explain in your PR description?
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.
Moved to a different file due to the windows build tags.
|
||
var NumberOfBytesPerCharacter = UnknownBytesPerCharacter | ||
|
||
func RenderEventXML(eventHandle EvtHandle, renderBuf []byte) ([]byte, error) { |
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.
Okay so you are moving into a different file. Please document that in your description
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.
Alright sure
if err != nil { | ||
log.Panicf("Encode to a valid TOML config fails because of %v", err) | ||
} | ||
return buf.String() | ||
} | ||
|
||
// ensure integers in arrays are preserved | ||
func processValue(val interface{}) interface{} { |
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.
shouldn't we have some kind of validation and returning error here. With the current implementation it seems like this function could never fail, is this true?
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.
yeah used a different approach to integer verification, reverted to original main code.
if eventLevels, ok := result[EventLevelsKey]; !ok { | ||
return | ||
} else { |
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.
With this change now, we keep going when the eventLevel is missing. Is this intentional?
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.
if the eventLevel is missing it will proceed to check the for eventids, actually this still sets eventlevels to an empty slice of string instead of nil, I will fix this.
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.
done!
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go
Outdated
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/windows_event_test.go
Outdated
Show resolved
Hide resolved
// Process value to ensure integers in arrays are preserved | ||
processedVal := processValue(val) |
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.
Interesting. There are some other integers that aren't mangled like log retention. I wonder why those work. Perhaps because they aren't in an array? We should understand why those integers don't require additional processing.
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go
Show resolved
Hide resolved
ffba86e
to
a4bbe23
Compare
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go
Outdated
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/collect_list/ruleEventIDs.go
Outdated
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/collect_list/ruleEventIDs.go
Show resolved
Hide resolved
translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go
Outdated
Show resolved
Hide resolved
097c665
to
dc6df12
Compare
translator/translate/logs/logs_collected/windows_events/collect_list/ruleEventIDs.go
Show resolved
Hide resolved
dc6df12
to
1ab8a1a
Compare
Description of the issue
The CloudWatch Agent currently filters Windows Event Logs based on security levels (i.e Error, Critical, Information...). This does not allow customers to only collect specific and relevant logs to CloudWatch
Description of changes
sample configuration of event ID filtering
Caution
This PR has a testing PR linked to here: aws/amazon-cloudwatch-agent-test#541, merge this first.
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Requirements
Before commiting your code, please do the following steps.
make fmt
andmake fmt-sh
make lint
Integration Tests
To run integration tests against this PR, add the
ready for testing
label.