feat(dns-gadget): Add more fields to DNS gadget #1943
Conversation
anubhabMajumdar
requested review from
alban,
blanquicet,
eiffel-fl,
mqasimsarfraz and
mauriciovasquezbernal
as code owners
anubhabMajumdar
changed the title
anubhabMajumdar
force-pushed
the
topic/anmajumdar/five-tuple-dns
branch
from
472c727
to
956ee13
Compare
There was a problem hiding this comment.
Hi.
Thank you for working on this.
I have some initials comments, particularly regarding integration tests.
I will test the gadget itself later today and if everything is OK, I will run the CI.
Do not hesitate if you have any questions or point you do not understand.
Best regards.
anubhabMajumdar
force-pushed
the
topic/anmajumdar/five-tuple-dns
branch
from
e34bedb
to
db6c053
Compare
Thanks for the review, addressed your comments. Let me know if the gadget works for you! |
There was a problem hiding this comment.
I tested it and it works fine:
RUNTIME.CONTAINERNAME PID TID COMM SRCIP DSTIP SRCP… DSTP… PROTO QR TYPE QTYPE NAME RCODE NUMA…
60299 60300 isc-net-00… 192.168.0.12 8.8.4.4 34905 53 udp Q OUTGOING A inspektor-gadget.io. 0
60299 60300 isc-net-00… 8.8.4.4 192.168.0.12 53 34905 udp R HOST A inspektor-gadget.io. NoError 2
Nonetheless, we can polish it, I advise you to do the following:
- Rebase everything on top of latest main.
- Split your initial commit in two commits: one for the fields addition, the other for the integration test.
2.5 Drop thebpf_htons()call in eBPF code andHtoNs()call in golang,load_half()will convert the network endianness to the host one for you, so no need to add extra conversion here:
diff --git a/pkg/gadgets/trace/dns/tracer/bpf/dns.c b/pkg/gadgets/trace/dns/tracer/bpf/dns.c
index 7357d3bb..222371c5 100644
--- a/pkg/gadgets/trace/dns/tracer/bpf/dns.c
+++ b/pkg/gadgets/trace/dns/tracer/bpf/dns.c
@@ -211,13 +211,13 @@ static __always_inline int output_dns_event(struct __sk_buff *skb,
// Check network protocol.
event->proto = load_byte(skb, ETH_HLEN + offsetof(struct iphdr, protocol));
if (event->proto == IPPROTO_TCP) {
- event->sport = bpf_htons(load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct tcphdr, source)));
- event->dport = bpf_htons(load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct tcphdr, dest)));
+ event->sport = load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct tcphdr, source));
+ event->dport = load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct tcphdr, dest));
} else if (event->proto == IPPROTO_UDP) {
- event->sport = bpf_htons(load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct udphdr, source)));
- event->dport = bpf_htons(load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct udphdr, dest)));
+ event->sport = load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct udphdr, source));
+ event->dport = load_half(skb, ETH_HLEN + sizeof(struct iphdr) + offsetof(struct udphdr, dest));
}
-
+
event->qr = flags.qr;
if (flags.qr == 1) {
diff --git a/pkg/gadgets/trace/dns/tracer/dns_bpfel.o b/pkg/gadgets/trace/dns/tracer/dns_bpfel.o
index fc5a4068..e1475189 100644
Binary files a/pkg/gadgets/trace/dns/tracer/dns_bpfel.o and b/pkg/gadgets/trace/dns/tracer/dns_bpfel.o differ
diff --git a/pkg/gadgets/trace/dns/tracer/tracer.go b/pkg/gadgets/trace/dns/tracer/tracer.go
index b942466b..644e2f65 100644
--- a/pkg/gadgets/trace/dns/tracer/tracer.go
+++ b/pkg/gadgets/trace/dns/tracer/tracer.go
@@ -230,8 +230,8 @@ func bpfEventToDNSEvent(bpfEvent *dnsEventT, netns uint64) (*types.Event, error)
event.Nameserver = event.DstIP
}
- event.SrcPort = gadgets.Htons(bpfEvent.Sport)
- event.DstPort = gadgets.Htons(bpfEvent.Dport)
+ event.SrcPort = bpfEvent.Sport
+ event.DstPort = bpfEvent.Dport
event.Protocol = gadgets.ProtoString(int(bpfEvent.Proto))
// Convert name into a string with dots- Try to use
normalize()to set to 0 things you do not care in the integration test. I think this will permit you to avoid adding thecompFn()argument. - Update the documentation in
docs/gadgets/trace/dns.mdfor thekubernetespart (I will deal with theigpart once merged).
Feel free to ask for clarification and again thank you for working on this!
There was a problem hiding this comment.
Thanks @anubhabMajumdar for this contribution! I have a couple of comments but it already looks good.
@eiffel-fl @blanquicet |
There was a problem hiding this comment.
Hi,
as the others already said the changes look good, thanks for that :)
Another improvement could be using the KubeIPResolver so that IP addresses will be automatically resolved to pod names or service names.
I experimented with it in burak-ok@315746c . Feel free to cherry-pick and modify it.
$ ./kubectl-gadget trace dns -n default -o columns=node,namespace,pod,name,qr,qtype,src
NODE NAMESPACE POD NAME QR QTYPE SRC
minikube-docker default test-pod google.de. Q A p/default/test-pod:56088
minikube-docker default test-pod google.de. Q AAAA p/default/test-pod:56088
minikube-docker default test-pod google.de. R AAAA s/kube-system/kube-dns:53
minikube-docker default test-pod google.de. R A s/kube-system/kube-dns:53
A couple of CI pipelines failed due to small typo in the test and formatting issues. I have fixed those. |
Nice one! Yeah, we should definitely do it. However, I think it could be done in another PR as this one, IMO, is ready to go! |
Hey, addressed Alban's comments and fixed one more issue with the integration test. Hopefully it will pass this time! |
Pipelines have passed, can I squash and merge from Github? |
What do you mean by squash? So, reworking your history would be welcomed. |
There was a problem hiding this comment.
LGTM, thanks!
So far in this project, we've used the merge button with the 'Merge commit' option without squashing commits from the web UI, so contributors can write the commit messages for individual commits, and I prefer to keep it this way for consistency.
For this PR, I think one commit is enough: the changes in the tests are not new tests but necessary changes to make the existing tests pass. Could you rebase the history in one patch?
As the only modifications you bring to the tests are modifications meant for it to pass, it can be part of the same commit than the one adding the feature. Nonetheless, we will not use GitHub feature to squash everything before we merge. |
Add the following fields to DNS gadget event: SrcIP DstIP SrcPort DstPort Protocol Also, enhance the intergration tests to test the changes. Signed-off-by: Anubhab Majumdar <anmajumdar@microsoft.com>
anubhabMajumdar
force-pushed
the
topic/anmajumdar/five-tuple-dns
branch
from
929c24e
to
b9e7897
Compare
Cleaned up the commits; let me know if this works! Thanks! |
| protoStr := fmt.Sprintf("UNKNOWN#%d", proto) | ||
| switch proto { | ||
| case unix.IPPROTO_TCP: | ||
| protoStr = "TCP" |
There was a problem hiding this comment.
I am wondering if this change has consequences regarding the documentation.
If so, I will tackle them in a follow-up PR.
| @@ -34,6 +34,11 @@ struct event_t { | |||
| }; | |||
| __u16 af; // AF_INET or AF_INET6 | |||
|
|
|||
| // Internet protocol and port numbers. | |||
There was a problem hiding this comment.
We should definitely check this structure padding.
Add more fields to DNS gadget event
Add the following fields to
DNSgadget event:How to use
Testing done
minikubecluster, snippet added above