Surface load errors when reading .kubeconfig files

duda · Feb 19, 2015 · 945616a · 945616a
1 parent 43088bc
commit 945616a
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 6 deletions.
diff --git a/pkg/client/clientcmd/loader.go b/pkg/client/clientcmd/loader.go
@@ -17,6 +17,7 @@ limitations under the License.
 package clientcmd
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -26,6 +27,7 @@ import (
 
 	clientcmdapi "github.com/GoogleCloudPlatform/kubernetes/pkg/client/clientcmd/api"
 	clientcmdlatest "github.com/GoogleCloudPlatform/kubernetes/pkg/client/clientcmd/api/latest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors"
 )
 
 const (
@@ -55,7 +57,8 @@ func NewClientConfigLoadingRules() *ClientConfigLoadingRules {
 //   2.  EnvVarPath
 //   3.  CurrentDirectoryPath
 //   4.  HomeDirectoryPath
-// Empty filenames are ignored.  Files with non-deserializable content produced errors.
+// A missing CommandLinePath file produces an error. Empty filenames or other missing files are ignored.
+// Read errors or files with non-deserializable content produce errors.
 // The first file to set a particular map key wins and map key's value is never changed.
 // BUT, if you set a struct value that is NOT contained inside of map, the value WILL be changed.
 // This results in some odd looking logic to merge in one direction, merge in the other, and then merge the two.
@@ -65,16 +68,30 @@ func NewClientConfigLoadingRules() *ClientConfigLoadingRules {
 // and only absolute file paths are returned.
 func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) {
 
+	errlist := []error{}
+
+	// Make sure a file we were explicitly told to use exists
+	if len(rules.CommandLinePath) > 0 {
+		if _, err := os.Stat(rules.CommandLinePath); os.IsNotExist(err) {
+			errlist = append(errlist, fmt.Errorf("The config file %v does not exist", rules.CommandLinePath))
+		}
+	}
+
 	kubeConfigFiles := []string{rules.CommandLinePath, rules.EnvVarPath, rules.CurrentDirectoryPath, rules.HomeDirectoryPath}
 
 	// first merge all of our maps
 	mapConfig := clientcmdapi.NewConfig()
 	for _, file := range kubeConfigFiles {
-		mergeConfigWithFile(mapConfig, file)
-		resolveLocalPaths(file, mapConfig)
+		if err := mergeConfigWithFile(mapConfig, file); err != nil {
+			errlist = append(errlist, err)
+		}
+		if err := resolveLocalPaths(file, mapConfig); err != nil {
+			errlist = append(errlist, err)
+		}
 	}
 
 	// merge all of the struct values in the reverse order so that priority is given correctly
+	// errors are not added to the list the second time
 	nonMapConfig := clientcmdapi.NewConfig()
 	for i := len(kubeConfigFiles) - 1; i >= 0; i-- {
 		file := kubeConfigFiles[i]
@@ -88,7 +105,7 @@ func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) {
 	mergo.Merge(config, mapConfig)
 	mergo.Merge(config, nonMapConfig)
 
-	return config, nil
+	return config, errors.NewAggregate(errlist)
 }
 
 func mergeConfigWithFile(startingConfig *clientcmdapi.Config, filename string) error {
@@ -98,8 +115,11 @@ func mergeConfigWithFile(startingConfig *clientcmdapi.Config, filename string) e
 	}
 
 	config, err := LoadFromFile(filename)
+	if os.IsNotExist(err) {
+		return nil
+	}
 	if err != nil {
-		return err
+		return fmt.Errorf("Error loading config file \"%s\": %v", filename, err)
 	}
 
 	mergo.Merge(startingConfig, config)
@@ -117,7 +137,7 @@ func resolveLocalPaths(filename string, config *clientcmdapi.Config) error {
 
 	configDir, err := filepath.Abs(filepath.Dir(filename))
 	if err != nil {
-		return err
+		return fmt.Errorf("Could not determine the absolute path of config file %s: %v", filename, err)
 	}
 
 	resolvedClusters := make(map[string]clientcmdapi.Cluster)

diff --git a/pkg/client/clientcmd/loader_test.go b/pkg/client/clientcmd/loader_test.go
@@ -22,6 +22,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/ghodss/yaml"
@@ -75,6 +76,74 @@ var (
 	}
 )
 
+func TestNonExistentCommandLineFile(t *testing.T) {
+	loadingRules := ClientConfigLoadingRules{
+		CommandLinePath: "bogus_file",
+	}
+
+	_, err := loadingRules.Load()
+	if err == nil {
+		t.Fatalf("Expected error for missing command-line file, got none")
+	}
+	if !strings.Contains(err.Error(), "bogus_file") {
+		t.Fatalf("Expected error about 'bogus_file', got %s", err.Error())
+	}
+}
+
+func TestToleratingMissingFiles(t *testing.T) {
+	loadingRules := ClientConfigLoadingRules{
+		EnvVarPath:           "bogus1",
+		CurrentDirectoryPath: "bogus2",
+		HomeDirectoryPath:    "bogus3",
+	}
+
+	_, err := loadingRules.Load()
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+}
+
+func TestErrorReadingFile(t *testing.T) {
+	commandLineFile, _ := ioutil.TempFile("", "")
+	defer os.Remove(commandLineFile.Name())
+
+	if err := ioutil.WriteFile(commandLineFile.Name(), []byte("bogus value"), 0644); err != nil {
+		t.Fatalf("Error creating tempfile: %v", err)
+	}
+
+	loadingRules := ClientConfigLoadingRules{
+		CommandLinePath: commandLineFile.Name(),
+	}
+
+	_, err := loadingRules.Load()
+	if err == nil {
+		t.Fatalf("Expected error for unloadable file, got none")
+	}
+	if !strings.Contains(err.Error(), commandLineFile.Name()) {
+		t.Fatalf("Expected error about '%s', got %s", commandLineFile.Name(), err.Error())
+	}
+}
+
+func TestErrorReadingNonFile(t *testing.T) {
+	tmpdir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("Couldn't create tmpdir")
+	}
+	defer os.Remove(tmpdir)
+
+	loadingRules := ClientConfigLoadingRules{
+		CommandLinePath: tmpdir,
+	}
+
+	_, err = loadingRules.Load()
+	if err == nil {
+		t.Fatalf("Expected error for non-file, got none")
+	}
+	if !strings.Contains(err.Error(), tmpdir) {
+		t.Fatalf("Expected error about '%s', got %s", tmpdir, err.Error())
+	}
+}
+
 func TestConflictingCurrentContext(t *testing.T) {
 	commandLineFile, _ := ioutil.TempFile("", "")
 	defer os.Remove(commandLineFile.Name())