syslog-ng icon indicating copy to clipboard operation
syslog-ng copied to clipboard

Add common proxy solution for all the supported transport methods that works transparently in the network() and the syslog() sources

Open alexb271 opened this issue 2 years ago • 20 comments

Resolves #4388

A new logtransport class for proxied socket has been implemented based on code from the proxied text server.

If everything is good then theoretically the new logtransport could allow deleting the proxied text server code in favor of configuring a regular text server with this new transport, however nothing has been deleted yet in this PR.

alexb271 avatar Jul 11 '23 14:07 alexb271

First of all, sorry for this taking this long. I find I have less than the ideal amount of time to do code level changes to syslog-ng these days.

This looks so MUCH better than the previous iteration, thanks for doing it.

One more ask: can you please also remove the entire old implementation as well? The check for transport(proxied-tcp) will shadow the old implementation anyway, so it's going to be just dead code. If anything wrong comes up with this implementation we would fix it instead of going back to the old one.

bazsi avatar Sep 07 '23 15:09 bazsi

First of all, sorry for this taking this long. I find I have less than the ideal amount of time to do code level changes to syslog-ng these days.

This looks so MUCH better than the previous iteration, thanks for doing it.

One more ask: can you please also remove the entire old implementation as well? The check for transport(proxied-tcp) will shadow the old implementation anyway, so it's going to be just dead code. If anything wrong comes up with this implementation we would fix it instead of going back to the old one.

Thank you for the review!

I have added commits to remove the proxied text server. It was used by the network driver originally, not the syslog driver, so I had to make a few tweaks in order to keep the network driver functioning correctly.

alexb271 avatar Sep 22 '23 18:09 alexb271

I am still doing the review, currently halfway through at commit 937bb62, but wanted to give some feedback already.

The overall big picture is nice and clean so far.

Thank you for your reply! Just to avoid any misunderstandings, I did not write the PROXY implementation, my PR is a refactor to move proxy processing into the logtransport layer so that it can be enabled for the syslog source driver. Because of this, I do not know what the magic numbers stand for. However, I can try to address smaller issues like adding typedefs and such.

alexb271 avatar Oct 09 '23 18:10 alexb271

Thanks for the clarification, I started the review by going through each commit and did not realize until now that it was a simple move (lib/logproto/logproto-proxied-text-server.c -> lib/transport/transport-proxied-socket.c). I will check the diff first and then go through the commits, and close my notes where I think it is not really relevant in this PR. Sorry for the confusion!

I see that the CI is failing due to the light test case test_pp_tls failing, maybe a followup is needed here as well? (I haven't checked it thoroughly yet, just guessing).

OverOrion avatar Oct 10 '23 07:10 OverOrion

I see that the CI is failing due to the light test case test_pp_tls failing, maybe a followup is needed here as well? (I haven't checked it thoroughly yet, just guessing).

I've tried to replicate the TLS functionality in the new logtransport as it was in the ProxiedTextServer. Unfortunately I don't have much experience with tls in syslog, so I would need some help fixing it if it's not working.

alexb271 avatar Oct 20 '23 09:10 alexb271

I see that the CI is failing due to the light test case test_pp_tls failing, maybe a followup is needed here as well? (I haven't checked it thoroughly yet, just guessing).

I've tried to replicate the TLS functionality in the new logtransport as it was in the ProxiedTextServer. Unfortunately I don't have much experience with tls in syslog, so I would need some help fixing it if it's not working.

@alexb271 would you still work on this or should/may I try to finish?

HofiOne avatar Nov 17 '23 08:11 HofiOne

I see that the CI is failing due to the light test case test_pp_tls failing, maybe a followup is needed here as well? (I haven't checked it thoroughly yet, just guessing).

I've tried to replicate the TLS functionality in the new logtransport as it was in the ProxiedTextServer. Unfortunately I don't have much experience with tls in syslog, so I would need some help fixing it if it's not working.

@alexb271 would you still work on this or should/may I try to finish?

I'm not actively troubleshooting this right now, so of course you would be welcome to investigate or finish it. If the solution turns out to be something very simple I've missed I can fix that too if required.

alexb271 avatar Nov 17 '23 08:11 alexb271

@alexb271 would you still work on this or should/may I try to finish?

I'm not actively troubleshooting this right now, so of course you would be welcome to investigate or finish it. If the solution turns out to be something very simple I've missed I can fix that too if required.

I do not know if it's possible correctly,(or what is the simplest solution here), but could you please give me rights to this fork (it seems github allows forking of a fork only if I do not yet have my own fork of the same origin), I do not want to move this branch or create a new PR later on, the best would be if I could just simply edit here anything

HofiOne avatar Nov 17 '23 12:11 HofiOne

@alexb271 would you still work on this or should/may I try to finish?

I'm not actively troubleshooting this right now, so of course you would be welcome to investigate or finish it. If the solution turns out to be something very simple I've missed I can fix that too if required.

I do not know if it's possible correctly,(or what is the simplest solution here), but could you please give me rights to this fork (it seems github allows forking of a fork only if I do not yet have my own fork of the same origin), I do not want to move this branch or create a new PR later on, the best would be if I could just simply edit here anything

The Allow edits by maintainers checkbox is already ticked on this PR, not sure what else would be needed for you to be able to add commits.

alexb271 avatar Nov 17 '23 14:11 alexb271

The Allow edits by maintainers checkbox is already ticked on this PR, not sure what else would be needed for you to be able to add commits.

Seems I need more rights, tried both https/ssh

Pushing to https://github.com/alexb271/syslog-ng.git remote: Permission to alexb271/syslog-ng.git denied to HofiOne. fatal: unable to access 'https://github.com/alexb271/syslog-ng.git/': The requested URL returned error: 403

and

Pushing to github.com:alexb271/syslog-ng.git [email protected]: Permission denied (publickey). fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists.

How do the settings look like for you here?

Screenshot 2023-11-21 at 10 19 40

HofiOne avatar Nov 21 '23 09:11 HofiOne

How do the settings look like for you here?

the invite worked, thanks for fixing the rights

HofiOne avatar Nov 21 '23 10:11 HofiOne

How do the settings look like for you here?

the invite worked, thanks for fixing the rights

I thought the 'allow edits' checkbox was enough, but apparently not. Glad the invite worked though, thanks for finishing this PR.

alexb271 avatar Nov 21 '23 10:11 alexb271

Build FAILURE

kira-syslogng avatar Nov 28 '23 17:11 kira-syslogng

Build FAILURE

kira-syslogng avatar Nov 29 '23 12:11 kira-syslogng

Build FAILURE

kira-syslogng avatar Nov 29 '23 12:11 kira-syslogng

Build FAILURE

kira-syslogng avatar Nov 29 '23 17:11 kira-syslogng

@OverOrion , @bazsi

I've added the first, draft version of how I'd like to solve this. I've rewritten the proxy handling part again, it is a kind of proxy class that is not inherited from LogTransport anymore.

Could you please double-check this solution prior I start to finalize the code, and fix/add the tests as well, just to avoid unnecessary later rounds?

Thank you in advance!

HofiOne avatar Nov 30 '23 15:11 HofiOne

Build FAILURE

kira-syslogng avatar Dec 04 '23 10:12 kira-syslogng

Build FAILURE

kira-syslogng avatar Dec 04 '23 11:12 kira-syslogng

Build FAILURE

kira-syslogng avatar Dec 04 '23 13:12 kira-syslogng

Build FAILURE

kira-syslogng avatar Jun 05 '24 10:06 kira-syslogng

Please also fix all the copyrights too

The copyright is incorrect in most of the files, it needs a separate run that corrects all of them, what more, something automatic if we want to follow the actual date in each year.

HofiOne avatar Jun 05 '24 10:06 HofiOne

Build FAILURE

kira-syslogng avatar Jun 05 '24 11:06 kira-syslogng

Build FAILURE

kira-syslogng avatar Jun 05 '24 20:06 kira-syslogng

@kira-syslogng test this please;

kovgeri01 avatar Jun 07 '24 07:06 kovgeri01