diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 77a3373e9bc91..9aeb5fd189a7c 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -17,10 +17,14 @@ limitations under the License. package e2e import ( + "flag" + "fmt" + "os" "testing" "k8s.io/kubernetes/test/e2e/framework" "k8s.io/kubernetes/test/e2e/framework/testfiles" + "k8s.io/kubernetes/test/e2e/framework/viperconfig" "k8s.io/kubernetes/test/e2e/generated" // test sources @@ -42,8 +46,16 @@ import ( _ "k8s.io/kubernetes/test/e2e/ui" ) +var viperConfig = flag.String("viper-config", "", "The name of a viper config file (https://github.com/spf13/viper#what-is-viper). All e2e command line parameters can also be configured in such a file. May contain a path and may or may not contain the file suffix. The default is to look for an optional file with `e2e` as base name. If a file is specified explicitly, it must be present.") + func init() { - framework.ViperizeFlags() + // Register framework flags, then handle flags and Viper config. + framework.HandleFlags() + if err := viperconfig.ViperizeFlags(*viperConfig, "e2e"); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + framework.AfterReadingAllFlags(&framework.TestContext) // TODO: Deprecating repo-root over time... instead just use gobindata_util.go , see #23987. // Right now it is still needed, for example by diff --git a/test/e2e/framework/config/config.go b/test/e2e/framework/config/config.go new file mode 100644 index 0000000000000..1cd4a052f6214 --- /dev/null +++ b/test/e2e/framework/config/config.go @@ -0,0 +1,234 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package config simplifies the declaration of configuration options. +// Right now the implementation maps them directly command line +// flags. When combined with test/e2e/framework/viper in a test suite, +// those flags then can also be read from a config file. +// +// Instead of defining flags one-by-one, developers annotate a +// structure with tags and then call a single function. This is the +// same approach as in https://godoc.org/github.com/jessevdk/go-flags, +// but implemented so that a test suite can continue to use the normal +// "flag" package. +// +// For example, a file storage/csi.go might define: +// +// var scaling struct { +// NumNodes int `default:"1" description:"number of nodes to run on"` +// Master string +// } +// _ = config.AddOptions(&scaling, "storage.csi.scaling") +// +// This defines the following command line flags: +// +// -storage.csi.scaling.numNodes= - number of nodes to run on (default: 1) +// -storage.csi.scaling.master= +// +// All fields in the structure must be exported and have one of the following +// types (same as in the `flag` package): +// - bool +// - time.Duration +// - float64 +// - string +// - int +// - int64 +// - uint +// - uint64 +// - and/or nested or embedded structures containing those basic types. +// +// Each basic entry may have a tag with these optional keys: +// +// usage: additional explanation of the option +// default: the default value, in the same format as it would +// be given on the command line and true/false for +// a boolean +// +// The names of the final configuration options are a combination of an +// optional common prefix for all options in the structure and the +// name of the fields, concatenated with a dot. To get names that are +// consistent with the command line flags defined by `ginkgo`, the +// initial character of each field name is converted to lower case. +// +// There is currently no support for aliases, so renaming the fields +// or the common prefix will be visible to users of the test suite and +// may breaks scripts which use the old names. +// +// The variable will be filled with the actual values by the test +// suite before running tests. Beware that the code which registers +// Ginkgo tests cannot use those config options, because registering +// tests and options both run before the E2E test suite handles +// parameters. +package config + +import ( + "flag" + "fmt" + "reflect" + "strconv" + "time" + "unicode" + "unicode/utf8" +) + +// CommandLine is the flag set that AddOptions adds to. Usually this +// is the same as the default in the flag package, but can also be +// something else (for example during testing). +var CommandLine = flag.CommandLine + +// AddOptions analyzes the options value and creates the necessary +// flags to populate it. +// +// The prefix can be used to root the options deeper in the overall +// set of options, with a dot separating different levels. +// +// The function always returns true, to enable this simplified +// registration of options: +// _ = AddOptions(...) +// +// It panics when it encounters an error, like unsupported types +// or option name conflicts. +func AddOptions(options interface{}, prefix string) bool { + optionsType := reflect.TypeOf(options) + if optionsType == nil { + panic("options parameter without a type - nil?!") + } + if optionsType.Kind() != reflect.Ptr || optionsType.Elem().Kind() != reflect.Struct { + panic(fmt.Sprintf("need a pointer to a struct, got instead: %T", options)) + } + addStructFields(optionsType.Elem(), reflect.Indirect(reflect.ValueOf(options)), prefix) + return true +} + +func addStructFields(structType reflect.Type, structValue reflect.Value, prefix string) { + for i := 0; i < structValue.NumField(); i++ { + entry := structValue.Field(i) + addr := entry.Addr() + structField := structType.Field(i) + name := structField.Name + r, n := utf8.DecodeRuneInString(name) + name = string(unicode.ToLower(r)) + name[n:] + usage := structField.Tag.Get("usage") + def := structField.Tag.Get("default") + if prefix != "" { + name = prefix + "." + name + } + if structField.PkgPath != "" { + panic(fmt.Sprintf("struct entry %q not exported", name)) + } + ptr := addr.Interface() + if structField.Anonymous { + // Entries in embedded fields are treated like + // entries, in the struct itself, i.e. we add + // them with the same prefix. + addStructFields(structField.Type, entry, prefix) + continue + } + if structField.Type.Kind() == reflect.Struct { + // Add nested options. + addStructFields(structField.Type, entry, name) + continue + } + // We could switch based on structField.Type. Doing a + // switch after getting an interface holding the + // pointer to the entry has the advantage that we + // immediately have something that we can add as flag + // variable. + // + // Perhaps generics will make this entire switch redundant someday... + switch ptr := ptr.(type) { + case *bool: + var defValue bool + parseDefault(&defValue, name, def) + CommandLine.BoolVar(ptr, name, defValue, usage) + case *time.Duration: + var defValue time.Duration + parseDefault(&defValue, name, def) + CommandLine.DurationVar(ptr, name, defValue, usage) + case *float64: + var defValue float64 + parseDefault(&defValue, name, def) + CommandLine.Float64Var(ptr, name, defValue, usage) + case *string: + CommandLine.StringVar(ptr, name, def, usage) + case *int: + var defValue int + parseDefault(&defValue, name, def) + CommandLine.IntVar(ptr, name, defValue, usage) + case *int64: + var defValue int64 + parseDefault(&defValue, name, def) + CommandLine.Int64Var(ptr, name, defValue, usage) + case *uint: + var defValue uint + parseDefault(&defValue, name, def) + CommandLine.UintVar(ptr, name, defValue, usage) + case *uint64: + var defValue uint64 + parseDefault(&defValue, name, def) + CommandLine.Uint64Var(ptr, name, defValue, usage) + default: + panic(fmt.Sprintf("unsupported struct entry type %q: %T", name, entry.Interface())) + } + } +} + +// parseDefault is necessary because "flag" wants the default in the +// actual type and cannot take a string. It would be nice to reuse the +// existing code for parsing from the "flag" package, but it isn't +// exported. +func parseDefault(value interface{}, name, def string) { + if def == "" { + return + } + checkErr := func(err error, value interface{}) { + if err != nil { + panic(fmt.Sprintf("invalid default %q for %T entry %s: %s", def, value, name, err)) + } + } + switch value := value.(type) { + case *bool: + v, err := strconv.ParseBool(def) + checkErr(err, *value) + *value = v + case *time.Duration: + v, err := time.ParseDuration(def) + checkErr(err, *value) + *value = v + case *float64: + v, err := strconv.ParseFloat(def, 64) + checkErr(err, *value) + *value = v + case *int: + v, err := strconv.Atoi(def) + checkErr(err, *value) + *value = v + case *int64: + v, err := strconv.ParseInt(def, 0, 64) + checkErr(err, *value) + *value = v + case *uint: + v, err := strconv.ParseUint(def, 0, strconv.IntSize) + checkErr(err, *value) + *value = uint(v) + case *uint64: + v, err := strconv.ParseUint(def, 0, 64) + checkErr(err, *value) + *value = v + default: + panic(fmt.Sprintf("%q: setting defaults not supported for type %T", name, value)) + } +} diff --git a/test/e2e/framework/config/config_test.go b/test/e2e/framework/config/config_test.go new file mode 100644 index 0000000000000..d6856e696ad64 --- /dev/null +++ b/test/e2e/framework/config/config_test.go @@ -0,0 +1,258 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "flag" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInt(t *testing.T) { + CommandLine = flag.NewFlagSet("test", 0) + var context struct { + Number int `default:"5" usage:"some number"` + } + require.NotPanics(t, func() { + AddOptions(&context, "") + }) + require.Equal(t, []simpleFlag{ + { + name: "number", + usage: "some number", + defValue: "5", + }}, + allFlags(CommandLine)) + assert.Equal(t, 5, context.Number) +} + +func TestLower(t *testing.T) { + CommandLine = flag.NewFlagSet("test", 0) + var context struct { + Ähem string + MixedCase string + } + require.NotPanics(t, func() { + AddOptions(&context, "") + }) + require.Equal(t, []simpleFlag{ + { + name: "mixedCase", + }, + { + name: "ähem", + }, + }, + allFlags(CommandLine)) +} + +func TestPrefix(t *testing.T) { + CommandLine = flag.NewFlagSet("test", 0) + var context struct { + Number int `usage:"some number"` + } + require.NotPanics(t, func() { + AddOptions(&context, "some.prefix") + }) + require.Equal(t, []simpleFlag{ + { + name: "some.prefix.number", + usage: "some number", + defValue: "0", + }}, + allFlags(CommandLine)) +} + +func TestRecursion(t *testing.T) { + CommandLine = flag.NewFlagSet("test", 0) + type Nested struct { + Number1 int `usage:"embedded number"` + } + var context struct { + Nested + A struct { + B struct { + C struct { + Number2 int `usage:"some number"` + } + } + } + } + require.NotPanics(t, func() { + AddOptions(&context, "") + }) + require.Equal(t, []simpleFlag{ + { + name: "a.b.c.number2", + usage: "some number", + defValue: "0", + }, + { + name: "number1", + usage: "embedded number", + defValue: "0", + }, + }, + allFlags(CommandLine)) +} + +func TestPanics(t *testing.T) { + assert.PanicsWithValue(t, `invalid default "a" for int entry prefix.number: strconv.Atoi: parsing "a": invalid syntax`, func() { + var context struct { + Number int `default:"a"` + } + AddOptions(&context, "prefix") + }) + + assert.PanicsWithValue(t, `invalid default "10000000000000000000" for int entry prefix.number: strconv.Atoi: parsing "10000000000000000000": value out of range`, func() { + var context struct { + Number int `default:"10000000000000000000"` + } + AddOptions(&context, "prefix") + }) + + assert.PanicsWithValue(t, `options parameter without a type - nil?!`, func() { + AddOptions(nil, "") + }) + + assert.PanicsWithValue(t, `need a pointer to a struct, got instead: *int`, func() { + number := 0 + AddOptions(&number, "") + }) + + assert.PanicsWithValue(t, `struct entry "prefix.number" not exported`, func() { + var context struct { + number int + } + AddOptions(&context, "prefix") + }) + + assert.PanicsWithValue(t, `unsupported struct entry type "prefix.someNumber": config.MyInt`, func() { + type MyInt int + var context struct { + SomeNumber MyInt + } + AddOptions(&context, "prefix") + }) +} + +func TestTypes(t *testing.T) { + CommandLine = flag.NewFlagSet("test", 0) + type Context struct { + Bool bool `default:"true"` + Duration time.Duration `default:"1ms"` + Float64 float64 `default:"1.23456789"` + String string `default:"hello world"` + Int int `default:"-1" usage:"some number"` + Int64 int64 `default:"-1234567890123456789"` + Uint uint `default:"1"` + Uint64 uint64 `default:"1234567890123456789"` + } + var context Context + require.NotPanics(t, func() { + AddOptions(&context, "") + }) + require.Equal(t, []simpleFlag{ + { + name: "bool", + defValue: "true", + isBool: true, + }, + { + name: "duration", + defValue: "1ms", + }, + { + name: "float64", + defValue: "1.23456789", + }, + { + name: "int", + usage: "some number", + defValue: "-1", + }, + { + name: "int64", + defValue: "-1234567890123456789", + }, + { + name: "string", + defValue: "hello world", + }, + { + name: "uint", + defValue: "1", + }, + { + name: "uint64", + defValue: "1234567890123456789", + }, + }, + allFlags(CommandLine)) + assert.Equal(t, + Context{true, time.Millisecond, 1.23456789, "hello world", + -1, -1234567890123456789, 1, 1234567890123456789, + }, + context, + "default values must match") + require.NoError(t, CommandLine.Parse([]string{ + "-int", "-2", + "-int64", "-9123456789012345678", + "-uint", "2", + "-uint64", "9123456789012345678", + "-string", "pong", + "-float64", "-1.23456789", + "-bool=false", + "-duration=1s", + })) + assert.Equal(t, + Context{false, time.Second, -1.23456789, "pong", + -2, -9123456789012345678, 2, 9123456789012345678, + }, + context, + "parsed values must match") +} + +func allFlags(fs *flag.FlagSet) []simpleFlag { + var flags []simpleFlag + fs.VisitAll(func(f *flag.Flag) { + s := simpleFlag{ + name: f.Name, + usage: f.Usage, + defValue: f.DefValue, + } + type boolFlag interface { + flag.Value + IsBoolFlag() bool + } + if fv, ok := f.Value.(boolFlag); ok && fv.IsBoolFlag() { + s.isBool = true + } + flags = append(flags, s) + }) + return flags +} + +type simpleFlag struct { + name string + usage string + defValue string + isBool bool +} diff --git a/test/e2e/framework/test_context.go b/test/e2e/framework/test_context.go index 39e374cb8f5d7..dfeed9f58c8a3 100644 --- a/test/e2e/framework/test_context.go +++ b/test/e2e/framework/test_context.go @@ -25,7 +25,6 @@ import ( "github.com/golang/glog" "github.com/onsi/ginkgo/config" - "github.com/spf13/viper" utilflag "k8s.io/apiserver/pkg/util/flag" restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" @@ -37,6 +36,34 @@ import ( const defaultHost = "http://127.0.0.1:8080" +// TestContextType contains test settings and global state. Due to +// historic reasons, it is a mixture of items managed by the test +// framework itself, cloud providers and individual tests. +// The goal is to move anything not required by the framework +// into the code which uses the settings. +// +// The recommendation for those settings is: +// - They are stored in their own context structure or local +// variables. +// - The standard `flag` package is used to register them. +// The flag name should follow the pattern ..... +// where the prefix is unlikely to conflict with other tests or +// standard packages and each part is in lower camel case. For +// example, test/e2e/storage/csi/context.go could define +// storage.csi.numIterations. +// - framework/config can be used to simplify the registration of +// multiple options with a single function call: +// var storageCSI { +// NumIterations `default:"1" usage:"number of iterations"` +// } +// _ config.AddOptions(&storageCSI, "storage.csi") +// - The direct use Viper in tests is possible, but discouraged because +// it only works in test suites which use Viper (which is not +// required) and the supported options cannot be +// discovered by a test suite user. +// +// Test suite authors can use framework/viper to make all command line +// parameters also configurable via a configuration file. type TestContextType struct { KubeConfig string KubemarkExternalKubeConfig string @@ -125,19 +152,13 @@ type TestContextType struct { // Indicates what path the kubernetes-anywhere is installed on KubernetesAnywherePath string - // Viper-only parameters. These will in time replace all flags. - - // Example: Create a file 'e2e.json' with the following: - // "Cadvisor":{ - // "MaxRetries":"6" - // } - - Viper string + // Cadvisor contains settings for test/e2e/instrumentation/monitoring. Cadvisor struct { MaxRetries int SleepDurationMS int } + // LoggingSoak contains settings for test/e2e/instrumentation/logging. LoggingSoak struct { Scale int MilliSecondsBetweenWaves int @@ -225,7 +246,6 @@ func RegisterCommonFlags() { flag.StringVar(&TestContext.ReportPrefix, "report-prefix", "", "Optional prefix for JUnit XML reports. Default is empty, which doesn't prepend anything to the default name.") flag.StringVar(&TestContext.ReportDir, "report-dir", "", "Path to the directory where the JUnit XML reports should be saved. Default is empty, which doesn't generate these reports.") flag.Var(utilflag.NewMapStringBool(&TestContext.FeatureGates), "feature-gates", "A set of key=value pairs that describe feature gates for alpha/experimental features.") - flag.StringVar(&TestContext.Viper, "viper-config", "e2e", "The name of the viper config i.e. 'e2e' will read values from 'e2e.json' locally. All e2e parameters are meant to be configurable by viper.") flag.StringVar(&TestContext.ContainerRuntime, "container-runtime", "docker", "The container runtime of cluster VM instances (docker/remote).") flag.StringVar(&TestContext.ContainerRuntimeEndpoint, "container-runtime-endpoint", "unix:///var/run/dockershim.sock", "The container runtime endpoint of cluster VM instances.") flag.StringVar(&TestContext.ContainerRuntimeProcessName, "container-runtime-process-name", "dockerd", "The name of the container runtime process.") @@ -311,27 +331,12 @@ func RegisterStorageFlags() { flag.StringVar(&TestContext.CSIImageRegistry, "csiImageRegistry", "quay.io/k8scsi", "overrides the default repository used for hostpathplugin/csi-attacher/csi-provisioner/driver-registrar images") } -// ViperizeFlags sets up all flag and config processing. Future configuration info should be added to viper, not to flags. -func ViperizeFlags() { - - // Part 1: Set regular flags. - // TODO: Future, lets eliminate e2e 'flag' deps entirely in favor of viper only, - // since go test 'flag's are sort of incompatible w/ flag, glog, etc. +// HandleFlags sets up all flags and parses the command line. +func HandleFlags() { RegisterCommonFlags() RegisterClusterFlags() RegisterStorageFlags() flag.Parse() - - // Part 2: Set Viper provided flags. - // This must be done after common flags are registered, since Viper is a flag option. - viper.SetConfigName(TestContext.Viper) - viper.AddConfigPath(".") - viper.ReadInConfig() - - // TODO Consider whether or not we want to use overwriteFlagsWithViperConfig(). - viper.Unmarshal(&TestContext) - - AfterReadingAllFlags(&TestContext) } func createKubeConfig(clientCfg *restclient.Config) *clientcmdapi.Config { diff --git a/test/e2e/framework/viperconfig/viperconfig.go b/test/e2e/framework/viperconfig/viperconfig.go new file mode 100644 index 0000000000000..9a318acb1c434 --- /dev/null +++ b/test/e2e/framework/viperconfig/viperconfig.go @@ -0,0 +1,142 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package viperconfig + +import ( + "flag" + "fmt" + "github.com/pkg/errors" + "path/filepath" + + "github.com/spf13/viper" +) + +const ( + viperFileNotFound = "Unsupported Config Type \"\"" +) + +// ViperizeFlags checks whether a configuration file was specified, reads it, and updates +// the configuration variables accordingly. Must be called after framework.HandleFlags() +// and before framework.AfterReadingAllFlags(). +// +// The logic is so that a required configuration file must be present. If empty, +// the optional configuration file is used instead, unless also empty. +// +// Files can be specified with just a base name ("e2e", matches "e2e.json/yaml/..." in +// the current directory) or with path and suffix. +func ViperizeFlags(requiredConfig, optionalConfig string) error { + viperConfig := optionalConfig + required := false + if requiredConfig != "" { + viperConfig = requiredConfig + required = true + } + if viperConfig == "" { + return nil + } + viper.SetConfigName(filepath.Base(viperConfig)) + viper.AddConfigPath(filepath.Dir(viperConfig)) + wrapError := func(err error) error { + if err == nil { + return nil + } + errorPrefix := fmt.Sprintf("viper config %q", viperConfig) + actualFile := viper.ConfigFileUsed() + if actualFile != "" && actualFile != viperConfig { + errorPrefix = fmt.Sprintf("%s = %q", errorPrefix, actualFile) + } + return errors.Wrap(err, errorPrefix) + } + + if err := viper.ReadInConfig(); err != nil { + // If the user specified a file suffix, the Viper won't + // find the file because it always appends its known set + // of file suffices. Therefore try once more without + // suffix. + ext := filepath.Ext(viperConfig) + if ext != "" && err.Error() == viperFileNotFound { + viper.SetConfigName(filepath.Base(viperConfig[0 : len(viperConfig)-len(ext)])) + err = viper.ReadInConfig() + } + if err != nil { + // If a config was required, then parsing must + // succeed. This catches syntax errors and + // "file not found". Unfortunately error + // messages are sometimes hard to understand, + // so try to help the user a bit. + switch err.Error() { + case viperFileNotFound: + if required { + return wrapError(errors.New("not found or not using a supported file format")) + } + // Proceed without config. + return nil + default: + // Something isn't right in the file. + return wrapError(err) + } + } + } + + // Update all flag values not already set with values found + // via Viper. We do this ourselves instead of calling + // something like viper.Unmarshal(&TestContext) because we + // want to support all values, regardless where they are + // stored. + return wrapError(viperUnmarshal()) +} + +// viperUnmarshall updates all command line flags with the corresponding values found +// via Viper, regardless whether the flag value is stored in TestContext, some other +// context or a local variable. +func viperUnmarshal() error { + var result error + set := make(map[string]bool) + + // Determine which values were already set explicitly via + // flags. Those we don't overwrite because command line + // flags have a higher priority. + flag.Visit(func(f *flag.Flag) { + set[f.Name] = true + }) + + flag.VisitAll(func(f *flag.Flag) { + if result != nil || + set[f.Name] || + !viper.IsSet(f.Name) { + return + } + + // In contrast to viper.Unmarshal(), values + // that have the wrong type (for example, a + // list instead of a plain string) will not + // trigger an error here. This could be fixed + // by checking the type ourselves, but + // probably isn't worth the effort. + // + // "%v" correctly turns bool, int, strings into + // the representation expected by flag, so those + // can be used in config files. Plain strings + // always work there, just as on the command line. + str := fmt.Sprintf("%v", viper.Get(f.Name)) + if err := f.Value.Set(str); err != nil { + result = fmt.Errorf("setting option %q from config file value: %s", f.Name, err) + } + }) + + return result +}