Skip to content
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

Merged
merged 16 commits into from
May 25, 2018
Merged

Syslog input plugin #4181

merged 16 commits into from
May 25, 2018

Conversation

leodido
Copy link
Contributor

@leodido leodido commented May 22, 2018

This PR introduces a syslog input service plugin (acting as a syslog receiver) supporting:

  • TLS (or TCP) as mandated by RFC5425
  • UDP as mandated by RFC5426

Thanks a lot to @goller for his feedback, help, and support.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@leodido leodido self-assigned this May 22, 2018
}

var sampleConfig = `
## Specify an ip or hostname with port - eg., localhost:6514, 10.0.0.1:6514
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

}
s.Closer = l
s.tcpListener = l
if tlsConfig, _ := s.TLSConfig(); tlsConfig != nil {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

## 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"
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

n, _, err := s.udpListener.ReadFrom(b)
if err != nil {
if !strings.HasSuffix(err.Error(), ": use of closed network connection") {
log.Println(err)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

p := rfc5424.NewParser()
mex, err := p.Parse(b[:n], &s.BestEffort)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to message.

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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.


The fields/tags which name is in **bold** will always be present when a valid Syslog message has been received.

### Syslog transport sender
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.


```toml
[[inputs.syslog]]
address = ":6514"
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

require.Error(t, receiver.Start(&testutil.Accumulator{}))
}

func getRandomString(n int) string {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be better now.

## Otherwise forced to the default.
# protocol = "tcp"

## TLS Config
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

conn, err := s.tcpListener.Accept()
if err != nil {
if !strings.HasSuffix(err.Error(), ": use of closed network connection") {
log.Println(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this log statement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

flds["facility_code"] = int(*msg.Facility())

if msg.Timestamp() != nil {
flds["timestamp"] = *msg.Timestamp()
Copy link
Contributor

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!

telegraf/metric/metric.go

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
}
}

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@danielnelson danielnelson added this to the 1.7.0 milestone May 25, 2018
@danielnelson danielnelson merged commit b789845 into master May 25, 2018
@danielnelson danielnelson deleted the feature/syslog-plugin branch May 25, 2018 18:40
maxunt pushed a commit that referenced this pull request Jun 26, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants