-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update RSyslog HLD with new functionality #1218
Conversation
|
doc/syslog/syslog-design.md
Outdated
# 1. SONiC Syslog Source IP | ||
|
||
## 1.1. High Level Design document | ||
|
||
## 1.2. Table of contents | ||
|
||
- [1. SONiC Syslog Source IP](#1-sonic-syslog-source-ip) | ||
- [1.1. High Level Design document](#11-high-level-design-document) | ||
- [1.2. Table of contents](#12-table-of-contents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to get rid of section numbers here and remove them from table of content.
You can use "<!-- omit in toc -->" directive. See here
doc/syslog/syslog-design.md
Outdated
@@ -503,12 +758,14 @@ module sonic-syslog { | |||
|
|||
leaf server_address { | |||
description "Syslog server IP address"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to update the description.
The server address represents either an IP address or a DNS domain name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
leaf format { | ||
description "Log format"; | ||
type log-format; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a default 'standard' field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@dharmaraj-gurusamy wants to be the reviewer. |
doc/syslog/syslog-design.md
Outdated
| 0.1 | 18/04/2022 | Nazarii Hnydyn | Initial version | | ||
|
||
## About this manual | ||
# 1. SONiC Syslog Source IP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original document has this as title & Introduction is marked as section 1. All section numbers seems incremented. Please correct the document headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
doc/syslog/syslog-design.md
Outdated
| Rev | Date | Author | Description | | ||
|:---:|:----------:|:--------------:|:----------------------| | ||
| 0.1 | 18/04/2022 | Nazarii Hnydyn | Initial version | | ||
| 0.2 | 08/01/2023 | Ido Avraham | extend capabilities | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add short note about the extended capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added detailed description
doc/syslog/syslog-design.md
Outdated
| Parameter | Default | Description | | ||
|:-------------------|:---------|:------------------------------------------------------------------------------| | ||
| format | standard | template format | | ||
| trap | notice | messages with severity equal or grater then this severity will be forwarded | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
severity will be a better name as trap may confuse with SNMP terminology. Please keep uniform name for both global & server specific configuration parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we defined this name in our internal usage so we would prefer to leave it like that.
is it acceptable by you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The man page (https://www.rsyslog.com/doc/v8-stable/configuration/filters.html) states priority / severity term. Hence, I would still recommend to rename either as severity / priority for better conveying the purpose.
doc/syslog/syslog-design.md
Outdated
| trap | priority | SYSLOG_CONFIG\|GLOBAL\|trap | | ||
| welf_firewall_name | trap | SYSLOG_CONFIG\|GLOBAL\|welf_firewall_name | | ||
|
||
* trap will set rules trap severity if not specified in the rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please share more details on this statement, preferably with an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@@ -444,7 +657,7 @@ config syslog add '2.2.2.2' \ | |||
config syslog del '2.2.2.2' | |||
``` | |||
|
|||
#### 2.6.2.2 Show command group | |||
#### 3.6.2.2. Show command group | |||
|
|||
**The following command shows syslog server configuration:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extended parameters shall be displayed in show command output also for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
doc/syslog/syslog-design.md
Outdated
or (/mvrf:sonic-mgmt_vrf/mvrf:MGMT_VRF_CONFIG/mvrf:vrf_global/mvrf:mgmtVrfEnabled = 'true')"; | ||
} | ||
|
||
leaf filter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter_type will be the better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will change
doc/syslog/syslog-design.md
Outdated
type rsyslog-protocol; | ||
} | ||
|
||
leaf trap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
severity will be the better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answered above
} | ||
|
||
leaf trap { | ||
type rsyslog-severity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
| device | none | Bind socket to given device (e.g., eth0/vrf0) | | ||
| ipfreebind | 2 | Manages the IP_FREEBIND option on the UDP socket | | ||
| filter | none | compares the log against the provided regular expression | | ||
| priority | notice | logs with specified priority and higher will be forwarded | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the default values be added in redis during upgrade scenarios (db_migrator script)? If not, we might end up in inconsistency after upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the keys are empty we will use the global default value in the template
doc/syslog/syslog-design.md
Outdated
| Parameter | Default | Description | | ||
|:-------------------|:---------|:------------------------------------------------------------------------------| | ||
| format | standard | template format | | ||
| trap | notice | messages with severity equal or grater then this severity will be forwarded | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The man page (https://www.rsyslog.com/doc/v8-stable/configuration/filters.html) states priority / severity term. Hence, I would still recommend to rename either as severity / priority for better conveying the purpose.
to merge the submodules, we first need to merge sonic-swss-common and then we can pass CI tests |
@iavraham any time line when feature can be approved and merged? Thank you! |
We answered/closed all comments in this PR. Now we are waiting for maintainers to approve and merge it. |
Will review before 5/25 |
|
||
Linux kernel decides which source IP to use within the default VRF. | ||
|
||
**Example:** | ||
``` | ||
*.* action(type="omfwd" target="2.2.2.2" protocol="udp") | ||
*.notice action(type="omfwd" target="2.2.2.2" protocol="udp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current 'show runningconfiguration syslog' is matching lines starting with '.'.
Will it break the show cli function? https://github.com/sonic-net/sonic-utilities/blob/master/show/main.py#L1687
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it break the show cli function?
It will. I fixed it here: sonic-net/sonic-utilities#2843
3b29864
to
cd54a88
Compare
e632798
to
f96a9f1
Compare
Adding the following functionality:
these are the implementation PRs for community review: