normalize route creation in xresolver (#494)

* add check if port is empty

* updated changelog

* add unit tests

Author: kcajmagic

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+## [v1.10.3]
+### Fixed
+- Fixed xresolver failing to create route when default port is used. [#494](https://github.com/xmidt-org/webpa-common/pull/494)
+
 ## [v1.10.2]
 ### Fixed
 - Fixed `ConsulWatch` in xresolver by storing and watching the correct part of the url. [#490](https://github.com/xmidt-org/webpa-common/pull/490)
@@ -106,7 +110,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
  - The first official release. We will be better about documenting changes 
    moving forward.
 
-[Unreleased]: https://github.com/xmidt-org/webpa-common/compare/v1.10.2...HEAD
+[Unreleased]: https://github.com/xmidt-org/webpa-common/compare/v1.10.3...HEAD
+[v1.10.2]: https://github.com/xmidt-org/webpa-common/compare/v1.10.2...v1.10.3
 [v1.10.2]: https://github.com/xmidt-org/webpa-common/compare/v1.10.1...v1.10.2
 [v1.10.1]: https://github.com/xmidt-org/webpa-common/compare/v1.10.0...v1.10.1
 [v1.10.0]: https://github.com/xmidt-org/webpa-common/compare/v1.9.0...v1.10.0

xresolver/types.go

@@ -3,9 +3,12 @@ package xresolver
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"net/url"
+	"regexp"
 	"strconv"
+	"strings"
 	"sync"
 )
 
@@ -40,17 +43,73 @@ type Route struct {
 	Port   int
 }
 
+// instancePattern is what NormalizeInstance expects to be matched.  This pattern is intentionally liberal, and allows
+// URIs that are disallowed under https://www.ietf.org/rfc/rfc2396.txt
+var instancePattern = regexp.MustCompile("^((?P<scheme>.+)://)?(?P<address>[^:]+)(:(?P<port>[0-9]+))?$")
+
+// NormalizeRoute canonicalizes a route string from a backend
+//
+// This function performs the following on the instance:
+//   (1) If instance is a blank string, e.g. contains only whitespace or is empty, an empty string is returned with an error
+//   (2) If the instance with whitespace trimmed off is not a valid instance, an error is returned with the trimmed instance string.
+//       This function is intentionally lenient on what is a valid instance string, e.g. "foobar.com", "foobar.com:8080", "asdf://foobar.com", etc
+//   (3) If there was no scheme prepended to the route, http will be used
+func NormalizeRoute(route string) (string, error) {
+	route = strings.TrimSpace(route)
+	if len(route) == 0 {
+		return route, errors.New("empty route is not allowed")
+	}
+
+	submatches := instancePattern.FindStringSubmatch(route)
+	if len(submatches) == 0 {
+		return route, fmt.Errorf("invalid route: %s", route)
+	}
+
+	var (
+		scheme  = submatches[2]
+		address = submatches[3]
+	)
+
+	if len(scheme) == 0 {
+		scheme = "http"
+	}
+
+	var port int
+	if portValue := submatches[5]; len(portValue) > 0 {
+		var err error
+		port, err = strconv.Atoi(submatches[5])
+		if err != nil {
+			// NOTE: Shouldn't ever hit this case, because the port is constrained by the regexp to be numeric
+			return route, err
+		}
+		return fmt.Sprintf("%s://%s:%d", scheme, address, port), nil
+
+	}
+	return fmt.Sprintf("%s://%s", scheme, address), nil
+}
+
 func CreateRoute(route string) (Route, error) {
+	route, err := NormalizeRoute(route)
+	if err != nil {
+		return Route{}, err
+	}
 	path, err := url.Parse(route)
 	if err != nil {
 		return Route{}, err
 	}
-	port, err := strconv.Atoi(path.Port())
-	return Route{
+
+	newRoute := Route{
 		Scheme: path.Scheme,
 		Host:   path.Hostname(),
-		Port:   port,
-	}, err
+	}
+	if path.Port() != "" {
+		port, err := strconv.Atoi(path.Port())
+		newRoute.Port = port
+		if err != nil {
+			return newRoute, err
+		}
+	}
+	return newRoute, nil
 }
 
 func (r Route) String() string {

xresolver/types_test.go

@@ -287,3 +287,41 @@ func IPv4Address(ipInt int64) string {
 
 	return b0 + "." + b1 + "." + b2 + "." + b3
 }
+
+func TestRouteCreation(t *testing.T) {
+	var (
+		testData = []struct {
+			name         string
+			route        string
+			expected     Route
+			expectsError bool
+		}{
+			{"empty route", "", Route{Scheme: "", Host: "", Port: 0}, true},
+			{"whitespace characters", "     \t\n\r", Route{Scheme: "", Host: "", Port: 0}, true},
+			{"bad url", "blah:blah:blah", Route{Scheme: "", Host: "", Port: 0}, true},
+			{"bad url with whitespace", " blah:blah:blah ", Route{Scheme: "", Host: "", Port: 0}, true},
+			{"no scheme with port", "somehost.com:8080", Route{Scheme: "http", Host: "somehost.com", Port: 8080}, false},
+			{"no scheme with port with whitespace", " somehost.com:8080 ", Route{Scheme: "http", Host: "somehost.com", Port: 8080}, false},
+			{"no scheme no port", "somehost.com", Route{Scheme: "http", Host: "somehost.com", Port: 0}, false},
+			{"ftp with port", "ftp://somehost.com:6060", Route{Scheme: "ftp", Host: "somehost.com", Port: 6060}, false},
+			{"http", "http://foobar.com", Route{Scheme: "http", Host: "foobar.com", Port: 0}, false},
+			{"https", "https://foobar.com", Route{Scheme: "https", Host: "foobar.com", Port: 0}, false},
+		}
+	)
+
+	for _, record := range testData {
+		t.Run(record.name, func(t *testing.T) {
+			var (
+				assert      = assert.New(t)
+				actual, err = CreateRoute(record.route)
+			)
+
+			assert.Equal(record.expected, actual)
+			if record.expectsError {
+				assert.Error(err)
+			} else {
+				assert.NoError(err)
+			}
+		})
+	}
+}