Skip to content

Conversation

@dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Nov 22, 2023

This change adds more introductory information about container
networking to the networking overview page, moving it from the docker
run reference page.

@netlify
Copy link

netlify bot commented Nov 22, 2023

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 5557874
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/65782cac623b6500080764de
😎 Deploy Preview https://deploy-preview-18753--docsdocker.netlify.app/network
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the area/networking Relates to anything around networking label Nov 22, 2023
@dvdksn dvdksn marked this pull request as ready for review November 29, 2023 10:51
@dvdksn dvdksn requested a review from thaJeztah November 29, 2023 10:51
Comment on lines +87 to +88
$ docker run -d --name redis example/redis --bind 127.0.0.1
$ docker run --rm -it --network container:redis example/redis-cli -h 127.0.0.1
Copy link
Contributor Author

@dvdksn dvdksn Nov 29, 2023

Choose a reason for hiding this comment

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

followup: s/redis/nginx/, maybe (not only here but generally)

@dvdksn dvdksn requested a review from akerouanton November 29, 2023 11:01
Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

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

LGTM!

@dvdksn dvdksn requested a review from a team November 30, 2023 14:04
thaJeztah
thaJeztah previously approved these changes Dec 4, 2023
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

two comments/questions, but LGTM otherwise


| Driver | Description |
| :-------- | :----------------------------------------------------------------------- |
| `bridge` | The default network driver. |
Copy link
Member

Choose a reason for hiding this comment

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

Do we document the Windows drivers, such as nat, default, and transparent anywhere? Also see moby/moby#46447

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷🏻 :windows-fire-fine:

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's create a tracking ticket for this one somewhere. There's quite a gap in our docs in this area because Microsoft at the time decided to document these in their own "container" documentation only, and not to contribute those parts to our docs 😞 (and most of these are not defined in our code, but provided by the Windows API / sdk so we may not even be aware of all options).

@thaJeztah
Copy link
Member

@dvdksn noticed this one wasn't merged yet; perhaps have a quick peek at my comments 😄

This change adds more introductory information about container
networking to the networking overview page, moving it from the docker
run reference page.

Signed-off-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
Comment on lines +69 to +70
The following flags aren't supported for containers using the `container:`
networking mode:
Copy link
Member

Choose a reason for hiding this comment

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

Some / all of these also apply to host mode networking (in which case the container doesn't have a network-namespace, and no IP-address of its own).

We can look at that in a follow-up as well.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM; let's go with what we have now, and look at follow-ups.

@thaJeztah thaJeztah merged commit 9254944 into docker:main Dec 12, 2023
@dvdksn dvdksn deleted the mv-nw-overview branch December 12, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/networking Relates to anything around networking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants