Skip to content

net=host: remove /var/run/docker/netns/default from OCI config#47101

Merged
thaJeztah merged 1 commit intomoby:masterfrom
AkihiroSuda:refactor-47100
Jan 18, 2024
Merged

net=host: remove /var/run/docker/netns/default from OCI config#47101
thaJeztah merged 1 commit intomoby:masterfrom
AkihiroSuda:refactor-47100

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda commented Jan 18, 2024 •
edited by thaJeztah
Loading

- What I did

Prior to this commit, a container running with --net=host had {"type":"network","path":"/var/run/docker/netns/default"} in the ``.linux.namespaces` field of the OCI Runtime Config, but this wasn't needed.

- How I did it
oci.RemoveNamespace(s, specs.NetworkNamespace) when networkMode.isHost()

- How to verify it
docker run --net=host still works, ip a in a container shows the host network interfaces, and the config.json has no reference to /var/run/docker/netns/default

- Description for the changelog

net=host: remove /var/run/docker/netns/default from OCI config

- A picture of a cute animal (not mandatory but encouraged)
🐧

@AkihiroSuda AkihiroSuda added area/networking Networking kind/refactor PR's that refactor, or clean-up code labels Jan 18, 2024
@AkihiroSuda AkihiroSuda mentioned this pull request Jan 18, 2024
Closed
@AkihiroSuda
net=host: remove /var/run/docker/netns/default from OCI config
ed15f1d 
Prior to this commit, a container running with `--net=host` had
`{"type":"network","path":"/var/run/docker/netns/default"}` in
the ``.linux.namespaces` field of the OCI Runtime Config,
but this wasn't needed.

Close issue 47100

Signed-off-by: Akihiro Suda <[email protected]>
thaJeztah
thaJeztah reviewed Jan 18, 2024
View reviewed changes
Comment thread daemon/oci_linux.go
case networkMode.IsHost():
setNamespace(s, specs.LinuxNamespace{
Type: specs.NetworkNamespace,
Path: c.NetworkSettings.SandboxKey,
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jan 18, 2024

Choose a reason for hiding this comment

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

Not for this PR, but we should probably look if that magic default SandboxKey is actually still serving any purpose, or if such containers should just not have a SandboxKey set.

(We may need to have a close look for Windows as well on that matter; perhaps there's some other rules there)

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 18, 2024

Looking at the code in this area; I wonder how we handle --network container:<some container> if that other container is using --network=host; this should probably either fail, or both containers should (implicitly) run with --network=host.

moby/daemon/oci_linux.go

Lines 269 to 277 in 4f9c865

case networkMode.IsContainer():
nc, err := daemon.getNetworkedContainer(c.ID, networkMode.ConnectedContainer())
if err != nil {
return err
}
setNamespace(s, specs.LinuxNamespace{
Type: specs.NetworkNamespace,
Path: fmt.Sprintf("/proc/%d/ns/net", nc.State.GetPID()),
})

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Jan 18, 2024 •
edited
Loading

Looking at the code in this area; I wonder how we handle --network container:<some container> if that other container is using --network=host; this should probably either fail, or both containers should (implicitly) run with --network=host.

The latter one seems correct (tested with Docker 24.0.7)

$ docker run --name=c0 -d --net=host alpine sleep infinity
d4545f1bb639a5f7410566c757527e39a4ee9bf07f1ea6cb12dec96ce0aaa7d6
$ docker run -it --rm --net=container:c0 alpine
/ # ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP qlen 1000
    link/ether 52:55:55:d5:a8:b8 brd ff:ff:ff:ff:ff:ff
    inet 192.168.5.15/24 brd 192.168.5.255 scope global dynamic eth0
       valid_lft 86309sec preferred_lft 86309sec
    inet6 fec0::5055:55ff:fed5:a8b8/64 scope site dynamic noprefixroute flags 100 
       valid_lft 86310sec preferred_lft 14310sec
    inet6 fe80::5055:55ff:fed5:a8b8/64 scope link 
       valid_lft forever preferred_lft forever
3: docker0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue state DOWN 
    link/ether 02:42:05:94:03:47 brd ff:ff:ff:ff:ff:ff
    inet 172.17.0.1/16 brd 172.17.255.255 scope global docker0
       valid_lft forever preferred_lft forever

This still continue to work with the current revision of the PR (ed15f1d)

thaJeztah
thaJeztah approved these changes Jan 18, 2024
View reviewed changes
Copy link
Copy Markdown
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.

It makes a lot of sense to me to not create a namespace for a container that doesn't have one, but would love to have some eyes from @akerouanton and @robmry to check if there's some thing I didn't consider.

akerouanton
akerouanton approved these changes Jan 18, 2024
View reviewed changes
Copy link
Copy Markdown
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.

I can't think of a use case where this "default" netns is needed. LGTM.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 18, 2024

@akerouanton this one good / safe to take for v25.0?

@akerouanton
Copy link
Copy Markdown
Member

akerouanton commented Jan 18, 2024

Yeah, I believe it's safe to release in v25.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 18, 2024

Thanks! Just double-checking in case I overlooked things 🤗

Let's bring this one in 👍

@thaJeztah thaJeztah merged commit 982f85b into moby:master Jan 18, 2024
@thaJeztah thaJeztah added this to the 25.0.0 milestone Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akerouanton akerouanton akerouanton approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/networking Networking kind/refactor PR's that refactor, or clean-up code process/cherry-pick/23.0 status/2-code-review

Projects

None yet

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

--net=host: remove /var/run/docker/netns/default from OCI Runtime Config

3 participants

@AkihiroSuda @thaJeztah @akerouanton