-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Syslog input plugin #4181
Syslog input plugin #4181
Conversation
Introducing an input service plugin acting like a syslog receiver as specified by RFC5425 (TLS/TCP) or RFC5426 (UDP).
plugins/inputs/syslog/syslog.go
Outdated
} | ||
|
||
var sampleConfig = ` | ||
## Specify an ip or hostname with port - eg., localhost:6514, 10.0.0.1:6514 |
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.
Use 2 space indent, comment anything out that has a default and doesn't need to be set (read_timeout)
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.
Ok.
plugins/inputs/syslog/syslog.go
Outdated
} | ||
s.Closer = l | ||
s.tcpListener = l | ||
if tlsConfig, _ := s.TLSConfig(); tlsConfig != nil { |
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 there is an error return it, Telegraf will refuse to start but that is the desired behavior 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.
Done.
plugins/inputs/syslog/syslog.go
Outdated
## Address and port to host the syslog receiver. | ||
## If no server is specified, then localhost is used as the host. | ||
## If no port is specified, 6514 is used (RFC5425#section-4.1). | ||
server = ":6514" |
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.
Use url style: tcp://:6514
mostly because it matches socket_listener.
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.
plugins/inputs/syslog/syslog.go
Outdated
n, _, err := s.udpListener.ReadFrom(b) | ||
if err != nil { | ||
if !strings.HasSuffix(err.Error(), ": use of closed network connection") { | ||
log.Println(err) |
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.
Don't log since the error is being added to the accumulator, which will also log.
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.
plugins/inputs/syslog/syslog.go
Outdated
} | ||
|
||
p := rfc5424.NewParser() | ||
mex, err := p.Parse(b[:n], &s.BestEffort) |
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.
mex?
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.
Changed to message
.
plugins/inputs/syslog/README.md
Outdated
Other available configurations are: | ||
|
||
- `keep_alive_period`, `max_connections` for stream sockets | ||
- `best_effort` to tell the parser to work until it is able to do and extract partial but valid info |
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.
Please add more details on how this works or add link to go-syslog repo.
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.
Ok.
plugins/inputs/syslog/README.md
Outdated
|
||
The fields/tags which name is in **bold** will always be present when a valid Syslog message has been received. | ||
|
||
### Syslog transport sender |
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.
Title this section something like RSYSLOG Integration
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.
Ok.
plugins/inputs/syslog/README.md
Outdated
|
||
```toml | ||
[[inputs.syslog]] | ||
address = ":6514" |
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 have a single configuration, no need to show it in multiple potential configurations.
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.
Ok, what configuration should we show? The minimal one?
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 showing the config for TLS.
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.
Run telegraf -usage syslog
and use the output.
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.
plugins/inputs/syslog/syslog_test.go
Outdated
require.Error(t, receiver.Start(&testutil.Accumulator{})) | ||
} | ||
|
||
func getRandomString(n int) 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.
It would be preferable to have non random strings in unittests, to increase determinism. Programmatic creation is fine of course.
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.
There's a pseudo-random source within this method using now as seed.
If you prefer we can stuck to a preconfigured seed value (so not now) or we can completely remove this and simply generate a string with length 7681.
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 would probably just generate a string with the desired length.
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.
} | ||
} | ||
|
||
func TestUDPInBestEffortMode(t *testing.T) { |
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 way these tests are structured make it more likely that they will fail due to a previous test case, because you are reusing the unit under test. I would create all tested code new for each test, unless you are specifically testing multiple connections.
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.
Should be better now.
plugins/inputs/syslog/syslog.go
Outdated
## Otherwise forced to the default. | ||
# protocol = "tcp" | ||
|
||
## TLS Config |
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 needs to be indented to the same level as the rest of the config
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.
plugins/inputs/syslog/syslog.go
Outdated
conn, err := s.tcpListener.Accept() | ||
if err != nil { | ||
if !strings.HasSuffix(err.Error(), ": use of closed network connection") { | ||
log.Println(err) |
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.
Remove this log statement
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.
e49fa4f
to
8da1f80
Compare
plugins/inputs/syslog/syslog.go
Outdated
flds["facility_code"] = int(*msg.Facility()) | ||
|
||
if msg.Timestamp() != nil { | ||
flds["timestamp"] = *msg.Timestamp() |
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 doesn't work because this is a time.Time and a field has to be one of these types. If this returns nil then the field is silently dropped!
Lines 251 to 286 in ce3b367
func convertField(v interface{}) interface{} { | |
switch v := v.(type) { | |
case float64: | |
return v | |
case int64: | |
return v | |
case string: | |
return v | |
case bool: | |
return v | |
case int: | |
return int64(v) | |
case uint: | |
return uint64(v) | |
case uint64: | |
return uint64(v) | |
case []byte: | |
return string(v) | |
case int32: | |
return int64(v) | |
case int16: | |
return int64(v) | |
case int8: | |
return int64(v) | |
case uint32: | |
return uint64(v) | |
case uint16: | |
return uint64(v) | |
case uint8: | |
return uint64(v) | |
case float32: | |
return float64(v) | |
default: | |
return nil | |
} | |
} |
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.
Probably should encode as an integer in nanosecond precision
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.
👍
49a52b5
to
625d18b
Compare
This PR introduces a syslog input service plugin (acting as a syslog receiver) supporting:
Thanks a lot to @goller for his feedback, help, and support.
Required for all PRs: