From 2dc2db5d020b7bd41cd2c4f5b0d1665d5a9248d3 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 5 Nov 2015 14:59:22 -0500 Subject: [PATCH] Config consumers should be able to recognize an empty config For UX, it would be better if we presented an error for validation that is "this config is empty" rather than the inaccurate "no server name" or "no context name" errors. Returns a typed error. --- .../unversioned/clientcmd/api/helpers.go | 8 +++++ pkg/client/unversioned/clientcmd/api/types.go | 2 ++ .../unversioned/clientcmd/client_config.go | 6 +++- .../unversioned/clientcmd/validation.go | 30 ++++++++++++++++++- .../unversioned/clientcmd/validation_test.go | 21 ++++++++++--- pkg/client/unversioned/helper.go | 7 ++++- 6 files changed, 67 insertions(+), 7 deletions(-) diff --git a/pkg/client/unversioned/clientcmd/api/helpers.go b/pkg/client/unversioned/clientcmd/api/helpers.go index e2d56570c6e..87330c5009f 100644 --- a/pkg/client/unversioned/clientcmd/api/helpers.go +++ b/pkg/client/unversioned/clientcmd/api/helpers.go @@ -31,6 +31,14 @@ func init() { redactedBytes = []byte(string(sDec)) } +// IsConfigEmpty returns true if the config is empty. +func IsConfigEmpty(config *Config) bool { + return len(config.AuthInfos) == 0 && len(config.Clusters) == 0 && len(config.Contexts) == 0 && + len(config.CurrentContext) == 0 && + len(config.Preferences.Extensions) == 0 && !config.Preferences.Colors && + len(config.Extensions) == 0 +} + // MinifyConfig read the current context and uses that to keep only the relevant pieces of config // This is useful for making secrets based on kubeconfig files func MinifyConfig(config *Config) error { diff --git a/pkg/client/unversioned/clientcmd/api/types.go b/pkg/client/unversioned/clientcmd/api/types.go index b5d9674df46..818ab25b2a8 100644 --- a/pkg/client/unversioned/clientcmd/api/types.go +++ b/pkg/client/unversioned/clientcmd/api/types.go @@ -24,6 +24,7 @@ import ( // Top level config objects and all values required for proper functioning are not "omitempty". Any truly optional piece of config is allowed to be omitted. // Config holds the information needed to build connect to remote kubernetes clusters as a given user +// IMPORTANT if you add fields to this struct, please update IsConfigEmpty() type Config struct { // Legacy field from pkg/api/types.go TypeMeta. // TODO(jlowdermilk): remove this after eliminating downstream dependencies. @@ -44,6 +45,7 @@ type Config struct { Extensions map[string]*runtime.EmbeddedObject `json:"extensions,omitempty"` } +// IMPORTANT if you add fields to this struct, please update IsConfigEmpty() type Preferences struct { Colors bool `json:"colors,omitempty"` // Extensions holds additional information. This is useful for extenders so that reads and writes don't clobber unknown fields diff --git a/pkg/client/unversioned/clientcmd/client_config.go b/pkg/client/unversioned/clientcmd/client_config.go index 0fc3ea63d27..38414ec5e70 100644 --- a/pkg/client/unversioned/clientcmd/client_config.go +++ b/pkg/client/unversioned/clientcmd/client_config.go @@ -233,7 +233,11 @@ func (config DirectClientConfig) ConfirmUsable() error { validationErrors := make([]error, 0) validationErrors = append(validationErrors, validateAuthInfo(config.getAuthInfoName(), config.getAuthInfo())...) validationErrors = append(validationErrors, validateClusterInfo(config.getClusterName(), config.getCluster())...) - + // when direct client config is specified, and our only error is that no server is defined, we should + // return a standard "no config" error + if len(validationErrors) == 1 && validationErrors[0] == ErrEmptyCluster { + return newErrConfigurationInvalid([]error{ErrEmptyConfig}) + } return newErrConfigurationInvalid(validationErrors) } diff --git a/pkg/client/unversioned/clientcmd/validation.go b/pkg/client/unversioned/clientcmd/validation.go index a631dc65d1d..bd1bd735e09 100644 --- a/pkg/client/unversioned/clientcmd/validation.go +++ b/pkg/client/unversioned/clientcmd/validation.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "os" + "reflect" "strings" clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api" @@ -27,7 +28,12 @@ import ( "k8s.io/kubernetes/pkg/util/validation" ) -var ErrNoContext = errors.New("no context chosen") +var ( + ErrNoContext = errors.New("no context chosen") + ErrEmptyConfig = errors.New("no configuration has been provided") + // message is for consistency with old behavior + ErrEmptyCluster = errors.New("cluster has no server defined") +) type errContextNotFound struct { ContextName string @@ -49,6 +55,16 @@ func IsContextNotFound(err error) bool { return strings.Contains(err.Error(), "context was not found for specified context") } +// IsEmptyConfig returns true if the provided error indicates the provided configuration +// is empty. +func IsEmptyConfig(err error) bool { + switch t := err.(type) { + case errConfigurationInvalid: + return len(t) == 1 && t[0] == ErrEmptyConfig + } + return err == ErrEmptyConfig +} + // errConfigurationInvalid is a set of errors indicating the configuration is invalid. type errConfigurationInvalid []error @@ -88,6 +104,10 @@ func IsConfigurationInvalid(err error) bool { func Validate(config clientcmdapi.Config) error { validationErrors := make([]error, 0) + if clientcmdapi.IsConfigEmpty(&config) { + return newErrConfigurationInvalid([]error{ErrEmptyConfig}) + } + if len(config.CurrentContext) != 0 { if _, exists := config.Contexts[config.CurrentContext]; !exists { validationErrors = append(validationErrors, &errContextNotFound{config.CurrentContext}) @@ -114,6 +134,10 @@ func Validate(config clientcmdapi.Config) error { func ConfirmUsable(config clientcmdapi.Config, passedContextName string) error { validationErrors := make([]error, 0) + if clientcmdapi.IsConfigEmpty(&config) { + return newErrConfigurationInvalid([]error{ErrEmptyConfig}) + } + var contextName string if len(passedContextName) != 0 { contextName = passedContextName @@ -143,6 +167,10 @@ func ConfirmUsable(config clientcmdapi.Config, passedContextName string) error { func validateClusterInfo(clusterName string, clusterInfo clientcmdapi.Cluster) []error { validationErrors := make([]error, 0) + if reflect.DeepEqual(clientcmdapi.Cluster{}, clusterInfo) { + return []error{ErrEmptyCluster} + } + if len(clusterInfo.Server) == 0 { if len(clusterName) == 0 { validationErrors = append(validationErrors, fmt.Errorf("default cluster has no server defined")) diff --git a/pkg/client/unversioned/clientcmd/validation_test.go b/pkg/client/unversioned/clientcmd/validation_test.go index 7bf3244ea3e..ca4843a8775 100644 --- a/pkg/client/unversioned/clientcmd/validation_test.go +++ b/pkg/client/unversioned/clientcmd/validation_test.go @@ -87,7 +87,7 @@ func TestConfirmUsableEmptyConfig(t *testing.T) { config := clientcmdapi.NewConfig() test := configValidationTest{ config: config, - expectedErrorSubstring: []string{"no context chosen"}, + expectedErrorSubstring: []string{"invalid configuration: no configuration has been provided"}, } test.testConfirmUsable("", t) @@ -96,7 +96,7 @@ func TestConfirmUsableMissingConfig(t *testing.T) { config := clientcmdapi.NewConfig() test := configValidationTest{ config: config, - expectedErrorSubstring: []string{"context was not found for"}, + expectedErrorSubstring: []string{"invalid configuration: no configuration has been provided"}, } test.testConfirmUsable("not-here", t) @@ -104,7 +104,8 @@ func TestConfirmUsableMissingConfig(t *testing.T) { func TestValidateEmptyConfig(t *testing.T) { config := clientcmdapi.NewConfig() test := configValidationTest{ - config: config, + config: config, + expectedErrorSubstring: []string{"invalid configuration: no configuration has been provided"}, } test.testConfig(t) @@ -132,6 +133,18 @@ func TestIsContextNotFound(t *testing.T) { } } +func TestIsEmptyConfig(t *testing.T) { + config := clientcmdapi.NewConfig() + + err := Validate(*config) + if !IsEmptyConfig(err) { + t.Errorf("Expected context not found, but got %v", err) + } + if !IsConfigurationInvalid(err) { + t.Errorf("Expected configuration invalid, but got %v", err) + } +} + func TestIsConfigurationInvalid(t *testing.T) { if newErrConfigurationInvalid([]error{}) != nil { t.Errorf("unexpected error") @@ -177,7 +190,7 @@ func TestValidateEmptyClusterInfo(t *testing.T) { config.Clusters["empty"] = &clientcmdapi.Cluster{} test := configValidationTest{ config: config, - expectedErrorSubstring: []string{"no server found for"}, + expectedErrorSubstring: []string{"cluster has no server defined"}, } test.testCluster("empty", t) diff --git a/pkg/client/unversioned/helper.go b/pkg/client/unversioned/helper.go index 6534ca2398a..141c0c6b065 100644 --- a/pkg/client/unversioned/helper.go +++ b/pkg/client/unversioned/helper.go @@ -327,6 +327,11 @@ func NewOrDie(c *Config) *Client { // running inside a pod running on kuberenetes. It will return an error if // called from a process not running in a kubernetes environment. func InClusterConfig() (*Config, error) { + host, port := os.Getenv("KUBERNETES_SERVICE_HOST"), os.Getenv("KUBERNETES_SERVICE_PORT") + if len(host) == 0 || len(port) == 0 { + return nil, fmt.Errorf("unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined") + } + token, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/" + api.ServiceAccountTokenKey) if err != nil { return nil, err @@ -341,7 +346,7 @@ func InClusterConfig() (*Config, error) { return &Config{ // TODO: switch to using cluster DNS. - Host: "https://" + net.JoinHostPort(os.Getenv("KUBERNETES_SERVICE_HOST"), os.Getenv("KUBERNETES_SERVICE_PORT")), + Host: "https://" + net.JoinHostPort(host, port), BearerToken: string(token), TLSClientConfig: tlsClientConfig, }, nil