feat: use non-connected socket for UDP resolver

This diff modifies the UDP resolver inside netxlite to use a
non-connected socket. We are doing this because we would like
to see a better error than "connection_refused" when there's
some ICMP error telling us (probably) that the target port
is unreachable.

While working on this topic, ensure there's not a lot of code
duplication by doing the following:

1. factoring out code to ParseUDPAddr that was used both by
internal/netxlite/quic.go and internal/dnsping;

2. ensuring we use the same UDP resolver engine to implement both
the UDP resolver and the dnsping.

While there, discover that we were not validating the query ID
of received replies 🤦 and ensure now we do that.

While there, recognize that we could use a better name for all the
structs implementing a given kind of DNS transport.

While there, recognize that we should definitely skip the dnsping
entries that failed when building the URLAddress lists.

internal/dnsping/engine.go

@@ -7,10 +7,7 @@ package dnsping
 //
 
 import (
-	"errors"
-	"net"
 	"sort"
-	"strconv"
 	"sync"
 	"time"
 
@@ -266,110 +263,97 @@ type resultWrapper struct {
 // avoid this issue by using a new UDP socket for each ping.
 func (e *Engine) singlePinger(wg *sync.WaitGroup, plan *SinglePingPlan,
 	deadline time.Time, out chan<- *resultWrapper) {
-	defer wg.Done() // synchronize with the parent
+
+	// synchronize with the parent
+	defer wg.Done()
 
 	// encode the query
-	data, qid, err := e.Encoder.Encode(plan.Domain, plan.QueryType, false)
+	rawQuery, qid, err := e.Encoder.Encode(plan.Domain, plan.QueryType, false)
 	if err != nil {
 		e.Logger.Warnf("dnsping: cannot encode query: %s", err.Error())
 		out <- &resultWrapper{Err: err}
 		return
 	}
 
-	// resolve the destination address
-	destAddr, err := e.resolveAddr(plan.ResolverAddress)
+	// send the query
+	pconn, expectedAddr, err := netxlite.DNSOverUDPWriteRawQueryTo(
+		e.Listener, plan.ResolverAddress, rawQuery)
 	if err != nil {
-		e.Logger.Warnf("dnsping: cannot resolve addr: %s", err.Error())
-		out <- &resultWrapper{Err: err}
-		return
-	}
-
-	// create UDP socket.
-	pconn, err := e.Listener.Listen(&net.UDPAddr{})
-	if err != nil {
-		e.Logger.Warnf("dnsping: cannot create UDP socket: %s", err.Error())
+		e.Logger.Warnf("dnsping: cannot send query: %s", err.Error())
 		out <- &resultWrapper{Err: err}
 		return
 	}
 	defer pconn.Close() // we own the connection
 
+	// start collecting replies
+	var flags int64
+	flags |= netxlite.DNSOverUDPIncludeRepliesFromUnexpectedServers
+	flags |= netxlite.DNSOverUDPCollectMultipleReplies
+	flags |= netxlite.DNSOverUDPOmitTimeoutIfSomeRepliesReturned
+	rrch := netxlite.DNSOverUDPReadRawRepliesFrom(pconn, expectedAddr, deadline, flags)
+
 	// start filling in the result struct
 	result := &SinglePingResult{
 		ID:              e.IDGenerator.Next(),
 		ResolverAddress: plan.ResolverAddress,
 		Domain:          plan.Domain,
 		QueryType:       plan.QueryType,
 		QueryID:         qid,
-		Query:           data,
+		Query:           rawQuery,
 		Started:         time.Now(),
 		Replies:         []*SinglePingReply{},
 	}
 
-	// ensure we eventually stop
-	pconn.SetDeadline(deadline)
-
-	// send query to the remote server
-	if _, err := pconn.WriteTo(data, destAddr); err != nil {
-		e.Logger.Warnf("dnsping: cannot send: %s", err.Error())
-		out <- &resultWrapper{Err: err}
-		return
-	}
-
-	// collect responses until the deadline expires
-	for {
-		buffer := make([]byte, 1<<13) // definitely enough room
-		count, srcAddr, err := pconn.ReadFrom(buffer)
-		if err != nil {
-			if err.Error() != netxlite.FailureGenericTimeoutError {
-				e.Logger.Warnf("dnsping: cannot recv: %s", err.Error())
-			}
-			break
-		}
-		e.received(result, time.Now(), buffer[:count], srcAddr)
+	// process raw round trip results
+	for rr := range rrch {
+		e.received(plan.ResolverAddress, result, rr)
 	}
 
 	// send result to parent.
 	out <- &resultWrapper{Result: result}
 }
 
-// ErrNotIPAddr indicates that you passed DNS ping an endpoint
-// address containing a domain name instead of an IP addr.
-var ErrNotIPAddr = errors.New("dnsping: passed an address containing a domain name")
-
-// resolveAddr maps address to an UDPAddr.
-func (e *Engine) resolveAddr(address string) (*net.UDPAddr, error) {
-	addr, port, err := net.SplitHostPort(address)
-	if err != nil {
-		return nil, err
-	}
-	ipAddr := net.ParseIP(addr)
-	if ipAddr == nil {
-		return nil, ErrNotIPAddr
-	}
-	dport, err := strconv.Atoi(port)
-	if err != nil {
-		return nil, err
-	}
-	udpAddr := &net.UDPAddr{
-		IP:   ipAddr,
-		Port: dport,
-		Zone: "",
-	}
-	return udpAddr, nil
-}
-
 // received is called when recvfrom returns successfully.
-func (e *Engine) received(
-	result *SinglePingResult, now time.Time, data []byte, srcAddr net.Addr) {
-	reply, err := e.Decoder.ParseReply(data)
+func (e *Engine) received(sourceAddress string,
+	result *SinglePingResult, rr *netxlite.DNSOverUDPRawReply) {
+	if rr.Error != nil {
+		result.Replies = append(result.Replies, &SinglePingReply{
+			ID:            e.IDGenerator.Next(),
+			SourceAddress: sourceAddress,
+			Reply:         []byte{},
+			Error:         archival.NewFlatFailure(rr.Error),
+			Finished:      rr.Received,
+			Rcode:         "",
+			Addresses:     []string{},
+			ALPNs:         []string{},
+		})
+		return
+	}
+	reply, err := e.Decoder.ParseReply(rr.RawReply, result.QueryID)
 	if err != nil {
-		// TODO(bassosimone): should we store this message?
-		e.Logger.Warnf("dnsping: cannot parse reply: %s", err.Error())
+		result.Replies = append(result.Replies, &SinglePingReply{
+			ID:            e.IDGenerator.Next(),
+			SourceAddress: sourceAddress,
+			Reply:         rr.RawReply,
+			Error:         archival.NewFlatFailure(err),
+			Finished:      rr.Received,
+			Rcode:         "",
+			Addresses:     []string{},
+			ALPNs:         []string{},
+		})
 		return
 	}
-	if reply.Id != result.QueryID {
-		// TODO(bassosimone): should we store this message?
-		e.Logger.Warnf("dnsping: reply with unknown ID: %s", err.Error())
+	if !rr.ValidSourceAddr {
+		result.Replies = append(result.Replies, &SinglePingReply{
+			ID:            e.IDGenerator.Next(),
+			SourceAddress: sourceAddress,
+			Reply:         rr.RawReply,
+			Error:         archival.FlatFailure(netxlite.FailureDNSReplyFromUnexpectedServer),
+			Finished:      rr.Received,
+			Rcode:         dns.RcodeToString[reply.Rcode],
+			Addresses:     []string{},
+			ALPNs:         []string{},
+		})
 		return
 	}
 	emojis := map[bool]string{
@@ -379,15 +363,15 @@ func (e *Engine) received(
 	emoji := emojis[len(result.Replies) > 0]
 	e.Logger.Infof("%s [dnsping] %s for %s/%s from %s in %s with dns.id %d",
 		emoji, dns.RcodeToString[reply.Rcode], result.Domain,
-		result.QueryTypeAsString(), srcAddr.String(),
-		now.Sub(result.Started), reply.Id)
+		result.QueryTypeAsString(), sourceAddress,
+		rr.Received.Sub(result.Started), reply.Id)
 	addrs, alpns, err := e.finishParsing(result.QueryType, reply)
 	result.Replies = append(result.Replies, &SinglePingReply{
 		ID:            e.IDGenerator.Next(),
-		SourceAddress: srcAddr.String(),
-		Reply:         data,
+		SourceAddress: sourceAddress,
+		Reply:         rr.RawReply,
 		Error:         e.errorToWrappedFlatFailure(err),
-		Finished:      now,
+		Finished:      rr.Received,
 		Rcode:         dns.RcodeToString[reply.Rcode],
 		Addresses:     addrs,
 		ALPNs:         alpns,

internal/dnsping/urladdress.go

@@ -41,6 +41,9 @@ func (spr *SinglePingResult) DNSLookupMeasurementList(
 		return
 	}
 	for _, entry := range spr.Replies {
+		if entry.Error != "" {
+			continue // ensure we skip any ping attempt that failed
+		}
 		out = append(out, &measurex.DNSLookupMeasurement{
 			ID:               entry.ID,
 			URLMeasurementID: urlMeasurementID,

internal/measurex/library.go

@@ -226,12 +226,12 @@ type netxliteLibrary struct{}
 
 func (nl *netxliteLibrary) NewDNSOverUDPTransport(
 	dialer model.Dialer, address string) model.DNSTransport {
-	return netxlite.NewDNSOverUDP(dialer, address)
+	return netxlite.NewDNSOverUDPTransport(dialer, address)
 }
 
 func (nl *netxliteLibrary) NewDNSOverHTTPSTransport(
 	clnt model.HTTPClient, network, address string) model.DNSTransport {
-	return &netxlite.DNSOverHTTPS{
+	return &netxlite.DNSOverHTTPSTransport{
 		Client:       clnt,
 		URL:          address,
 		HostOverride: "",

internal/model/netx.go

@@ -27,6 +27,8 @@ type DNSDecoder interface {
 	//
 	// - data contains the reply bytes read from a DNSTransport
 	//
+	// - queryID is the original query ID
+	//
 	// Returns:
 	//
 	// - on success, a list of IP addrs inside the reply and a nil error
@@ -38,7 +40,7 @@ type DNSDecoder interface {
 	//
 	// Note that the error returned by this function is not wrapped and
 	// it is your responsiblity to wrap it if needed.
-	DecodeLookupHost(qtype uint16, data []byte) ([]string, error)
+	DecodeLookupHost(qtype uint16, data []byte, queryID uint16) ([]string, error)
 
 	// DecodeHTTPS decodes an HTTPS reply.
 	//
@@ -54,7 +56,7 @@ type DNSDecoder interface {
 	//
 	// Note that the error returned by this function is not wrapped and
 	// it is your responsiblity to wrap it if needed.
-	DecodeHTTPS(data []byte) (*HTTPSSvc, error)
+	DecodeHTTPS(data []byte, queryID uint16) (*HTTPSSvc, error)
 
 	// ParseReply parses a reply without decoding it. This function
 	// will ONLY return error if data is not a valid DNS message. In
@@ -65,7 +67,7 @@ type DNSDecoder interface {
 	//
 	// Note that the error returned by this function is not wrapped and
 	// it is your responsiblity to wrap it if needed.
-	ParseReply(data []byte) (*dns.Msg, error)
+	ParseReply(data []byte, queryID uint16) (*dns.Msg, error)
 
 	// DecodeReplyLookupHost is like DecodeLookupHost but acts
 	// on an already parsed reply rather than on data.

internal/netxlite/classify.go

@@ -287,6 +287,9 @@ func ClassifyResolverError(err error) string {
 	if errors.Is(err, ErrOODNSServfail) {
 		return FailureDNSServfailError
 	}
+	if errors.Is(err, ErrDNSReplyWithWrongQueryID) {
+		return FailureDNSReplyWithWrongQueryID
+	}
 	return classifyGenericError(err)
 }
 

internal/netxlite/dnsdecoder.go

@@ -1,18 +1,26 @@
 package netxlite
 
 import (
+	"errors"
+
 	"github.com/bassosimone/websteps-illustrated/internal/model"
 	"github.com/miekg/dns"
 )
 
 // DNSDecoderMiekg uses github.com/miekg/dns to implement the Decoder.
 type DNSDecoderMiekg struct{}
 
-func (d *DNSDecoderMiekg) ParseReply(data []byte) (*dns.Msg, error) {
+// ErrDNSReplyWithWrongQueryID indicates we have got a DNS reply with the wrong queryID.
+var ErrDNSReplyWithWrongQueryID = errors.New(FailureDNSReplyWithWrongQueryID)
+
+func (d *DNSDecoderMiekg) ParseReply(data []byte, queryID uint16) (*dns.Msg, error) {
 	reply := &dns.Msg{}
 	if err := reply.Unpack(data); err != nil {
 		return nil, err
 	}
+	if reply.Id != queryID {
+		return nil, ErrDNSReplyWithWrongQueryID
+	}
 	return reply, nil
 }
 
@@ -33,8 +41,8 @@ func (d *DNSDecoderMiekg) rcodeToError(reply *dns.Msg) error {
 	}
 }
 
-func (d *DNSDecoderMiekg) DecodeHTTPS(data []byte) (*model.HTTPSSvc, error) {
-	reply, err := d.ParseReply(data)
+func (d *DNSDecoderMiekg) DecodeHTTPS(data []byte, queryID uint16) (*model.HTTPSSvc, error) {
+	reply, err := d.ParseReply(data, queryID)
 	if err != nil {
 		return nil, err
 	}
@@ -80,8 +88,9 @@ func (d *DNSDecoderMiekg) DecodeReplyLookupHTTPS(reply *dns.Msg) (*model.HTTPSSv
 	return out, nil
 }
 
-func (d *DNSDecoderMiekg) DecodeLookupHost(qtype uint16, data []byte) ([]string, error) {
-	reply, err := d.ParseReply(data)
+func (d *DNSDecoderMiekg) DecodeLookupHost(
+	qtype uint16, data []byte, queryID uint16) ([]string, error) {
+	reply, err := d.ParseReply(data, queryID)
 	if err != nil {
 		return nil, err
 	}

internal/netxlite/dnsoverhttps.go

@@ -11,8 +11,8 @@ import (
 	"github.com/bassosimone/websteps-illustrated/internal/model"
 )
 
-// DNSOverHTTPS is a DNS-over-HTTPS DNSTransport.
-type DNSOverHTTPS struct {
+// DNSOverHTTPSTransport is a DNS-over-HTTPS DNSTransport.
+type DNSOverHTTPSTransport struct {
 	// Client is the MANDATORY http client to use.
 	Client model.HTTPClient
 
@@ -28,22 +28,22 @@ type DNSOverHTTPS struct {
 	Protocol string
 }
 
-// NewDNSOverHTTPS creates a new DNSOverHTTPS instance.
+// NewDNSOverHTTPSTransport creates a new DNSOverHTTPS instance.
 //
 // Arguments:
 //
 // - client in http.Client-like type (e.g., http.DefaultClient);
 //
 // - URL is the DoH resolver URL (e.g., https://1.1.1.1/dns-query).
-func NewDNSOverHTTPS(client model.HTTPClient, URL string) *DNSOverHTTPS {
-	return NewDNSOverHTTPSWithHostOverride(client, URL, "")
+func NewDNSOverHTTPSTransport(client model.HTTPClient, URL string) *DNSOverHTTPSTransport {
+	return NewDNSOverHTTPSTransportWithHostOverride(client, URL, "")
 }
 
-// NewDNSOverHTTPSWithHostOverride creates a new DNSOverHTTPS
+// NewDNSOverHTTPSTransportWithHostOverride creates a new DNSOverHTTPSTransport
 // with the given Host header override.
-func NewDNSOverHTTPSWithHostOverride(
-	client model.HTTPClient, URL, hostOverride string) *DNSOverHTTPS {
-	return &DNSOverHTTPS{
+func NewDNSOverHTTPSTransportWithHostOverride(
+	client model.HTTPClient, URL, hostOverride string) *DNSOverHTTPSTransport {
+	return &DNSOverHTTPSTransport{
 		Client:       client,
 		URL:          URL,
 		HostOverride: hostOverride,
@@ -52,7 +52,7 @@ func NewDNSOverHTTPSWithHostOverride(
 }
 
 // RoundTrip sends a query and receives a reply.
-func (t *DNSOverHTTPS) RoundTrip(ctx context.Context, query []byte) ([]byte, error) {
+func (t *DNSOverHTTPSTransport) RoundTrip(ctx context.Context, query []byte) ([]byte, error) {
 	ctx, cancel := context.WithTimeout(ctx, 45*time.Second)
 	defer cancel()
 	req, err := http.NewRequest("POST", t.URL, bytes.NewReader(query))
@@ -80,23 +80,23 @@ func (t *DNSOverHTTPS) RoundTrip(ctx context.Context, query []byte) ([]byte, err
 }
 
 // RequiresPadding returns true for DoH according to RFC8467.
-func (t *DNSOverHTTPS) RequiresPadding() bool {
+func (t *DNSOverHTTPSTransport) RequiresPadding() bool {
 	return true
 }
 
 // Network returns the transport network, i.e., "doh".
-func (t *DNSOverHTTPS) Network() string {
+func (t *DNSOverHTTPSTransport) Network() string {
 	return t.Protocol
 }
 
 // Address returns the URL we're using for the DoH server.
-func (t *DNSOverHTTPS) Address() string {
+func (t *DNSOverHTTPSTransport) Address() string {
 	return t.URL
 }
 
 // CloseIdleConnections closes idle connections, if any.
-func (t *DNSOverHTTPS) CloseIdleConnections() {
+func (t *DNSOverHTTPSTransport) CloseIdleConnections() {
 	t.Client.CloseIdleConnections()
 }
 
-var _ model.DNSTransport = &DNSOverHTTPS{}
+var _ model.DNSTransport = &DNSOverHTTPSTransport{}