From c0cd3ca5ada275389f2ff579aa9712ac808974ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 8 Mar 2022 12:11:47 +0100 Subject: [PATCH] UPnP/SSDP: Cleaner, easier to test (and actually tested) code Convert "get own URLs" code into nicer chunks, and test those. This minimises the code that actually depends on the available network interfaces, and increases test coverage. Found a few bugs too. --- cmd/ssdp_server_poc/main.go | 7 +- internal/own_url/interfaces.go | 4 +- internal/own_url/own_url.go | 128 ++++++++++++++++--------------- internal/own_url/own_url_test.go | 57 ++++++++++++-- 4 files changed, 124 insertions(+), 72 deletions(-) diff --git a/cmd/ssdp_server_poc/main.go b/cmd/ssdp_server_poc/main.go index 836fe992..952b3b16 100644 --- a/cmd/ssdp_server_poc/main.go +++ b/cmd/ssdp_server_poc/main.go @@ -24,7 +24,7 @@ func main() { panic(err) } - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) defer cancel() // Handle Ctrl+C @@ -38,7 +38,10 @@ func main() { } }() - urls, err := own_url.AvailableURLs(ctx, "http", ":8080", false) + urls, err := own_url.AvailableURLs("http", ":8080") + if err != nil { + log.Fatal().Err(err).Msg("unable to construct list of URLs") + } urlStrings := []string{} for _, url := range urls { urlStrings = append(urlStrings, url.String()) diff --git a/internal/own_url/interfaces.go b/internal/own_url/interfaces.go index 61c93906..a9a60642 100644 --- a/internal/own_url/interfaces.go +++ b/internal/own_url/interfaces.go @@ -36,8 +36,8 @@ var ( ) // networkInterfaces returns a list of interface addresses. -// Only those addresses that can be eached by a unicast TCP/IP connection are returned. -func networkInterfaces(includeLinkLocal, includeLocalhost bool) ([]net.IP, error) { +// Only those addresses that can be reached by a unicast TCP/IP connection are returned. +func networkInterfaces() ([]net.IP, error) { log.Debug().Msg("iterating over all network interfaces") interfaces, err := net.Interfaces() diff --git a/internal/own_url/own_url.go b/internal/own_url/own_url.go index 61394b0b..57c036c2 100644 --- a/internal/own_url/own_url.go +++ b/internal/own_url/own_url.go @@ -2,7 +2,6 @@ package own_url import ( - "context" "fmt" "net" "net/url" @@ -10,69 +9,79 @@ import ( "github.com/rs/zerolog/log" ) -func AvailableURLs(ctx context.Context, schema, listen string, includeLocal bool) ([]*url.URL, error) { - var ( - host, port string - portnum int - err error - ) - +func AvailableURLs(schema, listen string) ([]url.URL, error) { if listen == "" { panic("empty 'listen' parameter") } - // Figure out which port we're supposted to listen on. - if host, port, err = net.SplitHostPort(listen); err != nil { - return nil, fmt.Errorf("unable to split host and port in address '%s': %w", listen, err) - } - if portnum, err = net.DefaultResolver.LookupPort(ctx, "listen", port); err != nil { - return nil, fmt.Errorf("unable to look up port '%s': %w", port, err) + hostURL := specificHostURL(schema, listen) + if hostURL != nil { + return []url.URL{*hostURL}, nil } + log.Debug().Str("listen", listen).Msg("not listening on any specific host") - // If the host is empty or ::0/0.0.0.0, show a list of URLs to connect to. - listenSpecificHost := false - var ip net.IP - if host != "" { - ip = net.ParseIP(host) - if ip == nil { - addrs, erresolve := net.DefaultResolver.LookupHost(ctx, host) - if erresolve != nil { - return nil, fmt.Errorf("unable to resolve listen host '%v': %w", host, erresolve) - } - if len(addrs) > 0 { - ip = net.ParseIP(addrs[0]) - } - } - if ip != nil && !ip.IsUnspecified() { - listenSpecificHost = true - } - } - - if listenSpecificHost { - // We can just construct a URL here, since we know it's a specific host anyway. - log.Debug().Str("host", ip.String()).Msg("listening on host") - - link := fmt.Sprintf("%s://%s:%d/", schema, host, portnum) - myURL, errparse := url.Parse(link) - if errparse != nil { - return nil, fmt.Errorf("unable to parse listen URL %s: %w", link, errparse) - } - return []*url.URL{myURL}, nil - } - - log.Debug().Str("host", host).Msg("not listening on any specific host") - - addrs, err := networkInterfaces(false, includeLocal) - if err == ErrNoInterface { - addrs, err = networkInterfaces(true, includeLocal) - } + addrs, err := networkInterfaces() if err != nil { return nil, err } log.Debug().Msg("iterating network interfaces to find possible URLs for Flamenco Manager.") + return urlsForNetworkInterfaces(schema, listen, addrs) +} - links := make([]*url.URL, 0) +// specificHostURL returns the hosts's URL if the "listen" string is specific enough, otherwise nil. +// Examples: "192.168.0.1:8080" is specific enough, "0.0.0.0:8080" and ":8080" are not. +func specificHostURL(scheme, listen string) *url.URL { + var ( + host string + err error + ) + + // Figure out which port we're supposted to listen on. + if host, _, err = net.SplitHostPort(listen); err != nil { + // This is annoying. SplitHostPort() doesn't return specific errors, so we + // have to test on the error message to see what's the problem. + // A missing port is fine, but other errors are not. + addrErr := err.(*net.AddrError) + if addrErr.Err != "missing port in address" { + log.Warn().Str("address", listen).Err(err).Msg("unable to split host and port in address") + return nil + } + + // 'listen' doesn't have a port number, so it's just the host. + host = listen + } + if host == "" { + // An empty host is never specific enough. + return nil + } + + ip := net.ParseIP(host) + if ip != nil && ip.IsUnspecified() { + // The host is "::0" or "0.0.0.0"; not specific. + return nil + } + + // We can just construct a URL here, since we know it's a specific host anyway. + return &url.URL{ + Scheme: scheme, + Host: listen, + Path: "/", + } +} + +func urlsForNetworkInterfaces(scheme, listen string, addrs []net.IP) ([]url.URL, error) { + // Find the port number in the 'listen' string. + var ( + port string + err error + ) + // Get the port number as integer. + if _, port, err = net.SplitHostPort(listen); err != nil { + return nil, fmt.Errorf("unable to split host and port in address %q", listen) + } + + links := make([]url.URL, 0) for _, addr := range addrs { var strAddr string if ipv4 := addr.To4(); ipv4 != nil { @@ -81,17 +90,12 @@ func AvailableURLs(ctx context.Context, schema, listen string, includeLocal bool strAddr = fmt.Sprintf("[%s]", addr) } - constructedURL := fmt.Sprintf("%s://%s:%d/", schema, strAddr, portnum) - parsedURL, err := url.Parse(constructedURL) - if err != nil { - log.Warn(). - Str("address", strAddr). - Str("url", constructedURL). - Err(err). - Msg("skipping address, as it results in an unparseable URL") - continue + urlForAddr := url.URL{ + Scheme: scheme, + Host: fmt.Sprintf("%s:%s", strAddr, port), + Path: "/", } - links = append(links, parsedURL) + links = append(links, urlForAddr) } return links, nil diff --git a/internal/own_url/own_url_test.go b/internal/own_url/own_url_test.go index 8996c18f..61e8f2af 100644 --- a/internal/own_url/own_url_test.go +++ b/internal/own_url/own_url_test.go @@ -2,25 +2,70 @@ package own_url import ( - "context" + "net" "testing" "time" "github.com/mattn/go-colorable" "github.com/rs/zerolog" "github.com/rs/zerolog/log" + "github.com/stretchr/testify/assert" ) func TestAvailableURLs(t *testing.T) { output := zerolog.ConsoleWriter{Out: colorable.NewColorableStdout(), TimeFormat: time.RFC3339} log.Logger = log.Output(output) - ctx, ctxCancel := context.WithTimeout(context.Background(), 10*time.Second) - defer ctxCancel() - - _, err := AvailableURLs(ctx, "http", ":9999", true) + // This should run without errors. It's hard to predict the returned URLs + // though, as they depend on the local network devices. + urls, err := AvailableURLs("http", ":9999") if err != nil { t.Fatal(err) } - // t.Fatalf("urls: %v", urls) + assert.NotEmpty(t, urls, "expected at least one URL to be returned") +} + +func TestSpecificHostURL(t *testing.T) { + tests := []struct { + name string + expect string // Empty string encodes "expect nil pointer" + listen string + }{ + {"Specific IPv4 with port", "http://192.168.0.1:8080/", "192.168.0.1:8080"}, + {"Specific IPv4 without port", "http://192.168.0.1/", "192.168.0.1"}, + {"Specific IPv6 with port", "http://[fe80::5054:ff:fede:2ad7]:8080/", "[fe80::5054:ff:fede:2ad7]:8080"}, + {"Specific IPv6 without port", "http://[fe80::5054:ff:fede:2ad7]/", "[fe80::5054:ff:fede:2ad7]"}, + + {"Wildcard IPv4", "", "0.0.0.0:8080"}, + {"Wildcard IPv6", "", "[::0]:8080"}, + {"No host, just port", "", ":8080"}, + + {"Invalid address", "http://this%20is%20not%20an%20address/", "this is not an address"}, + {"Invalid port", "", "192.168.0.1::too-many-colons"}, + } + + for _, test := range tests { + actual := specificHostURL("http", test.listen) + if test.expect == "" { + assert.Nil(t, actual, "for input %q", test.listen) + continue + } + if actual == nil { + t.Errorf("returned URL is nil for input %q", test.listen) + continue + } + assert.Equal(t, test.expect, actual.String(), "for input %q", test.listen) + } +} + +func TestURLsForNetworkInterfaces(t *testing.T) { + addrs := []net.IP{linkLocalIPv6, lanIPv4} + urls, err := urlsForNetworkInterfaces("http", ":9999", addrs) + if err != nil { + t.Fatal(err) + } + + assert.Len(t, urls, 2) + assert.Equal(t, "http://[fe80::5054:ff:fede:2ad7]:9999/", urls[0].String()) + assert.Equal(t, "http://192.168.0.1:9999/", urls[1].String()) }