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

Enable other containers to join network namespace of the none network #3443

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

Conversation

Shubhranshu153
Copy link
Contributor

Fixes: Unable to attach to container network created with none

Description

There is a use case with pause containers, where other containers attaches to the pause container network. The pause container is launched with network none. This requires the pause container have a copy of hosts, hostname and resolv conf.
It also seems to share a net namespace, the containers must also share a user namespace.

The solution is to have a copy of the hosts/hostname and resolv.conf. In case of container network, add userns and netns both.

Want to confirm is this an acceptable solution and i can send out an PR for it.
Steps to reproduce the issue

Create a pause container with network none.
Create another container with --net container:

Describe the results you received and expected

It would display errors with resolv.conf not found and once those configs are added would see an error with sys fs.
Expected result is to be able to connect to the network of pause container.
What version of nerdctl are you using?

1.7.5
Are you using a variant of nerdctl? (e.g., Rancher Desktop)

None
Host information

lima vm (fedora image), but can be reproduced in any architecture.

@Shubhranshu153 Shubhranshu153 marked this pull request as ready for review September 20, 2024 18:03
@AkihiroSuda
Copy link
Member

Want to confirm is this an acceptable solution

Yes if it works with Docker.

Needs an integration test.

}

resolvConfPath := filepath.Join(stateDir, "resolv.conf")
copyFileContent("/etc/resolv.conf", resolvConfPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

That would make containers using none to now use the host resolv.conf, right?
This does not seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked with docker and they have it for none, personally i dont think it should be part of none, but if it is not attaching to a none network namespace (custom use cases) seems to throw error.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not look like they have it here...

docker run --net none debian cat /etc/resolv.conf

# Generated by Docker Engine.
# This file can be edited; Docker Engine will not make further changes once it
# has been modified.

nameserver 192.168.5.2
search .

# Based on host file: '/run/systemd/resolve/resolv.conf' (legacy)
# Overrides: []

vs.

cat /etc/resolv.conf

nameserver 127.0.0.53
options edns0 trust-ad
search .

Copy link
Contributor Author

@Shubhranshu153 Shubhranshu153 Sep 21, 2024

Choose a reason for hiding this comment

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

this is my output (docker 25)

docker run --network none alpine:latest cat /etc/resolv.conf 
<redacted>
nameserver 10.4.4.10
options timeout:1 attempts:2
 sudo docker run --network none alpine:latest cat /etc/hostname
9d25f3a65441

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be fair i tested in another system i got same as yours too, so not sure which one is correct, but for some system is taking from /run/systemd/resolv/resolv.conf?

Copy link
Contributor

Choose a reason for hiding this comment

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

It likely does not matter much, since there will be no network anyhow. Concern here is more about information leakage.
I would suggest we either leave it empty or mimic the behavior of cniNetworkManager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if information leak is a concern in network none, if one can ssh from host they have access to /etc/resolv.conf of the host anyway.

I like the idea of mimicing the behavior of cniNetworkManager, that seems to take into account adding host, dns etc.

while we are on the topic, if we use --network container:containerd id with

		nameServers   = m.netOpts.DNSServers
		searchDomains = m.netOpts.DNSSearchDomains
		dnsOptions    = m.netOpts.DNSResolvConfOptions

dont we want to add them to the resolv.conf, similarly if --network host with these options, not sure they are getting added

[shubhum@lima-finch nerdctl]$ sudo nerdctl run  --network host --hostname default --dns test --dns-search testname alpine:latest cat /etc/resolv.conf
# This is /run/systemd/resolve/stub-resolv.conf managed by man:systemd-resolved(8).
# Do not edit.
#
# This file might be symlinked as /etc/resolv.conf. If you're looking at
# /etc/resolv.conf and seeing this text, you have followed the symlink.
#
# This is a dynamic resolv.conf file for connecting local clients to the
# internal DNS stub resolver of systemd-resolved. This file lists all
# configured search domains.
#
# Run "resolvectl status" to see details about the uplink DNS servers
# currently in use.
#
# Third party programs should typically not access this file directly, but only
# through the symlink at /etc/resolv.conf. To manage man:resolv.conf(5) in a
# different way, replace this symlink by a static file or a different symlink.
#
# See man:systemd-resolved.service(8) for details about the supported modes of
# operation for /etc/resolv.conf.

nameserver 127.0.0.53
options edns0 trust-ad
search ant.amazon.com amazon.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this configs to some generic default values

@Shubhranshu153 Shubhranshu153 force-pushed the fix-network-none branch 2 times, most recently from e99fd1d to 29760dc Compare October 26, 2024 14:54
@apostasie
Copy link
Contributor

@Shubhranshu153 will be slow to review - travelling right now. Will do asap.

Copy link
Contributor

@apostasie apostasie left a comment

Choose a reason for hiding this comment

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

Left a note about WriteDefaultResolveConf.
Otherwise, these changes seem good and will bring us more in line with what docker does, so +1.

This needs tests - especially around your use case of another container using the none container net.

@@ -150,6 +150,11 @@ type noneNetworkManager struct {
client *containerd.Client
}

func WriteDefaultResolvConf(filePath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the existing resolvconf.Build.
It is not currently safe to use it (not atomic, nor locking), but it is better IMO than duplicating the problem.

}

resolvConfPath := filepath.Join(stateDir, "resolv.conf")
if err := WriteDefaultResolvConf(resolvConfPath); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

See note above.

@Shubhranshu153
Copy link
Contributor Author

will add tests.

@Shubhranshu153 Shubhranshu153 force-pushed the fix-network-none branch 6 times, most recently from 1a627e3 to e80e165 Compare October 30, 2024 05:40
@Shubhranshu153
Copy link
Contributor Author

Left a note about WriteDefaultResolveConf. Otherwise, these changes seem good and will bring us more in line with what docker does, so +1.

This needs tests - especially around your use case of another container using the none container net.

so overall behaviour wise it is like docker but the configs dont still match, which is probably fine as we dont expose host details if we add specifically the things we want to expose.

@Shubhranshu153
Copy link
Contributor Author

Shubhranshu153 commented Oct 30, 2024

@apostasie added the tests, for the configs i have made it docker non compat because of the explanation above.
PTAL.
Thanks.

@Shubhranshu153 Shubhranshu153 force-pushed the fix-network-none branch 2 times, most recently from a2a9820 to ebc6d87 Compare October 30, 2024 09:42
go.mod Outdated Show resolved Hide resolved
@apostasie
Copy link
Contributor

@apostasie added the tests, for the configs i have made it docker non compat because of the explanation above. PTAL. Thanks.

Thanks @Shubhranshu153

I left a few comments on the tests. I will give this a spin locally later today too, but otherwise looks good.

@Shubhranshu153 Shubhranshu153 force-pushed the fix-network-none branch 4 times, most recently from c64c4f7 to cdb4929 Compare October 30, 2024 19:21
@Shubhranshu153 Shubhranshu153 force-pushed the fix-network-none branch 2 times, most recently from e2dc511 to eba7437 Compare October 30, 2024 20:36
@Shubhranshu153
Copy link
Contributor Author

@apostasie PTAL
Not sure why FreeBSD is failing

@Shubhranshu153
Copy link
Contributor Author

@AkihiroSuda @apostasie
need to add some test but need some guidance here
In Docker you can add something like:

docker run --rm   --dns 8.8.8.8   --dns 8.8.4.4 --add-host somewhat.example.com:127.0.0.1   --dns-search example.com  --network none  -it ubuntu:latest

and you can get them added

dev-dsk-shubhum-2b-5b633f8c % docker run --rm   --dns 8.8.8.8   --dns 8.8.4.4 --add-host somewhat.example.com:127.0.0.1   --dns-search example.com  --network none  -it ubuntu:latest cat /etc/hosts
127.0.0.1       localhost
::1     localhost ip6-localhost ip6-loopback
fe00::0 ip6-localnet
ff00::0 ip6-mcastprefix
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters
127.0.0.1       somewhat.example.com

(24-10-31 0:01:58) <0> [~]  
dev-dsk-shubhum-2b-5b633f8c % docker run --rm   --dns 8.8.8.8   --dns 8.8.4.4 --add-host somewhat.example.com:127.0.0.1   --dns-search example.com  --network none  -it ubuntu:latest cat /etc/resolv.conf 
search example.com
nameserver 8.8.8.8
nameserver 8.8.4.4
options timeout:1 attempts:2

So added these functionalities to none network, want some guidance if am missing functionality wise, i reused the code from host network as it seems it just used values from netopts. But i plan to refactor it to a single function (cleanup wise)

@apostasie
Copy link
Contributor

Will have a look as time allows.
Thanks @Shubhranshu153

@Shubhranshu153
Copy link
Contributor Author

@AkihiroSuda @apostasie
now that nerdctl 2.0 is out want to check if i can get some feedback on the above proposal.
Idea is to support the flags and behaviour as close as possible to docker.
Thank You

@AkihiroSuda AkihiroSuda added this to the v2.0.2 milestone Dec 4, 2024
@@ -124,6 +124,7 @@ require (
github.com/sirupsen/logrus v1.9.3 // indirect
github.com/spaolacci/murmur3 v1.1.0 // indirect
github.com/stefanberger/go-pkcs11uri v0.0.0-20230803200340-78284954bff6 // indirect
github.com/stretchr/objx v0.5.2 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to run go mod tidy use the nerdctl test framework and fix a few things in code, just checking if my logic is correct to add the options DNS and host options for none network and probably need to be done for host network as well

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.

3 participants