From 2df5de69d74ab1fbf1fbcec23d392193522b0364 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Tue, 28 Jun 2022 15:28:59 -0400 Subject: [PATCH] 16561: Handle implicit port numbers in getListenAddress, add tests. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/service/cmd.go | 10 +++- lib/service/cmd_test.go | 120 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/lib/service/cmd.go b/lib/service/cmd.go index b5e395bec8..9e45e0f7e8 100644 --- 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 } - 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 diff --git a/lib/service/cmd_test.go b/lib/service/cmd_test.go index 10591d9b55..7a1f98a8f0 100644 --- a/lib/service/cmd_test.go +++ b/lib/service/cmd_test.go @@ -11,7 +11,9 @@ import ( "crypto/tls" "fmt" "io/ioutil" + "net" "net/http" + "net/url" "os" "testing" "time" @@ -35,6 +37,124 @@ const ( 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) -- 2.30.2