Skip to content

Adjusting docker creation to include ports forwarding and docker network type #16248

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

azmy98
Copy link

@azmy98 azmy98 commented Aug 23, 2023

Why I did it

To support docker network configuration and support port forwarding from values in the manifest file

How I did it

update the docker_image_ctl.j2 file

How to verify it

In order to verify this feature, we need to have the complementary part in the sonic-utilities repo
and then need to install a docker image file with manifest that includes ports in it and network type as well using sonic-package-manager and validate that they are as expected using docker commands such as: 'docker ps' and 'docker inspect' commands

@azmy98 azmy98 requested a review from lguohan as a code owner August 23, 2023 09:13
@lguohan
Copy link
Collaborator

lguohan commented Sep 2, 2023

what feature requires this, can you explain the motivation?

@azmy98
Copy link
Author

azmy98 commented Sep 19, 2023

@lguohan

what feature requires this, can you explain the motivation?

the purpose of this change is to be able to support docker creation with specified network type (host, bridge) and the forwarding ports found in the docker manifest when installing docker image from sonic-package-manager. Please check the following PR: sonic-net/sonic-utilities#2952 to understand the whole picture. the manifest attributes will finally be exported to dictionary in docker_image_run_opt including the net type of the docker, so it will help us containerize the docker image and block it from accessing all the host network.

@@ -540,7 +540,9 @@ start() {
# TODO: Mellanox will remove the --tmpfs exception after SDK socket path changed in new SDK version
{%- endif %}
docker create {{docker_image_run_opt}} \
{%- if '--net' not in docker_image_run_opt %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if

This if will possibly override NET related feature in this file. Please unify the solution like docker_image_run_opt and stop using NET.

Copy link
Author

Choose a reason for hiding this comment

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

If a docker does not have network attribute, he will have a default value from this file. I used the if statement to make code backward compatible.

Copy link
Author

Choose a reason for hiding this comment

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

@qiluo-msft, can you please confirm?

@azmy98
Copy link
Author

azmy98 commented Nov 27, 2023

@qiluo-msft, there is a conflict also on this PR:

docker create {{docker_image_run_opt}} 
{%- if docker_container_name != "dhcp_server" %}
        --net=$NET \
{%- endif %}

can you please direct me to who wrote this new code:

{%- if docker_container_name != "dhcp_server" %}

since I want to know how to resolve this conflict because I think it is redundant if the --net is not in dhcp_server docker manifest.

Copy link

linux-foundation-easycla bot commented Feb 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@azmy98 azmy98 force-pushed the sonic_docker_image_ctl_update branch from 9b057a1 to 72d83e5 Compare August 12, 2024 09:44
@azmy98
Copy link
Author

azmy98 commented Sep 10, 2024

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-utilities

Copy link

No pipelines are associated with this pull request.

@azmy98
Copy link
Author

azmy98 commented Sep 10, 2024

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fastiuk
Copy link
Contributor

fastiuk commented Oct 8, 2024

@qiluo-msft , could you please merge it? It was approved, so now ready to be merged

@fastiuk
Copy link
Contributor

fastiuk commented Oct 14, 2024

@qiluo-msft , could you please approve this PR? It was approved

@fastiuk
Copy link
Contributor

fastiuk commented Oct 21, 2024

@qiluo-msft gentle reminder to merge this PR

1 similar comment
@fastiuk
Copy link
Contributor

fastiuk commented Jan 6, 2025

@qiluo-msft gentle reminder to merge this PR

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.

7 participants