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

Jump to bottom

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 3 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
2 changes: 2 additions & 0 deletions dockers/docker-database/Dockerfile.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ RUN apt-get clean -y && \
s/^# unixsocket/unixsocket/; \
s/redis-server.sock/redis.sock/g; \
s/^client-output-buffer-limit pubsub [0-9]+mb [0-9]+mb [0-9]+/client-output-buffer-limit pubsub 0 0 0/; \
s/^bind 127.0.0.1 ::1$/# bind 127.0.0.1 ::1/; \
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.

bind 127.0.0.1 ::1

It will bind to all the interfaces visibile to database container. To be super cautious, could you add a sonic-mgmt testcase to ensure redis is not expose external port on sonic? #Closed

s/^protected-mode yes/protected-mode no/; \
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.

no

This is not safe, right? #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Redis protected mode only prevents the Redis server from responding to queries outside the loopback interfaces.
Docker network mode was changed from host to bridged. This requires to be able to connect from host IP to Redis docker IP.

s/^notify-keyspace-events ""$/notify-keyspace-events AKE/ \
' /etc/redis/redis.conf

Expand Down
2 changes: 1 addition & 1 deletion dockers/docker-database/supervisord.conf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ dependent_startup=true
{%- else -%}
{%- set LOOPBACK_IP = '' -%}
{%- endif -%}
command=/bin/bash -c "{ [[ -s /var/lib/{{ redis_inst }}/dump.rdb ]] || rm -f /var/lib/{{ redis_inst }}/dump.rdb; } && mkdir -p /var/lib/{{ redis_inst }} && exec /usr/bin/redis-server /etc/redis/redis.conf --bind {{ LOOPBACK_IP }} {{ redis_items['hostname'] }} --port {{ redis_items['port'] }} --unixsocket {{ redis_items['unix_socket_path'] }} --pidfile /var/run/redis/{{ redis_inst }}.pid --dir /var/lib/{{ redis_inst }}"
command=/bin/bash -c "{ [[ -s /var/lib/{{ redis_inst }}/dump.rdb ]] || rm -f /var/lib/{{ redis_inst }}/dump.rdb; } && mkdir -p /var/lib/{{ redis_inst }} && exec /usr/bin/redis-server /etc/redis/redis.conf --port {{ redis_items['port'] }} --unixsocket {{ redis_items['unix_socket_path'] }} --pidfile /var/run/redis/{{ redis_inst }}.pid --dir /var/lib/{{ redis_inst }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

In SONiC Chassis, the SWSS container on the linecard connect to local database as well as the database_chassis instance on supervisor. This info is got from database_global.json. Will this be possible after this change

snippet from database_global.json on linecard

INSTANCES": {
        "redis": {
            "hostname": "127.0.0.1",
            "port": 6379,
            "unix_socket_path": "/var/run/redis/redis.sock",
            "persistence_for_warm_boot": "yes"
        },
        "redis_chassis": {
            "hostname": "redis_chassis.server",
            "port": 6380,
            "unix_socket_path": "/var/run/redis-chassis/redis_chassis.sock",
            "persistence_for_warm_boot": "yes"
        }
    },

ycoheNvidia marked this conversation as resolved.
Show resolved Hide resolved
priority=2
autostart=false
autorestart=false
Expand Down
8 changes: 1 addition & 7 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,16 +13,10 @@
# AGENT BEHAVIOUR
#

# Listen for connections on all ip addresses, including eth0, ipv4 lo
# Listen for connections on all ip addresses, including eth0, ipv4 and ipv6 lo
ycoheNvidia marked this conversation as resolved.
Show resolved Hide resolved
#
{% 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 {{ agentip }}{% if port %}:{{ port }}{% endif %}{% if vrf %}%{{ vrf }}{% endif %}{{ "" }}
{% endfor %}
{% else %}
agentAddress udp:161
agentAddress udp6:161
{% endif %}

###############################################################################
#
Expand Down
57 changes: 56 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 @@ -483,7 +483,15 @@ start() {
{%- endif %}

if [ -z "$DEV" ]; then
NET="host"
{%- if docker_container_name == "database" %}
NET="bridge"
PORT_MAP="-p 127.0.0.1:6379:6379"
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.

6379

multi-DB usage may require more ports. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

If you mean multi-asic case. Its working. Each asic db docker has its own IP with mapped 6379 listening port and shared redis.sock socket file.
If there are more use cases, that may require different ports please elaborate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You may need to fetch the ports for all redis instances from conf files: "/var/run/redis/sonic-db/database_config.json".

With multi-DB feature, there will be multiple-namespace, and multiple instance per namespace. ref: https://github.com/sonic-net/SONiC/blob/master/doc/database/multi_namespace_db_instances.md

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, checking

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

Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

mix tabs and spaces. #Closed

{%- elif docker_container_name == "snmp" %}
NET="bridge"
{%- 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 @@ -523,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 @@ -538,9 +548,51 @@ 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 [ -z "$DEV" ]; then
pmon_sdk_sock="SX_API_SOCKET_FILE=/var/run/sx_sdk/sx_api.sock"
syncd_flag="-v"
syncd_var="/dev/shm:/dev/shm:rw"
ipc_var=""
else
pmon_sdk_sock="SX_API_SOCKET_FILE=/var/run/sx_sdk/sx_api_0.sock"
syncd_flag="--device"
syncd_var="/dev/sxdevs/sxcdev$(( $DEV + 1 )):/dev/sxdevs/sxcdev"
ipc_var="private"
fi
{%- 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


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]:-}"

# 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}} \
--net=$NET \
{%- if docker_container_name == "database" %}
$PORT_MAP \
{%- 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 #}
{%- if install_debug_image == "y" %}
Expand Down Expand Up @@ -654,6 +706,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