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

[docker_image_ctl.j2] Share UTS namespace with host OS (#4169) #4219

Closed
wants to merge 1 commit into from

Conversation

qiluo-msft
Copy link
Collaborator

@qiluo-msft qiluo-msft commented Mar 4, 2020

  • TODO: after merged this PR, we need to cherry-pick below into 201811
41ae7a21 2020-02-29 | [snmp] remove hostname change as it share uts namespace with host (#4206) [Stepan Blyshchak]

Instead of updating hostname manualy on Config DB hostname change,
simply share containers UTS namespace with host OS.
Ideally, instead of setting --uts=host for every container in SONiC,
this setting can be set per container if feature requires.
One behaviour change is introduced in this commit, when --privileged
or --cap-add=CAP_SYS_ADMIN and --uts=host are combined, container
has privilege to change host OS and every other container hostname.
Such privilege should be fixed by limiting containers capabilities.

Signed-off-by: Stepan Blyschak stepanb@mellanox.com

- 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)

Instead of updating hostname manualy on Config DB hostname change,
simply share containers UTS namespace with host OS.
Ideally, instead of setting `--uts=host` for every container in SONiC,
this setting can be set per container if feature requires.
One behaviour change is introduced in this commit, when `--privileged`
or `--cap-add=CAP_SYS_ADMIN` and `--uts=host` are combined, container
has privilege to change host OS and every other container hostname.
Such privilege should be fixed by limiting containers capabilities.

Signed-off-by: Stepan Blyschak <stepanb@mellanox.com>
@@ -144,6 +144,7 @@ start() {
# TODO: Mellanox will remove the --tmpfs exception after SDK socket path changed in new SDK version
{%- endif %}
docker create {{docker_image_run_opt}} \
--uts=host \{# W/A: this should be set per-docker, for those dockers which really need host's UTS namespace #}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you think of an instance where we would not want to share the host's UTS namespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cannot 😄
This is a modified cherry-pick following master branch.


In reply to: 387377484 [](ancestors = 387377484)

Copy link
Contributor

Choose a reason for hiding this comment

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

😄 Then maybe add a "maybe" to the beginning of the comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jleveque @qiluo-msft I think swss, syncd, pmon don't need to care about the hostname, if they do IMO it looks like a design mistake. The idea behind docker containers is isolation, why do we need docker containers at all if we want to share and map inside container almost everything?
Besides, --priviledge + --uts=host will give container priviledges to change host OS hostname.
The only containers I know which need host's hostname inside container are snmp and dhcp_relay but it doesn't look like they need --priviledge option or do they?
I would like to keep this comment and revise containers isolation in sonic in general in the future.

@lguohan
Copy link
Collaborator

lguohan commented Mar 4, 2020

why do we need this in 201811?

@qiluo-msft
Copy link
Collaborator Author

Closed since we already fixed it in #4241 and this PR is not needed.

@qiluo-msft qiluo-msft closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants