Testing: use structured arguments

This converts the internal testing package to use structured arguments,
introducing a new helper to preserve compat when specifying legacy args.

pkg/internal/testing/controlplane/apiserver.go

@@ -44,6 +44,11 @@ type APIServer struct {
 	//
 	// If not specified, the minimal set of arguments to run the APIServer will
 	// be used.
+	//
+	// They will be loaded into the same argument set as Configure.  Each flag
+	// will be Append-ed to the configured arguments just before launch.
+	//
+	// Deprecated: use Configure instead.
 	Args []string
 
 	// CertDir is a path to a directory containing whatever certificates the
@@ -72,20 +77,34 @@ type APIServer struct {
 	Err io.Writer
 
 	processState *process.State
+
+	// args contains the structured arguments to use for running the API server
+	// Lazily initialized by .Configure(), Defaulted eventually with .defaultArgs()
+	args *process.Arguments
+}
+
+// Configure returns Arguments that may be used to customize the
+// flags used to launch the API server.  A set of defaults will
+// be applied underneath.
+func (s *APIServer) Configure() *process.Arguments {
+	if s.args == nil {
+		s.args = process.EmptyArguments()
+	}
+	return s.args
 }
 
 // Start starts the apiserver, waits for it to come up, and returns an error,
 // if occurred.
 func (s *APIServer) Start() error {
 	if s.processState == nil {
-		if err := s.setState(); err != nil {
+		if err := s.setProcessState(); err != nil {
 			return err
 		}
 	}
 	return s.processState.Start(s.Out, s.Err)
 }
 
-func (s *APIServer) setState() error {
+func (s *APIServer) setProcessState() error {
 	if s.EtcdURL == nil {
 		return fmt.Errorf("expected EtcdURL to be configured")
 	}
@@ -133,15 +152,42 @@ func (s *APIServer) setState() error {
 		return err
 	}
 
-	args := s.Args
-	if len(args) == 0 {
-		args = APIServerDefaultArgs
-	}
-
-	s.processState.Args, err = process.RenderTemplates(args, s)
+	s.processState.Args, err = process.TemplateAndArguments(s.Args, s.Configure(), process.TemplateDefaults{
+		Data:     s,
+		Defaults: s.defaultArgs(),
+		// as per kubernetes-sigs/controller-runtime#641, we need this (we
+		// probably need other stuff too, but this is the only thing that was
+		// previously considered a "minimal default")
+		MinimalDefaults: map[string][]string{
+			"service-cluster-ip-range": []string{"10.0.0.0/24"},
+		},
+	})
 	return err
 }
 
+func (s *APIServer) defaultArgs() map[string][]string {
+	args := map[string][]string{
+		"advertise-address":        []string{"127.0.0.1"},
+		"service-cluster-ip-range": []string{"10.0.0.0/24"},
+		"allow-privileged":         []string{"true"},
+		// we're keeping this disabled because if enabled, default SA is
+		// missing which would force all tests to create one in normal
+		// apiserver operation this SA is created by controller, but that is
+		// not run in integration environment
+		"disable-admission-plugins": []string{"ServiceAccount"},
+		"cert-dir":                  []string{s.CertDir},
+		"secure-port":               []string{strconv.Itoa(s.SecurePort)},
+	}
+	if s.EtcdURL != nil {
+		args["etcd-servers"] = []string{s.EtcdURL.String()}
+	}
+	if s.URL != nil {
+		args["insecure-port"] = []string{s.URL.Port()}
+		args["insecure-bind-address"] = []string{s.URL.Hostname()}
+	}
+	return args
+}
+
 func (s *APIServer) populateAPIServerCerts() error {
 	_, statErr := os.Stat(filepath.Join(s.CertDir, "apiserver.crt"))
 	if !os.IsNotExist(statErr) {

pkg/internal/testing/controlplane/etcd.go

@@ -36,6 +36,11 @@ type Etcd struct {
 	//
 	// If not specified, the minimal set of arguments to run the Etcd will be
 	// used.
+	//
+	// They will be loaded into the same argument set as Configure.  Each flag
+	// will be Append-ed to the configured arguments just before launch.
+	//
+	// Deprecated: use Configure instead.
 	Args []string
 
 	// DataDir is a path to a directory in which etcd can store its state.
@@ -59,22 +64,24 @@ type Etcd struct {
 
 	// processState contains the actual details about this running process
 	processState *process.State
+
+	// args contains the structured arguments to use for running etcd.
+	// Lazily initialized by .Configure(), Defaulted eventually with .defaultArgs()
+	args *process.Arguments
 }
 
 // Start starts the etcd, waits for it to come up, and returns an error, if one
 // occoured.
 func (e *Etcd) Start() error {
 	if e.processState == nil {
-		if err := e.setState(); err != nil {
+		if err := e.setProcessState(); err != nil {
 			return err
 		}
 	}
 	return e.processState.Start(e.Out, e.Err)
 }
 
-func (e *Etcd) setState() error {
-	var err error
-
+func (e *Etcd) setProcessState() error {
 	e.processState = &process.State{
 		Dir:          e.DataDir,
 		Path:         e.Path,
@@ -106,12 +113,11 @@ func (e *Etcd) setState() error {
 	e.StartTimeout = e.processState.StartTimeout
 	e.StopTimeout = e.processState.StopTimeout
 
-	args := e.Args
-	if len(args) == 0 {
-		args = EtcdDefaultArgs
-	}
-
-	e.processState.Args, err = process.RenderTemplates(args, e)
+	var err error
+	e.processState.Args, err = process.TemplateAndArguments(e.Args, e.Configure(), process.TemplateDefaults{
+		Data:     e,
+		Defaults: e.defaultArgs(),
+	})
 	return err
 }
 
@@ -121,6 +127,28 @@ func (e *Etcd) Stop() error {
 	return e.processState.Stop()
 }
 
+func (e *Etcd) defaultArgs() map[string][]string {
+	args := map[string][]string{
+		"listen-peer-urls": []string{"http://localhost:0"},
+		"data-dir":         []string{e.DataDir},
+	}
+	if e.URL != nil {
+		args["advertise-client-urls"] = []string{e.URL.String()}
+		args["listen-client-urls"] = []string{e.URL.String()}
+	}
+	return args
+}
+
+// Configure returns Arguments that may be used to customize the
+// flags used to launch etcd.  A set of defaults will
+// be applied underneath.
+func (e *Etcd) Configure() *process.Arguments {
+	if e.args == nil {
+		e.args = process.EmptyArguments()
+	}
+	return e.args
+}
+
 // EtcdDefaultArgs exposes the default args for Etcd so that you
 // can use those to append your own additional arguments.
 var EtcdDefaultArgs = []string{

pkg/internal/testing/process/arguments.go

@@ -3,11 +3,13 @@ package process
 import (
 	"bytes"
 	"html/template"
+	"sort"
+	"strings"
 )
 
 // RenderTemplates returns an []string to render the templates
 //
-// Deprecated: this should be removed when we remove APIServer.Args.
+// Deprecated: will be removed in favor of Arguments
 func RenderTemplates(argTemplates []string, data interface{}) (args []string, err error) {
 	var t *template.Template
 
@@ -30,6 +32,81 @@ func RenderTemplates(argTemplates []string, data interface{}) (args []string, er
 	return
 }
 
+// SliceToArguments converts a slice of arguments to structured arguments,
+// appending each argument that starts with `--` and contains an `=` to the
+// argument set, returning the rest.
+//
+// Deprecated: will be removed when RenderTemplates is removed
+func SliceToArguments(sliceArgs []string, args *Arguments) []string {
+	var rest []string
+	for i, arg := range sliceArgs {
+		if arg == "--" {
+			rest = append(rest, sliceArgs[i:]...)
+			return rest
+		}
+		// skip non-flag arguments, skip arguments w/o equals because we
+		// can't tell if the next argument should take a value
+		if !strings.HasPrefix(arg, "--") || !strings.Contains(arg, "=") {
+			rest = append(rest, arg)
+			continue
+		}
+
+		parts := strings.SplitN(arg[2:], "=", 2)
+		name := parts[0]
+		val := parts[1]
+
+		args.AppendNoDefaults(name, val)
+	}
+
+	return rest
+}
+
+// TemplateDefaults specifies defaults to be used for joining structured arguments with templates.
+//
+// Deprecated: will be removed when RenderTemplates is removed
+type TemplateDefaults struct {
+	// Data will be used to render the template.
+	Data interface{}
+	// Defaults will be used to default structured arguments if no template is passed.
+	Defaults map[string][]string
+	// MinimalDefaults will be used to default structured arguments if a template is passed.
+	// Use this for flags which *must* be present.
+	MinimalDefaults map[string][]string // for api server service-cluster-ip-range
+}
+
+// TemplateAndArguments joins structured arguments and non-structured arguments, preserving existing
+// behavior.  Namely:
+//
+// 1. if templ has len > 0, it will be rendered against data
+// 2. the rendered template values that look like `--foo=bar` will be split
+//    and appended to args, the rest will be kept around
+// 3. the given args will be rendered as string form.  If a template is given,
+//    no defaults will be used, otherwise defaults will be used
+// 4. a result of [args..., rest...] will be returned
+//
+// Deprecated: will be removed when RenderTemplates is removed
+func TemplateAndArguments(templ []string, args *Arguments, data TemplateDefaults) ([]string, error) {
+	if len(templ) == 0 { // 3 & 4 (no template case)
+		return args.AsStrings(data.Defaults), nil
+	}
+
+	// 1: render the template
+	rendered, err := RenderTemplates(templ, data.Data)
+	if err != nil {
+		return nil, err
+	}
+
+	// 2: filter out structured args and add them to args
+	rest := SliceToArguments(rendered, args)
+
+	// 3 (template case): render structured args, no defaults (matching the
+	// legacy case where if Args was specified, no defaults were used)
+	res := args.AsStrings(data.MinimalDefaults)
+
+	// 4: return the rendered structured args + all non-structured args
+	return append(res, rest...), nil
+}
+
 // EmptyArguments constructs an empty set of flags with no defaults.
 func EmptyArguments() *Arguments {
 	return &Arguments{