From f03578e82ee77ded83387b27b275fa442933203b Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Tue, 12 May 2020 03:51:38 -0400 Subject: [PATCH] 16392: Add trailing slash to URLs like https://example in configs. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/boot/supervisor.go | 10 +++++----- lib/config/cmd_test.go | 2 +- lib/config/deprecated.go | 13 +++++++------ lib/config/deprecated_keepstore.go | 5 +++-- lib/config/deprecated_keepstore_test.go | 16 +++++++++------- lib/config/deprecated_test.go | 8 ++++---- lib/service/cmd.go | 3 +++ sdk/go/arvados/config.go | 4 ++++ sdk/go/arvados/config_test.go | 9 +++++++++ .../app/controllers/user_sessions_controller.rb | 2 +- .../api/config/initializers/omniauth_init.rb | 2 +- .../crunch-dispatch-slurm_test.go | 2 +- services/keepproxy/keepproxy.go | 2 +- services/ws/service_test.go | 2 +- 14 files changed, 50 insertions(+), 30 deletions(-) diff --git a/lib/boot/supervisor.go b/lib/boot/supervisor.go index c444ec3001..c3dd4bf9b2 100644 --- a/lib/boot/supervisor.go +++ b/lib/boot/supervisor.go @@ -568,7 +568,7 @@ func (super *Supervisor) autofillConfig(cfg *arvados.Config) error { if p == "0" { p = nextPort(h) } - cluster.Services.Controller.ExternalURL = arvados.URL{Scheme: "https", Host: net.JoinHostPort(h, p)} + cluster.Services.Controller.ExternalURL = arvados.URL{Scheme: "https", Host: net.JoinHostPort(h, p), Path: "/"} } for _, svc := range []*arvados.Service{ &cluster.Services.Controller, @@ -594,14 +594,14 @@ func (super *Supervisor) autofillConfig(cfg *arvados.Config) error { svc == &cluster.Services.WebDAV || svc == &cluster.Services.WebDAVDownload || svc == &cluster.Services.Workbench1 { - svc.ExternalURL = arvados.URL{Scheme: "https", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost))} + svc.ExternalURL = arvados.URL{Scheme: "https", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost)), Path: "/"} } else if svc == &cluster.Services.Websocket { - svc.ExternalURL = arvados.URL{Scheme: "wss", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost))} + svc.ExternalURL = arvados.URL{Scheme: "wss", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost)), Path: "/"} } } if len(svc.InternalURLs) == 0 { svc.InternalURLs = map[arvados.URL]arvados.ServiceInstance{ - arvados.URL{Scheme: "http", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost))}: arvados.ServiceInstance{}, + arvados.URL{Scheme: "http", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost)), Path: "/"}: arvados.ServiceInstance{}, } } } @@ -629,7 +629,7 @@ func (super *Supervisor) autofillConfig(cfg *arvados.Config) error { } if super.ClusterType == "test" { // Add a second keepstore process. - cluster.Services.Keepstore.InternalURLs[arvados.URL{Scheme: "http", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost))}] = arvados.ServiceInstance{} + cluster.Services.Keepstore.InternalURLs[arvados.URL{Scheme: "http", Host: fmt.Sprintf("%s:%s", super.ListenHost, nextPort(super.ListenHost)), Path: "/"}] = arvados.ServiceInstance{} // Create a directory-backed volume for each keepstore // process. diff --git a/lib/config/cmd_test.go b/lib/config/cmd_test.go index 3c420a04eb..74c3cc9694 100644 --- a/lib/config/cmd_test.go +++ b/lib/config/cmd_test.go @@ -148,7 +148,7 @@ Clusters: code := DumpCommand.RunCommand("arvados config-dump", []string{"-config", "-"}, bytes.NewBufferString(in), &stdout, &stderr) c.Check(code, check.Equals, 0) c.Check(stdout.String(), check.Matches, `(?ms).*TimeoutBooting: 10m\n.*`) - c.Check(stdout.String(), check.Matches, `(?ms).*http://localhost:12345: {}\n.*`) + c.Check(stdout.String(), check.Matches, `(?ms).*http://localhost:12345/: {}\n.*`) } func (s *CommandSuite) TestDump_UnknownKey(c *check.C) { diff --git a/lib/config/deprecated.go b/lib/config/deprecated.go index 0689efa440..bbbc9acf75 100644 --- a/lib/config/deprecated.go +++ b/lib/config/deprecated.go @@ -100,7 +100,7 @@ func applyDeprecatedNodeProfile(hostname string, ssi systemServiceInstance, svc if strings.HasPrefix(host, ":") { host = hostname + host } - svc.InternalURLs[arvados.URL{Scheme: scheme, Host: host}] = arvados.ServiceInstance{} + svc.InternalURLs[arvados.URL{Scheme: scheme, Host: host, Path: "/"}] = arvados.ServiceInstance{} } func (ldr *Loader) loadOldConfigHelper(component, path string, target interface{}) error { @@ -153,6 +153,7 @@ func loadOldClientConfig(cluster *arvados.Cluster, client *arvados.Client) { } if client.APIHost != "" { cluster.Services.Controller.ExternalURL.Host = client.APIHost + cluster.Services.Controller.ExternalURL.Path = "/" } if client.Scheme != "" { cluster.Services.Controller.ExternalURL.Scheme = client.Scheme @@ -268,7 +269,7 @@ func (ldr *Loader) loadOldWebsocketConfig(cfg *arvados.Config) error { cluster.PostgreSQL.ConnectionPool = *oc.PostgresPool } if oc.Listen != nil { - cluster.Services.Websocket.InternalURLs[arvados.URL{Host: *oc.Listen}] = arvados.ServiceInstance{} + cluster.Services.Websocket.InternalURLs[arvados.URL{Host: *oc.Listen, Path: "/"}] = arvados.ServiceInstance{} } if oc.LogLevel != nil { cluster.SystemLogs.LogLevel = *oc.LogLevel @@ -327,7 +328,7 @@ func (ldr *Loader) loadOldKeepproxyConfig(cfg *arvados.Config) error { loadOldClientConfig(cluster, oc.Client) if oc.Listen != nil { - cluster.Services.Keepproxy.InternalURLs[arvados.URL{Host: *oc.Listen}] = arvados.ServiceInstance{} + cluster.Services.Keepproxy.InternalURLs[arvados.URL{Host: *oc.Listen, Path: "/"}] = arvados.ServiceInstance{} } if oc.DefaultReplicas != nil { cluster.Collections.DefaultReplication = *oc.DefaultReplicas @@ -413,11 +414,11 @@ func (ldr *Loader) loadOldKeepWebConfig(cfg *arvados.Config) error { loadOldClientConfig(cluster, oc.Client) if oc.Listen != nil { - cluster.Services.WebDAV.InternalURLs[arvados.URL{Host: *oc.Listen}] = arvados.ServiceInstance{} - cluster.Services.WebDAVDownload.InternalURLs[arvados.URL{Host: *oc.Listen}] = arvados.ServiceInstance{} + cluster.Services.WebDAV.InternalURLs[arvados.URL{Host: *oc.Listen, Path: "/"}] = arvados.ServiceInstance{} + cluster.Services.WebDAVDownload.InternalURLs[arvados.URL{Host: *oc.Listen, Path: "/"}] = arvados.ServiceInstance{} } if oc.AttachmentOnlyHost != nil { - cluster.Services.WebDAVDownload.ExternalURL = arvados.URL{Host: *oc.AttachmentOnlyHost} + cluster.Services.WebDAVDownload.ExternalURL = arvados.URL{Host: *oc.AttachmentOnlyHost, Path: "/"} } if oc.ManagementToken != nil { cluster.ManagementToken = *oc.ManagementToken diff --git a/lib/config/deprecated_keepstore.go b/lib/config/deprecated_keepstore.go index 401764c87a..186ffc3371 100644 --- a/lib/config/deprecated_keepstore.go +++ b/lib/config/deprecated_keepstore.go @@ -118,7 +118,7 @@ func (ldr *Loader) loadOldKeepstoreConfig(cfg *arvados.Config) error { return err } - myURL := arvados.URL{Scheme: "http"} + myURL := arvados.URL{Scheme: "http", Path: "/"} if oc.TLSCertificateFile != nil && oc.TLSKeyFile != nil { myURL.Scheme = "https" } @@ -137,7 +137,7 @@ func (ldr *Loader) loadOldKeepstoreConfig(cfg *arvados.Config) error { cluster.TLS.Key = "file://" + *v } if v := oc.Listen; v != nil { - if _, ok := cluster.Services.Keepstore.InternalURLs[arvados.URL{Scheme: myURL.Scheme, Host: *v}]; ok { + if _, ok := cluster.Services.Keepstore.InternalURLs[arvados.URL{Scheme: myURL.Scheme, Host: *v, Path: "/"}]; ok { // already listed myURL.Host = *v } else if len(*v) > 1 && (*v)[0] == ':' { @@ -537,6 +537,7 @@ func keepServiceURL(ks arvados.KeepService) arvados.URL { url := arvados.URL{ Scheme: "http", Host: net.JoinHostPort(ks.ServiceHost, strconv.Itoa(ks.ServicePort)), + Path: "/", } if ks.ServiceSSLFlag { url.Scheme = "https" diff --git a/lib/config/deprecated_keepstore_test.go b/lib/config/deprecated_keepstore_test.go index abc507fdad..dab308c9d1 100644 --- a/lib/config/deprecated_keepstore_test.go +++ b/lib/config/deprecated_keepstore_test.go @@ -109,7 +109,7 @@ Clusters: TLS: {Insecure: true} Services: Controller: - ExternalURL: "https://`+os.Getenv("ARVADOS_API_HOST")+`" + ExternalURL: "https://`+os.Getenv("ARVADOS_API_HOST")+`/" `, ` Clusters: z1111: @@ -120,7 +120,7 @@ Clusters: InternalURLs: "http://`+hostname+`:25107": {Rendezvous: `+s.ksByPort[25107].UUID[12:]+`} Controller: - ExternalURL: "https://`+os.Getenv("ARVADOS_API_HOST")+`" + ExternalURL: "https://`+os.Getenv("ARVADOS_API_HOST")+`/" SystemLogs: Format: text LogLevel: debug @@ -254,7 +254,7 @@ Volumes: ReadOnly: true StorageAccountName: storageacctname StorageAccountKeyFile: `+secretkeyfile.Name()+` - StorageBaseURL: https://example.example + StorageBaseURL: https://example.example/ ContainerName: testctr LocationConstraint: true AzureReplication: 4 @@ -268,7 +268,7 @@ Volumes: }, &arvados.AzureVolumeDriverParameters{ StorageAccountName: "storageacctname", StorageAccountKey: "secretkeydata", - StorageBaseURL: "https://example.example", + StorageBaseURL: "https://example.example/", ContainerName: "testctr", RequestTimeout: arvados.Duration(time.Minute * 3), ListBlobsRetryDelay: arvados.Duration(time.Minute * 4), @@ -333,7 +333,7 @@ func (s *KeepstoreMigrationSuite) testDeprecatedVolume(c *check.C, oldconfigdata c.Check(v.Driver, check.Equals, expectvol.Driver) c.Check(v.Replication, check.Equals, expectvol.Replication) - avh, ok := v.AccessViaHosts[arvados.URL{Scheme: "http", Host: hostname + ":12345"}] + avh, ok := v.AccessViaHosts[arvados.URL{Scheme: "http", Host: hostname + ":12345", Path: "/"}] c.Check(ok, check.Equals, true) c.Check(avh.ReadOnly, check.Equals, expectvol.ReadOnly) @@ -516,6 +516,7 @@ Volumes: url := arvados.URL{ Scheme: "http", Host: fmt.Sprintf("%s:%d", hostname, port), + Path: "/", } _, ok := before["zzzzz-nyw5e-readonlyonother"].AccessViaHosts[url] c.Check(ok, check.Equals, false) @@ -543,6 +544,7 @@ Volumes: url := arvados.URL{ Scheme: "http", Host: fmt.Sprintf("%s:%d", hostname, port), + Path: "/", } _, ok := before["zzzzz-nyw5e-writableonother"].AccessViaHosts[url] c.Check(ok, check.Equals, false) @@ -572,7 +574,7 @@ Volumes: hostname, err := os.Hostname() c.Assert(err, check.IsNil) - _, ok := newvol.AccessViaHosts[arvados.URL{Scheme: "http", Host: fmt.Sprintf("%s:%d", hostname, port)}] + _, ok := newvol.AccessViaHosts[arvados.URL{Scheme: "http", Host: fmt.Sprintf("%s:%d", hostname, port), Path: "/"}] c.Check(ok, check.Equals, true) } @@ -601,7 +603,7 @@ Volumes: c.Check(logs, check.Matches, `(?ms).*you should remove the legacy keepstore config file.*`) c.Check(logs, check.Matches, `(?ms).*you should migrate the legacy keepstore configuration file on host keep1.zzzzz.example.com.*`) c.Check(logs, check.Not(check.Matches), `(?ms).*should migrate.*keep0.zzzzz.example.com.*`) - c.Check(logs, check.Matches, `(?ms).*keepstore configured at http://keep2.zzzzz.example.com:25107 does not have access to any volumes.*`) + c.Check(logs, check.Matches, `(?ms).*keepstore configured at http://keep2.zzzzz.example.com:25107/ does not have access to any volumes.*`) c.Check(logs, check.Matches, `(?ms).*Volumes.zzzzz-nyw5e-possconfigerror.AccessViaHosts refers to nonexistent keepstore server http://keep00.zzzzz.example.com:25107.*`) } diff --git a/lib/config/deprecated_test.go b/lib/config/deprecated_test.go index 58c27e984a..8a49c2cf8d 100644 --- a/lib/config/deprecated_test.go +++ b/lib/config/deprecated_test.go @@ -117,7 +117,7 @@ func (s *LoadSuite) TestLegacyKeepWebConfig(c *check.C) { cluster, err := testLoadLegacyConfig(content, "-legacy-keepweb-config", c) c.Check(err, check.IsNil) - c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com"}) + c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"}) c.Check(cluster.SystemRootToken, check.Equals, "abcdefg") c.Check(cluster.Collections.WebDAVCache.TTL, check.Equals, arvados.Duration(60*time.Second)) @@ -127,7 +127,7 @@ func (s *LoadSuite) TestLegacyKeepWebConfig(c *check.C) { c.Check(cluster.Collections.WebDAVCache.MaxPermissionEntries, check.Equals, 100) c.Check(cluster.Collections.WebDAVCache.MaxUUIDEntries, check.Equals, 100) - c.Check(cluster.Services.WebDAVDownload.ExternalURL, check.Equals, arvados.URL{Host: "download.example.com"}) + c.Check(cluster.Services.WebDAVDownload.ExternalURL, check.Equals, arvados.URL{Host: "download.example.com", Path: "/"}) c.Check(cluster.Services.WebDAVDownload.InternalURLs[arvados.URL{Host: ":80"}], check.NotNil) c.Check(cluster.Services.WebDAV.InternalURLs[arvados.URL{Host: ":80"}], check.NotNil) @@ -160,7 +160,7 @@ func (s *LoadSuite) TestLegacyKeepproxyConfig(c *check.C) { c.Check(err, check.IsNil) c.Check(cluster, check.NotNil) - c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com"}) + c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"}) c.Check(cluster.SystemRootToken, check.Equals, "abcdefg") c.Check(cluster.ManagementToken, check.Equals, "xyzzy") c.Check(cluster.Services.Keepproxy.InternalURLs[arvados.URL{Host: ":80"}], check.Equals, arvados.ServiceInstance{}) @@ -228,7 +228,7 @@ func (s *LoadSuite) TestLegacyArvGitHttpdConfig(c *check.C) { c.Check(err, check.IsNil) c.Check(cluster, check.NotNil) - c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com"}) + c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"}) c.Check(cluster.SystemRootToken, check.Equals, "abcdefg") c.Check(cluster.ManagementToken, check.Equals, "xyzzy") c.Check(cluster.Git.GitCommand, check.Equals, "/test/git") diff --git a/lib/service/cmd.go b/lib/service/cmd.go index 1e7a9a36ed..901fda2289 100644 --- a/lib/service/cmd.go +++ b/lib/service/cmd.go @@ -177,6 +177,9 @@ func getListenAddr(svcs arvados.Services, prog arvados.ServiceName, log logrus.F } else if url, err := url.Parse(want); err != nil { return arvados.URL{}, fmt.Errorf("$ARVADOS_SERVICE_INTERNAL_URL (%q): %s", want, err) } else { + if url.Path == "" { + url.Path = "/" + } return arvados.URL(*url), nil } diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index 38de6b8ea4..69de3f05e2 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -303,6 +303,10 @@ func (su *URL) UnmarshalText(text []byte) error { u, err := url.Parse(string(text)) if err == nil { *su = URL(*u) + if su.Path == "" && su.Host != "" { + // http://example really means http://example/ + su.Path = "/" + } } return err } diff --git a/sdk/go/arvados/config_test.go b/sdk/go/arvados/config_test.go index e4d26e03fd..8c77e29287 100644 --- a/sdk/go/arvados/config_test.go +++ b/sdk/go/arvados/config_test.go @@ -5,6 +5,8 @@ package arvados import ( + "encoding/json" + "github.com/ghodss/yaml" check "gopkg.in/check.v1" ) @@ -71,3 +73,10 @@ func (s *ConfigSuite) TestInstanceTypeFixup(c *check.C) { c.Check(itm["foo8"].IncludedScratch, check.Equals, ByteSize(0)) } } + +func (s *ConfigSuite) TestURLTrailingSlash(c *check.C) { + var a, b map[URL]bool + json.Unmarshal([]byte(`{"https://foo.example": true}`), &a) + json.Unmarshal([]byte(`{"https://foo.example/": true}`), &b) + c.Check(a, check.DeepEquals, b) +} diff --git a/services/api/app/controllers/user_sessions_controller.rb b/services/api/app/controllers/user_sessions_controller.rb index 85f32772b1..582b98cf2d 100644 --- a/services/api/app/controllers/user_sessions_controller.rb +++ b/services/api/app/controllers/user_sessions_controller.rb @@ -89,7 +89,7 @@ class UserSessionsController < ApplicationController flash[:notice] = 'You have logged off' return_to = params[:return_to] || root_url - redirect_to "#{Rails.configuration.Services.SSO.ExternalURL}/users/sign_out?redirect_uri=#{CGI.escape return_to}" + redirect_to "#{Rails.configuration.Services.SSO.ExternalURL}users/sign_out?redirect_uri=#{CGI.escape return_to}" end # login - Just bounce to /auth/joshid. The only purpose of this function is diff --git a/services/api/config/initializers/omniauth_init.rb b/services/api/config/initializers/omniauth_init.rb index 5610999a94..35a318b94f 100644 --- a/services/api/config/initializers/omniauth_init.rb +++ b/services/api/config/initializers/omniauth_init.rb @@ -11,7 +11,7 @@ if defined? CUSTOM_PROVIDER_URL Rails.logger.warn "Copying omniauth from globals in legacy config file." Rails.configuration.Login["ProviderAppID"] = APP_ID Rails.configuration.Login["ProviderAppSecret"] = APP_SECRET - Rails.configuration.Services["SSO"]["ExternalURL"] = CUSTOM_PROVIDER_URL + Rails.configuration.Services["SSO"]["ExternalURL"] = CUSTOM_PROVIDER_URL.sub(/\/$/, "") + "/" else Rails.application.config.middleware.use OmniAuth::Builder do provider(:josh_id, diff --git a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go index 3c14f7044c..480434de65 100644 --- a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go +++ b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go @@ -424,7 +424,7 @@ BatchSize: 99 err = s.disp.configure("crunch-dispatch-slurm", []string{"-config", tmpfile.Name()}) c.Check(err, IsNil) - c.Check(s.disp.cluster.Services.Controller.ExternalURL, Equals, arvados.URL{Scheme: "https", Host: "example.com"}) + c.Check(s.disp.cluster.Services.Controller.ExternalURL, Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"}) c.Check(s.disp.cluster.SystemRootToken, Equals, "abcdefg") c.Check(s.disp.cluster.Containers.SLURM.SbatchArgumentsList, DeepEquals, []string{"--foo", "bar"}) c.Check(s.disp.cluster.Containers.CloudVMs.PollInterval, Equals, arvados.Duration(12*time.Second)) diff --git a/services/keepproxy/keepproxy.go b/services/keepproxy/keepproxy.go index 188173274f..0191e5ba45 100644 --- a/services/keepproxy/keepproxy.go +++ b/services/keepproxy/keepproxy.go @@ -119,7 +119,7 @@ func run(logger log.FieldLogger, cluster *arvados.Cluster) error { // If a config file is available, use the keepstores defined there // instead of the legacy autodiscover mechanism via the API server for k := range cluster.Services.Keepstore.InternalURLs { - arv.KeepServiceURIs = append(arv.KeepServiceURIs, k.String()) + arv.KeepServiceURIs = append(arv.KeepServiceURIs, strings.TrimRight(k.String(), "/")) } if cluster.SystemLogs.LogLevel == "debug" { diff --git a/services/ws/service_test.go b/services/ws/service_test.go index 7213dcad2a..13726836a4 100644 --- a/services/ws/service_test.go +++ b/services/ws/service_test.go @@ -198,7 +198,7 @@ ManagementToken: qqqqq c.Check(err, check.IsNil) c.Check(cluster, check.NotNil) - c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com"}) + c.Check(cluster.Services.Controller.ExternalURL, check.Equals, arvados.URL{Scheme: "https", Host: "example.com", Path: "/"}) c.Check(cluster.SystemRootToken, check.Equals, "abcdefg") c.Check(cluster.PostgreSQL.Connection, check.DeepEquals, arvados.PostgreSQLConnection{ -- 2.30.2