dns: Parse packet in user space #2544

mauriciovasquezbernal · 2024-02-26T19:40:17Z

dns: Parse DNS packet in user space

The current logic tries to parse the full DNS packet in eBPF. This is
diffcult to do a imposes some limitations, like not supporting:
- DNS over TCP
- Uncompressed messages
- More than 8 answers

This commit changes approach to parce the DNS packet on user space.

TODO:

Allow configuring ports

Fixes #2008

flyth

This makes handling the packets much easier - excited to see how we solve that for the image based gadgets 😄

pkg/gadgets/trace/dns/tracer/bpf/dns.c

blanquicet

I only reviewed the first commit for now. I already have some comments and many questions 😅. Sorry if I commented things that are changed in the second commit, feel free to immediately resolve them.

pkg/gadgets/trace/dns/tracer/bpf/dns-common.h

pkg/gadgets/trace/dns/tracer/bpf/dns.c

pkg/gadgets/trace/dns/tracer/tracer.go

anubhabMajumdar · 2024-02-27T20:53:16Z

@mauriciovasquezbernal Thanks for the change.

I tested this change in the following way:

make build-gadgets
make minikube-start && make minikube-deploy
Exec into minikube and run nslookup -type=a dc.services.visualstudio.com. and nslookup -type=aaaa dc.services.visualstudio.com.

 ./kubectl-gadget trace dns -A                                                                               ✔
K8S.NODE                     K8S.NAMESPACE                K8S.POD                      PID        TID        COMM            QR TYPE      QTYPE      NAME                         RCODE          NUMANS…
minikube-docker                                                                        97501      57694      dockerd         Q  OUTGOING  A          dc.services.visualstudio.co…                0
ERRO[0004] decoding dns layer: resource record length exceeds data
minikube-docker                                                                        0          0                          R  OUTGOING  A          dc.services.visualstudio.co… NoError        23
minikube-docker                                                                        71848      71849      isc-worker0000  R  HOST      A          dc.services.visualstudio.co… NoError        23
minikube-docker                                                                        97501      57693      dockerd         Q  OUTGOING  AAAA       dc.services.visualstudio.co…                0
minikube-docker                                                                        97501      57693      dockerd         R  HOST      AAAA       dc.services.visualstudio.co… NoError        13
minikube-docker                                                                        0          0                          R  OUTGOING  AAAA       dc.services.visualstudio.co… NoError        13
minikube-docker                                                                        71910      71911      isc-worker0000  R  HOST      AAAA       dc.services.visualstudio.co… NoError        13

blanquicet

Interesting approach! Like Mike, looking forward to seeing this one implemented as image-based gadget.

The code looks good to me already from code inspection. I'll wait for the port params implementation and then I will test it.

pkg/gadgets/trace/dns/tracer/bpf/dns.c

blanquicet

Tested and working as expected. LGTM!

docs/builtin-gadgets/trace/dns.md

eiffel-fl

Hi!

I really like the idea, as it simplifies our code base.
But I have one concern with regard to the types.

Best regards.

pkg/gadgets/trace/dns/tracer/bpf/dns-common.h

pkg/gadgets/trace/dns/tracer/bpf/dns.c

eiffel-fl · 2024-02-29T03:57:40Z

pkg/gadgets/trace/dns/tracer/bpf/dns.c

 // Map of DNS query to timestamp so we can calculate latency from query sent to answer received.
 struct query_key_t {
 	__u64 pid_tgid;
 	__u16 id;
+	__u16 pad[3]; // this is needed, otherwise the verifier claims an invalid read from stack


Where exactly the verifier rejects the program?
When you this as a key? Or when you initialize it?

pkg/gadgets/trace/dns/tracer/tracer_test.go

pkg/gadgets/trace/dns/tracer/tracer.go

eiffel-fl · 2024-02-29T04:16:29Z

pkg/gadgets/trace/dns/tracer/tracer.go

+	// This is hardcoded for now as the datapath assumes the DNS packet is UDP over IPv4
+	// without any options. Next iterations will fix this.
+	// Ethernet + IP + UDP
+	const dnsOff = 14 + 20 + 8


Please, add some documentation and links on the why of these magical values.

I think this is pretty clear that those are the size of the headers. I could add add links to the RFCs of those protocols but it's an overkill on my opinion.

I could add add links to the RFCs of those protocols but it's an overkill on my opinion.

Documenting is never overkill, please add them.

eiffel-fl · 2024-02-29T04:23:36Z

pkg/gadgets/trace/dns/tracer/tracer.go

-	if !ok {
-		event.QType = "UNASSIGNED"
+	if len(dnsLayer.Questions) > 0 {
+		question := dnsLayer.Questions[0]


I do not understand something.
Before, we had this big array qTypeNames, now we rely on this golang package.
Sadly, the golang package does not define as many types as our previous array:
https://pkg.go.dev/github.com/google/gopacket@v1.1.19/layers#DNSType
So, are we loosing some types by using this package? Or before we defined more types than needed?

Good question, I have no idea. I wanted to use gopacket as much as possible to parse this values as I think they are a good source of trust for that. @alban do you have any opinion here? Should we continue using our big array or switch to gopacket?

IMO, it doesn't make sense to use gopacket parsing for some things and not for others. I agree gopacket is indeed a good source of truth, if we consider there is something missing, we should open a PR on gopacket instead of use a custom parsing. In this case in specific, honestly, I think people will mainly use the DNS gadget for tracing IPv4 and IPv6 request, and those are already included in the gopacket parsing. Said that, I suggest using the gopacket parsing also here.

eiffel-fl

Hi!

It works on my side:

$ sudo ./ig trace dns                                                                                                                                              mauricio/parse-dns-userspace % u=
RUNTIME.CONTAINERNAME                                                 PID        TID        COMM             QR TYPE      QTYPE      NAME                                                                 RCODE                                NUMANSW…
test-trace-dns                                                        21188      21189      isc-net-0000     Q  OUTGOING  A          inspektor-gadget.io.                                                                                      0       
test-trace-dns                                                        21188      21189      isc-net-0000     R  HOST      A          inspektor-gadget.io.                                                 No Error                             2
$ uname -a                                                                                                                                                         mauricio/parse-dns-userspace % u=
Linux pwmachine 6.5.0-21-generic #21~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Feb  9 13:32:52 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

But I still have some concerns regarding the DNS type.

Best regards.

alban · 2024-03-04T13:30:18Z

integration/ig/k8s/trace_dns_test.go

-					Rcode:      "NXDomain",
+					Rcode:      "Non-Existent Domain",


Should the width of the column be adapted?

pkg/gadgets/trace/dns/types/dns.go:

Rcode string `json:"rcode,omitempty" column:"rcode,minWidth:8"`

I changed it to 12.

pkg/gadgets/trace/dns/tracer/tracer.go

pkg/gadgets/trace/dns/tracer/bpf/dns.c

pkg/gadgets/trace/dns/tracer/tracer.go

The current logic tries to parse the full DNS packet in eBPF. This is diffcult to do a imposes some limitations, like not supporting: - DNS over TCP - Uncompressed messages - More than 8 answers This commit changes approach to parce the DNS packet on user space. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

mauriciovasquezbernal · 2024-03-04T19:27:25Z

Opened #2574 as a follow up work on this.,

mauriciovasquezbernal requested review from alban and blanquicet as code owners February 26, 2024 19:40

mauriciovasquezbernal force-pushed the mauricio/parse-dns-userspace branch 2 times, most recently from 61d056b to 48150fe Compare February 26, 2024 21:20

flyth reviewed Feb 27, 2024

View reviewed changes

pkg/gadgets/trace/dns/tracer/bpf/dns.c Outdated Show resolved Hide resolved

pkg/gadgets/trace/dns/tracer/bpf/dns.c Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/parse-dns-userspace branch 3 times, most recently from 90702e9 to 452dea3 Compare February 27, 2024 18:46

blanquicet reviewed Feb 27, 2024

View reviewed changes

blanquicet reviewed Feb 28, 2024

View reviewed changes

pkg/gadgets/trace/dns/tracer/bpf/dns.c Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/parse-dns-userspace branch from 452dea3 to eec00f0 Compare February 28, 2024 15:35

mauriciovasquezbernal requested review from eiffel-fl and mqasimsarfraz as code owners February 28, 2024 15:35

blanquicet approved these changes Feb 28, 2024

View reviewed changes

docs/builtin-gadgets/trace/dns.md Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/parse-dns-userspace branch 3 times, most recently from 3592eec to 616a3d1 Compare February 28, 2024 22:05

eiffel-fl reviewed Feb 29, 2024

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/parse-dns-userspace branch from 616a3d1 to c27a48c Compare February 29, 2024 16:32

anubhabMajumdar approved these changes Feb 29, 2024

View reviewed changes

eiffel-fl reviewed Mar 1, 2024

View reviewed changes

alban reviewed Mar 4, 2024

View reviewed changes

pkg/gadgets/trace/dns/tracer/tracer.go Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/parse-dns-userspace branch from c27a48c to d772f2e Compare March 4, 2024 15:23

mauriciovasquezbernal merged commit fcc209f into main Mar 4, 2024
59 checks passed

mauriciovasquezbernal deleted the mauricio/parse-dns-userspace branch March 4, 2024 16:31

mauriciovasquezbernal mentioned this pull request Mar 5, 2024

trace dns: support uncompressed messages #2467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: Parse packet in user space #2544

dns: Parse packet in user space #2544

mauriciovasquezbernal commented Feb 26, 2024 •

edited

flyth left a comment

blanquicet left a comment

anubhabMajumdar commented Feb 27, 2024

blanquicet left a comment

blanquicet left a comment

eiffel-fl left a comment

eiffel-fl Feb 29, 2024

eiffel-fl Feb 29, 2024

mauriciovasquezbernal Feb 29, 2024

eiffel-fl Mar 1, 2024

eiffel-fl Feb 29, 2024

mauriciovasquezbernal Feb 29, 2024

blanquicet Mar 1, 2024 •

edited

eiffel-fl left a comment

alban Mar 4, 2024

mauriciovasquezbernal Mar 4, 2024

mauriciovasquezbernal commented Mar 4, 2024

dns: Parse packet in user space #2544

dns: Parse packet in user space #2544

Conversation

mauriciovasquezbernal commented Feb 26, 2024 • edited

flyth left a comment

Choose a reason for hiding this comment

blanquicet left a comment

Choose a reason for hiding this comment

anubhabMajumdar commented Feb 27, 2024

blanquicet left a comment

Choose a reason for hiding this comment

blanquicet left a comment

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blanquicet Mar 1, 2024 • edited

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Mar 4, 2024

mauriciovasquezbernal commented Feb 26, 2024 •

edited

blanquicet Mar 1, 2024 •

edited