Fixes duplicate runs (#401)

* Ignore duplicate plugins during plugin loading.
* Adds lots of tests for merging behavior.
* Adds tests to some other parts of the code relating to configuration manipulation.

Signed-off-by: Chuck Ha <chuck@heptio.com>

cmd/sonobuoy/app/sonobuoyconfig.go

@@ -20,6 +20,8 @@ import (
  "encoding/json"
  "io/ioutil"
 
+ "github.com/imdario/mergo"
+
  "github.com/heptio/sonobuoy/pkg/client"
  "github.com/heptio/sonobuoy/pkg/config"
  "github.com/pkg/errors"
@@ -68,18 +70,25 @@ func (c *SonobuoyConfig) Get() *config.Config {
 }
 
 // GetConfigWithMode creates a config with the following algorithm:
-// If the SonobuoyConfig isn't nil, use that
-// If not, use the supplied Mode to modify a default config
+// If no config is supplied defaults will be returned.
+// If a config is supplied then the default values will be merged into the supplied config
+// in order to allow users to supply a minimal config that will still work.
 func GetConfigWithMode(sonobuoyCfg *SonobuoyConfig, mode client.Mode) *config.Config {
+ conf := config.New()
+
  suppliedConfig := sonobuoyCfg.Get()
  if suppliedConfig != nil {
- return suppliedConfig
+ // Provide defaults but don't overwrite any customized configuration.
+ mergo.Merge(suppliedConfig, conf)
+ conf = suppliedConfig
  }
 
- defaultConfig := config.New()
- modeConfig := mode.Get()
- if modeConfig != nil {
- defaultConfig.PluginSelections = modeConfig.Selectors
+ // if there are no plugins yet, set some based on the mode, otherwise use whatever was supplied.
+ if len(conf.PluginSelections) == 0 {
+ modeConfig := mode.Get()
+ if modeConfig != nil {
+ conf.PluginSelections = modeConfig.Selectors
+ }
  }
- return defaultConfig
+ return conf
 }

cmd/sonobuoy/app/sonobuoyconfig_test.go

@@ -0,0 +1,117 @@
+/*
+Copyright 2018 Heptio Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package app
+
+import (
+ "testing"
+
+ "github.com/heptio/sonobuoy/pkg/client"
+ "github.com/heptio/sonobuoy/pkg/config"
+ "github.com/heptio/sonobuoy/pkg/plugin"
+)
+
+func TestGetConfigWithMode(t *testing.T) {
+ defaultPluginSearchPath := config.New().PluginSearchPath
+ tcs := []struct {
+ name string
+ mode client.Mode
+ inputcm *SonobuoyConfig
+ expected *config.Config
+ }{
+ {
+ name: "Conformance mode when supplied config is nil (nothing interesting happens)",
+ mode: client.Conformance,
+ inputcm: &SonobuoyConfig{},
+ expected: &config.Config{
+ PluginSelections: []plugin.Selection{
+ plugin.Selection{Name: "e2e"},
+ plugin.Selection{Name: "systemd-logs"},
+ },
+ PluginSearchPath: defaultPluginSearchPath,
+ },
+ },
+ {
+ name: "Quick mode and a non-nil supplied config",
+ mode: client.Quick,
+ inputcm: &SonobuoyConfig{
+ Config: config.Config{
+ Aggregation: plugin.AggregationConfig{
+ BindAddress: "10.0.0.1",
+ },
+ },
+ // TODO(chuckha) consider exporting raw or not depending on it.
+ raw: "not nil",
+ },
+ expected: &config.Config{
+ PluginSelections: []plugin.Selection{
+ plugin.Selection{Name: "e2e"},
+ },
+ Aggregation: plugin.AggregationConfig{
+ BindAddress: "10.0.0.1",
+ BindPort: config.DefaultAggregationServerBindPort,
+ },
+ PluginSearchPath: defaultPluginSearchPath,
+ },
+ },
+ {
+ name: "Conformance mode with plugin selection specified",
+ mode: client.Conformance,
+ inputcm: &SonobuoyConfig{
+ Config: config.Config{
+ PluginSelections: []plugin.Selection{
+ plugin.Selection{
+ Name: "systemd-logs",
+ },
+ },
+ },
+ raw: "not empty",
+ },
+ expected: &config.Config{
+ PluginSelections: []plugin.Selection{
+ plugin.Selection{
+ Name: "systemd-logs",
+ },
+ },
+ PluginSearchPath: defaultPluginSearchPath,
+ Aggregation: plugin.AggregationConfig{
+ BindAddress: config.DefaultAggregationServerBindAddress,
+ BindPort: config.DefaultAggregationServerBindPort,
+ },
+ },
+ },
+ }
+
+ for _, tc := range tcs {
+ t.Run(tc.name, func(t *testing.T) {
+ conf := GetConfigWithMode(tc.inputcm, tc.mode)
+ if len(conf.PluginSelections) != len(tc.expected.PluginSelections) {
+ t.Fatalf("expected %v plugin selections but found %v", tc.expected.PluginSelections, conf.PluginSelections)
+ }
+ for _, ps := range conf.PluginSelections {
+ found := false
+ for _, expectedPs := range tc.expected.PluginSelections {
+ if ps.Name == expectedPs.Name {
+ found = true
+ }
+ }
+ if !found {
+ t.Fatalf("looking for plugin selection %v but did not find it", ps)
+ }
+ }
+ })
+ }
+}

pkg/client/gen.go

@@ -21,12 +21,9 @@ import (
  "encoding/json"
  "strings"
 
- "github.com/imdario/mergo"
-
  "github.com/pkg/errors"
 
  "github.com/heptio/sonobuoy/pkg/buildinfo"
- "github.com/heptio/sonobuoy/pkg/config"
  "github.com/heptio/sonobuoy/pkg/templates"
 )
 
@@ -46,8 +43,6 @@ type templateValues struct {
 
 // GenerateManifest fills in a template with a Sonobuoy config
 func (c *SonobuoyClient) GenerateManifest(cfg *GenConfig) ([]byte, error) {
- // Provide defaults but don't overwrite any customized configuration.
- mergo.Merge(cfg.Config, config.New())
  marshalledConfig, err := json.Marshal(cfg.Config)
  if err != nil {
  return nil, errors.Wrap(err, "couldn't marshall selector")

pkg/client/gen_test.go

@@ -3,7 +3,7 @@ package client_test
 import (
  "bytes"
  "encoding/json"
- "fmt"
+ "reflect"
  "testing"
 
  "github.com/heptio/sonobuoy/pkg/client"
@@ -21,20 +21,15 @@ func TestGenerateManifest(t *testing.T) {
  expected *config.Config
  }{
  {
- name: "easy",
+ name: "Defaults in yield a default manifest.",
  inputcm: &client.GenConfig{
  E2EConfig: &client.E2EConfig{},
  Config: &config.Config{},
  },
- expected: &config.Config{
- Aggregation: plugin.AggregationConfig{
- BindAddress: "0.0.0.0",
- BindPort: 8080,
- },
- },
+ expected: &config.Config{},
  },
  {
- name: "override bind address",
+ name: "Overriding the bind address",
  inputcm: &client.GenConfig{
  E2EConfig: &client.E2EConfig{},
  Config: &config.Config{
@@ -46,12 +41,11 @@ func TestGenerateManifest(t *testing.T) {
  expected: &config.Config{
  Aggregation: plugin.AggregationConfig{
  BindAddress: "10.0.0.1",
- BindPort: 8080,
  },
  },
  },
  {
- name: "https://github.com/heptio/sonobuoy/issues/390",
+ name: "Overriding the plugin selection",
  inputcm: &client.GenConfig{
  E2EConfig: &client.E2EConfig{},
  Config: &config.Config{
@@ -68,11 +62,21 @@ func TestGenerateManifest(t *testing.T) {
  Name: "systemd-logs",
  },
  },
- Aggregation: plugin.AggregationConfig{
- BindAddress: "0.0.0.0",
- BindPort: 8080,
+ Aggregation: plugin.AggregationConfig{},
+ },
+ },
+ {
+ name: "The plugin search path is not modified",
+ inputcm: &client.GenConfig{
+ E2EConfig: &client.E2EConfig{},
+ Config: &config.Config{
+ PluginSearchPath: []string{"a", "b", "c", "a"},
  },
  },
+ expected: &config.Config{
+ Aggregation: plugin.AggregationConfig{},
+ PluginSearchPath: []string{"a", "b", "c", "a"},
+ },
  },
  }
 
@@ -101,33 +105,19 @@ func TestGenerateManifest(t *testing.T) {
  if !ok {
  t.Fatal("was not a config map...")
  }
- // Ignore everything but the config map we're looking for
+
+ // TODO(chuckha) test other pieces of the generated yaml
  if cm.ObjectMeta.Name != "sonobuoy-config-cm" {
  continue
  }
 
  configuration := &config.Config{}
- fmt.Println(cm.Data["config.json"])
  err = json.Unmarshal([]byte(cm.Data["config.json"]), configuration)
  if err != nil {
  t.Errorf("got error %v", err)
  }
- if configuration.UUID == "" {
- t.Error("Expected UUID to not be empty")
- }
- if configuration.Aggregation.BindAddress != tc.expected.Aggregation.BindAddress {
- t.Errorf("Expected %v but got %v", tc.expected.Aggregation.BindAddress, configuration.Aggregation.BindAddress)
- }
- if configuration.Aggregation.BindPort != tc.expected.Aggregation.BindPort {
- t.Errorf("Expected %v but got %v", tc.expected.Aggregation.BindPort, configuration.Aggregation.BindPort)
- }
- if len(configuration.PluginSelections) != len(tc.expected.PluginSelections) {
- t.Fatalf("Expected %v plugins but found %v", len(configuration.PluginSelections), len(tc.expected.PluginSelections))
- }
- for i, ps := range configuration.PluginSelections {
- if tc.expected.PluginSelections[i] != ps {
- t.Fatalf("Expected plugin %v but found plugin %v", tc.expected.PluginSelections[i], ps)
- }
+ if !reflect.DeepEqual(configuration, tc.expected) {
+ t.Fatalf("Expected %v to equal %v", tc.expected, configuration)
  }
  }
  })

pkg/config/config.go

@@ -29,8 +29,12 @@ import (
 const (
  // DefaultNamespace is the namespace where the master and plugin workers will run (but not necessarily the pods created by the plugin workers).
  DefaultNamespace = "heptio-sonobuoy"
- // DefaultKubeConformanceImage is the URL of the docker image to run for the kube conformance tests
+ // DefaultKubeConformanceImage is the URL of the docker image to run for the kube conformance tests.
  DefaultKubeConformanceImage = "gcr.io/heptio-images/kube-conformance:latest"
+ // DefaultAggregationServerBindPort is the default port for the aggregation server to bind to.
+ DefaultAggregationServerBindPort = 8080
+ // DefaultAggregationServerBindAddress is the default address for the aggregation server to bind to.
+ DefaultAggregationServerBindAddress = "0.0.0.0"
  // MasterPodName is the name of the main pod that runs plugins and collects results.
  MasterPodName = "sonobuoy"
  // MasterContainerName is the name of the main container in the master pod.
@@ -244,8 +248,8 @@ func New() *Config {
 
  cfg.Namespace = DefaultNamespace
 
- cfg.Aggregation.BindAddress = "0.0.0.0"
- cfg.Aggregation.BindPort = 8080
+ cfg.Aggregation.BindAddress = DefaultAggregationServerBindAddress
+ cfg.Aggregation.BindPort = DefaultAggregationServerBindPort
  cfg.Aggregation.TimeoutSeconds = 5400 // 90 minutes
 
  cfg.PluginSearchPath = []string{

pkg/config/config_test.go

@@ -14,16 +14,18 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package config
+package config_test
 
 import (
  "reflect"
  "testing"
+
+ "github.com/heptio/sonobuoy/pkg/config"
 )
 
 func TestDefaults(t *testing.T) {
- cfg1 := New()
- cfg2 := New()
+ cfg1 := config.New()
+ cfg2 := config.New()
 
  if reflect.DeepEqual(&cfg2, &cfg1) {
  t.Fatalf("Defaults should not match UUIDs collided")

pkg/plugin/loader/loader.go

@@ -37,7 +37,7 @@ import (
 // address (host:port) and returning all of the active, configured plugins for
 // this sonobuoy run.
 func LoadAllPlugins(namespace, sonobuoyImage, imagePullPolicy string, searchPath []string, selections []plugin.Selection) (ret []plugin.Interface, err error) {
- pluginDefinitionFiles := []string{}
+ pluginDefinitionFiles := make(map[string]struct{})
  for _, dir := range searchPath {
  wd, _ := os.Getwd()
  logrus.Infof("Scanning plugins in %v (pwd: %v)", dir, wd)
@@ -53,11 +53,13 @@ func LoadAllPlugins(namespace, sonobuoyImage, imagePullPolicy string, searchPath
  if err != nil {
  return []plugin.Interface{}, errors.Wrapf(err, "couldn't scan %v for plugins", dir)
  }
- pluginDefinitionFiles = append(pluginDefinitionFiles, files...)
+ for _, file := range files {
+ pluginDefinitionFiles[file] = struct{}{}
+ }
  }
 
  pluginDefinitions := []*manifest.Manifest{}
- for _, file := range pluginDefinitionFiles {
+ for file := range pluginDefinitionFiles {
  definitionFile, err := loadDefinitionFromFile(file)
  if err != nil {
  return []plugin.Interface{}, errors.Wrapf(err, "couldn't load plugin definition file %v", file)