16561: Handle implicit port numbers in getListenAddress, add tests.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/lib/service/cmd.go b/lib/service/cmd.go
index b5e395bec828c879a9e86ac646cf82026873422c..9e45e0f7e828a340d5eee6d024eb9f8da21cf948 100644 (file)
--- a/lib/service/cmd.go
+++ b/lib/service/cmd.go
@@ -273,7 +273,15 @@ func getListenAddr(svcs arvados.Services, prog arvados.ServiceName, log logrus.F
                        // intermediate proxy/routing)
                        listenURL = internalURL
                }
                        // intermediate proxy/routing)
                        listenURL = internalURL
                }

-               listener, err := net.Listen("tcp", listenURL.Host)
+               listenAddr := listenURL.Host
+               if _, _, err := net.SplitHostPort(listenAddr); err != nil {
+                       // url "https://foo.example/" (with no
+                       // explicit port name/number) means listen on
+                       // the well-known port for the specified
+                       // protocol, "foo.example:https".
+                       listenAddr = net.JoinHostPort(listenAddr, listenURL.Scheme)
+               }
+               listener, err := net.Listen("tcp", listenAddr)

                if err == nil {
                        listener.Close()
                        return listenURL, internalURL, nil
                if err == nil {
                        listener.Close()
                        return listenURL, internalURL, nil

diff --git a/lib/service/cmd_test.go b/lib/service/cmd_test.go
index 10591d9b55cf44beb41e7a898a296f20a0aab851..7a1f98a8f0af2c3ac188fe15be171f6ca1e26f6b 100644 (file)
--- a/lib/service/cmd_test.go
+++ b/lib/service/cmd_test.go
@@ -11,7 +11,9 @@ import (
        "crypto/tls"
        "fmt"
        "io/ioutil"
        "crypto/tls"
        "fmt"
        "io/ioutil"

+       "net"

        "net/http"
        "net/http"

+       "net/url"

        "os"
        "testing"
        "time"
        "os"
        "testing"
        "time"


@@ -35,6 +37,124 @@ const (
        contextKey key = iota
 )
 
        contextKey key = iota
 )
 

+func (*Suite) TestGetListenAddress(c *check.C) {
+       // Find an available port on the testing host, so the test
+       // cases don't get confused by "already in use" errors.
+       listener, err := net.Listen("tcp", ":")
+       c.Assert(err, check.IsNil)
+       _, unusedPort, err := net.SplitHostPort(listener.Addr().String())
+       c.Assert(err, check.IsNil)
+       listener.Close()
+
+       defer os.Unsetenv("ARVADOS_SERVICE_INTERNAL_URL")
+       for idx, trial := range []struct {
+               // internalURL => listenURL, both with trailing "/"
+               // because config loader always adds it
+               internalURLs     map[string]string
+               envVar           string
+               expectErrorMatch string
+               expectLogsMatch  string
+               expectListen     string
+               expectInternal   string
+       }{
+               {
+                       internalURLs:   map[string]string{"http://localhost:" + unusedPort + "/": ""},
+                       expectListen:   "http://localhost:" + unusedPort + "/",
+                       expectInternal: "http://localhost:" + unusedPort + "/",
+               },
+               { // implicit port 80 in InternalURLs
+                       internalURLs:     map[string]string{"http://localhost/": ""},
+                       expectListen:     "http://localhost/",
+                       expectInternal:   "http://localhost/",
+                       expectErrorMatch: `.*:80: bind: permission denied`,
+               },
+               { // implicit port 443 in InternalURLs
+                       internalURLs:   map[string]string{"https://host.example/": "http://localhost:" + unusedPort + "/"},
+                       expectListen:   "http://localhost:" + unusedPort + "/",
+                       expectInternal: "https://host.example/",
+               },
+               {
+                       internalURLs:   map[string]string{"https://hostname.example/": "http://localhost:8000/"},
+                       expectListen:   "http://localhost:8000/",
+                       expectInternal: "https://hostname.example/",
+               },
+               {
+                       internalURLs: map[string]string{
+                               "https://hostname1.example/": "http://localhost:12435/",
+                               "https://hostname2.example/": "http://localhost:" + unusedPort + "/",
+                       },
+                       envVar:         "https://hostname2.example", // note this works despite missing trailing "/"
+                       expectListen:   "http://localhost:" + unusedPort + "/",
+                       expectInternal: "https://hostname2.example/",
+               },
+               { // cannot listen on any of the ListenURLs
+                       internalURLs: map[string]string{
+                               "https://hostname1.example/": "http://1.2.3.4:" + unusedPort + "/",
+                               "https://hostname2.example/": "http://1.2.3.4:" + unusedPort + "/",
+                       },
+                       expectErrorMatch: "configuration does not enable the \"arvados-controller\" service on this host",
+               },
+               { // cannot listen on any of the (implied) ListenURLs
+                       internalURLs: map[string]string{
+                               "https://1.2.3.4/": "",
+                               "https://1.2.3.5/": "",
+                       },
+                       expectErrorMatch: "configuration does not enable the \"arvados-controller\" service on this host",
+               },
+               { // impossible port number
+                       internalURLs: map[string]string{
+                               "https://host.example/": "http://0.0.0.0:1234567",
+                       },
+                       expectErrorMatch: `.*:1234567: listen tcp: address 1234567: invalid port`,
+               },
+               {
+                       // env var URL not mentioned in config = obey env var, with warning
+                       internalURLs:    map[string]string{"https://hostname1.example/": "http://localhost:8000/"},
+                       envVar:          "https://hostname2.example",
+                       expectListen:    "https://hostname2.example/",
+                       expectInternal:  "https://hostname2.example/",
+                       expectLogsMatch: `.*\Qpossible configuration error: listening on https://hostname2.example/ (from $ARVADOS_SERVICE_INTERNAL_URL) even though configuration does not have a matching InternalURLs entry\E.*\n`,
+               },
+               {
+                       // env var + empty config = obey env var, with warning
+                       envVar:          "https://hostname.example",
+                       expectListen:    "https://hostname.example/",
+                       expectInternal:  "https://hostname.example/",
+                       expectLogsMatch: `.*\Qpossible configuration error: listening on https://hostname.example/ (from $ARVADOS_SERVICE_INTERNAL_URL) even though configuration does not have a matching InternalURLs entry\E.*\n`,
+               },
+       } {
+               c.Logf("trial %d %+v", idx, trial)
+               os.Setenv("ARVADOS_SERVICE_INTERNAL_URL", trial.envVar)
+               var logbuf bytes.Buffer
+               log := ctxlog.New(&logbuf, "text", "info")
+               services := arvados.Services{Controller: arvados.Service{InternalURLs: map[arvados.URL]arvados.ServiceInstance{}}}
+               for k, v := range trial.internalURLs {
+                       u, err := url.Parse(k)
+                       c.Assert(err, check.IsNil)
+                       si := arvados.ServiceInstance{}
+                       if v != "" {
+                               u, err := url.Parse(v)
+                               c.Assert(err, check.IsNil)
+                               si.ListenURL = arvados.URL(*u)
+                       }
+                       services.Controller.InternalURLs[arvados.URL(*u)] = si
+               }
+               listenURL, internalURL, err := getListenAddr(services, "arvados-controller", log)
+               if trial.expectLogsMatch != "" {
+                       c.Check(logbuf.String(), check.Matches, trial.expectLogsMatch)
+               }
+               if trial.expectErrorMatch != "" {
+                       c.Check(err, check.ErrorMatches, trial.expectErrorMatch)
+                       continue
+               }
+               if !c.Check(err, check.IsNil) {
+                       continue
+               }
+               c.Check(listenURL.String(), check.Equals, trial.expectListen)
+               c.Check(internalURL.String(), check.Equals, trial.expectInternal)
+       }
+}
+

 func (*Suite) TestCommand(c *check.C) {
        cf, err := ioutil.TempFile("", "cmd_test.")
        c.Assert(err, check.IsNil)
 func (*Suite) TestCommand(c *check.C) {
        cf, err := ioutil.TempFile("", "cmd_test.")
        c.Assert(err, check.IsNil)