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.

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

ssh and snmp allow list #1363

Merged
merged 2 commits into from
Feb 9, 2018
Merged

ssh and snmp allow list #1363

merged 2 commits into from
Feb 9, 2018

Conversation

byu343
Copy link
Contributor

@byu343 byu343 commented Feb 2, 2018

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

autorestart=true
stdout_logfile=syslog
stderr_logfile=syslog

Copy link
Contributor

Choose a reason for hiding this comment

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

Since snmpd-config-updater will attempt to restart the SNMP-related processes, this section should be moved to the bottom of the file, after snmp and snmp-subagent, and please change the block as follows:

[program:snmpd-config-updater]
command=/usr/bin/snmpd-config-updater
priority=5
autostart=false
autorestart=false
startsecs=0
stdout_logfile=syslog
stderr_logfile=syslog

You will also need to add the following line to the end of dockers/docker-snmp-sv2/start.sh to start the process at the appropriate time, after snmp and snmp-subagent have been started:

supervisorctl start snmpd-config-updater

Copy link

Choose a reason for hiding this comment

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

Any reason to set autorestart to false (current script simply exits in case of connection error with redis).
Maybe the config-updater should actually run first, so snmpd does not need to be restarted needlessly. Which means that config-updater should check if snmpd is running before restarting it.
However, "service snmpd reload" is just sending SIGHUP to snmpd, which will not restart snmpd, just cause it to re-read its config file, so it is pretty graceful (see below that the pid of snmpd has not changed):

root@sonic:/# vi /sbin/start-stop-daemon
root@sonic:/# ps -ef | grep snmpd
Debian-+ 28 1 0 22:47 ? 00:00:00 /usr/sbin/snmpd -f -LS4d -u Debian-snmp -g Debian-snmp -I -smux mteTrigger mteTriggerConf ifTable ifXTable inetCidrRouteTable ipCidrRouteTable ip disk_hw -p /run/snmpd.pid
root 43 32 0 22:56 ? 00:00:00 grep snmpd
root@sonic:/# service snmpd reload
[....] Reloading SNMP services:: snmpdroot@sonic:/#
root@sonic:/# ps -ef | grep snmpd
Debian-+ 28 1 0 22:47 ? 00:00:00 /usr/sbin/snmpd -f -LS4d -u Debian-snmp -g Debian-snmp -I -smux mteTrigger mteTriggerConf ifTable ifXTable inetCidrRouteTable ipCidrRouteTable ip disk_hw -p /run/snmpd.pid
root 52 32 0 22:56 ? 00:00:00 grep snmpd
root@sonic:/#

So I would argue that the "reload" option is better than a "stop then restart" one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to set autorestart to false?.

Since this solution is only to be run on Arista platforms, I was envisioning the config-updater exiting immediately if it determines the platform is not an Arista platform. In this case, setting autorestart=true would cause supervisor to continually and needlessly restart config-updater only to have it exit immediately again.

However, if we perform the platform check in the start.sh script and only ever call supervisorctl start snmpd-config-updater if it is an Arista platform, then we could set autorestart=true.

That said, since the sshd config-updater runs in the base image, not in a Docker container, it is started by systemd, so it requires a different solution, which I envisioned as mentioned above, where it exits immediately if it determines the platform is not an Arista platform. If we go that route, I think it makes sense to use the same method in both sshd and snmp config-updaters for consistency, which is how I arrived at the first solution I envisioned above.

current script simply exits in case of connection error with redis

Once this PR is merged, I will modify the config-updaters to communicate with the SONiC ConfigDB rather than using the direct Redis connection concept implemented here. My modifications should eliminate the situation you describe.

Maybe the config-updater should actually run first, so snmpd does not need to be restarted needlessly. Which means that config-updater should check if snmpd is running before restarting it.

I agree that snmpd should not be restarted needlessly. However, as mentioned above since the config-updater should only run on Arista platforms, it should not be responsible for starting snmpd or smnp-subagent.

However, "service snmpd reload" is just sending SIGHUP to snmpd, which will not restart snmpd, just cause it to re-read its config file, so it is pretty graceful

I agree that the SIGHUP signal is more graceful. However, we should not mix supervisor and systemd. What do you think about manually sending the SIGHUP signal instead of calling the restart_snmp_processes() function I suggested? Something like:

os.system("kill -HUP $(pgrep snmpd)")

Copy link

Choose a reason for hiding this comment

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

I see...
Yes, just sending the kill and bypassing systemd sounds good.
Still not clear on the autostart: I would think that if the config-updater runs first/early, then that closes the window where ACLs are not yet in place. In such a case we would rather do: os.system("kill -HUP $(pgrep snmpd) > /dev/null 2> /dev/null || :") since it could fail and failure is OK. Also, if the config-updater exits on finding it is not required, then the config-updater related changes become more local (not touching dockers/docker-snmp-sv2/start.sh).

Copy link
Contributor

Choose a reason for hiding this comment

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

Still not clear on the autostart: I would think that if the config-updater runs first/early, then that closes the window where ACLs are not yet in place. In such a case we would rather do: os.system("kill -HUP $(pgrep snmpd) > /dev/null 2> /dev/null || :") since it could fail and failure is OK. Also, if the config-updater exits on finding it is not required, then the config-updater related changes become more local (not touching dockers/docker-snmp-sv2/start.sh).

I agree that keeping the config-updater changes as local as possible is a good thing, as well as closing the window where ACLs are not yet in place. Now that we've agreed to send the SIGHUP directly instead of restarting the process, I'm OK with starting snmpd-config-updater early, sending the signal and ignoring failure upon startup.

With this solution, the timing with which snmpd-config-updater starts is irrelevant, therefore we could set autostart=true and not touch dockers/docker-snmp-sv2/start.sh. In this case we could also set autorestart=unexpected in order to have supervisor restart snmpd-config-updater only if it exits unexpectedly (i.e., if it exits immediately because it's running on a non-Arista platform, it will not get restarted, otherwise it will). That section would resemble the following:

[program:snmpd-config-updater]
command=/usr/bin/snmpd-config-updater
priority=1
autostart=true
autorestart=unexpected
startsecs=0
stdout_logfile=syslog
stderr_logfile=syslog

Copy link

Choose a reason for hiding this comment

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

autorestart=unexpected: that's cool and a good idea!

f.write("%s\n" % this_community)
f.close()
os.rename(filename_tmp, filename)
os.system("service snmpd reload") # TODO: check return code and log error somewhere
Copy link
Contributor

Choose a reason for hiding this comment

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

Inside Docker containers, we manage our processes using supervisor, not systemd. Therefore, You should replace this line with restart_snmp_processes() and add the following function:

def restart_snmp_processes():
    ret = os.system("supervisorctl stop snmp-subagent")
    if os.WEXITSTATUS(ret) != 0:
        log_warning("Failed to stop snmp-subagent")

    ret = os.system("supervisorctl stop snmpd")
    if os.WEXITSTATUS(ret) != 0:
        log_warning("Failed to stop snmpd")

    ret = os.system("supervisorctl start snmpd")
    if os.WEXITSTATUS(ret) != 0:
        log_error("Failed to start snmpd")

    ret = os.system("supervisorctl start snmp-subagent")
    if os.WEXITSTATUS(ret) != 0:
        log_error("Failed to start snmp-subagent")

@@ -0,0 +1,82 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

To align more closely with the current SONiC file structure, this file doesn't belong in src/. It is better suited to reside under files/image_config/. I recommend creating an ssh subdirectory there, and move this file to files/image_config/ssh/sshd-clear-denied-sessions.

@@ -0,0 +1,133 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

To align more closely with the current SONiC file structure, this file doesn't belong in src/. It is better suited to reside under files/image_config/. I recommend creating an ssh subdirectory there, and move this file to files/image_config/ssh/sshd-config-updater.

@@ -0,0 +1,14 @@
[Unit]
Copy link
Contributor

Choose a reason for hiding this comment

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

To align more closely with the current SONiC file structure, this file doesn't belong in src/. It is better suited to reside under files/image_config/. I recommend creating an ssh subdirectory there, and move this file to files/image_config/ssh/sshd-config-updater.service.

sudo cp src/config-file-updaters/sshd-clear-denied-sessions $FILESYSTEM_ROOT/usr/bin
sudo chmod +x $FILESYSTEM_ROOT/usr/bin/sshd-clear-denied-sessions
sudo cp src/libwrap/tcp-wrappers-7.6.q/tcpdmatch $FILESYSTEM_ROOT/usr/bin

Copy link
Contributor

Choose a reason for hiding this comment

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

This file will have to be modified to match the file relocations I requested below. It will need to change to:

# Service to update the sshd config file based on database changes
sudo cp $IMAGE_CONFIGS/ssh/sshd-config-updater.service $FILESYSTEM_ROOT/etc/systemd/system
sudo mkdir -p $FILESYSTEM_ROOT/etc/systemd/system/multi-user.target.wants
cd $FILESYSTEM_ROOT/etc/systemd/system/multi-user.target.wants/
sudo ln -s ../sshd-config-updater.service sshd-config-updater.service
cd -
sudo cp $IMAGE_CONFIGS/ssh/sshd-config-updater $FILESYSTEM_ROOT/usr/bin/
sudo chmod +x $FILESYSTEM_ROOT/usr/bin/sshd-config-updater
sudo cp $IMAGE_CONFIGS/ssh/sshd-clear-denied-sessions $FILESYSTEM_ROOT/usr/bin
sudo chmod +x $FILESYSTEM_ROOT/usr/bin/sshd-clear-denied-sessions
sudo cp src/libwrap/tcp-wrappers-7.6.q/tcpdmatch $FILESYSTEM_ROOT/usr/bin

@jleveque jleveque merged commit ecf5c8d into sonic-net:master Feb 9, 2018
renukamanavalan added a commit to renukamanavalan/sonic-buildimage that referenced this pull request Apr 26, 2021
* 9dba93f disk_check: Check & mount RO as RW using tmpfs (sonic-net#1569)
* c3963c5 Fix remove ip rif (sonic-net#1535)
* 41d8ddc [config][generic-update] Adding apply-patch, rollback, checkpoints commands (sonic-net#1536)
* a3d37f1 [console] Display success message after line cleared (sonic-net#1579)
* b10c157 RADIUS Management User Authentication Feature (sonic-net#1521)
* 59ed6f3 platform pre-check for reboot in master branch (sonic-net#1556)
* f5efe89 [acl] Use a list instead of a comma-separated string for ACL port list (sonic-net#1519)
* e296a69 No more IP validation as it is more likely a URL (sonic-net#1555)
* d5f5382 [CLI][queue counters] add JSON output option for queue counters (sonic-net#1505)
* 176cc4a 1) Loopback interfaces with valid nexthop IP are not ignored/treated as loopback. (sonic-net#1565)
* 149ccbd [techsupport] Update show ip interface command (sonic-net#1562)
* 0e84418 Stop PMON docker before cold and soft reboots (sonic-net#1514)
* eba5c04 Fix Multi-ASIC show specific resursive route by using common parsing function (sonic-net#1560)
* e57e7f7 cache the bvid to vlan translations (sonic-net#1523)
* 38f9f60 sonic-installer: fix py3 issues in bootloader.aboot (sonic-net#1553)
* 02b263a [voq/inbandif] Voq inbandif port (sonic-net#1363)
* 0539789 [load_minigraph]: Avoid starting PFCWD for EPMS devicetype (sonic-net#1552)
* 030293c Use 'importlib' module in lieu of deprecated 'imp' module (sonic-net#1450)
* 50e5c61 Fixed the possibility of using uninitialized variable in route_check.py (sonic-net#1551)
lguohan pushed a commit that referenced this pull request Apr 27, 2021
* 9dba93f disk_check: Check & mount RO as RW using tmpfs (#1569)
* c3963c5 Fix remove ip rif (#1535)
* 41d8ddc [config][generic-update] Adding apply-patch, rollback, checkpoints commands (#1536)
* a3d37f1 [console] Display success message after line cleared (#1579)
* b10c157 RADIUS Management User Authentication Feature (#1521)
* 59ed6f3 platform pre-check for reboot in master branch (#1556)
* f5efe89 [acl] Use a list instead of a comma-separated string for ACL port list (#1519)
* e296a69 No more IP validation as it is more likely a URL (#1555)
* d5f5382 [CLI][queue counters] add JSON output option for queue counters (#1505)
* 176cc4a 1) Loopback interfaces with valid nexthop IP are not ignored/treated as loopback. (#1565)
* 149ccbd [techsupport] Update show ip interface command (#1562)
* 0e84418 Stop PMON docker before cold and soft reboots (#1514)
* eba5c04 Fix Multi-ASIC show specific resursive route by using common parsing function (#1560)
* e57e7f7 cache the bvid to vlan translations (#1523)
* 38f9f60 sonic-installer: fix py3 issues in bootloader.aboot (#1553)
* 02b263a [voq/inbandif] Voq inbandif port (#1363)
* 0539789 [load_minigraph]: Avoid starting PFCWD for EPMS devicetype (#1552)
* 030293c Use 'importlib' module in lieu of deprecated 'imp' module (#1450)
* 50e5c61 Fixed the possibility of using uninitialized variable in route_check.py (#1551)
stepanblyschak pushed a commit to stepanblyschak/sonic-buildimage that referenced this pull request May 10, 2021
Inband port can be made available in PORT table. But regular port handlngs are
not applicable for Inband port. This PR has change to avoid regular port handling for inband port for route_check and sfpshow script.
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-buildimage that referenced this pull request May 23, 2021
* 9dba93f disk_check: Check & mount RO as RW using tmpfs (sonic-net#1569)
* c3963c5 Fix remove ip rif (sonic-net#1535)
* 41d8ddc [config][generic-update] Adding apply-patch, rollback, checkpoints commands (sonic-net#1536)
* a3d37f1 [console] Display success message after line cleared (sonic-net#1579)
* b10c157 RADIUS Management User Authentication Feature (sonic-net#1521)
* 59ed6f3 platform pre-check for reboot in master branch (sonic-net#1556)
* f5efe89 [acl] Use a list instead of a comma-separated string for ACL port list (sonic-net#1519)
* e296a69 No more IP validation as it is more likely a URL (sonic-net#1555)
* d5f5382 [CLI][queue counters] add JSON output option for queue counters (sonic-net#1505)
* 176cc4a 1) Loopback interfaces with valid nexthop IP are not ignored/treated as loopback. (sonic-net#1565)
* 149ccbd [techsupport] Update show ip interface command (sonic-net#1562)
* 0e84418 Stop PMON docker before cold and soft reboots (sonic-net#1514)
* eba5c04 Fix Multi-ASIC show specific resursive route by using common parsing function (sonic-net#1560)
* e57e7f7 cache the bvid to vlan translations (sonic-net#1523)
* 38f9f60 sonic-installer: fix py3 issues in bootloader.aboot (sonic-net#1553)
* 02b263a [voq/inbandif] Voq inbandif port (sonic-net#1363)
* 0539789 [load_minigraph]: Avoid starting PFCWD for EPMS devicetype (sonic-net#1552)
* 030293c Use 'importlib' module in lieu of deprecated 'imp' module (sonic-net#1450)
* 50e5c61 Fixed the possibility of using uninitialized variable in route_check.py (sonic-net#1551)
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
* 9dba93f disk_check: Check & mount RO as RW using tmpfs (sonic-net#1569)
* c3963c5 Fix remove ip rif (sonic-net#1535)
* 41d8ddc [config][generic-update] Adding apply-patch, rollback, checkpoints commands (sonic-net#1536)
* a3d37f1 [console] Display success message after line cleared (sonic-net#1579)
* b10c157 RADIUS Management User Authentication Feature (sonic-net#1521)
* 59ed6f3 platform pre-check for reboot in master branch (sonic-net#1556)
* f5efe89 [acl] Use a list instead of a comma-separated string for ACL port list (sonic-net#1519)
* e296a69 No more IP validation as it is more likely a URL (sonic-net#1555)
* d5f5382 [CLI][queue counters] add JSON output option for queue counters (sonic-net#1505)
* 176cc4a 1) Loopback interfaces with valid nexthop IP are not ignored/treated as loopback. (sonic-net#1565)
* 149ccbd [techsupport] Update show ip interface command (sonic-net#1562)
* 0e84418 Stop PMON docker before cold and soft reboots (sonic-net#1514)
* eba5c04 Fix Multi-ASIC show specific resursive route by using common parsing function (sonic-net#1560)
* e57e7f7 cache the bvid to vlan translations (sonic-net#1523)
* 38f9f60 sonic-installer: fix py3 issues in bootloader.aboot (sonic-net#1553)
* 02b263a [voq/inbandif] Voq inbandif port (sonic-net#1363)
* 0539789 [load_minigraph]: Avoid starting PFCWD for EPMS devicetype (sonic-net#1552)
* 030293c Use 'importlib' module in lieu of deprecated 'imp' module (sonic-net#1450)
* 50e5c61 Fixed the possibility of using uninitialized variable in route_check.py (sonic-net#1551)
mssonicbld added a commit that referenced this pull request May 26, 2024
…tically (#19074)

#### Why I did it
src/sonic-sairedis
```
* c3ad5ce - (HEAD -> master, origin/master, origin/HEAD) [saiproxy] Add SAI proxy library (#1363) (10 hours ago) [Kamil Cudnik]
* d4a085f - Skip ASIC_TEMPERATURE attributes from sairedis recording (#1375) (19 hours ago) [saksarav-nokia]
* 456780e - [nvidia] update config_syncd_nvidia_bluefield() (#1380) (22 hours ago) [Yakiv Huryk]
* 79b27c0 - Add check for global SAI api used (#1379) (30 hours ago) [Kamil Cudnik]
* 0adbe25 - (origin/202405) [submodule] Update SAI submodule to v1.14 (#1381) (2 days ago) [Kamil Cudnik]
```
#### How I did it
#### How to verify it
#### Description for the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants