feat: forward guest DNS to Docker's embedded resolver - #793
Conversation
✅ Deploy Preview for urunc canceled.
|
25a6350 to
a743f29
Compare
a743f29 to
6723c67
Compare
|
Hey @cmainas, I have implemented the changes we discussed during our sync. Could you please take a look and review them? This will allow us to make any necessary changes before we extend this logic to other unikernels. |
22a5784 to
89b24e6
Compare
cmainas
left a comment
There was a problem hiding this comment.
Hello @alimx07 ,
thank you for all the changes and the fix. A few comments:
- In the e2e testing the docker network does not exist and therefore creating the contianer will fail. We need to setup the network. Maybe in https://github.com/urunc-dev/urunc/blob/main/tests/e2e/docker_test.go#L25 and also remove later.
- Let;s find a better name for loclahost.go. Maybe something like "dns_at_localhost.go"?
- I think I got confused with the
genericRulesanddockerRules. Have we found a case where we should apply thegenericRules(e.g. a CNI) rather thandockerRules? - We can wrap the execution of iptables in a function, instead of having cmd and cmd.Run in various places.
- We should check if we can obtain the information we gather form iptables from other sources (e.g. netlink).
- We can restructure the code and this logic in the network_dynamic.go (the only mode where these changes apply) and avoid re-opening the tap device etc. We can me use of https://github.com/urunc-dev/urunc/blob/main/pkg/network/network_dynamic.go#L23C6-L23C20 for passing information.
- We should also clean up all the rules we apply.
39be2e9 to
7e73541
Compare
| info.DNSServer = n.DNSForwarder.VirtIP.String() | ||
| netlog.Debugf("localhost forwarding applied: virtual resolvIP %s", info.DNSServer) | ||
| } | ||
|
|
There was a problem hiding this comment.
Since users are already familiar with the urunc localhost resolver issue, making this a hard failure is probably the wrong move. I'll fail open here for now. We can always flag it later if needed.
There was a problem hiding this comment.
I think my comment and the original code have disappeared, so I do not really know what we are talking about here. However, it is ok not to fail hard. We cna easily change that.
There was a problem hiding this comment.
One note though. If RewriteResolvConf fails and Apply succeeds, we will end up with some network setup which might create issues. So, for every failure step, we need to perform the necessary cleanup. This can be addressed in a future PR, but we should at least document this with a TODO.
There was a problem hiding this comment.
I have added it as TODO now, I will look into add after adding more unikernels :)
| _, err := commonCmdExec(cmdBase) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
@cmainas , Regarding network. I think using the same pattern as container (rm , create) is the cleanest way here.
I know this is out of the scope of the PR now, and it just being larger and larger, but what touched here is minimal as possible. Also , a future PR will be opened (by Me) adding more test cases (instead of docker) touching other CLI tools. so if you are good, we can move with this right now.
There was a problem hiding this comment.
It is ok for the time being, let's just make sure we record it so we can fix it later.
| info.DNSServer = n.DNSForwarder.VirtIP.String() | ||
| netlog.Debugf("localhost forwarding applied: virtual resolvIP %s", info.DNSServer) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think my comment and the original code have disappeared, so I do not really know what we are talking about here. However, it is ok not to fail hard. We cna easily change that.
| _, err := commonCmdExec(cmdBase) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
It is ok for the time being, let's just make sure we record it so we can fix it later.
| // ClearRules removes DNAT rules dockerRules could have added. | ||
| func clearRules() error { | ||
|
|
||
| // This will be run from kill() so no memory state could be used. |
There was a problem hiding this comment.
We do not need to keep any memory, we just need to remove the rules we applied before. Is there something that restricts us form doing so?
There was a problem hiding this comment.
No I am just write a comment as reasoning for future reader why we do not just store iptables from create command and delete them later instead of query and detect them again.
| return "", "", fmt.Errorf("could not list TCP sockets in namespace: %w", err) | ||
| } | ||
| for _, s := range tcp { | ||
| if s.InetDiagMsg.ID.Source.String() == loIP.String() { |
There was a problem hiding this comment.
We should make this check more precise because there might be other open TCP sockets here and we might end up with the wrong port. We should also check the state of the socket to be a listener.
| return "", "", fmt.Errorf("could not list UDP sockets in namespace: %w", err) | ||
| } | ||
| for _, s := range udp { | ||
| if s.InetDiagMsg.ID.Source.String() == loIP.String() { |
There was a problem hiding this comment.
Similarly with the previous comment, we have to make this check more precise too. Looking it up, one idea is to see of the destination is "0.0.0.0".
| info.DNSServer = n.DNSForwarder.VirtIP.String() | ||
| netlog.Debugf("localhost forwarding applied: virtual resolvIP %s", info.DNSServer) | ||
| } | ||
|
|
There was a problem hiding this comment.
One note though. If RewriteResolvConf fails and Apply succeeds, we will end up with some network setup which might create issues. So, for every failure step, we need to perform the necessary cleanup. This can be addressed in a future PR, but we should at least document this with a TODO.
| fields := strings.Fields(line) | ||
| if len(fields) >= 2 && fields[0] == "nameserver" { | ||
| if ip := net.ParseIP(fields[1]); ip != nil && ip.IsLoopback() { | ||
| return ip |
There was a problem hiding this comment.
The loop here is correct, but there might be cases where the /etc/resolv/conf file has more than one nameserver entries. So we should check if there is just a single nameserver with the loopback.
There was a problem hiding this comment.
okay you are right, lets be determinstic about this and to give the context and visuallize it.
type Forwarder struct {
VirtIP net.IP // virtual resolver IP the guest dials
LoIP net.IP // loopback IP the real resolver listens on
ResolvConf string // host path of the container's resolv.conf
custom rules // scenario-specific extra rules, picked by Detect
}
// loNameserver returns the first loopback nameserver in resolv.conf data, if any.
func loNameserver(data string) net.IP {
for line := range strings.SplitSeq(data, "\n") {
fields := strings.Fields(line)
if len(fields) >= 2 && fields[0] == "nameserver" {
if ip := net.ParseIP(fields[1]); ip != nil && ip.IsLoopback() {
return ip
}
}
}
return nil
}if we have resolv.conf as
nameserver 127.0.0.10
nameserver 127.0.0.20
nameserver 8.8.8.8our loNameserver function above will detect and set our loIP as 127.0.0.10 (first localhost nameserver), which will be the one in TC rules and Iptables one. Knowing that we should only rewrite the first nameserver in our resolv.conf, so final result will be:
nameserver virtIP
nameserver 127.0.0.20
nameserver 8.8.8.8I know that a pushback here is to rewrite all localhost nameservers , but this will lead us to also write more iptables rules and TC rules, depending only on the first localhost nameserver is acceptable I think.
There was a problem hiding this comment.
Sure, but I meant something else. In the file format you mentioned
nameserver 127.0.0.10
nameserver 127.0.0.20
nameserver 8.8.8.8
if the first two entries fail, then 8.8.8.8 will be used and it will work (for some dns resolutions). So what I meant is we target specifically the docker case with a single nameserver at a localhost address (at least for this PR). In the future we can extend the current implementation and address the above scenario too.
There was a problem hiding this comment.
Since the current implementation works for docker case, we can just leave it like that. If I understood correctly from docker, there will be no case with multiple nameserver entries in user defined networks.
|
Also, one more thing, do not forget to add yourself in https://github.com/urunc-dev/urunc/blob/main/.github/contributors.yaml |
| return nil | ||
| } | ||
|
|
||
| func uint16Ptr(v uint16) *uint16 { |
There was a problem hiding this comment.
I have tried but it fails everytime.
There was a problem hiding this comment.
What does fail mean? Is it a compiler error, a runtime error, other failure?
7e73541 to
b27f329
Compare
|
@cmainas . Everything almost done. Let me if you have any other comments. meanwhile I will be working on sideContainers case for now :) |
|
Hello @alimx07 , please rebase over the main branch. |
b27f329 to
d77880e
Compare
cmainas
left a comment
There was a problem hiding this comment.
Hello @alimx07 , some comments:
- the unit tests need to be added in https://github.com/urunc-dev/urunc/blob/main/Makefile#L236C43-L236C55 so they run though the CI. Check
test_networkhow it is declared and used. - the rebase seems that changed the go.mod file. Also, please avoid merge commits they do not add any value in the git history.
| golang.org/x/net v0.56.0 | ||
| golang.org/x/sys v0.46.0 | ||
| google.golang.org/grpc v1.81.1 | ||
| k8s.io/cri-api v0.36.2 |
There was a problem hiding this comment.
Just random go mod tidy. No problems I revert it now and rebased also :)
| // and set our `loIP` as `127.0.0.10` (first loopback nameserver), | ||
| // which will be the one in TC rules and Iptables one. | ||
| // Knowing that we should only rewrite the first nameserver in our `resolv.conf`, | ||
| // so final result will be : |
| for _, line := range lines { | ||
| fields := strings.Fields(line) | ||
|
|
||
| // if we have `resolv.conf` as : |
c27ecf5 to
9220678
Compare
Detect first loopback nameserver in our resolv.conf and rewrite to user defined virtIP (configured in urunc constants) and apply some TC rules in general and IP rules (dockercase) so we can route this DNS quiries to host localhost DNS server. Signed-off-by: Ali Mohamed <amx746@gmail.com>
9220678 to
f278a84
Compare
Description
On Docker user custom networks the DNS resolver (127.0.0.11) is loopback only and unreachable by the unikernel guest. Detect this case per container, expose the resolver via a virtual resolver IP
configured by userusing tc redirects between the tap and lo , and rewrite the guest'sresolv.confto point at that IP in our unikernel islinuxone. Also we add a custom rules DNAT rules according to the enviroment (e.g. Docker -> PREROUTING DNAT(for DNS server ports))Two TC rules added on tap and lo:
src=ResolvIPinto TapRelated issues
How was this tested?
Tested in user custom docker network with urunc container
u1and normal oneu2:u1lookup ongithub.comu1lookup onu2u2lookup onu1LLM usage
Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).