Skip to content

Fix invalid output of syslog IPv6 servers#1933

Merged
qiluo-msft merged 5 commits intosonic-net:masterfrom
wen587:show_syslog
Dec 3, 2021
Merged

Fix invalid output of syslog IPv6 servers#1933
qiluo-msft merged 5 commits intosonic-net:masterfrom
wen587:show_syslog

Conversation

@wen587
Copy link
Copy Markdown
Contributor

@wen587 wen587 commented Nov 17, 2021

What I did

Modify the filter function of show runningconfiguration syslog

How I did it

Filter by ending "]" rather than split by ":"

How to verify it

add a syslog v6 server sudo config syslog add f587::1:1
run command show runningconfiguration syslog

Previous command output (if the output of a command-line utility has changed)

admin@vlab-01:~$ show runningconfiguration syslog
Syslog Servers
----------------
[10.0.0.5]
[10.0.0.6]
[f587

New command output (if the output of a command-line utility has changed)

admin@vlab-01:~$ show runningconfiguration syslog
Syslog Servers
----------------
[10.0.0.5]
[10.0.0.6]
[f587::1:1]

@wen587 wen587 requested review from qiluo-msft and tsvanduyn and removed request for tsvanduyn November 17, 2021 13:48
@wen587 wen587 marked this pull request as ready for review November 18, 2021 08:08
@wen587 wen587 requested a review from lolyu November 18, 2021 09:17
@qiluo-msft qiluo-msft requested a review from tsvanduyn November 19, 2021 05:15
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 25, 2021

This pull request introduces 1 alert when merging 7e4eb10 into e63f47e - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

elif re_ipv6.match(line):
server = re_ipv6.match(line).group(1)
else:
continue
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 30, 2021

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
Copy Markdown
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

To match below cases:
*.* @IPv4:port
*.* @[IPv4]:port
*.* @[IPv6]:port
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 30, 2021

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
Copy Markdown
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.

show/main.py Outdated
"""
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
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 30, 2021

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
Copy Markdown
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.

@qiluo-msft qiluo-msft merged commit 5172972 into sonic-net:master Dec 3, 2021
@@ -1356,16 +1356,32 @@ def show_run_snmp(db, ctx):
@runningconfiguration.command()
@click.option('--verbose', is_flag=True, help="Enable verbose output")
def syslog(verbose):
Copy link
Copy Markdown
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?

abdosi pushed a commit that referenced this pull request Dec 8, 2021
#### What I did
Modify the filter function of `show runningconfiguration syslog`
#### How I did it
Filter by ending "]" rather than split by ":"
#### How to verify it
add a syslog v6 server `sudo config syslog add f587::1:1`
run command `show runningconfiguration syslog`
#### Previous command output (if the output of a command-line utility has changed)
```
admin@vlab-01:~$ show runningconfiguration syslog
Syslog Servers
----------------
[10.0.0.5]
[10.0.0.6]
[f587
```
#### New command output (if the output of a command-line utility has changed)
```
admin@vlab-01:~$ show runningconfiguration syslog
Syslog Servers
----------------
[10.0.0.5]
[10.0.0.6]
[f587::1:1]
```
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.

2 participants