From 22cff6bb354d6980c33a0f471d5860f968e853a6 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Fri, 27 Sep 2019 14:12:22 -0400 Subject: [PATCH] 13647: Check ARVADOS_* env vars when loading config. Previously they were only checked when starting up a service, so they weren't noticed by config-dump etc., and couldn't be used by the keepstore config migration when loading the keep_services endpoint. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/config/cmd_test.go | 6 ++++++ lib/config/deprecated.go | 28 +++++++++++++++++++++++++ lib/config/deprecated_keepstore.go | 4 ++-- lib/config/deprecated_keepstore_test.go | 4 ++++ lib/config/load.go | 1 + lib/config/load_test.go | 6 ++++++ lib/service/cmd.go | 18 ---------------- 7 files changed, 47 insertions(+), 20 deletions(-) diff --git a/lib/config/cmd_test.go b/lib/config/cmd_test.go index 019d5edd01..fb1cba38b4 100644 --- a/lib/config/cmd_test.go +++ b/lib/config/cmd_test.go @@ -24,6 +24,12 @@ var ( type CommandSuite struct{} +func (s *CommandSuite) SetUpSuite(c *check.C) { + os.Unsetenv("ARVADOS_API_HOST") + os.Unsetenv("ARVADOS_API_HOST_INSECURE") + os.Unsetenv("ARVADOS_API_TOKEN") +} + func (s *CommandSuite) TestBadArg(c *check.C) { var stderr bytes.Buffer code := DumpCommand.RunCommand("arvados config-dump", []string{"-badarg"}, bytes.NewBuffer(nil), bytes.NewBuffer(nil), &stderr) diff --git a/lib/config/deprecated.go b/lib/config/deprecated.go index 0a030fb040..d0e61dbca0 100644 --- a/lib/config/deprecated.go +++ b/lib/config/deprecated.go @@ -7,6 +7,7 @@ package config import ( "fmt" "io/ioutil" + "net/url" "os" "strings" @@ -474,3 +475,30 @@ func (ldr *Loader) loadOldGitHttpdConfig(cfg *arvados.Config) error { cfg.Clusters[cluster.ClusterID] = *cluster return nil } + +func (ldr *Loader) loadOldEnvironmentVariables(cfg *arvados.Config) error { + if os.Getenv("ARVADOS_API_TOKEN") == "" && os.Getenv("ARVADOS_API_HOST") == "" { + return nil + } + cluster, err := cfg.GetCluster("") + if err != nil { + return err + } + if tok := os.Getenv("ARVADOS_API_TOKEN"); tok != "" && cluster.SystemRootToken == "" { + ldr.Logger.Warn("SystemRootToken missing from cluster config, falling back to ARVADOS_API_TOKEN environment variable") + cluster.SystemRootToken = tok + } + if apihost := os.Getenv("ARVADOS_API_HOST"); apihost != "" && cluster.Services.Controller.ExternalURL.Host == "" { + ldr.Logger.Warn("Services.Controller.ExternalURL missing from cluster config, falling back to ARVADOS_API_HOST(_INSECURE) environment variables") + u, err := url.Parse("https://" + apihost) + if err != nil { + return fmt.Errorf("cannot parse ARVADOS_API_HOST: %s", err) + } + cluster.Services.Controller.ExternalURL = arvados.URL(*u) + if i := os.Getenv("ARVADOS_API_HOST_INSECURE"); i != "" && i != "0" { + cluster.TLS.Insecure = true + } + } + cfg.Clusters[cluster.ClusterID] = *cluster + return nil +} diff --git a/lib/config/deprecated_keepstore.go b/lib/config/deprecated_keepstore.go index a7d41e9853..29019296d6 100644 --- a/lib/config/deprecated_keepstore.go +++ b/lib/config/deprecated_keepstore.go @@ -584,7 +584,7 @@ func keepServiceIsMe(ks arvados.KeepService, hostname string, listen string) boo // been warned about in loadOldKeepstoreConfig() -- i.e., unmigrated // keepstore hosts other than the present host, and obsolete content // in the keep_services table. -func (ldr *Loader) checkPendingKeepstoreMigrations(cluster arvados.Cluster) error { +func (ldr *Loader) checkPendingKeepstoreMigrations(cluster *arvados.Cluster) error { if cluster.Services.Controller.ExternalURL.String() == "" { ldr.Logger.Debug("Services.Controller.ExternalURL not configured -- skipping check for pending keepstore config migrations") return nil @@ -593,7 +593,7 @@ func (ldr *Loader) checkPendingKeepstoreMigrations(cluster arvados.Cluster) erro ldr.Logger.Debug("(Loader).SkipAPICalls == true -- skipping check for pending keepstore config migrations") return nil } - client, err := arvados.NewClientFromConfig(&cluster) + client, err := arvados.NewClientFromConfig(cluster) if err != nil { return err } diff --git a/lib/config/deprecated_keepstore_test.go b/lib/config/deprecated_keepstore_test.go index d1028e8bd0..62f3f1e3fe 100644 --- a/lib/config/deprecated_keepstore_test.go +++ b/lib/config/deprecated_keepstore_test.go @@ -30,6 +30,10 @@ type KeepstoreMigrationSuite struct { var _ = check.Suite(&KeepstoreMigrationSuite{}) func (s *KeepstoreMigrationSuite) SetUpSuite(c *check.C) { + os.Setenv("ARVADOS_API_HOST", os.Getenv("ARVADOS_TEST_API_HOST")) + os.Setenv("ARVADOS_API_HOST_INSECURE", "1") + os.Setenv("ARVADOS_API_TOKEN", arvadostest.AdminToken) + // We don't need the keepstore servers, but we do need // keep_services listings that point to localhost, rather than // the apiserver fixtures that point to fictional hosts diff --git a/lib/config/load.go b/lib/config/load.go index d9ee97b516..61d80a3c58 100644 --- a/lib/config/load.go +++ b/lib/config/load.go @@ -245,6 +245,7 @@ func (ldr *Loader) Load() (*arvados.Config, error) { // * no primary config was loaded, and this is the // legacy config file for the current component for _, err := range []error{ + ldr.loadOldEnvironmentVariables(&cfg), ldr.loadOldKeepstoreConfig(&cfg), ldr.loadOldKeepWebConfig(&cfg), ldr.loadOldCrunchDispatchSlurmConfig(&cfg), diff --git a/lib/config/load_test.go b/lib/config/load_test.go index 17e0af7ba0..2e521ab093 100644 --- a/lib/config/load_test.go +++ b/lib/config/load_test.go @@ -46,6 +46,12 @@ func testLoader(c *check.C, configdata string, logdst io.Writer) *Loader { type LoadSuite struct{} +func (s *LoadSuite) SetUpSuite(c *check.C) { + os.Unsetenv("ARVADOS_API_HOST") + os.Unsetenv("ARVADOS_API_HOST_INSECURE") + os.Unsetenv("ARVADOS_API_TOKEN") +} + func (s *LoadSuite) TestEmpty(c *check.C) { cfg, err := testLoader(c, "", nil).Load() c.Check(cfg, check.IsNil) diff --git a/lib/service/cmd.go b/lib/service/cmd.go index d5619b87b3..ddff5f47a4 100644 --- a/lib/service/cmd.go +++ b/lib/service/cmd.go @@ -12,7 +12,6 @@ import ( "io" "net" "net/http" - "net/url" "os" "strings" @@ -110,23 +109,6 @@ func (c *command) RunCommand(prog string, args []string, stdin io.Reader, stdout } ctx = context.WithValue(ctx, contextKeyURL{}, listenURL) - if cluster.SystemRootToken == "" { - logger.Warn("SystemRootToken missing from cluster config, falling back to ARVADOS_API_TOKEN environment variable") - cluster.SystemRootToken = os.Getenv("ARVADOS_API_TOKEN") - } - if cluster.Services.Controller.ExternalURL.Host == "" { - logger.Warn("Services.Controller.ExternalURL missing from cluster config, falling back to ARVADOS_API_HOST(_INSECURE) environment variables") - u, err := url.Parse("https://" + os.Getenv("ARVADOS_API_HOST")) - if err != nil { - err = fmt.Errorf("ARVADOS_API_HOST: %s", err) - return 1 - } - cluster.Services.Controller.ExternalURL = arvados.URL(*u) - if i := os.Getenv("ARVADOS_API_HOST_INSECURE"); i != "" && i != "0" { - cluster.TLS.Insecure = true - } - } - reg := prometheus.NewRegistry() handler := c.newHandler(ctx, cluster, cluster.SystemRootToken, reg) if err = handler.CheckHealth(); err != nil { -- 2.30.2