From 392d68e6c1cf870e2fc4420217f1e74901621ef1 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Wed, 8 Jul 2020 16:35:44 -0400 Subject: [PATCH] Replace MasterURL with AggregatorURL in WorkerConfig (#1141) We renamed the `master` command to `aggregator` in #847 (released in v0.15.3). However, we left some of the existing uses in place to prevent backwards incompatibility. This change renames the `MasterURL` field in `WorkerConfig` to `AggregatorURL` in keeping with our terminology elsewhere. Worker containers are configured using environment variables. This change renames the environment variable but also adds some compatibility for the original environment variable to still be processed. It is possible for different image versions to be used for the aggregator and the worker (although it would have to be explicitly set in the generated manifest). This changes allows older versions of Sonobuoy to be used as the aggregator image but does not allow older versions to be used as the worker image. Although some compatibility is broken, this seems acceptable as we don't recommend mixing versions of images within a single sonobuoy run. Signed-off-by: Bridget McErlean --- cmd/sonobuoy/app/worker.go | 12 ++--- pkg/config/loader.go | 1 + pkg/plugin/driver/base.go | 2 +- pkg/plugin/driver/base_test.go | 2 +- pkg/plugin/interface.go | 4 +- pkg/worker/config.go | 13 +++++- pkg/worker/config_test.go | 85 ++++++++++++++++++++++++++++++++++ 7 files changed, 108 insertions(+), 11 deletions(-) create mode 100644 pkg/worker/config_test.go diff --git a/cmd/sonobuoy/app/worker.go b/cmd/sonobuoy/app/worker.go index b85f03247..cfb208b2e 100644 --- a/cmd/sonobuoy/app/worker.go +++ b/cmd/sonobuoy/app/worker.go @@ -104,8 +104,8 @@ func loadAndValidateConfig() (*plugin.WorkerConfig, error) { } var errlst []string - if cfg.MasterURL == "" { - errlst = append(errlst, "MasterURL not set") + if cfg.AggregatorURL == "" { + errlst = append(errlst, "AggregatorURL not set") } if cfg.ResultsDir == "" { errlst = append(errlst, "ResultsDir not set") @@ -163,13 +163,13 @@ func runGather(global bool) error { return errors.Wrap(err, "getting HTTP client") } - resultURL, err := url.Parse(cfg.MasterURL) + resultURL, err := url.Parse(cfg.AggregatorURL) if err != nil { - return errors.Wrap(err, "parsing MasterURL") + return errors.Wrap(err, "parsing AggregatorURL") } - progressURL, err := url.Parse(cfg.MasterURL) + progressURL, err := url.Parse(cfg.AggregatorURL) if err != nil { - return errors.Wrap(err, "parsing MasterURL") + return errors.Wrap(err, "parsing AggregatorURL") } if global { diff --git a/pkg/config/loader.go b/pkg/config/loader.go index b6365d67d..8b01bb653 100644 --- a/pkg/config/loader.go +++ b/pkg/config/loader.go @@ -26,6 +26,7 @@ import ( "github.com/vmware-tanzu/sonobuoy/pkg/buildinfo" "github.com/vmware-tanzu/sonobuoy/pkg/plugin" pluginloader "github.com/vmware-tanzu/sonobuoy/pkg/plugin/loader" + "github.com/pkg/errors" uuid "github.com/satori/go.uuid" ) diff --git a/pkg/plugin/driver/base.go b/pkg/plugin/driver/base.go index ffa79ad28..39ae000a8 100644 --- a/pkg/plugin/driver/base.go +++ b/pkg/plugin/driver/base.go @@ -168,7 +168,7 @@ func (b *Base) workerEnvironment(hostname string, cert *tls.Certificate, progres Value: b.GetName(), }, { - Name: "MASTER_URL", + Name: "AGGREGATOR_URL", Value: hostname, }, { diff --git a/pkg/plugin/driver/base_test.go b/pkg/plugin/driver/base_test.go index 92e50f260..15f23f9ef 100644 --- a/pkg/plugin/driver/base_test.go +++ b/pkg/plugin/driver/base_test.go @@ -195,7 +195,7 @@ func TestCreateWorkerContainerDefinition(t *testing.T) { Value: b.GetName(), }, { - Name: "MASTER_URL", + Name: "AGGREGATOR_URL", Value: aggregatorURL, }, { diff --git a/pkg/plugin/interface.go b/pkg/plugin/interface.go index 31733a45c..b4a6e86e7 100644 --- a/pkg/plugin/interface.go +++ b/pkg/plugin/interface.go @@ -136,8 +136,8 @@ type AggregationConfig struct { // WorkerConfig is the file given to the sonobuoy worker to configure it to phone home. type WorkerConfig struct { - // MasterURL is the URL we talk to the aggregator pod on for submitting results - MasterURL string `json:"masterurl,omitempty" mapstructure:"masterurl"` + // AggregatorURL is the URL we talk to the aggregator pod on for submitting results + AggregatorURL string `json:"aggregatorurl,omitempty" mapstructure:"aggregatorurl"` // NodeName is the node name we should call ourselves when sending results NodeName string `json:"nodename,omitempty" mapstructure:"nodename"` diff --git a/pkg/worker/config.go b/pkg/worker/config.go index 42e8b3c4a..a2bf15f07 100644 --- a/pkg/worker/config.go +++ b/pkg/worker/config.go @@ -32,13 +32,22 @@ func setConfigDefaults(ac *plugin.WorkerConfig) { ac.ProgressUpdatesPort = defaultProgressUpdatesPort } +func processDeprecatedVariables() { + // Default to using deprecated "masterurl" key if "aggregatorurl" key is not set. + // Remove in v0.19.0 + viper.BindEnv("masterurl", "MASTER_URL") + if viper.Get("masterurl") != nil && viper.Get("aggregatorurl") == nil { + viper.Set("aggregatorurl", viper.Get("masterurl")) + } +} + // LoadConfig loads the configuration for the sonobuoy worker from environment // variables, returning a plugin.WorkerConfig struct with defaults applied func LoadConfig() (*plugin.WorkerConfig, error) { config := &plugin.WorkerConfig{} var err error - viper.BindEnv("masterurl", "MASTER_URL") + viper.BindEnv("aggregatorurl", "AGGREGATOR_URL") viper.BindEnv("nodename", "NODE_NAME") viper.BindEnv("resultsdir", "RESULTS_DIR") viper.BindEnv("resulttype", "RESULT_TYPE") @@ -50,6 +59,8 @@ func LoadConfig() (*plugin.WorkerConfig, error) { setConfigDefaults(config) + processDeprecatedVariables() + if err = viper.Unmarshal(config); err != nil { return nil, errors.WithStack(err) } diff --git a/pkg/worker/config_test.go b/pkg/worker/config_test.go new file mode 100644 index 000000000..872927c9a --- /dev/null +++ b/pkg/worker/config_test.go @@ -0,0 +1,85 @@ +package worker + +import ( + "github.com/spf13/viper" + "github.com/vmware-tanzu/sonobuoy/pkg/plugin" + "os" + "reflect" + "testing" +) + +func TestLoadConfig(t *testing.T) { + testCases := []struct { + desc string + expectedCfg *plugin.WorkerConfig + env map[string]string + }{ + { + desc: "No environment variables results in default config values", + expectedCfg: &plugin.WorkerConfig{ + ResultsDir: "/tmp/results", + ProgressUpdatesPort: "8099", + }, + }, + { + desc: "Aggregator URL is set in config if env var is set", + expectedCfg: &plugin.WorkerConfig{ + AggregatorURL: "aggregator", + ResultsDir: plugin.ResultsDir, + ProgressUpdatesPort: defaultProgressUpdatesPort, + }, + env: map[string]string{ + "AGGREGATOR_URL": "aggregator", + }, + }, + { + desc: "Aggregator URL is set in config if only deprecated master url env var is set", + expectedCfg: &plugin.WorkerConfig{ + AggregatorURL: "master", + ResultsDir: plugin.ResultsDir, + ProgressUpdatesPort: defaultProgressUpdatesPort, + }, + env: map[string]string{ + "MASTER_URL": "master", + }, + }, + { + desc: "Aggregator URL env var takes precedence if both new and deprecated env vars set", + expectedCfg: &plugin.WorkerConfig{ + AggregatorURL: "aggregator", + ResultsDir: plugin.ResultsDir, + ProgressUpdatesPort: defaultProgressUpdatesPort, + }, + env: map[string]string{ + "MASTER_URL": "master", + "AGGREGATOR_URL": "aggregator", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + viper.Reset() + for k, v := range tc.env { + if err := os.Setenv(k, v); err != nil { + t.Fatalf("unable to set environment variable %q to value %q", k, v) + } + } + defer func() { + for k := range tc.env { + if err := os.Unsetenv(k); err != nil { + t.Fatalf("unable to unset environment variable %q", k) + } + } + }() + + cfg, err := LoadConfig() + if err != nil { + t.Fatalf("unexepected err from LoadConfig %q", err) + } + if !reflect.DeepEqual(cfg, tc.expectedCfg) { + t.Fatalf("expected config to be %q, got %q", tc.expectedCfg, cfg) + } + }) + } +}