-
Notifications
You must be signed in to change notification settings - Fork 618
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,11 @@ package main | |
import ( | ||
"bufio" | ||
"bytes" | ||
"context" | ||
"errors" | ||
"fmt" | ||
"io" | ||
"net/http" | ||
"os" | ||
"path/filepath" | ||
"runtime" | ||
|
@@ -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" | ||
) | ||
|
||
|
@@ -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) | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should work but not that accurate as this, becuase |
||
}) | ||
} | ||
|
||
func TestRunUlimit(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to confirm, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "".
|
||
return types.GlobalCommandOptions{ | ||
Debug: debug, | ||
DebugFull: debugFull, | ||
|
@@ -83,5 +87,6 @@ func processRootCmdFlags(cmd *cobra.Command) (types.GlobalCommandOptions, error) | |
InsecureRegistry: insecureRegistry, | ||
HostsDir: hostsDir, | ||
Experimental: experimental, | ||
HostGatewayIP: hostGatewayIP, | ||
}, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ package defaults | |
|
||
import ( | ||
"fmt" | ||
"net" | ||
"os" | ||
"path/filepath" | ||
|
||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two blockers of using bridge IP. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "" | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.