Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e: reject unknown providers #73402

Merged
merged 3 commits into from
Feb 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 13 additions & 5 deletions test/e2e/framework/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -53,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
Expand Down
43 changes: 28 additions & 15 deletions test/e2e/framework/test_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"fmt"
"io/ioutil"
"os"
"sort"
"strings"
"time"

"github.com/onsi/ginkgo/config"
Expand Down Expand Up @@ -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 (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.")
Expand Down Expand Up @@ -399,22 +401,33 @@ 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 {
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a //TODO: when providers are extracted this code should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a TODO and link to #70194

I've kept the wording a bit more open-ended because I can imagine scenarios where kubernetes/kubernetes itself has no provider-specific code, but still supports building custom testsuites where such code is included. That would allow different cloud providers to share common tests in a neutral location when those tests depend on cloud-provider specific code. Whether that is something that Kubernetes as an organization wants to support I don't know.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really a fan, but if your long term goal is to extract provider code then I'll be ok with this if there are tracking/umbrealla issues that outline path forwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't have plans in that direction, but it is getting tracked here: #70194

}
}