Fix/simplify the config loading logic enabling empty resources (#725)

The method to load the config was utilizing viper which is a
common library used alongside cobra CLI tools. However, we were
using very little of its functionality and some of its logic
was bypassing the intent of our data structures: most clearly,
the empty but non-nil resources were still being treated as nil
meaning that you would query all resources instead of none of them.

Summary:
 - Sending a config with empty (but not nil) resources will now
actually lead to no query being done (other than Sonobuoy
gathering its own pod logs at the current time)
 - Removes viper from the main config handling in order to simplify
the logic and clarify the code flow. Uses basic file/json handling
to unmarshal the data now.
 - Continues to load the config from the pwd or default location
with an environment override.
 - Adds a test to ensure `LoadConfig` now properly treats empty
values for the Resources.

Fixes #721

Signed-off-by: John Schnake <jschnake@vmware.com>

pkg/config/loader.go

@@ -17,42 +17,46 @@ limitations under the License.
 package config
 
 import (
+	"encoding/json"
 	"fmt"
+	"io/ioutil"
 	"os"
 	"strings"
 
 	"github.com/heptio/sonobuoy/pkg/buildinfo"
 	"github.com/heptio/sonobuoy/pkg/plugin"
 	pluginloader "github.com/heptio/sonobuoy/pkg/plugin/loader"
 	"github.com/pkg/errors"
-	"github.com/spf13/viper"
+)
+
+const (
+	defaultCfgFileName  = "config.json"
+	fallbackCfgFileName = "/etc/sonobuoy/config.json"
 )
 
 // LoadConfig will load the current sonobuoy configuration using the filesystem
 // and environment variables, and returns a config object
 func LoadConfig() (*Config, error) {
-	var err error
-	cfg := New()
-
-	// 0 - load defaults
-	viper.SetConfigType("json")
-	viper.SetConfigName("config")
-	viper.AddConfigPath("/etc/sonobuoy/")
-	viper.AddConfigPath(".")
-
-	// Allow specifying a custom config file via the SONOBUOY_CONFIG env var
-	if forceCfg := os.Getenv("SONOBUOY_CONFIG"); forceCfg != "" {
-		viper.SetConfigFile(forceCfg)
+	cfg := &Config{}
+
+	pathsToTry := []string{}
+	envCfgFileName := os.Getenv("SONOBUOY_CONFIG")
+	if envCfgFileName != "" {
+		pathsToTry = []string{envCfgFileName}
+	} else {
+		pathsToTry = []string{defaultCfgFileName, fallbackCfgFileName}
 	}
 
-	// 1 - Read in the config file.
-	if err = viper.ReadInConfig(); err != nil {
-		return nil, errors.WithStack(err)
+	jsonFile, fpath, err := openFiles(pathsToTry...)
+	if err != nil {
+		return nil, errors.Wrap(err, "open config")
 	}
+	defer jsonFile.Close()
 
-	// 2 - Unmarshal the Config struct
-	if err = viper.Unmarshal(cfg); err != nil {
-		return nil, errors.WithStack(err)
+	b, err := ioutil.ReadAll(jsonFile)
+	err = json.Unmarshal(b, cfg)
+	if err != nil {
+		return nil, errors.Wrapf(err, "unmarshal config file %q", fpath)
 	}
 
 	// 3 - figure out what address we will tell pods to dial for aggregation
@@ -75,11 +79,6 @@ func LoadConfig() (*Config, error) {
 		cfg.ResultsDir = resultsDir
 	}
 
-	// Always take what the user set for Resources, even if it is empty. Do not
-	// merge with the defaults. Sending the empty slice queries everything using
-	// the dynamic client.
-	cfg.Resources = viper.GetStringSlice("Resources")
-
 	// 5 - Load any plugins we have
 	err = loadAllPlugins(cfg)
 	if err != nil {
@@ -151,3 +150,26 @@ func loadAllPlugins(cfg *Config) error {
 
 	return nil
 }
+
+// openFiles tries opening each of the files given, returning the first file/error
+// that either opens correctly or provides an error which is not os.IsNotExist(). The
+// string is the filename corresponding to the file/error returned.
+func openFiles(paths ...string) (*os.File, string, error) {
+	var f *os.File
+	var path string
+	var err error
+
+	for _, path = range paths {
+		f, err = os.Open(path)
+		switch {
+		case err == nil:
+			return f, path, nil
+		case err != nil && os.IsNotExist(err):
+			continue
+		default:
+			return nil, path, errors.Wrap(err, "opening config file")
+		}
+	}
+
+	return f, path, errors.Wrap(err, "opening config file")
+}

pkg/config/loader_test.go

@@ -18,6 +18,7 @@ package config
 
 import (
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"reflect"
@@ -26,6 +27,78 @@ import (
 	"github.com/heptio/sonobuoy/pkg/plugin"
 )
 
+func TestOpenConfigFile(t *testing.T) {
+
+	// Set up 3 files with contents matching their path for this test. Cleanup afterwards.
+	f1, f2 := "TestOpenConfigFile1", "TestOpenConfigFile2"
+	for _, v := range []string{f1, f2} {
+		err := ioutil.WriteFile(v, []byte(v), 0644)
+		if err != nil {
+			t.Fatalf("Failed to setup test files: %v", err)
+		}
+		defer os.Remove(v)
+	}
+
+	testCases := []struct {
+		desc       string
+		files      []string
+		expectPath string
+		expectErr  string
+	}{
+		{
+			desc:       "Open existing file",
+			files:      []string{f1},
+			expectPath: f1,
+		}, {
+			desc:      "File DNE",
+			files:     []string{"bad"},
+			expectErr: "opening config file: open bad: no such file or directory",
+		}, {
+			desc:       "File DNE and falls back to next file",
+			files:      []string{"bad", f1},
+			expectPath: f1,
+		}, {
+			desc:       "Returns first good file",
+			files:      []string{f2, f1},
+			expectPath: f2,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			f, fpath, err := openFiles(tc.files...)
+			if f != nil {
+				defer f.Close()
+			}
+
+			switch {
+			case err != nil && len(tc.expectErr) == 0:
+				t.Fatalf("Expected nil error but got %q", err)
+			case err != nil && len(tc.expectErr) > 0:
+				if fmt.Sprint(err) != tc.expectErr {
+					t.Errorf("Expected error \n\t%q\nbut got\n\t%q", tc.expectErr, err)
+				}
+				return
+			case err == nil && len(tc.expectErr) > 0:
+				t.Fatalf("Expected error %q but got nil", tc.expectErr)
+			default:
+				// No error
+			}
+
+			if fpath != tc.expectPath {
+				t.Errorf("Expected %v but got %v", tc.expectPath, fpath)
+			}
+
+			b, err := ioutil.ReadAll(f)
+			if err != nil {
+				t.Fatalf("Failed to read file %v: %v", fpath, err)
+			}
+			if string(b) != fpath {
+				t.Errorf("Expected contents of file %v to be %v but got %v", fpath, fpath, string(b))
+			}
+		})
+	}
+}
+
 func TestSaveAndLoad(t *testing.T) {
 	cfg := New()
 
@@ -71,6 +144,9 @@ func TestDefaultResources(t *testing.T) {
 	if len(cfg.Resources) != 0 {
 		t.Error("Default resources should not be applied if specified in config")
 	}
+	if cfg.Resources == nil {
+		t.Error("Empty resources should not be converted to nil")
+	}
 
 	// Check that not specifying resources results in all the defaults
 	blob = `{}`
@@ -86,7 +162,7 @@ func TestDefaultResources(t *testing.T) {
 	}
 
 	// Check that specifying one resource results in one resource
-	blob = `{"Resources": "Pods"}`
+	blob = `{"Resources": ["Pods"]}`
 	if err = ioutil.WriteFile("./config.json", []byte(blob), 0644); err != nil {
 		t.Fatalf("Failed to write default config.json: %v", err)
 	}
@@ -104,9 +180,9 @@ func TestLoadAllPlugins(t *testing.T) {
 	cfg := &Config{
 		PluginSearchPath: []string{"./examples/plugins.d"},
 		PluginSelections: []plugin.Selection{
-			plugin.Selection{Name: "systemd-logs"},
-			plugin.Selection{Name: "e2e"},
-			plugin.Selection{Name: "heptio-e2e"},
+			{Name: "systemd-logs"},
+			{Name: "e2e"},
+			{Name: "heptio-e2e"},
 		},
 	}