From f3d79e152eaac1cd1c5ac79e0a0bbd0997abdb17 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 28 Jan 2019 14:35:23 +0100 Subject: [PATCH 1/3] e2e: reject unknown providers This finishes the work started for 1.13: instead of merely warning about an unknown value given to --profile, the test/e2e/e2e.test binary will now print an error and refuse to run. Fixes: #70200 --- test/e2e/framework/provider.go | 11 +++++++++ test/e2e/framework/test_context.go | 36 +++++++++++++++++------------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/test/e2e/framework/provider.go b/test/e2e/framework/provider.go index c3e7851f4f..9c8a902b99 100644 --- a/test/e2e/framework/provider.go +++ b/test/e2e/framework/provider.go @@ -45,6 +45,17 @@ func RegisterProvider(name string, factory Factory) { providers[name] = factory } +// GetProviders returns the names of all currently registered providers. +func GetProviders() []string { + mutex.Lock() + defer mutex.Unlock() + var providerNames []string + for name := range providers { + providerNames = append(providerNames, name) + } + return providerNames +} + func init() { // "local" or "skeleton" can always be used. RegisterProvider("local", func() (ProviderInterface, error) { diff --git a/test/e2e/framework/test_context.go b/test/e2e/framework/test_context.go index 58df02d437..0fc595cdc7 100644 --- a/test/e2e/framework/test_context.go +++ b/test/e2e/framework/test_context.go @@ -21,6 +21,8 @@ import ( "fmt" "io/ioutil" "os" + "sort" + "strings" "time" "github.com/onsi/ginkgo/config" @@ -264,7 +266,7 @@ func RegisterClusterFlags() { flag.StringVar(&TestContext.KubeVolumeDir, "volume-dir", "/var/lib/kubelet", "Path to the directory containing the kubelet volumes.") flag.StringVar(&TestContext.CertDir, "cert-dir", "", "Path to the directory containing the certs. Default is empty, which doesn't use certs.") flag.StringVar(&TestContext.RepoRoot, "repo-root", "../../", "Root directory of kubernetes repository, for finding test files.") - flag.StringVar(&TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, etc.)") + flag.StringVar(&TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, skeleton, etc.)") flag.StringVar(&TestContext.Tooling, "tooling", "", "The tooling in use (kops, gke, etc.)") flag.StringVar(&TestContext.KubectlPath, "kubectl-path", "kubectl", "The kubectl binary to use. For development, you might use 'cluster/kubectl.sh' here.") flag.StringVar(&TestContext.OutputDir, "e2e-output-dir", "/tmp", "Output directory for interesting/useful test data, like performance data, benchmarks, and other metrics.") @@ -399,22 +401,26 @@ func AfterReadingAllFlags(t *TestContextType) { } // Make sure that all test runs have a valid TestContext.CloudConfig.Provider. + // TODO: whether and how long this code is needed is getting discussed + // in https://github.com/kubernetes/kubernetes/issues/70194. var err error TestContext.CloudConfig.Provider, err = SetupProviderConfig(TestContext.Provider) - if err == nil { - return - } - if !os.IsNotExist(errors.Cause(err)) { - Failf("Failed to setup provider config: %v", err) - } - // We allow unknown provider parameters for historic reasons. At least log a - // warning to catch typos. - // TODO (https://github.com/kubernetes/kubernetes/issues/70200): - // - remove the fallback for unknown providers - // - proper error message instead of Failf (which panics) - klog.Warningf("Unknown provider %q, proceeding as for --provider=skeleton.", TestContext.Provider) - TestContext.CloudConfig.Provider, err = SetupProviderConfig("skeleton") if err != nil { - Failf("Failed to setup fallback skeleton provider config: %v", err) + if os.IsNotExist(errors.Cause(err)) { + // Provide a more helpful error message when the provider is unknown. + var providers []string + for _, name := range GetProviders() { + // The empty string is accepted, but looks odd in the output below unless we quote it. + if name == "" { + name = `""` + } + providers = append(providers, name) + } + sort.Strings(providers) + klog.Errorf("Unknown provider %q. The following providers are known: %v", TestContext.Provider, strings.Join(providers, " ")) + } else { + klog.Errorf("Failed to setup provider config for %q: %v", TestContext.Provider, err) + } + os.Exit(1) } } From dde3445a4540fe67122fefab931b9458e0639c3c Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 29 Jan 2019 19:50:40 +0100 Subject: [PATCH 2/3] e2e: change default for --provider The empty string was the default and then triggered a special warning. There's no good reason for that behavior, so now the special handling for "unset provider" is gone and "skeleton" is the non-empty default for the value. --- test/e2e/framework/provider.go | 7 ++----- test/e2e/framework/test_context.go | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/e2e/framework/provider.go b/test/e2e/framework/provider.go index 9c8a902b99..ea8e02c73c 100644 --- a/test/e2e/framework/provider.go +++ b/test/e2e/framework/provider.go @@ -64,11 +64,8 @@ func init() { RegisterProvider("skeleton", func() (ProviderInterface, error) { return NullProvider{}, nil }) - // The empty string also works, but triggers a warning. - RegisterProvider("", func() (ProviderInterface, error) { - Logf("The --provider flag is not set. Treating as a conformance test. Some tests may not be run.") - return NullProvider{}, nil - }) + // The empty string used to be accepted in the past, but is not + // a valid value anymore. } // SetupProviderConfig validates the chosen provider and creates diff --git a/test/e2e/framework/test_context.go b/test/e2e/framework/test_context.go index 0fc595cdc7..f1bd4d839f 100644 --- a/test/e2e/framework/test_context.go +++ b/test/e2e/framework/test_context.go @@ -266,7 +266,7 @@ func RegisterClusterFlags() { flag.StringVar(&TestContext.KubeVolumeDir, "volume-dir", "/var/lib/kubelet", "Path to the directory containing the kubelet volumes.") flag.StringVar(&TestContext.CertDir, "cert-dir", "", "Path to the directory containing the certs. Default is empty, which doesn't use certs.") flag.StringVar(&TestContext.RepoRoot, "repo-root", "../../", "Root directory of kubernetes repository, for finding test files.") - flag.StringVar(&TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, skeleton, etc.)") + flag.StringVar(&TestContext.Provider, "provider", "skeleton", "The name of the Kubernetes provider (gce, gke, local, skeleton (the default), etc.)") flag.StringVar(&TestContext.Tooling, "tooling", "", "The tooling in use (kops, gke, etc.)") flag.StringVar(&TestContext.KubectlPath, "kubectl-path", "kubectl", "The kubectl binary to use. For development, you might use 'cluster/kubectl.sh' here.") flag.StringVar(&TestContext.OutputDir, "e2e-output-dir", "/tmp", "Output directory for interesting/useful test data, like performance data, benchmarks, and other metrics.") From 7b5e65977cd56472510a74155631c8980d3ffdc7 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 31 Jan 2019 15:54:48 +0100 Subject: [PATCH 3/3] e2e: "skeleton" as fallback, empty string as default Not accepting --provider= (i.e. setting an empty provider name) broke some test jobs. As suggested in https://github.com/kubernetes/kubernetes/pull/73402#issuecomment-459368230, now --provider= and not passing --provider at all both trigger a message and then continue as if --provider=skeleton had been used. --- test/e2e/framework/test_context.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/e2e/framework/test_context.go b/test/e2e/framework/test_context.go index f1bd4d839f..d8f6e15658 100644 --- a/test/e2e/framework/test_context.go +++ b/test/e2e/framework/test_context.go @@ -266,7 +266,7 @@ func RegisterClusterFlags() { flag.StringVar(&TestContext.KubeVolumeDir, "volume-dir", "/var/lib/kubelet", "Path to the directory containing the kubelet volumes.") flag.StringVar(&TestContext.CertDir, "cert-dir", "", "Path to the directory containing the certs. Default is empty, which doesn't use certs.") flag.StringVar(&TestContext.RepoRoot, "repo-root", "../../", "Root directory of kubernetes repository, for finding test files.") - flag.StringVar(&TestContext.Provider, "provider", "skeleton", "The name of the Kubernetes provider (gce, gke, local, skeleton (the default), etc.)") + flag.StringVar(&TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, skeleton (the fallback if not set), etc.)") flag.StringVar(&TestContext.Tooling, "tooling", "", "The tooling in use (kops, gke, etc.)") flag.StringVar(&TestContext.KubectlPath, "kubectl-path", "kubectl", "The kubectl binary to use. For development, you might use 'cluster/kubectl.sh' here.") flag.StringVar(&TestContext.OutputDir, "e2e-output-dir", "/tmp", "Output directory for interesting/useful test data, like performance data, benchmarks, and other metrics.") @@ -403,6 +403,13 @@ func AfterReadingAllFlags(t *TestContextType) { // Make sure that all test runs have a valid TestContext.CloudConfig.Provider. // TODO: whether and how long this code is needed is getting discussed // in https://github.com/kubernetes/kubernetes/issues/70194. + if TestContext.Provider == "" { + // Some users of the e2e.test binary pass --provider=. + // We need to support that, changing it would break those usages. + Logf("The --provider flag is not set. Continuing as if --provider=skeleton had been used.") + TestContext.Provider = "skeleton" + } + var err error TestContext.CloudConfig.Provider, err = SetupProviderConfig(TestContext.Provider) if err != nil {