Source external DNS queries from container namespace #895
Conversation
mavenugo
changed the title
|
this enhancement must work for both loopback IPs and Self-IP address. |
|
@sanimej @mavenugo With the following fix, we see that the multi-tenant DNS servers are being honored and handled as expected: PTAL and incorporate as necessary. I can open a PR for this if required. |
|
@DivyaVavili The implementation in this PR to generate the queries from the container namespace might have some changes. We are also considering the option of generating all external queries from the container namespace by default. I will ping you when the updated changes are available for you to try it out. |
|
Sure @sanimej... Thanks for the update... Will wait for the changes... |
sanimej
changed the title
|
LGTM |
|
btw, I tried the patch to confirm if the DNS query is originated from the container namespace and the following capture (in a fake external DNS in the same network confirmed it). Yes. it works as advertised. now the behavior is identical to 1.9.x and before. LGTM. |
| c := &dns.Client{Net: w.LocalAddr().Network()} | ||
| addr := fmt.Sprintf("%s:%d", r.extDNS[i], 53) | ||
| var extConn net.Conn | ||
| r.sb.execFunc(func() { |
There was a problem hiding this comment.
Let's not get into the container namespace every time in the critical path when we need to make an external query. Let's create these connections during sandbox creation time and then just it here when we need to do external query.
There was a problem hiding this comment.
@mrjana Yes, creating the connection during the query is sub-optimal. First I tried creating the connection as part of the SetupFunc. But the Dial fails; probably because the external connectivity is not fully setup yet.. The error I was getting was "dial udp 8.8.8.8:53: connect: network is unreachable".
Can we do this instead; on a first external query lets create the connection and let it persist after that ?
There was a problem hiding this comment.
@sanimej Yes I am fine with lazy init but initializing once.
Signed-off-by: Santhosh Manohar <[email protected]>
| err = co.WriteMsg(query) | ||
| if err != nil { | ||
| log.Debugf("Send to DNS server failed, %s", err) | ||
| if proto == "tcp" { |
There was a problem hiding this comment.
Shouldn't we do this in a defer? So that we will know we always cleanup now and in future if more error conditions or other paths are introduced in the code path.
There was a problem hiding this comment.
Since we are looping through all the external servers if the read or write fails for the first one the other servers have to be tried.. To handle it in a defer a list of connections have to be maintained making it a bit kludgy. This looked cleaner. wdyt ?
There was a problem hiding this comment.
Yeah but you don't need to maintain a list. defer is like a closure and will do variable capture of the co at that iteration to create the closure defer function and push it into a stack and it will execute all of those defer functions in that stack when we exit the function. All take care automatically. No need for us to maintain any list of connections.
|
@mrjana Addressed the comments.. PTAL |
| extConn.SetDeadline(time.Now().Add(extIOTimeout)) | ||
| co := &dns.Conn{Conn: extConn} | ||
|
|
||
| cleanup := func() { |
There was a problem hiding this comment.
This is not what I meant :-)
Just do this instead of defining a cleanup func like this:
defer func() {
if proto == "tcp" {
co.Close()
}
}
and remove the defers that you added in all the error paths. That way it will always be closed at the end of the function and you don't need to remember to add them in all error paths. That is what the main benefit I was trying to get from the change.
|
Thanks @sanimej for taking care of all the comments. LGTM. Will merge when CI is green. |
Source external DNS queries from container namespace
Signed-off-by: Santhosh Manohar [email protected]