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

feat: resolve special ip host-gateway #1978

Merged
merged 1 commit into from
Feb 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions cmd/nerdctl/container_run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
dockeropts "github.com/docker/docker/opts"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -707,10 +708,20 @@ func createContainer(ctx context.Context, cmd *cobra.Command, client *containerd
return nil, nil, err
}
extraHosts = strutil.DedupeStrSlice(extraHosts)
for _, host := range extraHosts {
if _, err := dopts.ValidateExtraHost(host); err != nil {
for i, host := range extraHosts {
if _, err := dockercliopts.ValidateExtraHost(host); err != nil {
return nil, nil, err
}
parts := strings.SplitN(host, ":", 2)
// If the IP Address is a string called "host-gateway", replace this value with the IP address stored
// in the daemon level HostGateway IP config variable.
if parts[1] == dockeropts.HostGatewayName {
if globalOptions.HostGatewayIP == "" {
return nil, nil, fmt.Errorf("unable to derive the IP value for host-gateway")
}
parts[1] = globalOptions.HostGatewayIP
extraHosts[i] = fmt.Sprintf(`%s:%s`, parts[0], parts[1])
}
}
internalLabels.extraHosts = extraHosts

Expand Down Expand Up @@ -995,7 +1006,7 @@ func readKVStringsMapfFromLabel(cmd *cobra.Command) (map[string]string, error) {
return nil, err
}
labelsFilePath = strutil.DedupeStrSlice(labelsFilePath)
labels, err := dopts.ReadKVStrings(labelsFilePath, labelsMap)
labels, err := dockercliopts.ReadKVStrings(labelsFilePath, labelsMap)
if err != nil {
return nil, err
}
Expand Down
36 changes: 35 additions & 1 deletion cmd/nerdctl/container_run_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ package main
import (
"bufio"
"bytes"
"context"
"errors"
"fmt"
"io"
"net/http"
"os"
"path/filepath"
"runtime"
Expand All @@ -32,7 +35,6 @@ import (
"github.com/containerd/nerdctl/pkg/rootlessutil"
"github.com/containerd/nerdctl/pkg/strutil"
"github.com/containerd/nerdctl/pkg/testutil"

"gotest.tools/v3/assert"
)

Expand Down Expand Up @@ -150,6 +152,38 @@ func TestRunAddHost(t *testing.T) {
return nil
})
base.Cmd("run", "--rm", "--add-host", "10.0.0.1:testing.example.com", testutil.AlpineImage, "cat", "/etc/hosts").AssertFail()

response := "This is the expected response for --add-host special IP test."
http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
io.WriteString(w, response)
})
const hostPort = 8081
s := http.Server{Addr: fmt.Sprintf(":%d", hostPort), Handler: nil, ReadTimeout: 30 * time.Second}
go s.ListenAndServe()
defer s.Shutdown(context.Background())
base.Cmd("run", "--rm", "--add-host", "test:host-gateway", testutil.NginxAlpineImage, "curl", fmt.Sprintf("test:%d", hostPort)).AssertOutExactly(response)
}

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

Choose a reason for hiding this comment

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

why is it docker incompatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

base.Cmd("run", "--rm", "--host-gateway-ip", "192.168.5.2", "--add-host", "test:host-gateway", testutil.AlpineImage, "cat", "/etc/hosts").AssertOutWithFunc(func(stdout string) error {
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
Comment on lines +172 to +185
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

})
}

func TestRunUlimit(t *testing.T) {
Expand Down
5 changes: 5 additions & 0 deletions cmd/nerdctl/flagutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ func processRootCmdFlags(cmd *cobra.Command) (types.GlobalCommandOptions, error)
if err != nil {
return types.GlobalCommandOptions{}, err
}
hostGatewayIP, err := cmd.Flags().GetString("host-gateway-ip")
if err != nil {
return types.GlobalCommandOptions{}, err
}
Comment on lines +73 to +76
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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")
			}

return types.GlobalCommandOptions{
Debug: debug,
DebugFull: debugFull,
Expand All @@ -83,5 +87,6 @@ func processRootCmdFlags(cmd *cobra.Command) (types.GlobalCommandOptions, error)
InsecureRegistry: insecureRegistry,
HostsDir: hostsDir,
Experimental: experimental,
HostGatewayIP: hostGatewayIP,
}, nil
}
3 changes: 2 additions & 1 deletion cmd/nerdctl/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, "NERDCTL_HOST_GATEWAY_IP", "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the host. It has no effect without setting --add-host")
return aliasToBeInherited, nil
}

Expand Down Expand Up @@ -228,7 +229,7 @@ Config file ($NERDCTL_TOML): %s
}
if appNeedsRootlessParentMain(cmd, args) {
// reexec /proc/self/exe with `nsenter` into RootlessKit namespaces
return rootlessutil.ParentMain()
return rootlessutil.ParentMain(globalOptions.HostGatewayIP)
}
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/nerdctl/main_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func shellCompleteNamespaceNames(cmd *cobra.Command, args []string, toComplete s
return nil, cobra.ShellCompDirectiveError
}
if rootlessutil.IsRootlessParent() {
_ = rootlessutil.ParentMain()
_ = rootlessutil.ParentMain(globalOptions.HostGatewayIP)
return nil, cobra.ShellCompDirectiveNoFileComp
}
if err != nil {
Expand Down Expand Up @@ -60,7 +60,7 @@ func shellCompleteSnapshotterNames(cmd *cobra.Command, args []string, toComplete
return nil, cobra.ShellCompDirectiveError
}
if rootlessutil.IsRootlessParent() {
_ = rootlessutil.ParentMain()
_ = rootlessutil.ParentMain(globalOptions.HostGatewayIP)
return nil, cobra.ShellCompDirectiveNoFileComp
}
client, ctx, cancel, err := clientutil.NewClient(cmd.Context(), globalOptions.Namespace, globalOptions.Address)
Expand Down
5 changes: 4 additions & 1 deletion docs/command-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ Network flags:
- :whale: `--dns-search`: Set custom DNS search domains
- :whale: `--dns-opt, --dns-option`: Set DNS options
- :whale: `-h, --hostname`: Container host name
- :whale: `--add-host`: Add a custom host-to-IP mapping (host:ip)
- :whale: `--add-host`: Add a custom host-to-IP mapping (host:ip). `ip` could be a special string `host-gateway`,
- which will be resolved to the `host-gateway-ip` in nerdctl.toml or global flag.
- :whale: `--ip`: Specific static IP address(es) to use
- :whale: `--mac-address`: Specific MAC address to use. Be aware that it does not
check if manually specified MAC addresses are unique. Supports network
Expand Down Expand Up @@ -1599,6 +1600,8 @@ 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. It has no effect without setting --add-host
- Default: the IP address of the host

The global flags can be also specified in `/etc/nerdctl/nerdctl.toml` (rootful) and `~/.config/nerdctl/nerdctl.toml` (rootless).
See [`./config.md`](./config.md).
Expand Down
29 changes: 15 additions & 14 deletions docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,21 @@ experimental = true

## Properties

| TOML property | CLI flag | Env var | Description | Availability \*1 |
|-------------------|--------------------------|---------------------------|-----------------------------|---------------|
| `debug` | `--debug` | | Debug mode | Since 0.16.0 |
| `debug_full` | `--debug-full` | | Debug mode (with full output) | Since 0.16.0 |
| `address` | `--address`,`--host`,`-a`,`-H` | `$CONTAINERD_ADDRESS` | containerd address | Since 0.16.0 |
| `namespace` | `--namespace`,`-n` | `$CONTAINERD_NAMESPACE` | containerd namespace | Since 0.16.0 |
| `snapshotter` | `--snapshotter`,`--storage-driver` | `$CONTAINERD_SNAPSHOTTER` | containerd snapshotter | Since 0.16.0 |
| `cni_path` | `--cni-path` | `$CNI_PATH` | CNI binary directory | Since 0.16.0 |
| `cni_netconfpath` | `--cni-netconfpath` | `$NETCONFPATH` | CNI config directory | Since 0.16.0 |
| `data_root` | `--data-root` | | Persistent state directory | Since 0.16.0 |
| `cgroup_manager` | `--cgroup-manager` | | cgroup manager | Since 0.16.0 |
| `insecure_registry` | `--insecure-registry` | | Allow insecure registry | Since 0.16.0 |
| `hosts_dir` | `--hosts-dir` | | `certs.d` directory | Since 0.16.0 |
| `experimental` | `--experimental` | `NERDCTL_EXPERIMENTAL` | Enable [experimental features](experimental.md) | Since 0.22.3 |
| TOML property | CLI flag | Env var | Description | Availability \*1 |
|---------------------|------------------------------------|---------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------|
| `debug` | `--debug` | | Debug mode | Since 0.16.0 |
| `debug_full` | `--debug-full` | | Debug mode (with full output) | Since 0.16.0 |
| `address` | `--address`,`--host`,`-a`,`-H` | `$CONTAINERD_ADDRESS` | containerd address | Since 0.16.0 |
| `namespace` | `--namespace`,`-n` | `$CONTAINERD_NAMESPACE` | containerd namespace | Since 0.16.0 |
| `snapshotter` | `--snapshotter`,`--storage-driver` | `$CONTAINERD_SNAPSHOTTER` | containerd snapshotter | Since 0.16.0 |
| `cni_path` | `--cni-path` | `$CNI_PATH` | CNI binary directory | Since 0.16.0 |
| `cni_netconfpath` | `--cni-netconfpath` | `$NETCONFPATH` | CNI config directory | Since 0.16.0 |
| `data_root` | `--data-root` | | Persistent state directory | Since 0.16.0 |
| `cgroup_manager` | `--cgroup-manager` | | cgroup manager | Since 0.16.0 |
| `insecure_registry` | `--insecure-registry` | | Allow insecure registry | Since 0.16.0 |
| `hosts_dir` | `--hosts-dir` | | `certs.d` directory | Since 0.16.0 |
| `experimental` | `--experimental` | `NERDCTL_EXPERIMENTAL` | Enable [experimental features](experimental.md) | Since 0.22.3 |
| `host_gateway_ip` | `--host-gateway-ip` | `NERDCTL_HOST_GATEWAY_IP` | IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the host. It has no effect without setting --add-host | Since 1.3.0 |

The properties are parsed in the following precedence:
1. CLI flag
Expand Down
2 changes: 2 additions & 0 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type Config struct {
InsecureRegistry bool `toml:"insecure_registry"`
HostsDir []string `toml:"hosts_dir"`
Experimental bool `toml:"experimental"`
HostGatewayIP string `toml:"host_gateway_ip"`
AkihiroSuda marked this conversation as resolved.
Show resolved Hide resolved
}

// New creates a default Config object statically,
Expand All @@ -56,5 +57,6 @@ func New() *Config {
InsecureRegistry: false,
HostsDir: ncdefaults.HostsDirs(),
Experimental: true,
HostGatewayIP: ncdefaults.HostGatewayIP(),
}
}
4 changes: 4 additions & 0 deletions pkg/defaults/defaults_freebsd.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,7 @@ func NerdctlTOML() string {
func HostsDirs() []string {
return []string{"/etc/containerd/certs.d", "/etc/docker/certs.d"}
}

func HostGatewayIP() string {
return ""
}
17 changes: 17 additions & 0 deletions pkg/defaults/defaults_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package defaults

import (
"fmt"
"net"
"os"
"path/filepath"

Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@fahedouch fahedouch Feb 23, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM the real host IP

addrs, err := net.InterfaceAddrs()
if err != nil {
return ""
}
for _, addr := range addrs {
if ipnet, ok := addr.(*net.IPNet); ok && !ipnet.IP.IsLoopback() {
if ipnet.IP.To4() != nil {
return ipnet.IP.String()
}
}
}
return ""
}
4 changes: 4 additions & 0 deletions pkg/defaults/defaults_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,7 @@ func HostsDirs() []string {
filepath.Join(programData, "docker\\certs.d"),
}
}

func HostGatewayIP() string {
return ""
}
3 changes: 2 additions & 1 deletion pkg/rootlessutil/parent.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func RootlessKitChildPid(stateDir string) (int, error) {
return strconv.Atoi(pidStr)
}

func ParentMain() error {
func ParentMain(hostGatewayIP string) error {
if !IsRootlessParent() {
return errors.New("should not be called when !IsRootlessParent()")
}
Expand Down Expand Up @@ -104,5 +104,6 @@ func ParentMain() error {
os.Setenv("ROOTLESSKIT_STATE_DIR", stateDir)
os.Setenv("ROOTLESSKIT_PARENT_EUID", strconv.Itoa(os.Geteuid()))
os.Setenv("ROOTLESSKIT_PARENT_EGID", strconv.Itoa(os.Getegid()))
os.Setenv("NERDCTL_HOST_GATEWAY_IP", hostGatewayIP)
return syscall.Exec(arg0, args, os.Environ())
}