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

Container net host remove #15176

Closed
wants to merge 9 commits into from
Closed

Container net host remove #15176

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b9bf960
SNMP container hardening
ycoheNvidia May 16, 2023
5ee5af7
Remove net flag in redis docker
andriydnvd Apr 13, 2023
8e537a1
Merge branch 'sonic-net:master' into container_net_host_remove
ycoheNvidia May 30, 2023
238a198
Merge branch 'master' into container_net_host_remove
ycoheNvidia Jul 31, 2023
5296455
Merge branch 'master' of github.com:ycoheNvidia/sonic-buildimage into…
ycoheNvidia Aug 14, 2023
621b54a
Removed one space in file header
ycoheNvidia Sep 11, 2023
9169746
Removed tabs and irrelevant lines
ycoheNvidia Sep 18, 2023
f568e64
Merge branch 'master' of github.com:ycoheNvidia/sonic-buildimage into…
ycoheNvidia Sep 28, 2023
10c37ae
Indentation
ycoheNvidia Oct 16, 2023
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
46 changes: 2 additions & 44 deletions dockers/docker-snmp/snmpd.conf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,52 +13,10 @@
# AGENT BEHAVIOUR
#

# Listen for connections on all ip addresses, including eth0, ipv4 lo for multi-asic platform
# Listen on managment and loopback0 ips for single asic platform
#
{% macro protocol(ip_addr) %}
{%- if ip_addr|ipv6 -%}
{{ 'udp6' }}
{%- else -%}
{{ 'udp' }}
{%- endif -%}
{% endmacro %}

{% if SNMP_AGENT_ADDRESS_CONFIG %}
{% for (agentip, port, vrf) in SNMP_AGENT_ADDRESS_CONFIG %}
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

SNMP_AGENT_ADDRESS_CONFIG

This feature is still useful, and there are some other PRs trying to extend it. #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 am sorry, I wasn't aware of the other PR's for this feature.
The logic of address/port/vrf was moved to the docker create port and address forwarding. Since only user defined ports and addresses will be forwarded, this logic will no longer be relevant here

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 have updated the code to include the latest merged PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain "The logic of address/port/vrf was moved to the docker create port and address forwarding"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current implementation, snmpd has its configuration in snmpd.conf which defines which addresses and ports can be used for query, how and if vrf is being used, etc.
Removing snmp container from host network means that it is not exposed to any of these addresses by default,
What we did here was to use docker address:port forwarding as the method to implement the logic for it. When a user configures address and port to be used for snmp queries - this address:port tuple will be used when snmp docker is created, similar to a firewall. In what we offer here, the snmp demon inside the container will listen to all packets, and the docker networking logic (address:port forwarding) will forward only the relevant packets, as configured by user

agentAddress {{ protocol(agentip) }}:[{{ agentip }}]{% if port %}:{{ port }}{% endif %}{% if vrf %}%{{ vrf }}{% endif %}{{ "" }}
{% endfor %}
{% elif NAMESPACE_COUNT is not defined or NAMESPACE_COUNT|int <= 1 %}
{% if MGMT_INTERFACE is defined %}
{% for intf, ip in MGMT_INTERFACE %}
{% set agentip = ip.split('/')[0]|lower %}
{% set zoneid = '' %}
# Use interface as zoneid for link local ipv6
{% if agentip.startswith('fe80') %}
{% set zoneid = '%' + intf %}
{% endif %}
agentAddress {{ protocol(agentip) }}:[{{ agentip }}{{ zoneid }}]:161
{% endfor %}
{% endif %}
{% if LOOPBACK_INTERFACE is defined %}
{% for lo in LOOPBACK_INTERFACE %}
{% if lo | length == 2 %}
{% set intf = lo[0] %}
{% set agentip = lo[1].split('/')[0]|lower %}
{% set zoneid = '' %}
# Use interface as zoneid for link local ipv6
{% if agentip.startswith('fe80') %}
{% set zoneid = '%' + intf %}
{% endif %}
agentAddress {{ protocol(agentip) }}:[{{ agentip }}{{ zoneid }}]:161
{% endif %}
{% endfor %}
{% endif %}
{% else %}
# Listen for connections on all ip addresses, including eth0, ipv4 and ipv6 lo
#
agentAddress udp:161
agentAddress udp6:161
{% endif %}

###############################################################################
#
# ACCESS CONTROL
Expand Down
47 changes: 46 additions & 1 deletion files/build_templates/docker_image_ctl.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,12 @@ start() {
{%- endif %}

if [ -z "$DEV" ]; then
NET="host"
{%- if docker_container_name == "snmp" %}
ycoheNvidia marked this conversation as resolved.
Show resolved Hide resolved
NET="bridge"
Copy link
Contributor

@yaqiangz yaqiangz Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I just tried image built by this PR yesterday, version: SONiC.master-15176.360116-6bb24c135. Looks like rsyslog inside snmp container cannot work well. I cannot find any syslog generated by snmp container (contains 'snmp#') in local host syslog file or our syslog server, and process containercfgd which is to dynamically generate rsyslog configuration file inside container doesn't startup. Not sure whether you have verified this, could you confirm that?
@Yarden-Z FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

@ycoheNvidia could check if change like this is needed. #4738

{%- else %}
NET="host"
{%- endif %}


# For Multi-ASIC platform we have to mount the redis paths for database instances running in different
# namespaces, into the single instance dockers like snmp, pmon on linux host. These global dockers
Expand Down Expand Up @@ -526,6 +531,8 @@ start() {
{%- if docker_container_name == "database" %}
NET="bridge"
DB_OPT=$DB_OPT" -v /var/run/redis$DEV:/var/run/redis:rw "
{%- elif docker_container_name == "snmp" %}
NET="bridge"
Copy link
Contributor

Choose a reason for hiding this comment

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

NET="bridge"

Checking best practice (https://docs.docker.com/network/network-tutorial-standalone/), default bridge network is not best choice for production. User-defined bridge networks is best choice for production. Can we use user-defined bridge network here?

Copy link

Choose a reason for hiding this comment

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

I agree, defining a user-defined bridge is the best case solution for this item.
Problem is - this FR wasn't scoped to define this user-bridge and therefor remained in the general bridge domain.
If such a bridge will be defined - then we can add it to the SNMP container and redis container (and any future container) as well.

{%- else %}
NET="container:database$DEV"
DB_OPT=""
Expand All @@ -541,10 +548,45 @@ start() {
{%- endif %}
{%- if sonic_asic_platform == "mellanox" %}
# TODO: Mellanox will remove the --tmpfs exception after SDK socket path changed in new SDK version
{%- endif %}
{%- if docker_container_name == "snmp" %}
# get snmp listening address and port list from redis
ycoheNvidia marked this conversation as resolved.
Show resolved Hide resolved
addr_port_values=$(python3 -c 'from swsscommon.swsscommon import ConfigDBConnector; cfg_db = ConfigDBConnector(); cfg_db.connect(wait_for_init=True, retry_on=True); [print(k[0] + "|" + k[1]) for k in cfg_db.get_keys("SNMP_AGENT_ADDRESS_CONFIG|*")]')
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this logic moved here instead of doing in snmpd.conf.j2 file?
What is the value add we get?
This is done during docker create. If we are just restarting snmp server and there is a change in the SNMP_AGENT_ADDRESS_CONFIG in config_db, will the new change get picked up if we just restart running docker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved here because port and address forwarding can only be done during docker create. What it means that in order to harden this container's network - each address/port change for this service will require docker removal and creation

Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR will kill the feature of SNMP_AGENT_ADDRESS_CONFIG/port. Do you want to implement the same feature here or in another PR?

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 am not sure I understand your question. This PR and sonic-net/sonic-snmpagent#281 moves the logic of snmp networking management from the container to the host. The host will forward the relevant packets only (SNMP_AGENT_ADDRESS_CONFIG for example) and the container will take all packets sent to port 161

NAMESPACE_COUNT=$NUM_ASIC
if [ -z $addr_port_values ]; then
if [ -z $NAMESPACE_COUNT ] || [ $NAMESPACE_COUNT -lt 2 ]; then
addr_port_values=$(python3 -c 'from swsscommon.swsscommon import ConfigDBConnector; cfg_db = ConfigDBConnector(); cfg_db.connect(wait_for_init=True, retry_on=True); [print(k[1].split("/")[0].lower() + "%" + k[0]) if len(k) == 2 and k[1].split('/')[0].lower().startswith("fe80") else print(k[1].split("/")[0].lower()) for k in cfg_db.get_keys("LOOPBACK_INTERFACE|*")+cfg_db.get_keys("MGMT_INTERFACE|*") if len(k) == 2]')
Copy link
Contributor

Choose a reason for hiding this comment

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

here currently only Loopback and Mgmt IP addresses are used.
There is SNMP_AGENT_ADDRESS_CONFIG table which can be configured with the IPs for snmpd to listen on.
This change does not include using that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuvarnaMeenakshi you either missed line 556 which addresses the specific SNMP_AGENT_ADDRESS_CONFIG table, or I miss something. I followed the logic that was found in snmpd.conf.j2 and translated it here

Copy link
Contributor

Choose a reason for hiding this comment

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

This is throwing a syntax error.
python3 -c 'from swsscommon.swsscommon import ConfigDBConnector; cfg_db = ConfigDBConnector(); cfg_db.connect(wait_for_init=True, retry_on=True); [print(k[1].split("/")[0].lower() + "%" + k[0]) if len(k) == 2 and k[1].split('/')[0].lower().startswith("fe80") else print(k[1].split("/")[0].lower()) for k in cfg_db.get_keys("LOOPBACK_INTERFACE|")+cfg_db.get_keys("MGMT_INTERFACE|") if len(k) == 2]'

Why do we need the "fe80" check?
We can keep the fe80 condition in the snmpd.conf.j2 file?

fi
fi
fwd_port_values=""
# Loop over each value in the list
while read -r value; do
# Split the value and port number (if any)
parts=(${value//|/ })
value="${parts[0]}"
port="${parts[1]:-}"
if [ -z $port ]; then
port=161
fi
# Check if the value is an IPv4 address
if [ "$fwd_port_values" != "" ]; then
fwd_port_values="$fwd_port_values"$'\n'
fi
if [[ $value =~ ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
# IPv4 address, use as is
fwd_port_values="$fwd_port_values$value:$port:161/udp"
else
# Not an IPv4 address, wrap in []
fwd_port_values="$fwd_port_values[$value]:$port:161/udp"
fi
done <<< "$addr_port_values"

{%- endif %}
docker create {{docker_image_run_opt}} \
{%- if docker_container_name != "dhcp_server" %}
--net=$NET \
{%- elif docker_container_name == "snmp" %}
$(echo "$fwd_port_values" | sed 's/^/-p /' | tr '\n' ' ') \
{%- endif %}
-e RUNTIME_OWNER=local \
--uts=host \{# W/A: this should be set per-docker, for those dockers which really need host's UTS namespace #}
Expand Down Expand Up @@ -660,6 +702,9 @@ stop() {
/usr/local/bin/container stop -t 60 $DOCKERNAME
{%- else %}
/usr/local/bin/container stop $DOCKERNAME
{%- if docker_container_name == "snmp" %}
docker rm $DOCKERNAME
Copy link
Collaborator

Choose a reason for hiding this comment

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

docker rm

Why remove docker container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docker container behavior requires that port forwarding is done during "docker run", and cannot be changed afterwards. Using port forwarding as a method of support net host remove will require removing docker stop, as it is called as part of service restart (docker stop + docker run).
An example for flow:

  1. snmp being configured -> snmp service established -> snmp docker created with "docker run" with relevant port forwarding arguments
  2. snmp configuration changes -> snmp service restart -> snmp old docker deleted + calling "docker run" with new port forwarding values

Copy link
Collaborator

Choose a reason for hiding this comment

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

If there is hot patches inside docker container, original behavior is that the patches will survive config reload or reboot. But this PR lose the capabilities.

Copy link

Choose a reason for hiding this comment

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

Are there real use-cases where a docker is being loaded with patches inside it?
There might also be some local files (such as temp config and such in every container), is this a real use-case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it is possible in production environment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still applicable.

{%- endif %}
{%- endif %}
}

Expand Down
Loading