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.

Sign up for GitHub

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

Fix invalid output of syslog IPv6 servers #1933

Merged
merged 5 commits into from
Dec 3, 2021
Merged

Fix invalid output of syslog IPv6 servers #1933

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac1f573
Fix invalid output of IPv6 syslog servers
wen587 Nov 17, 2021
7e4eb10
Utilize regex for exact match
wen587 Nov 25, 2021
ea7efa4
lgtm issue
wen587 Nov 26, 2021
f7d3f3a
1. make port optional; 2. cover tcp server format
wen587 Dec 3, 2021
8848651
Add match for IPv6 dual address
wen587 Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions show/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1356,16 +1356,29 @@ def show_run_snmp(db, ctx):
@runningconfiguration.command()
@click.option('--verbose', is_flag=True, help="Enable verbose output")
def syslog(verbose):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some unit tests?

"""Show Syslog running configuration"""
"""Show Syslog running configuration
To match below cases:
*.* @IPv4:port
*.* @[IPv4]:port
*.* @[IPv6]:port
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

port

The port is optional and the default is 514. ref: https://linux.die.net/man/5/rsyslog.conf #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make the port optional.

"""
syslog_servers = []
syslog_dict = {}
re_ipv4_1 = re.compile(r'^\*\.\* @(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}):\d+')
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

^*.* @(\d{1,3}.\d{1,3}.\d{1,3}.\d{1,3}):\d+

Thanks for being strict when writing a regex!
However since it is a show command, and the data is from a file in filesystem, should we be more relax and do not check for a valid ipv4/v6 address?

we can delegate the check to rsyslogd -N1. If anything wrong, we report an empty syslog running config. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Qi, according to rsyslog IPv6 support, I think we can just consider the cases of *.* @[IPv4]:port and *.* @[IPv6]:port which are all included in square bracket.
This way, I think the initial commit in this PR which checks ending ']' is enough.

re_ipv4_2 = re.compile(r'^\*\.\* @\[(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})\]:\d+')
re_ipv6 = re.compile(r'^\*\.\* @\[([0-9a-fA-F:]+)\]:\d+')
with open("/etc/rsyslog.conf") as syslog_file:
data = syslog_file.readlines()
for line in data:
if line.startswith("*.* @"):
line = line.split(":")
server = line[0][5:]
syslog_servers.append(server)
if re_ipv4_1.match(line):
server = re_ipv4_1.match(line).group(1)
elif re_ipv4_2.match(line):
server = re_ipv4_2.match(line).group(1)
elif re_ipv6.match(line):
server = re_ipv6.match(line).group(1)
else:
continue
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

continue

Should we report error message on stderr instead of being silent? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. I am checking all lines in rsyslogd.conf.
Even if only check lines start with *.* @, it is possible we have tcp format server starts with *.* @@. It is not enabled in the rsyslogd.conf.
Seems current show run syslog command only care about UDP server

syslog_servers.append("[{}]".format(server))
syslog_dict['Syslog Servers'] = syslog_servers
print(tabulate(syslog_dict, headers=list(syslog_dict.keys()), tablefmt="simple", stralign='left', missingval=""))

Expand Down