Fixes support for syslog when salt is running as non-root user#68890
Fixes support for syslog when salt is running as non-root user#68890shadow38 wants to merge 3 commits intosaltstack:3006.xfrom
Conversation
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey. There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
|
@shadow38 please open an issue. Add a changelog for it and make sure we have test coverage. |
|
|
||
| parsed_log_path = urlparse(path) | ||
|
|
||
| if parsed_log_path.scheme in ("tcp", "udp", "file"): |
There was a problem hiding this comment.
Should unix:// also be supported?
There was a problem hiding this comment.
I see that this is probably based on the documentation at https://docs.saltproject.io/en/latest/ref/configuration/logging/index.html#std-conf_log-log_file which does not include unix://. It seems like unix should also be supported but probably out of scope of this if it doesn't just work by adding it here.
There was a problem hiding this comment.
The is_syslog_path function is an extract of the syslog path parser from saltstack core source code. If is massively based on the setup_logfile_handler function. I didn't revalidate the whole parsing assuming that the parsing was already correct.
| ) | ||
|
|
||
| if not is_writeable(logfile, check_parent=True): | ||
| if not is_syslog_path(logfile) and not is_writeable(logfile, check_parent=True): |
There was a problem hiding this comment.
The docstring of is_syslog_path says log-facility is optional but returns False if the prefix is tcp:///udp:// and the log-facility is not provided. In general I don't think we care that the file is specifically a "syslog" path. It seems like we should allow writing to any log specified with file://,tcp://,udp:// which would be more robust.
There was a problem hiding this comment.
According the last example in the documentation at https://docs.saltproject.io/en/latest/ref/configuration/logging/index.html#std-conf_log-log_file log_file: udp://loghost:10514 should also be supported and work.
There was a problem hiding this comment.
The docstring of
is_syslog_pathsayslog-facility is optionalbut returns False if the prefix istcp:///udp://and the log-facility is not provided. In general I don't think we care that the file is specifically a "syslog" path. It seems like we should allow writing to any log specified withfile://,tcp://,udp://which would be more robust.
If you think that checking that path starts with file, tcp or udp is correct, I can change my PR. Your proposal is the same as the initial PR related to this bug : https://github.com/saltstack/salt/pull/62263/changes . I just thought that parsing the syslog path was a "cleaner" way to manage this bug.
There was a problem hiding this comment.
According the last example in the documentation at https://docs.saltproject.io/en/latest/ref/configuration/logging/index.html#std-conf_log-log_file
log_file: udp://loghost:10514should also be supported and work.
I just performed some tests and udp://loghost:10514 is correctly parsed as a valid syslog path in the is_syslog_path function. I'll try to add some pytest in this PR
shadow38
force-pushed
the
syslog-support-3006
branch
2 times, most recently
from
7929cd2 to
33a57ec
Compare
shadow38
force-pushed
the
syslog-support-3006
branch
from
64a70f0 to
352f6ba
Compare
3 participants
What does this PR do?
Adds support for
file://scheme in syslog handler configuration.What issues does this PR fix or reference?
Closes #68801 (previous PR targeting master, reopened for 3006.x as requested by @dwoz)
Previous Commits
Cherry-picked from previous PR #68801 and rebased onto 3006.x