diff --git a/internal/measurexlite/dns.go b/internal/measurexlite/dns.go index fd86eaa..008b7d4 100644 --- a/internal/measurexlite/dns.go +++ b/internal/measurexlite/dns.go @@ -114,7 +114,7 @@ type DNSNetworkAddresser interface { func NewArchivalDNSLookupResultFromRoundTrip(index int64, started time.Duration, reso DNSNetworkAddresser, query model.DNSQuery, response model.DNSResponse, addrs []string, err error, finished time.Duration) *model.ArchivalDNSLookupResult { return &model.ArchivalDNSLookupResult{ - Answers: archivalAnswersFromAddrs(addrs), + Answers: newArchivalDNSAnswers(addrs, response), Engine: reso.Network(), Failure: tracex.NewFailure(err), Hostname: query.Domain(), @@ -125,8 +125,16 @@ func NewArchivalDNSLookupResultFromRoundTrip(index int64, started time.Duration, } } -// archivalAnswersFromAddrs generates model.ArchivalDNSAnswer from an array of addresses -func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) { +// newArchivalDNSAnswers generates []model.ArchivalDNSAnswer from [addrs] and [resp]. +func newArchivalDNSAnswers(addrs []string, resp model.DNSResponse) (out []model.ArchivalDNSAnswer) { + // Design note: in principle we might want to extract everything from the + // response but, when we're called by netxlite, netxlite has already extracted + // the addresses to return them to the caller, so I think it's fine to keep + // this extraction code as such rather than suppressing passing the addrs from + // netxlite. Also, a wrong IP address is a bug because netxlite should not + // return invalid IP addresses from its resolvers, so we want to know about that. + + // Include IP addresses extracted by netxlite for _, addr := range addrs { ipv6, err := netxlite.IsIPv6(addr) if err != nil { @@ -142,6 +150,7 @@ func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) { AnswerType: "A", Hostname: "", IPv4: addr, + IPv6: "", TTL: nil, }) case true: @@ -150,11 +159,32 @@ func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) { ASOrgName: org, AnswerType: "AAAA", Hostname: "", + IPv4: "", IPv6: addr, TTL: nil, }) } } + + // Include additional answer types when a response is available + if resp != nil { + + // Include CNAME if available + if cname, err := resp.DecodeCNAME(); err == nil && cname != "" { + out = append(out, model.ArchivalDNSAnswer{ + ASN: 0, + ASOrgName: "", + AnswerType: "CNAME", + Hostname: cname, + IPv4: "", + IPv6: "", + TTL: nil, + }) + } + + // TODO(bassosimone): what other fields generally present inside A/AAAA replies + // would it be useful to extract here? Perhaps, the SoA field? + } return } diff --git a/internal/measurexlite/dns_test.go b/internal/measurexlite/dns_test.go index fb0c94e..43cb25b 100644 --- a/internal/measurexlite/dns_test.go +++ b/internal/measurexlite/dns_test.go @@ -135,6 +135,9 @@ func TestNewResolver(t *testing.T) { } return []string{"1.1.1.1"}, nil }, + MockDecodeCNAME: func() (string, error) { + return "dns.google.", nil + }, } return response, nil }, @@ -177,7 +180,7 @@ func TestNewResolver(t *testing.T) { if ev.Engine != "mocked" { t.Fatal("unexpected engine") } - if len(ev.Answers) != 1 { + if len(ev.Answers) != 2 { t.Fatal("expected single answer in DNSLookup event") } if ev.QueryType == "A" && ev.Answers[0].IPv4 != "1.1.1.1" { @@ -186,6 +189,9 @@ func TestNewResolver(t *testing.T) { if ev.QueryType == "AAAA" && ev.Answers[0].IPv6 != "fe80::a00:20ff:feb9:4c54" { t.Fatal("unexpected AAAA query result") } + if ev.Answers[1].AnswerType != "CNAME " && ev.Answers[1].Hostname != "dns.google." { + t.Fatal("unexpected second answer (expected CNAME)", ev.Answers[1]) + } } }) }) @@ -205,6 +211,9 @@ func TestNewResolver(t *testing.T) { } return []string{"1.1.1.1"}, nil }, + MockDecodeCNAME: func() (string, error) { + return "dns.google.", nil + }, } return response, nil }, @@ -350,8 +359,12 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) { addrs := []string{"1.1.1.1"} finished := trace.TimeNow() // 1. fill the trace - err := trace.OnDelayedDNSResponse(started, txp, query, &mocks.DNSResponse{}, - addrs, nil, finished) + dnsResponse := &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "", netxlite.ErrOODNSNoAnswer + }, + } + err := trace.OnDelayedDNSResponse(started, txp, query, dnsResponse, addrs, nil, finished) // 2. read the trace got := trace.DelayedDNSResponseWithTimeout(context.Background(), time.Second) if err != nil { @@ -388,8 +401,12 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) { addrs := []string{"1.1.1.1"} finished := trace.TimeNow() // 1. attempt to write into the trace - err := trace.OnDelayedDNSResponse(started, txp, query, &mocks.DNSResponse{}, - addrs, nil, finished) + dnsResponse := &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "", netxlite.ErrOODNSNoAnswer + }, + } + err := trace.OnDelayedDNSResponse(started, txp, query, dnsResponse, addrs, nil, finished) if !errors.Is(err, ErrDelayedDNSResponseBufferFull) { t.Fatal("unexpected error", err) } @@ -427,10 +444,15 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) { addrs := []string{"1.1.1.1"} finished := trace.TimeNow() events := 4 + dnsResponse := &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "", netxlite.ErrOODNSNoAnswer + }, + } for i := 0; i < events; i++ { // fill the trace trace.delayedDNSResponse <- NewArchivalDNSLookupResultFromRoundTrip(trace.Index, started.Sub(trace.ZeroTime), - txp, query, &mocks.DNSResponse{}, addrs, nil, finished.Sub(trace.ZeroTime)) + txp, query, dnsResponse, addrs, nil, finished.Sub(trace.ZeroTime)) } ctx, cancel := context.WithCancel(context.Background()) cancel() // we ensure that the context cancels before draining all the events @@ -465,8 +487,13 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) { } addrs := []string{"1.1.1.1"} finished := trace.TimeNow() + dnsResponse := &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "", netxlite.ErrOODNSNoAnswer + }, + } trace.delayedDNSResponse <- NewArchivalDNSLookupResultFromRoundTrip(trace.Index, started.Sub(trace.ZeroTime), - txp, query, &mocks.DNSResponse{}, addrs, nil, finished.Sub(trace.ZeroTime)) + txp, query, dnsResponse, addrs, nil, finished.Sub(trace.ZeroTime)) got := trace.DelayedDNSResponseWithTimeout(context.Background(), time.Second) if len(got) != 1 { t.Fatal("unexpected output from trace") @@ -475,52 +502,156 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) { }) } -func TestAnswersFromAddrs(t *testing.T) { +func TestNewArchivalDNSAnswers(t *testing.T) { tests := []struct { - name string - args []string + name string + addrs []string + resp model.DNSResponse + expected []model.ArchivalDNSAnswer }{{ name: "with valid input", - args: []string{"1.1.1.1", "fe80::a00:20ff:feb9:4c54"}, + addrs: []string{ + "8.8.4.4", + "2001:4860:4860::8844", + }, + resp: nil, + expected: []model.ArchivalDNSAnswer{{ + ASN: 15169, + ASOrgName: "Google LLC", + AnswerType: "A", + Hostname: "", + IPv4: "8.8.4.4", + IPv6: "", + TTL: nil, + }, { + ASN: 15169, + ASOrgName: "Google LLC", + AnswerType: "AAAA", + Hostname: "", + IPv4: "", + IPv6: "2001:4860:4860::8844", + TTL: nil, + }}, }, { name: "with invalid IPv4 address", - args: []string{"1.1.1.1.1", "fe80::a00:20ff:feb9:4c54"}, + addrs: []string{ + "1.1.1.1.1", // invalid because it has five dots + "2001:4860:4860::8844", + }, + resp: nil, + expected: []model.ArchivalDNSAnswer{{ + ASN: 15169, + ASOrgName: "Google LLC", + AnswerType: "AAAA", + Hostname: "", + IPv4: "", + IPv6: "2001:4860:4860::8844", + TTL: nil, + }}, }, { name: "with invalid IPv6 address", - args: []string{"1.1.1.1", "fe80::a00:20ff:feb9:::4c54"}, + addrs: []string{ + "8.8.4.4", + "fe80::a00:20ff:feb9:::4c54", // invalid because it has ::: + }, + resp: nil, + expected: []model.ArchivalDNSAnswer{{ + ASN: 15169, + ASOrgName: "Google LLC", + AnswerType: "A", + Hostname: "", + IPv4: "8.8.4.4", + IPv6: "", + TTL: nil, + }}, }, { - name: "with empty input", - args: []string{}, + name: "with empty input", + addrs: []string{}, + resp: nil, + expected: nil, }, { - name: "with nil input", - args: nil, + name: "with nil input", + addrs: nil, + resp: nil, + expected: nil, + }, { + name: "with valid IPv4 address and CNAME", + addrs: []string{ + "8.8.8.8", + }, + resp: &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "dns.google.", nil + }, + }, + expected: []model.ArchivalDNSAnswer{{ + ASN: 15169, + ASOrgName: "Google LLC", + AnswerType: "A", + Hostname: "", + IPv4: "8.8.8.8", + IPv6: "", + TTL: nil, + }, { + ASN: 0, + ASOrgName: "", + AnswerType: "CNAME", + Hostname: "dns.google.", + IPv4: "", + IPv6: "", + TTL: nil, + }}, + }, { + name: "with valid IPv6 address and CNAME", + addrs: []string{ + "2001:4860:4860::8844", + }, + resp: &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "dns.google.", nil + }, + }, + expected: []model.ArchivalDNSAnswer{{ + ASN: 15169, + ASOrgName: "Google LLC", + AnswerType: "AAAA", + Hostname: "", + IPv4: "", + IPv6: "2001:4860:4860::8844", + TTL: nil, + }, { + ASN: 0, + ASOrgName: "", + AnswerType: "CNAME", + Hostname: "dns.google.", + IPv4: "", + IPv6: "", + TTL: nil, + }}, + }, { + name: "with DecodeCNAME error", + addrs: []string{}, + resp: &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "", errors.New("mocked errorr") + }, + }, + expected: nil, + }, { + name: "with DecodeCNAME success and no CNAME", + addrs: []string{}, + resp: &mocks.DNSResponse{ + MockDecodeCNAME: func() (string, error) { + return "", nil + }, + }, + expected: nil, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := archivalAnswersFromAddrs(tt.args) - var idx int - for _, inp := range tt.args { - ip6, err := netxlite.IsIPv6(inp) - if err != nil { - continue - } - if idx >= len(got) { - t.Fatal("unexpected array length") - } - answer := got[idx] - if ip6 { - if answer.AnswerType != "AAAA" || answer.IPv6 != inp { - t.Fatal("unexpected output", answer) - } - } else { - if answer.AnswerType != "A" || answer.IPv4 != inp { - t.Fatal("unexpected output", answer) - } - } - idx++ - } - if idx != len(got) { - t.Fatal("unexpected array length", len(got)) + got := newArchivalDNSAnswers(tt.addrs, tt.resp) + if diff := cmp.Diff(tt.expected, got); diff != "" { + t.Fatal(diff) } }) }