Host-agent DNS forwarder improvements (fixes #770)

[dee-kryvenko]

Previous issues:

• Cisco AnyConnect VPN with DNS #270
• [Mac] K3s DNS server rancher-sandbox/rancher-desktop#702

Fixes: rancher-sandbox/rancher-desktop#770

• Listen for TCP traffic
• Fix responding with more than one A record
• Add AAAA, CNAME, NS, MX and TXT forwarding
• Fix iptables forward rules so hostagent is accessible from containers

[jandubois]

Once you are done with this PR please squash commits into either a single commit, or two commits: one for adding the TCP port, and one for adding additional A records.

[AkihiroSuda]

Is this still a draft?

[jandubois]

Is this still a draft?

I think it is waiting for feedback from me. Will try to do it today.

[jandubois]

Is this still a draft?

Most of the discussion is actually happening in rancher-sandbox/rancher-desktop#770

[jandubois]

@dee-kryvenko Sorry, I can't finish the review right now. I want to understand the logic behind recursion, and if returning CNAME and A records in the same response is fine etc (I assume it is), but my mind has been broken by looking at too many iptables docs; will have to finish on the weekend, or Monday.

I'll also want to take a look into adding support for SRV queries.

[dee-kryvenko]

@jandubois added the SRV too.

As to the recursion - so the client can request server to do a recursion for it (req.RecursionDesired). If the server responds with reply.RecursionAvailable = false - client will swallow the bullet and do recursion on it's own. Otherwise - the response is squashed, here's what Google responds with:

> dig @8.8.8.8 updates.jenkins.io

; <<>> DiG 9.10.6 <<>> @8.8.8.8 updates.jenkins.io
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 33705
;; flags: qr rd ra; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;updates.jenkins.io.		IN	A

;; ANSWER SECTION:
updates.jenkins.io.	686	IN	CNAME	mirrors.jenkins.io.
mirrors.jenkins.io.	741	IN	A	52.202.51.185

;; Query time: 16 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Fri Oct 15 18:14:04 PDT 2021
;; MSG SIZE  rcvd: 85

This is one of the things I mentioned in rancher-sandbox/rancher-desktop#770 - I don't know the RFC and I don't know if it's a correct behavior, I just observed other public recursive resolvers are doing that.

[dee-kryvenko]

Well, and here goes the first - I was wrong. Apparently recursion only meant for the zone delegation stuff and not the CNAME. Here's what Google does:

> dig @8.8.8.8 updates.jenkins.io +norecurse

; <<>> DiG 9.10.6 <<>> @8.8.8.8 updates.jenkins.io +norecurse
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 63316
;; flags: qr ra; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;updates.jenkins.io.		IN	A

;; ANSWER SECTION:
updates.jenkins.io.	2796	IN	CNAME	mirrors.jenkins.io.
mirrors.jenkins.io.	2893	IN	A	52.202.51.185

;; Query time: 9 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Fri Oct 15 18:46:15 PDT 2021
;; MSG SIZE  rcvd: 85

So it always gives both CNAME and A.

[jandubois]

I'm done with this round of reviews now.

The important changes are using the canonical name instead of the query name when looking up A and AAAA entries for a CNAME record, and dropping the erroneous CNAME for SRV queries.

[dee-kryvenko]

@AkihiroSuda - just a feedback, git commit --amend followed by git push --force-with-lease kind of freaks me out as I am loosing all the contextual history on the PR. Which means if I mess up with amend or need to get back to the older version of the code due to feedback on the PR - it won't be as easy to revert which kind of defeats usefulness of Git... Have you considered to only enable Allow squash merging and disable rest of the merging options? That will keep master branch history clean and linear while not taking away my git log in my own branch.

[jandubois]

Almost there, but the hdr.Name = lookupName change is still required for A and AAAA answers.

[jandubois]

With this final change I would be willing to merge:

--- pkg/hostagent/dns.go
+++ pkg/hostagent/dns.go
@@ -97,12 +97,14 @@ func (h *Handler) handleQuery(w dns.ResponseWriter, req *dns.Msg) {
                                        ipv6 := ip.To4() == nil
                                        if q.Qtype == dns.TypeA && !ipv6 {
                                                hdr.Rrtype = dns.TypeA
+                                               hdr.Name = lookupName
                                                a = &dns.A{
                                                        Hdr: hdr,
                                                        A:   ip.To4(),
                                                }
                                        } else if q.Qtype == dns.TypeAAAA && ipv6 {
                                                hdr.Rrtype = dns.TypeAAAA
+                                               hdr.Name = lookupName
                                                a = &dns.AAAA{
                                                        Hdr:  hdr,
                                                        AAAA: ip.To16(),

[jandubois]

Which means if I mess up with amend or need to get back to the older version of the code due to feedback on the PR - it won't be as easy to revert which kind of defeats usefulness of Git...

Are you familiar with git reflog? It keeps the history of your HEAD for the last 90 days, so you are always able to return to an older commit by simply running git reset --hard 1234567.

Github also shows you the old and new SHA1 of your branch for each forced push, so that also allows you to switch back to any older commit.

And the string "force-pushed" in the Github UI is a hyperlink that lets you see the difference between the previous and the new branch.

So there are plenty of ways to recover your older commits and branches. After 90 days the reflog no longer holds a reference to them, so a garbage collection run might remove them. But chances are high that if you haven't missed them in 90 days, you will never need them again.

[dee-kryvenko]

Are you familiar with git reflog?

I am, but I don't find working with detached heads even remotely comparable, not to mention that many of us have more than one workspation and it brings additional complexity when you need partial rollback or something. I find it useful to have contextual history in the PR available handy as sometimes it helps the reviewer (or even a user) to easily find and browse what was done and why it wasn't included in the final version.

[dee-kryvenko]

I mean Allow squash merging does exactly that - squashes everything into one new commit on top of the current HEAD, so it is always a fast-forward merges and linear history.

[jandubois]

I mean Allow squash merging does exactly that - squashes everything into one new commit on top of the current HEAD, so it is always a fast-forward merges and linear history.

I believe the idea is that the PR author is the one to decide how to structure commits. We don't want each PR to be a single commit; you should still separate unrelated changes into their own commits, with a focused commit message (when possible).

But anything that is just part of development history, like any changes that are being reverted in later commits, should not become part of permanent history.

There is no requirement to always squash your commits right away, just during the final code review before they are being merged.

[jandubois]

LGTM

[dee-kryvenko]

We don't want each PR to be a single commit

I must have misunderstood your earlier request. Here is a good use case - GitHub feature that allowed you to propose a change that I could accept into this PR with a single click. Ideally this should be all I need to do, so we can streamline the process as much as possible towards contributors encouragement... Now, do I have to still squash it before the merge or maintainer will do it at merge using https://docs.github.com/en/github/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-pull-request-commits?
Also, DCO bot doesn't seem to like missing sign off... I'm not a lawyer but why is my cryptographic signature isn't enough?

[jandubois]

And just as I approve it, the DCO bot complains about the new commit not being signed.

It happens all the time when people commit using the web ui...

Please fetch, squash, make sure it is signed and push once more.

[jandubois]

Also, DCO bot doesn't seem to like missing sign off... I'm not a lawyer but why is my cryptographic signature isn't enough?

There is no signature on the latest commit:

commit 19ebfb6fb8fc04db9d4f5bca50eb8ebf8f945a5f (HEAD -> fix-dns-server-tcp)
Author: Dee Kryvenko <109895+dee-kryvenko@users.noreply.github.com>
Date:   Tue Oct 19 20:00:23 2021 -0700

    Fix undefined cname race condition

    Co-authored-by: Jan Dubois <jan@jandubois.com>

[dee-kryvenko]

There is a GPG signature:

> git verify-commit HEAD                                       
gpg: Signature made Tue Oct 19 20:00:23 2021 PDT
gpg:                using RSA key 4AEE18F83AFDEB23
gpg: Can't check signature: No public key

I thought it's "mine" (one GitHub auto generated for my account), but turns out it is GitHub's one that they use for everyone making changes in the UI. That's what gives that commit a Verified badge.

I will re-sign with mine, but I think what DCO is complaining about is a sign-off and not a signature (a text in the commit message). I'm slightly confused why this is important and not the actual signature, as the signature is what gives it authenticity and not the text, right?

[jandubois]

LGTM

[jandubois]

I think what DCO is complaining about is a sign-off and not a signature (a text in the commit message). I'm slightly confused why this is important and not the actual signature, as the signature is what gives it authenticity and not the text, right?

Yes, the DCO bot cares only about the sign-off. I'm somewhat confused how this holds legal significance, given how easily it can be spoofed, but it is a requirement from all the big foundations nowadays that all contributions have to come with a DCO "declaration".

It is certainly a barrier to contribution, but compared to the old days, when you had to send a CLA by snail-mail and include a copyright assignment, this is much simpler and less intrusive.

So to keep the option open to eventually transfer Lima to e.g. the CNCF or the Linux Foundation, or whatever a good home would be, we need to insist on getting DCO sign-off on every commit.

And I think this would also prevent us from "squash during merge" because we could not attach a sign-off to the squashed commit; only the original developer can do that.

[jandubois]

Thank you for your work, and your patience going through the whole process!

[dee-kryvenko]

because we could not attach a sign-off to the squashed commit; only the original developer can do that.

Ah, good point. Well... classics... legal vs progress :)

Thanks to you too - it was fun and I've learned a few things about DNS.