-
Notifications
You must be signed in to change notification settings - Fork 83
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
IPv6: #259 #253 #276
base: master
Are you sure you want to change the base?
IPv6: #259 #253 #276
Conversation
@AkihiroSuda can you enable ipv6 for the Docker containers? |
@@ -41,9 +41,13 @@ int api_bindlisten(const char *api_socket) | |||
struct api_hostfwd { | |||
int id; | |||
int is_udp; | |||
struct in_addr host_addr; | |||
int is_ipv4; | |||
int is_ipv6; |
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 shouldn’t need is_ipv4.
Checking !is_ipv6 should be enough.
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 can pass tcp6/4 as proto to explicitly just enable either.
If we pass tcp as proto, ipv6 is enabled and the host address
is either 0.0.0.0 or ::0, both are enabled.
@@ -224,7 +238,7 @@ If `guest_addr` is not specified, then it will be set to the default address tha | |||
```console | |||
(namespace)$ json='{"execute": "list_hostfwd"}' | |||
(namespace)$ echo -n $json | nc -U /tmp/slirp4netns.sock | |||
{"return": {"entries": [{"id": 42, "proto": "tcp", "host_addr": "0.0.0.0", "host_port": 8080, "guest_addr": "10.0.2.100", "guest_port": 80}]}} | |||
{"return": {"entries": [{"id": 42, "proto": "tcp", "host_addr": "0.0.0.0", "host_addr6": "::", "host_port": 8080, "guest_addr": "10.0.2.100", "guest_addr6": "fd00::100", "guest_port": 80}]}} |
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.
I’m not sure we should mix up v4 and v6 in a single entry.
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.
What should we return in api_handle_req_add_hostfwd
when we add both tcp4 and tcp6? An array of IDs?
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.
Avoid adding tcp4 and tcp6 in a single request/response.
Mixing up them in a single request/response makes it hard to implement RootlessKit integration.
https://github.com/rootless-containers/rootlesskit/blob/c5481c1cfd6f6c42c8ec1868fbc543f2b0a916a5/pkg/api/openapi.yaml#L70-L90
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.
I tried to implement it like Go's net listen, which does listen on both 4 and 6 if you set tcp with global ip 0.0.0.0 or :: if I understood it correctly.
If we don't need that I can change it like that?
- host_addr: 0.0.0.0, proto: tcp -> v4
- host_addr: 0.0.0.0, proto: tcp6 -> error
- host_addr: ::, proto: tcp -> v6
- host_addr: ::, proto: tcp4 -> error
Thanks for working on this. Could you use real name for signing?
Do you mean Docker containers on the CI? I don’t think we have IPv6 there. |
- implement forwarding - implement random CIDR Signed-off-by: Jasmin Fazlic <[email protected]>
Yes, ok too bad, I guess we can then test the port forwarding in the vagrant box. |
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.
left some comments.
Could you please describe what you are doing in the commit message?
@@ -119,16 +123,59 @@ static int api_handle_req_add_hostfwd(Slirp *slirp, int fd, struct api_ctx *ctx, | |||
free(fwd); | |||
goto finish; | |||
} | |||
#if SLIRP_CONFIG_VERSION_MAX >= 3 | |||
int flags = (fwd->is_udp ? SLIRP_HOSTFWD_UDP : 0); |
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 we also check that SLIRP_HOSTFWD_UDP
is defined?
free(fwd); | ||
goto finish; | ||
} | ||
flags |= SLIRP_HOSTFWD_V6ONLY; |
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.
SLIRP_HOSTFWD_V6ONLY
might not be defined
goto finish; | ||
} | ||
flags |= SLIRP_HOSTFWD_V6ONLY; | ||
if (slirp_add_hostxfwd(slirp, (const struct sockaddr *)&fwd->host6, |
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 check for slirp_add_hostxfwd
at configure time
wrc = write(fd, api_ok, strlen(api_ok)); | ||
if (fwd->is_ipv4) { | ||
#if SLIRP_CONFIG_VERSION_MAX >= 3 | ||
if (slirp_remove_hostxfwd(slirp, |
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.
same for slirp_remove_hostxfwd
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.
Edit: nevermind
Checking for SLIRP_HOSTFWD_V6ONLY
at the moment would imply SLIRP_HOSTFWD_UDP
and the xfwd
functions. Can we just check for that to enable the IPv6 code path?
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.
sure, that should work as well
What's current status? |
Sorry, I'm out. |
I've been running this branch built with libslirp from master for well over a month now and I'm yet to experience any issues. |
I'm interested in working on this. Can I hijack this PR and @pfandl's work ? |
Yes, please |
Do you still plan this? |
Signed-off-by: fassl [email protected]