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

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

stepanblyschak
Copy link
Collaborator

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

admin@arc-switch1004:~$ hostname
arc-switch1004
admin@arc-switch1004:~$ docker exec -it swss hostname
arc-switch1004
admin@arc-switch1004:~$ sudo hostname sonic
admin@arc-switch1004:~$ hostname
sonic
admin@arc-switch1004:~$ docker exec -it swss hostname
sonic

- 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>
Copy link
Collaborator

@mykolaf mykolaf left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution!

@jleveque
Copy link
Contributor

Retest vsimage please

@jleveque
Copy link
Contributor

Thanks, @stepanblyschak!

I think we could also safely cherry-pick this into the 201911 branch. Any objections?

@stepanblyschak
Copy link
Collaborator Author

@jleveque Sure, it should be in 201911

@mykolaf
Copy link
Collaborator

mykolaf commented Feb 20, 2020

retest vsimage please

@jleveque
Copy link
Contributor

Retest vsimage please

@stepanblyschak
Copy link
Collaborator Author

retest vsimage please

1 similar comment
@lguohan
Copy link
Collaborator

lguohan commented Feb 25, 2020

retest vsimage please

@liat-grozovik liat-grozovik merged commit 1ef7403 into sonic-net:master Feb 26, 2020
stepanblyschak pushed a commit to stepanblyschak/sonic-buildimage that referenced this pull request Feb 26, 2020
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>

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

Conflicts:
	files/build_templates/docker_image_ctl.j2
lguohan added a commit that referenced this pull request Feb 28, 2020
@qiluo-msft
Copy link
Collaborator

Should we also cherry-pick to 201811 branch? Are you aware of any dependencies, such as docker engine's version?

@stepanblyschak
Copy link
Collaborator Author

@qiluo-msft docker engine in 201811 supports this feature. Why do we need to cherry-pick this PR into 201811? The idea behind this PR was to replace updateHostName functionality with simpler approach, but 201811 does not update hostname inside containers.

qiluo-msft pushed a commit to qiluo-msft/sonic-buildimage that referenced this pull request Mar 4, 2020
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>
@qiluo-msft
Copy link
Collaborator

I notice this PR could not directly cherry-pick, so submit a new one #4219.
The --uts feature is useful to keep host/container hostnames in sync.

@rlhui
Copy link
Contributor

rlhui commented Mar 15, 2020

@jleveque , based on Qi's last comment, is this PR still needed in 201911? If so, please remove the label and please confirm. Thanks.

@jleveque
Copy link
Contributor

@rlhui: Qi's comment is regarding the 201811 branch. This PR still needs to be cherry-picked into 201911.

rlhui pushed a commit that referenced this pull request Mar 23, 2020
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>
tiantianlv pushed a commit to SONIC-DEV/sonic-buildimage that referenced this pull request Apr 24, 2020
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>
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.

10 participants