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

promtail: add RFC3164 syslog support #7845

Closed
wants to merge 4 commits into from
Closed

Conversation

aparcar
Copy link

@aparcar aparcar commented Dec 4, 2022

I'm no Go programmer so somewhat glued ends together, please be patient. If this is of general interest I'm happy to fill the gaps like testing, documentation and change log. Since this requires changes of a vendor package, I created a PR there as well.

What this PR does / why we need it:

It adds RFC3164 support to promtail. Commonly used for OpenWrt and Ubiquiti syslogs.

Which issue(s) this PR fixes:

I opened a PR directly instead of an issue first.

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

This is needed to parse RFC3164 syslogs.

Signed-off-by: Paul Spooren <[email protected]>
both nontransparent and octetcounting use RFC5424 which isn't used by
many devices, i.e. OpenWrt or Ubiquiti routers.

Add a new function called NewParserRFC3164 to both.

Signed-off-by: Paul Spooren <[email protected]>
This adds support for RFC3164 syslogs which are used by many devices, in
my use case OpenWrt and Ubiquiti routers. A new option called
`is_rfc3164_message` is added to the syslog which makes the incomming
logs to be handled as RFC3164.

Signed-off-by: Paul Spooren <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Dec 4, 2022

CLA assistant check
All committers have signed the CLA.

Signed-off-by: Paul Spooren <[email protected]>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 7, 2022
@MasslessParticle MasslessParticle self-assigned this Jan 13, 2023
@MasslessParticle
Copy link
Contributor

This is a reasonable idea. Given this is setup as a configuration option, it'd probably be cleaner to implement it as it's own target rather than having if rfc3164... everywhere.

Of course, also it'd need tests

Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

How likely do you think it is to get influxdata/go-syslog#46 into the upstream library (the project is rather stale)?

@@ -14,7 +14,7 @@ import (
// the callback function with the parsed messages. The parser automatically
// detects octet counting.
// The function returns on EOF or unrecoverable errors.
func ParseStream(r io.Reader, callback func(res *syslog.Result), maxMessageLength int) error {
func ParseStream(isRFC3164Message bool, r io.Reader, callback func(res *syslog.Result), maxMessageLength int) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put this new argument to the end?

@chaudum
Copy link
Contributor

chaudum commented Jan 16, 2023

This is a reasonable idea. Given this is setup as a configuration option, it'd probably be cleaner to implement it as it's own target rather than having if rfc3164... everywhere.

Of course, also it'd need tests

I'm not sure about making a separate target, but I agree that we should have a better abstraction for the various syslog implementations.

E.g. in the config, there are would now be options for UseRFC5424Message and IsRFC3164Message, which are hard to distinguish and are not mutually exclusive.

Maybe the syslog target should have a single option syslog_format which could default to rfc5424 and also takes rfc3164 as possible value.

That configuration change would probably be breaking and therefore something for Loki/Promtail 3.0?

@chaudum
Copy link
Contributor

chaudum commented Jan 18, 2023

How likely do you think it is to get influxdata/go-syslog#46 into the upstream library (the project is rather stale)?

@MasslessParticle @aparcar What do you think about forking the go-syslog project to the grafana Github org and reference the fork as long as the addition is not added to the upstream project?

@MasslessParticle
Copy link
Contributor

@chaudum That sounds reasonable. This project doesn't change much. In the past, they've been generally responsive to PRs -- I've actually got one with a former teammate in there -- so I hope we don't have to sit on the fork forever.

@MasslessParticle
Copy link
Contributor

Maybe the syslog target should have a single option syslog_format which could default to rfc5424 and also takes rfc3164 as possible value.

This is totally reasonable. Putting the syslog stuff behind an interface would help keep the implementation cleaner

@MasslessParticle
Copy link
Contributor

Closing this one for inactivity

@MasslessParticle
Copy link
Contributor

Sounds like there's still some demand for this but we may have to pick it up ourselves

@MasslessParticle
Copy link
Contributor

I've forked the parser to our org and merged the patch this depends on

@aparcar
Copy link
Author

aparcar commented Mar 31, 2023

Uhm where can I follow your work? I can also give this another go

@MasslessParticle
Copy link
Contributor

@aparcar The forked library you can rely on is here: https://github.com/grafana/go-syslog

I can also give this another go

That would be great!

@catap
Copy link
Contributor

catap commented Apr 26, 2024

@aparcar, @MasslessParticle any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants