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 <bmcerlean@vmware.com>

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 {

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"
 )

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,
 		},
 		{

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,
 			},
 			{

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"`

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)
 	}

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)
+			}
+		})
+	}
+}