-
Notifications
You must be signed in to change notification settings - Fork 99
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
port: add ChildIP #206
port: add ChildIP #206
Conversation
it is needed for Podman: containers/podman#5138 |
This doesn't seem to be able to propagate src IP. Wouldn't this just make the src IP to 10.0.2.100 ? |
Yes, it won't propagate the srcIP but at least we can set it to something different than 127.0.0.1 that causes issues with some container images |
@AkihiroSuda are you fine with the change as it is? |
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.
Looks good but a couple of nits. Thanks
@@ -106,7 +106,11 @@ func (d *childDriver) handleConnectRequest(c *net.UnixConn, req *msg.Request) er | |||
return errors.Errorf("unknown proto: %q", req.Proto) | |||
} | |||
var dialer net.Dialer | |||
targetConn, err := dialer.Dial(req.Proto, fmt.Sprintf("127.0.0.1:%d", req.Port)) | |||
ip := req.IP |
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.
We need to verify that this is a valid IPv4 string
if ip == "" { | ||
ip = "127.0.0.1" | ||
} | ||
targetConn, err := dialer.Dial(req.Proto, fmt.Sprintf("%s:%d", ip, req.Port)) |
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.
Could you also update pkg/port/{slirp4netns,socat}?
pkg/port/port.go
Outdated
@@ -10,6 +10,7 @@ type Spec struct { | |||
ParentIP string `json:"parentIP,omitempty"` // IPv4 address. can be empty (0.0.0.0). | |||
ParentPort int `json:"parentPort,omitempty"` | |||
ChildPort int `json:"childPort,omitempty"` | |||
ChildIP string `json:"childIP,omitempty"` |
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.
Please add godoc comment to clarify that this defaults to 127.0.0.1
allow users to override the IP to use for the connection inside the network namespace. It is useful e.g. with slirp4netns to override the IP to "10.0.2.100". Signed-off-by: Giuseppe Scrivano <[email protected]>
thanks for the review. I've addressed your comments and pushed a new version |
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.
Thanks, I'll ship a new release tomorrow
Signed-off-by: Akihiro Suda <[email protected]>
follow-up for "port: add ChildIP" (#206) + release v0.12.0
Signed-off-by: Akihiro Suda <[email protected]>
allow users to override the IP to use for the connection inside the
network namespace.
It is useful e.g. with slirp4netns to override the IP to "10.0.2.100".
Signed-off-by: Giuseppe Scrivano [email protected]