feat: resolve special ip host-gateway #1978

ningziwen · 2023-02-07T04:38:44Z

#1979

Tests and docs will be added when the issue got concensus.

Signed-off-by: Ziwen Ning [email protected]

pkg/config/config.go

AkihiroSuda · 2023-02-10T06:50:13Z

cmd/nerdctl/main.go

@@ -171,6 +171,7 @@ func initRootCmdFlags(rootCmd *cobra.Command, tomlPath string) (*pflag.FlagSet,
 	rootCmd.PersistentFlags().StringSlice("hosts-dir", cfg.HostsDir, "A directory that contains <HOST:PORT>/hosts.toml (containerd style) or <HOST:PORT>/{ca.cert, cert.pem, key.pem} (docker style)")
 	// Experimental enable experimental feature, see in https://github.com/containerd/nerdctl/blob/main/docs/experimental.md
 	AddPersistentBoolFlag(rootCmd, "experimental", nil, nil, cfg.Experimental, "NERDCTL_EXPERIMENTAL", "Control experimental: https://github.com/containerd/nerdctl/blob/main/docs/experimental.md")
+	rootCmd.PersistentFlags().String("host-gateway-ip", cfg.HostGatewayIP, "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the default bridge")


Any reason not to just use the host IP?

The default bridge IP isn't useful for rootless because the slirp is launched with --disable-host-loopback for the security reason

nerdctl/extras/rootless/containerd-rootless.sh

Line 123 in cffdf87

--disable-host-loopback --port-driver=$CONTAINERD_ROOTLESS_ROOTLESSKIT_PORT_DRIVER \

Thanks for the suggestion. I just tried to use host ip as default, as the commit I pushed.

I tested the built nerdctl in Lima. The returned host ip can't be used to access host (curl failed).

The static Lima guest IP (192.168.5.15) can be used to access host (curl succeeded). However it seems like a loopback IP which was skipped by the loopback check.

I tried in ubuntu linux host and it works. I think the host IP is VM has special setup. I will follow the route of making host IP the default as it should work for normal cases. VM users will need to use the config to set the their special IP.

I finally used env variable the pass the host IP to nsenter. Let me know if you have different thoughts.

djdongjin · 2023-02-20T01:44:22Z

pkg/testutil/testutil_linux.go

@@ -25,6 +25,7 @@ func mirrorOf(s string) string {

 var (
 	AlpineImage                 = mirrorOf("alpine:3.13")
+	AmazonLinux2Image           = "public.ecr.aws/amazonlinux/amazonlinux:2"


why is a new image needed here?

I need to use curl in tests but alpine doesn't have it and AL2 has it. Let me know if you have suggestions for better image choice.

I see. I'm okay with that. The nginx image should have curl, can you try that?

That works. Changed to it.

djdongjin · 2023-02-20T01:45:48Z

pkg/defaults/defaults_linux.go

@@ -130,3 +131,18 @@ func HostsDirs() []string {
 		filepath.Join(xch, "docker/certs.d"),
 	}
 }
+
+func HostGatewayIP() string {


can you add a comment here? since it's a new export non-test func.

djdongjin · 2023-02-20T01:49:28Z

cmd/nerdctl/container_run.go

+			gateway, err := cmd.Flags().GetString("host-gateway-ip")
+			if err != nil {
+				return nil, nil, err
+			}


host-gateway-ip has been parsed and passed via globalOptions right? If so you can use globalOptions.HostGatewayIP

djdongjin · 2023-02-20T01:54:47Z

cmd/nerdctl/flagutil.go

+	hostGatewayIP, err := cmd.Flags().GetString("host-gateway-ip")
+	if err != nil {
+		return types.GlobalCommandOptions{}, err
+	}


just to confirm, HostGatewayIP will never be empty right? Since the doc mentions it defauts to the IP address of the host.

From nerdctl external it can't be empty.

But it can be empty string "" when passing to here. When there was any issue getting host ip, it would be empty here. In container_run.go, it will return error when it is still "".

if globalOptions.HostGatewayIP == "" { return nil, nil, fmt.Errorf("unable to derive the IP value for host-gateway") }

djdongjin · 2023-02-20T14:15:15Z

cmd/nerdctl/container_run_linux_test.go

+func TestRunAddHostWithCustomHostGatewayIP(t *testing.T) {
+	// Not parallelizable (https://github.com/containerd/nerdctl/issues/1127)
+	base := testutil.NewBase(t)
+	testutil.DockerIncompatible(t)


why is it docker incompatible?

docker run --host-gateway-ip "192.168.5.2" --add-host=name:host-gateway -it run amazonlinux bash unknown flag: --host-gateway-ip See 'docker run --help'.

Docker has host-gateway-ip in config but not in global flag. From my understanding, there is a convention in nerdctl that transferring all the config to global flag first before using them. So the global flag is docker incompatible.

djdongjin · 2023-02-20T14:16:34Z

cmd/nerdctl/container_run_linux_test.go

+		var found bool
+		sc := bufio.NewScanner(bytes.NewBufferString(stdout))
+		for sc.Scan() {
+			//removing spaces and tabs separating items
+			line := strings.ReplaceAll(sc.Text(), " ", "")
+			line = strings.ReplaceAll(line, "\t", "")
+			if strings.Contains(line, "192.168.5.2test") {
+				found = true
+			}
+		}
+		if !found {
+			return errors.New("host was not added")
+		}
+		return nil


Does a .AssertOutContains("192.168.5.2test") work?

It should work but not that accurate as this, becuase 192.168.5.2test can also appear as a host alias instead of IP if the tested code is wrong. (Given tests can't assume tested code is correct)

djdongjin

LGTM, thanks

AkihiroSuda · 2023-02-21T16:35:08Z

cmd/nerdctl/main.go

@@ -171,6 +171,7 @@ func initRootCmdFlags(rootCmd *cobra.Command, tomlPath string) (*pflag.FlagSet,
 	rootCmd.PersistentFlags().StringSlice("hosts-dir", cfg.HostsDir, "A directory that contains <HOST:PORT>/hosts.toml (containerd style) or <HOST:PORT>/{ca.cert, cert.pem, key.pem} (docker style)")
 	// Experimental enable experimental feature, see in https://github.com/containerd/nerdctl/blob/main/docs/experimental.md
 	AddPersistentBoolFlag(rootCmd, "experimental", nil, nil, cfg.Experimental, "NERDCTL_EXPERIMENTAL", "Control experimental: https://github.com/containerd/nerdctl/blob/main/docs/experimental.md")
+	AddPersistentStringFlag(rootCmd, "host-gateway-ip", nil, nil, nil, aliasToBeInherited, cfg.HostGatewayIP, "HOST_GATEWAY_IP", "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the default bridge")


Does Docker use HOST_GATEWAY_IP?

Otherwise:

Suggested change

AddPersistentStringFlag(rootCmd, "host-gateway-ip", nil, nil, nil, aliasToBeInherited, cfg.HostGatewayIP, "HOST_GATEWAY_IP", "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the default bridge")

AddPersistentStringFlag(rootCmd, "host-gateway-ip", nil, nil, nil, aliasToBeInherited, cfg.HostGatewayIP, "NERDCTL_HOST_GATEWAY_IP", "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the default bridge")

Defaults to the IP address of the default bridge

Doesn't seem consistent with the actual implementation

fahedouch · 2023-02-22T15:52:38Z

cmd/nerdctl/container_run.go

@@ -57,7 +57,8 @@ import (
 	"github.com/containerd/nerdctl/pkg/referenceutil"
 	"github.com/containerd/nerdctl/pkg/strutil"
 	"github.com/containerd/nerdctl/pkg/taskutil"
-	dopts "github.com/docker/cli/opts"
+	dockercliopts "github.com/docker/cli/opts"
+	dockerops "github.com/docker/docker/opts"


typo dockeropts

fahedouch · 2023-02-22T16:27:06Z

pkg/defaults/defaults_linux.go

+	}
+	for _, addr := range addrs {
+		if ipnet, ok := addr.(*net.IPNet); ok && !ipnet.IP.IsLoopback() {
+			if ipnet.IP.To4() != nil || ipnet.IP.To16() != nil {


we do not support ipv6 yet. You should return empty string for ipv6

fahedouch · 2023-02-22T16:52:44Z

pkg/defaults/defaults_linux.go

@@ -130,3 +131,19 @@ func HostsDirs() []string {
 		filepath.Join(xch, "docker/certs.d"),
 	}
 }
+
+// HostGatewayIP returns the non-loop-back host ip if available and returns empty string if running into error.
+func HostGatewayIP() string {


the HostGatewayIP should be the container network interface IP and not from other network interfaces

I think by defaulit we are returning the host's real ip now instead of any interface IP. The host ip is accessible from containers.

docker is setting the host bridge net interface IP. not sure if the host real IP will fit here, because the host net interface IP that's attached to the container need to be the gateway to route traffic from the container to host.

There are two blockers of using bridge IP.
One is it is not deterministic in nerdctl as I mentioned here For networking, Nerdctl and Docker are using different technology. Nerdctl uses CNI but Docker uses libnetwork, which caused the gap.

The other is it is not useful for rootless based on @AkihiroSuda 's comment.

I guess Docker compatibility is more about consistent user experience instead of implementation details. The default host gateway ip is just something that allows accessing host from containers. Only if the default ip can satisfy this use case, it should be fine.

Let me know if you have better suggestions.

SGTM the real host IP

fahedouch · 2023-02-24T09:17:07Z

docs/command-reference.md

@@ -1599,6 +1600,7 @@ Flags:
 - :nerd_face: `--cgroup-manager=(cgroupfs|systemd|none)`: cgroup manager
  - Default: "systemd" on cgroup v2 (rootful & rootless), "cgroupfs" on v1 rootful, "none" on v1 rootless
 - :nerd_face: `--insecure-registry`: skips verifying HTTPS certs, and allows falling back to plain HTTP
+- :nerd_face: `--host-gateway-ip`: IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the host


I think we need to mention that --host-gateway-ip have no effect without setting --add-host
cc @ningziwen

Signed-off-by: Ziwen Ning <[email protected]>

fahedouch

Thanks

AkihiroSuda

Thanks

ningziwen mentioned this pull request Feb 7, 2023

Resolve special IP host-gateway #1979

Closed

ningziwen force-pushed the host-gateway branch 2 times, most recently from 1256885 to 31e6a78 Compare February 10, 2023 06:05

AkihiroSuda reviewed Feb 10, 2023

View reviewed changes

pkg/config/config.go Show resolved Hide resolved

AkihiroSuda reviewed Feb 10, 2023

View reviewed changes

ningziwen mentioned this pull request Feb 13, 2023

feat: add tests for special IP in --add-host flag runfinch/common-tests#29

Merged

ningziwen force-pushed the host-gateway branch from 3e50c26 to c818c73 Compare February 19, 2023 23:31

ningziwen marked this pull request as ready for review February 20, 2023 00:11

ningziwen requested a review from AkihiroSuda February 20, 2023 00:11

AkihiroSuda added this to the v1.3.0 (tentative) milestone Feb 20, 2023

djdongjin reviewed Feb 20, 2023

View reviewed changes

ningziwen requested review from djdongjin and AkihiroSuda and removed request for AkihiroSuda and djdongjin February 20, 2023 06:24

djdongjin reviewed Feb 20, 2023

View reviewed changes

djdongjin approved these changes Feb 21, 2023

View reviewed changes

AkihiroSuda reviewed Feb 21, 2023

View reviewed changes

ningziwen force-pushed the host-gateway branch from 3b653ea to 6e24ef3 Compare February 21, 2023 17:06

ningziwen requested a review from AkihiroSuda February 21, 2023 18:08

fahedouch reviewed Feb 22, 2023

View reviewed changes

ningziwen force-pushed the host-gateway branch from 6e24ef3 to f80a844 Compare February 22, 2023 20:20

ningziwen requested review from fahedouch and removed request for AkihiroSuda February 22, 2023 20:20

fahedouch reviewed Feb 24, 2023

View reviewed changes

feat: resolve special ip host-gateway

a30e765

Signed-off-by: Ziwen Ning <[email protected]>

ningziwen force-pushed the host-gateway branch from f80a844 to a30e765 Compare February 24, 2023 18:14

ningziwen requested a review from fahedouch February 24, 2023 18:14

fahedouch approved these changes Feb 25, 2023

View reviewed changes

AkihiroSuda approved these changes Feb 28, 2023

View reviewed changes

AkihiroSuda merged commit 6d085c1 into containerd:main Feb 28, 2023

ningziwen mentioned this pull request Mar 21, 2023

Finch cannot resolve special IP host-gateway runfinch/finch#209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: resolve special ip host-gateway #1978

feat: resolve special ip host-gateway #1978

ningziwen commented Feb 7, 2023 •

edited

Loading

AkihiroSuda Feb 10, 2023

AkihiroSuda Feb 10, 2023

ningziwen Feb 10, 2023

ningziwen Feb 14, 2023

ningziwen Feb 20, 2023

djdongjin Feb 20, 2023

ningziwen Feb 20, 2023

djdongjin Feb 20, 2023

ningziwen Feb 20, 2023 •

edited

Loading

djdongjin Feb 20, 2023

djdongjin Feb 20, 2023

djdongjin Feb 20, 2023

ningziwen Feb 20, 2023

djdongjin Feb 20, 2023

ningziwen Feb 20, 2023

djdongjin Feb 20, 2023

ningziwen Feb 20, 2023

djdongjin left a comment

AkihiroSuda Feb 21, 2023

AkihiroSuda Feb 21, 2023

fahedouch Feb 22, 2023

fahedouch Feb 22, 2023

fahedouch Feb 22, 2023

ningziwen Feb 22, 2023

fahedouch Feb 23, 2023 •

edited

Loading

ningziwen Feb 23, 2023

fahedouch Feb 23, 2023

fahedouch Feb 24, 2023

fahedouch left a comment

AkihiroSuda left a comment

	AddPersistentStringFlag(rootCmd, "host-gateway-ip", nil, nil, nil, aliasToBeInherited, cfg.HostGatewayIP, "HOST_GATEWAY_IP", "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the default bridge")
	AddPersistentStringFlag(rootCmd, "host-gateway-ip", nil, nil, nil, aliasToBeInherited, cfg.HostGatewayIP, "NERDCTL_HOST_GATEWAY_IP", "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the default bridge")

feat: resolve special ip host-gateway #1978

feat: resolve special ip host-gateway #1978

Conversation

ningziwen commented Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningziwen Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djdongjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahedouch Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahedouch left a comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

ningziwen commented Feb 7, 2023 •

edited

Loading

ningziwen Feb 20, 2023 •

edited

Loading

fahedouch Feb 23, 2023 •

edited

Loading