-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix event handler types filter when using bulk export API
#61787
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: master
Are you sure you want to change the base?
Conversation
types filter when using bulk export APItypes filter when using bulk export API
| // filter out unwanted event types (in the v1 event export logic this was an internal behavior | ||
| // of the event processing helper since it would perform conversion prior to storing the event | ||
| // in its internal buffer). | ||
| if len(j.app.Config.Types) > 0 && !slices.Contains(j.app.Config.Types, evt.Event.Type) { |
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.
No need for the length check since the slice contains does exactly that check before.
Should we create a map to avoid looping through config types?
Similar to skip events
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.
My understanding was that if the types option was an empty slice, we would forward all event types (default behavior). Then slices.Contains would return false given the slice is empty, and the event type would be erroneously skipped.
I can change it into a map, although the length check will still be required
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 are correct. I forgot about that
| return nil | ||
| } | ||
|
|
||
| j.app.log.DebugContext(ctx, "Not skipping event", "type", evt.Event.Type) |
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'd remove this one. The event handler processes many thousands of events, we probably don't need to debug log the normal operation.
| require.NotRegexp(t, regexp.MustCompile("\"Event sent\".*type=role.created"), out.String()) | ||
| require.Regexp(t, regexp.MustCompile("\"Event sent\".*type=join_token.create"), out.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.
Compile these regexes outside of the loop so you don't waste time recompiling them for each test case.
| @@ -38,7 +38,6 @@ You may specify configuration options via command line arguments, environment va | |||
| | skip-session-types | Comma-separated list of session event types to skip | FDFWD_SKIP_SESSION_TYPES | | |||
| | start-time | Minimum event time (RFC3339 format) | FDFWD_START_TIME | | |||
| | timeout | Polling timeout | FDFWD_TIMEOUT | | |||
| | cursor | Start cursor value | FDFWD_CURSOR | | |||
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 was this removed?
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.
The cursor CLI option (and environment variable) do not seem to exist anymore
| @@ -357,6 +357,8 @@ Loop: | |||
|
|
|||
| _, ok = j.app.Config.SkipSessionTypes[e.Type] | |||
| if !ok { | |||
| j.app.log.DebugContext(ctx, "Not skipping session event", "type", e.Type) | |||
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.
Similarly, I'd remove this one.
| } | ||
|
|
||
| switch e.GetType() { | ||
| switch evt.Type { |
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.
Was this change necessary?
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.
Wasn't necessary, although it matches how we are operating on the newly created event (hence removing the unused method fields as well)
Although if there is some other drawback or inconvenience I can revert it
r0mant
left a comment
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.
Just a small comment in addition to what other people commented on already.
Good job figuring this out @kshi36!
| // filter out unwanted event types (in the v1 event export logic this was an internal behavior | ||
| // of the event processing helper since it would perform conversion prior to storing the event | ||
| // in its internal buffer). | ||
| if len(j.app.Config.Types) > 0 && !slices.Contains(j.app.Config.Types, evt.Event.Type) { | ||
| j.app.log.DebugContext(ctx, "Skipping event from types filter", "type", evt.Event.Type) |
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.
To echo Zac's similar comments, I would consider removing all of these extra debug statements that are logged whenever an event is processed, whether it's skipped or not. They can still clog the logs if you e.g. choose to skip a frequent event.
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 like the idea of logging a debug message when dropping an event. This can help troubleshoot issues like this one: #61729.
For the config I agree that this is not especially useful as we usually have access to the config.
Fixes #56099
Manual Tests
Test:
typesfilter works correctly for clients using new bulk export APItomlconfiguration file with the following options:types="user.login"filter forwards onlyuser.loginevents to fluentd audit events endpointskip-event-types="user.login"filter skips forwardinguser.loginevents to fluentd audit events endpointskip-session-types="db.session.query"filter skips forwardingdb.session.queryevents to fluentd session events endpointtomlconfig file and CLI argumentschangelog: Fixed bug where event handler
typesfilter is ignored for Teleport clients using Athena storage backend