feat(promtail): Support of RFC3164 aka BSD Syslog #12810

catap · 2024-04-28T10:24:57Z

What this PR does / why we need it:

This PR introduce support of RFC3164 at promtail with additional hacks to support old and wired implementations.

It includes different BSD system such as OpenBSD and some embeded system. Frankly speaking all of them that I have access: from OpenBSD server to Unifi UA and IP Socket and Synology NAS with Nginx, works with this changes.

Both protocol TCP and UDP works.

It based on #7845 and requries some changes inside used go-syslog library which was accomulated as single PR influxdata/go-syslog#55 but upstream of that library seems to be not so active.

It is tested with config like:

server:
  disable: true

positions:
  filename: /tmp/promtail-syslog-positions.yml

clients:
  - url: http://127.0.0.1:3100/loki/api/v1/push

scrape_configs:
  - job_name: syslog
    syslog:
      listen_address: 127.0.0.1:1514
      listen_protocol: tcp # or udp
      is_rfc3164_message: true
      labels:
        job: syslog
    relabel_configs:
      - source_labels: [__syslog_message_severity]
        target_label: level
      - source_labels: [__syslog_message_facility]
        target_label: facility
      - source_labels: [__syslog_connection_hostname]
        target_label: host
      - source_labels: [__syslog_connection_ip_address]
        target_label: ip
      - source_labels: [__syslog_message_hostname]
        target_label: hostname
      - source_labels: [__syslog_message_app_name]
        target_label: app
      - source_labels: [__syslog_message_proc_id]
        target_label: proc
      - source_labels: [__syslog_message_msg_id]
        target_label: msgid

Which issue(s) this PR fixes:
A few times was made quite clear that such things aren't supported. So, no issue were opened.

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here

CLAassistant · 2024-04-28T10:25:05Z

All committers have signed the CLA.

cstyan · 2024-04-30T00:42:30Z

hey @catap we're technically calling promtail feature complete but I think we can make a case for this really being a fix rather than being a feature if promtail didn't work with BSD syslog before, though we would definitely want to wait for your changes to be upstreamed via influxdata/go-syslog#55 first

catap · 2024-04-30T01:44:03Z

Hey @cstyan!

Yes, BSD syslog were nuke when support of syslog was introduced at #1275 since when was one attempt #7845 to do it which I've reused.

As part of that attempt the PR: influxdata/go-syslog#46 with required changes inside go-syslog were created but nobody had reviewed it.

It was December 2022.

I doubt that my PR will be merged some time soon, let be honest.

But this changes which is localized inside not used before code allows me to introduce support of different devices which sends logs to Loki. From BSD system to Nginx with Unifi Networks and so on.

So, what's a plan here? I see that https://github.com/grafana/go-syslog exists and I may open PR into this repository, but as far as I see nobody uses it.

catap · 2024-04-30T14:34:53Z

Just a rebase to the last main.

cstyan · 2024-04-30T20:24:48Z

Hey @cstyan!

Yes, BSD syslog were nuke when support of syslog was introduced at #1275 since when was one attempt #7845 to do it which I've reused.

As part of that attempt the PR: influxdata/go-syslog#46 with required changes inside go-syslog were created but nobody had reviewed it.

It was December 2022.

I doubt that my PR will be merged some time soon, let be honest.

But this changes which is localized inside not used before code allows me to introduce support of different devices which sends logs to Loki. From BSD system to Nginx with Unifi Networks and so on.

So, what's a plan here? I see that https://github.com/grafana/go-syslog exists and I may open PR into this repository, but as far as I see nobody uses it.

Unfortunately I don't have any suggestions here at the moment. Given we're winding down our own work on promtail and (to my knowledge) don't use the syslog target ourselves I don't really want to take on maintenance of this code within the Loki repo. It's likely better to ask the Alloy team to take on maintenance of the code either directly or via a fork, though I doubt that will happen anytime soon either. They have their own open issue tracking support for the same RFC: grafana/alloy#305

My only other suggestion would be opening an issue on the go-syslog repo itself asking if the repo is unmaintained and trying to get in contact with someone from Influx who has worked on it in the past. It's likely it would also get no response, or a response of "yes, unmaintained". In either case, hopefully that would entice someone else in the Go community to take on maintenance of a fork.

catap · 2024-05-01T07:37:21Z

Unfortunately I don't have any suggestions here at the moment. Given we're winding down our own work on promtail and (to my knowledge) don't use the syslog target ourselves I don't really want to take on maintenance of this code within the Loki repo. It's likely better to ask the Alloy team to take on maintenance of the code either directly or via a fork, though I doubt that will happen anytime soon either. They have their own open issue tracking support for the same RFC: grafana/alloy#305

My only other suggestion would be opening an issue on the go-syslog repo itself asking if the repo is unmaintained and trying to get in contact with someone from Influx who has worked on it in the past. It's likely it would also get no response, or a response of "yes, unmaintained". In either case, hopefully that would entice someone else in the Go community to take on maintenance of a fork.

From another hand: do you see any objection to merge? If this code will be removed soon enough, when you decided to nuke promtail, why not keep it with this feature?

It doesn't change any existed code, just add a new one feature which should be enabled by hand.

Upstream project looks like to be forgetten to be archived, but this code increases use case of loki right now, that should attract new users for Loki.

cstyan · 2024-05-01T15:55:27Z

From another hand: do you see any objection to merge? If this code will be removed soon enough, when you decided to nuke promtail, why not keep it with this feature?

Yes, because we're not just getting rid of promtail. It's going to be maintained with security and bug fixes for (I'm just trying to make a reasonable guess here) a year or two if not more.

What we are not doing is supporting additional features. We could make an argument for this being a fix but the actual changes you're making to go-syslog would need to be present in the upstream repo or a fork that someone is committed to maintaining (which we would then need to switch to using instead of influx's repo).

catap · 2024-05-01T21:50:18Z

What we are not doing is supporting additional features. We could make an argument for this being a fix but the actual changes you're making to go-syslog would need to be present in the upstream repo or a fork that someone is committed to maintaining (which we would then need to switch to using instead of influx's repo).

I see your point and I haven't got any resources to promise you to support go-syslog for next couple of years.

Frankly speaking after xz-history I understand why you decided to go this way.

But, as I said before, I really think that this PR will benefite Lokie and Grafana ecosystem.

Maybe as better approach you may tag someone from Alloy team? I had quite a fast look to their code and it seems like a fork of promtail.

cstyan · 2024-05-01T22:44:32Z

I see your point and I haven't got any resources to promise you to support go-syslog for next couple of years.
But, as I said before, I really think that this PR will benefite Lokie and Grafana ecosystem.

I don't disagree.

Maybe as better approach you may tag someone from Alloy team?

We can ping them like this @grafana/grafana-alloy-maintainers, but we also had a discussion internally, the consensus was that we don't have the bandwidth to maintain go-syslog ourselves and finding another solution is low priority at the moment.

I had quite a fast look to their code and it seems like a fork of promtail.

It's a hard fork of some of promtails code, yes.

catap · 2024-05-03T15:21:58Z

Maybe as better approach you may tag someone from Alloy team?

We can ping them like this @grafana/grafana-alloy-maintainers, but we also had a discussion internally, the consensus was that we don't have the bandwidth to maintain go-syslog ourselves and finding another solution is low priority at the moment.

Seems that github included comma into name of group and decided to not ping them. So, here I'm to ping: @grafana/grafana-alloy-maintainers

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. Co-Authored-By: Paul Spooren <mail@aparcar.org> Signed-off-by: Kirill A. Korinsky <kirill@korins.ky>

catap · 2024-05-20T19:46:17Z

@cstyan here an update. @leodido had forked his go-syslog which he decided to continue support, see his response here: influxdata/go-syslog#56 (comment)

After that he had merged the PR, and here I'm back with reduced PR which:

switch go-syslog to an actual upstream (and the last release);
enable support of old syslog at promtail.

cstyan · 2024-05-20T21:22:15Z

@cstyan here an update. @leodido had forked his go-syslog which he decided to continue support, see his response here: influxdata/go-syslog#56 (comment)

That's great news 👍

leodido · 2024-05-21T08:33:52Z

Yup, I decided to keep maintaining it at https://github.com/leodido/go-syslog
With write access it's way more easy to maintain stuff :)
You can already go get github.com/leodido/go-syslog/v4 (v4.1.0 is the latest atm)

cstyan · 2024-05-21T22:15:21Z

clients/pkg/promtail/scrapeconfig/scrapeconfig.go

+	// IsRFC3164Message defines wether the log is formated as RFC3164
+	IsRFC3164Message bool `yaml:"is_rfc3164_message"`
+


Sorry @catap I thought I left this comment yesterday:

Is there a way we can just detect whether each message is RFC3164 or RFC5424 format, rather than adding another config option? Is it possible that a single target could output logs in both formats?

@cstyan nope, it is impossible to determine which format is used. Usually sender sends in supported format, or it may have a selector where someone may choose the format.

The best that I can do here is use handleMessageRFC3164 as fallback. If RFC5424 can't parse it, let use RFC3164 but it may lead to the case when RFC5424 parses it wrong way, so, I suggest to avoid that.

Okay that's fine.

Lets keep the logic as is then, except can we change the config option to be SyslogFormat which has multiple string possibilities, like one of RFC5425, RFC3164, another RFC, etc.? And then have a validation function when creating the loading the syslog config or creating the syslog target.

And it's default value can be RFC5424. That way we don't need to add another config field if at some point the future we add support for yet another format. it just has to be another possible option for the validation function.

We also need a reference for this in the syslog section of docs/sources/send-data/promtail/configuration.md

Here the catch: right now we have two options:

// UseRFC5424Message defines whether the full RFC5424 formatted syslog // message should be pushed to Loki UseRFC5424Message bool `yaml:"use_rfc5424_message"` // IsRFC3164Message defines wether the log is formated as RFC3164 IsRFC3164Message bool `yaml:"is_rfc3164_message"`

and I feel that this should be reworked as weel. To something like:

SyslogFormat: RFC5424 (default) or RFC3164;

UseFullSyslogMessage: default is false

and I feel that this should be reworked as weel. To something like:

SyslogFormat: RFC5424 (default) or RFC3164;
UseFullSyslogMessage: default is false

this makes sense to me, we would have to deprecate UseRFC5424Message for now and have the actual value for use syslog be an or of UseRFC5424 and UseFullSyslogFormat: false when SyslogFormat is RFC5424

I can present your PR to the team as a "we should do this as a bug fix, it also technically requires these config changes" and we should be able to get it merged

@cstyan I was wrong, RFC3164 hasn't got string method and implement it isn't something that seems easy. So, let keep it as is.

catap · 2024-05-22T22:24:01Z

@cstyan to make review easy, I've added all remarks as dedicated commit. Feel free to ask, if I need to squash it.

cstyan

No need to squash/force push. It's actually easier to review if you push new commits for new changes. We always squash when merging back into main anyways.

cstyan · 2024-05-22T23:01:52Z

clients/pkg/promtail/scrapeconfig/scrapeconfig.go

@@ -202,15 +211,19 @@ type SyslogTargetConfig struct {
 	// message should be pushed to Loki
 	UseRFC5424Message bool `yaml:"use_rfc5424_message"`


I think we wanted a second new config option PushFullSyslogMessage? we still need to keep this one as well, and mark it as deprecated

I can't do it, because RFC3164 message has lack of String() method. See implementation for RFC5424: https://github.com/leodido/go-syslog/blob/v4.1.0/rfc5424/builder.go#L9348-L9408

The issue that RFC3164 isn't well structured and quite flexible, and rebuild original message is quite a challenge.

Sorry to misslead you.

clients/pkg/promtail/scrapeconfig/scrapeconfig.go

cstyan · 2024-05-22T23:07:00Z

clients/pkg/promtail/targets/syslog/syslogtarget.go

@@ -225,7 +225,7 @@ func (t *SyslogTarget) handleMessageRFC3164(connLabels labels.Labels, msg syslog
 }

 func (t *SyslogTarget) handleMessage(connLabels labels.Labels, msg syslog.Message) {
-	if t.config.IsRFC3164Message {
+	if t.config.IsRFC3164Message() {


I think switching on the config.SyslogFormat would be cleaner

Here possible, but it is using in few more places, and it leads to == and need to export the contant name, or hardcoded strings...

So, I've decided to use trivial method which can be refactored.

Anyway, here only two RFC for Syslog, and I doubt that someone will make 3rd soon enough.

Co-authored-by: Callum Styan <callumstyan@gmail.com>

cstyan · 2024-05-23T00:19:49Z

okay, I think we're probably good to go

I'll take another look in the morning

catap · 2024-05-23T00:22:38Z

@cstyan may I ask you to re-run tests, this time with go fmt!

cstyan

just one last change needed imo

cstyan · 2024-05-23T23:00:23Z

clients/pkg/promtail/scrapeconfig/scrapeconfig.go

@@ -202,12 +211,19 @@ type SyslogTargetConfig struct {
 	// message should be pushed to Loki
 	UseRFC5424Message bool `yaml:"use_rfc5424_message"`

+	// Syslog format used at the target.


I think we should clarify the valid options here, and that rfc5424 is the default.

we could implement the default in the parser or like this:

loki/clients/pkg/promtail/targets/syslog/syslogtarget.go

Lines 233 to 238 in efdae3d

func (t *SyslogTarget) transportProtocol() string {

if t.config.ListenProtocol != "" {

return t.config.ListenProtocol

}

return defaultProtocol

}

It looks like we don't have a dedicated config parsing function on this path, so maybe in NewSyslogTarget would make the most sense to detect if SyslogFormat is empty string and if so set the default

catap · 2024-05-28T17:13:34Z

@cstyan ping

catap requested a review from a team as a code owner April 28, 2024 10:24

pull-request-size bot added the size/M label Apr 28, 2024

catap force-pushed the promtail-RFC3164 branch from daf14b5 to ed2956a Compare April 29, 2024 13:48

catap changed the title ~~Support of RFC3164 at promtail~~ feat(promtail): Support of RFC3164 aka BSD Syslog Apr 29, 2024

cstyan self-assigned this Apr 30, 2024

catap force-pushed the promtail-RFC3164 branch from ed2956a to 50bcbc2 Compare April 30, 2024 14:34

catap mentioned this pull request May 1, 2024

Error parsing syslog stream UDP grafana/alloy#305

Open

catap added 2 commits May 20, 2024 20:42

vendor: github.com/leodido/go-syslog

1a2ebf7

catap force-pushed the promtail-RFC3164 branch from 50bcbc2 to 0320106 Compare May 20, 2024 19:43

pull-request-size bot added size/L and removed size/M labels May 20, 2024

cstyan reviewed May 21, 2024

View reviewed changes

catap requested a review from cstyan May 21, 2024 22:35

Addressed review remarks

1426356

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label May 22, 2024

cstyan reviewed May 22, 2024

View reviewed changes

Update clients/pkg/promtail/scrapeconfig/scrapeconfig.go

10f1033

Co-authored-by: Callum Styan <callumstyan@gmail.com>

catap force-pushed the promtail-RFC3164 branch from 7b70a6a to 10f1033 Compare May 22, 2024 23:36

catap requested a review from cstyan May 22, 2024 23:37

go fmt

d6818a9

cstyan reviewed May 23, 2024

View reviewed changes

Addressed future review remarks

e64f8a9

catap requested a review from cstyan May 24, 2024 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(promtail): Support of RFC3164 aka BSD Syslog #12810

feat(promtail): Support of RFC3164 aka BSD Syslog #12810

catap commented Apr 28, 2024 •

edited

CLAassistant commented Apr 28, 2024 •

edited

cstyan commented Apr 30, 2024

catap commented Apr 30, 2024

catap commented Apr 30, 2024

cstyan commented Apr 30, 2024

catap commented May 1, 2024

cstyan commented May 1, 2024

catap commented May 1, 2024

cstyan commented May 1, 2024

catap commented May 3, 2024

catap commented May 20, 2024

cstyan commented May 20, 2024

leodido commented May 21, 2024

cstyan May 21, 2024

catap May 21, 2024

cstyan May 22, 2024

catap May 22, 2024

cstyan May 22, 2024

catap May 22, 2024

catap commented May 22, 2024

cstyan left a comment

cstyan May 22, 2024

catap May 22, 2024

cstyan May 22, 2024

catap May 22, 2024

cstyan commented May 23, 2024

catap commented May 23, 2024

cstyan left a comment

cstyan May 23, 2024

cstyan May 23, 2024

cstyan May 23, 2024

catap commented May 28, 2024

		// IsRFC3164Message defines wether the log is formated as RFC3164
		IsRFC3164Message bool `yaml:"is_rfc3164_message"`

		@@ -202,15 +211,19 @@ type SyslogTargetConfig struct {
		// message should be pushed to Loki
		UseRFC5424Message bool `yaml:"use_rfc5424_message"`

	func (t *SyslogTarget) transportProtocol() string {
	if t.config.ListenProtocol != "" {
	return t.config.ListenProtocol
	}
	return defaultProtocol
	}

feat(promtail): Support of RFC3164 aka BSD Syslog #12810

Are you sure you want to change the base?

feat(promtail): Support of RFC3164 aka BSD Syslog #12810

Conversation

catap commented Apr 28, 2024 • edited

CLAassistant commented Apr 28, 2024 • edited

cstyan commented Apr 30, 2024

catap commented Apr 30, 2024

catap commented Apr 30, 2024

cstyan commented Apr 30, 2024

catap commented May 1, 2024

cstyan commented May 1, 2024

catap commented May 1, 2024

cstyan commented May 1, 2024

catap commented May 3, 2024

catap commented May 20, 2024

cstyan commented May 20, 2024

leodido commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catap commented May 22, 2024

cstyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstyan commented May 23, 2024

catap commented May 23, 2024

cstyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catap commented May 28, 2024

catap commented Apr 28, 2024 •

edited

CLAassistant commented Apr 28, 2024 •

edited