[containercfgd] Add containercfgd and syslog rate limit configuration support#12489
[containercfgd] Add containercfgd and syslog rate limit configuration support#12489saiarcot895 merged 7 commits intosonic-net:masterfrom
Conversation
Junchao-Mellanox
requested review from
lguohan,
qiluo-msft and
xumia
as code owners
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Check notice
Code scanning / CodeQL
Statement has no effect
Junchao-Mellanox
force-pushed
the
srl-cfgd
branch
from
4098f04 to
b6800d6
Compare
|
Build issue pending on PR #12510 |
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-buildimag |
|
/AzurePipelines run Azure.sonic-buildimag |
|
No pipelines are associated with this pull request. |
|
/AzurePipelines run Azure.sonic-buildimag |
|
No pipelines are associated with this pull request. |
|
/azprun Azure.sonic-buildimage |
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @saiarcot895, could you please kindly help review this? |
| output = run_command('sonic-cfggen -d -t /usr/share/sonic/templates/rsyslog-container.conf.j2 -a "{{\\"target_ip\\": \\"{}\\", \\"container_name\\": \\"{}\\" }}"'.format(self.target_ip, container_name)) | ||
| f.write(output) | ||
| run_command('cp {} {}'.format(self.TMP_SYSLOG_CONF_PATH, self.SYSLOG_CONF_PATH)) | ||
| run_command("supervisorctl restart rsyslogd") |
There was a problem hiding this comment.
Can these be split into individual entries (such as ["cp", self.TMP_SYSLOG_CONF_PATH, self.SYSLOG_CONF_PATH] and passed directly into subprocess.check_output instead of using shlex.split? That will prevent code injections from outside sources doing something that shouldn't be done.
There was a problem hiding this comment.
Hi @saiarcot895 , thanks for the comment. I suppose shlex.split will split cp /tmp/file /tmp/file_copy into ['cp', '/tmp/file', '/tmp/file_copy'], it also prevents code injections. And the benefit is that:
- The original command is a complete string which is more readable than a list
shlex.splitwill do the split for user automatically which prevent user from incorrectly splitting command.
So, I prefer to keep this. What do you say?
There was a problem hiding this comment.
That's true, because it's split into an array, the first token will always be the command that gets executed. However, if, hypothetically, self.TMP_SYSLOG_CONF_PATH was /tmp/file name, then shlex.split will split cp /tmp/file name /tmp/file_copy into ["cp", "/tmp/file", "name", "/tmp/file_copy"], which is incorrect.
I think considering we know what these arguments should be, just doing the splitting ourselves would be better. (It would also make the JSON argument a couple lines above this a bit easier to read).
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
… support (sonic-net#12489) * [containercfgd] Add containercfgd and syslog rate limit configuration support * Fix build issue * Fix checker issue * Fix review comment * Fix review comment * Update containercfgd.py
|
Cherry-pick PR to 202211: #13361 |
mssonicbld
added
Included in 202211 Branch
and removed
Approved for 202211 Branch
Created PR to 202211 Branch
labels
…guration support (sonic-net#12489)" This reverts commit 3b3837a.
10 participants
HLD: sonic-net/SONiC#1049
Why I did it
Add a containercfgd to handler container side CONFIG DB changes
How I did it
How to verify it
unit test
Which release branch to backport (provide reason below if selected)
Description for the changelog
Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)