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

http: add support for proxy #3253

Merged
merged 2 commits into from
May 4, 2020
Merged

Conversation

vatsal-encs
Copy link
Contributor

fixes #3232

@vatsal-encs vatsal-encs reopened this Apr 21, 2020
@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

2 similar comments
@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@MrAnno
Copy link
Collaborator

MrAnno commented Apr 21, 2020

@kira-syslogng ok to test

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@furiel
Copy link
Collaborator

furiel commented Apr 22, 2020

Thank you for your pull request. At first glance it looks good to me.

  • Please don't forget to free the proxy string.
  • Please add a news file.
  • I mentioned in the issue that it would be nice to have a warning if users use different proxy scheme than the url.

Unfortunately, I have a few things to do now, but i try to come back with a proper review as soon as possible. In the meantime, could you tell your experiences? It would help the review greatly.

  • How did you test the feature? What proxy implementation did you use, and how did you configure it?
  • Can two different http destinations have different proxys? Is it possible to have one http destination with proxy, and another without proxy?
  • The proxy setting in the grammar indeed stronger than the proxy environment variables?
  • What happens if we have only one http destination, but have two urls and two workers? Do both workers use the same proxy?
  • What happens to the messages if the proxy is not available? Does syslog-ng drop the messages?
  • What happens to the messages if the proxy is available, but the remote isn't?

bazsi
bazsi previously approved these changes Apr 23, 2020
Copy link
Collaborator

@bazsi bazsi left a comment

Choose a reason for hiding this comment

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

I think a news file entry with an example configuration would be pretty useful, perhaps with important details included from https://curl.haxx.se/libcurl/c/CURLOPT_PROXY.html.

Apart from that the code looks good to me.

furiel
furiel previously requested changes Apr 23, 2020
Copy link
Collaborator

@furiel furiel left a comment

Choose a reason for hiding this comment

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

After discussing with @bazsi , I was wrong about the scheme mismatch. I tried the code, and indeed with http url and https proxy, syslog-ng still tries to connect to the https proxy.

So the only thing left to do is to fix the memory leak, then this PR can go in.

modules/http/http.h Show resolved Hide resolved
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@furiel
Copy link
Collaborator

furiel commented Apr 27, 2020

Thank you. Finally, please add a news file:
https://github.com/syslog-ng/syslog-ng/blob/master/CONTRIBUTING.md#news-file

@furiel furiel dismissed their stale review April 27, 2020 05:41

memleak fixed

@vatsal-encs
Copy link
Contributor Author

Thank you. Finally, please add a news file:
https://github.com/syslog-ng/syslog-ng/blob/master/CONTRIBUTING.md#news-file

@furiel : Okay, I have added it.

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Signed-off-by: Vatsal Sisodiya <vatsal.encs@gmail.com>
Signed-off-by: Vatsal Sisodiya <vatsal.encs@gmail.com>
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@furiel
Copy link
Collaborator

furiel commented May 4, 2020

Thank you. The patchset looks good now.

Unfortunately pytest-linters is broken, and as I see it is a bug in virtualenv, that the maintainers are currently trying to address in pypa/virtualenv#1811. We can wait a little until it is resolved. Or we can just merge this as it is: python code has not been changed, so the linters should not find any problem.

@alltilla
Copy link
Collaborator

alltilla commented May 4, 2020

Merging, as travis fails for an environment error.

@alltilla alltilla merged commit 0845f82 into syslog-ng:master May 4, 2020
@vatsal-encs vatsal-encs deleted the http-proxy branch May 4, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for http/https proxy
6 participants