From ddfdfdff78f6ef84c514cf16308835bc23feb325 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Thu, 25 Mar 2021 15:38:47 -0700 Subject: [PATCH] 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. --- .../testing/controlplane/apiserver.go | 62 ++++++++-- pkg/internal/testing/controlplane/etcd.go | 48 ++++++-- pkg/internal/testing/process/arguments.go | 79 +++++++++++- .../testing/process/arguments_test.go | 113 ++++++++++++++++++ 4 files changed, 283 insertions(+), 19 deletions(-) diff --git a/pkg/internal/testing/controlplane/apiserver.go b/pkg/internal/testing/controlplane/apiserver.go index eae05eaa6a..56631e16c3 100644 --- a/pkg/internal/testing/controlplane/apiserver.go +++ b/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) { diff --git a/pkg/internal/testing/controlplane/etcd.go b/pkg/internal/testing/controlplane/etcd.go index fde45e20ed..af28533f11 100644 --- a/pkg/internal/testing/controlplane/etcd.go +++ b/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{ diff --git a/pkg/internal/testing/process/arguments.go b/pkg/internal/testing/process/arguments.go index 4d9edd6f5b..206fc2e902 100644 --- a/pkg/internal/testing/process/arguments.go +++ b/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{ diff --git a/pkg/internal/testing/process/arguments_test.go b/pkg/internal/testing/process/arguments_test.go index fb7da9ed82..9a49382fe6 100644 --- a/pkg/internal/testing/process/arguments_test.go +++ b/pkg/internal/testing/process/arguments_test.go @@ -81,6 +81,119 @@ var _ = Describe("Arguments Templates", func() { ContainSubstring("can't evaluate field"), )) }) + + Context("when joining with structured Arguments", func() { + var ( + args *Arguments + templ = []string{ + "--cheese=parmesean", + "-om", + "nom nom nom", + "--sharpness={{ .sharpness }}", + } + data = TemplateDefaults{ + Data: map[string]string{"sharpness": "extra"}, + Defaults: map[string][]string{ + "cracker": []string{"ritz"}, + "pickle": []string{"kosher-dill"}, + }, + MinimalDefaults: map[string][]string{ + "pickle": []string{"kosher-dill"}, + }, + } + ) + BeforeEach(func() { + args = EmptyArguments() + }) + + Context("when a template is given", func() { + It("should use minimal defaults", func() { + Expect(TemplateAndArguments(templ, args, data)).To(SatisfyAll( + Not(ContainElement("--cracker=ritz")), + ContainElement("--pickle=kosher-dill"), + )) + }) + + It("should render the template against the data", func() { + Expect(TemplateAndArguments(templ, args, data)).To(ContainElements( + "--sharpness=extra", + )) + }) + + It("should append the rendered template to structured arguments", func() { + args.Append("cheese", "cheddar") + + Expect(TemplateAndArguments(templ, args, data)).To(Equal([]string{ + "--cheese=cheddar", + "--cheese=parmesean", + "--pickle=kosher-dill", + "--sharpness=extra", + "-om", + "nom nom nom", + })) + }) + }) + + Context("when no template is given", func() { + It("should render the structured arguments with the given defaults", func() { + args. + Append("cheese", "cheddar", "parmesean"). + Append("cracker", "triscuit") + + Expect(TemplateAndArguments(nil, args, data)).To(Equal([]string{ + "--cheese=cheddar", + "--cheese=parmesean", + "--cracker=ritz", + "--cracker=triscuit", + "--pickle=kosher-dill", + })) + }) + }) + }) + + Context("when converting to structured Arguments", func() { + var args *Arguments + BeforeEach(func() { + args = EmptyArguments() + }) + + It("should skip arguments that don't start with `--`", func() { + rest := SliceToArguments([]string{"-first", "second", "--foo=bar"}, args) + Expect(rest).To(Equal([]string{"-first", "second"})) + Expect(args.AsStrings(nil)).To(Equal([]string{"--foo=bar"})) + }) + + It("should skip arguments that don't contain an `=` because they're ambiguous", func() { + rest := SliceToArguments([]string{"--first", "--second", "--foo=bar"}, args) + Expect(rest).To(Equal([]string{"--first", "--second"})) + Expect(args.AsStrings(nil)).To(Equal([]string{"--foo=bar"})) + }) + + It("should stop at the flag terminator (`--`)", func() { + rest := SliceToArguments([]string{"--first", "--second", "--", "--foo=bar"}, args) + Expect(rest).To(Equal([]string{"--first", "--second", "--", "--foo=bar"})) + Expect(args.AsStrings(nil)).To(BeEmpty()) + }) + + It("should split --foo=bar into Append(foo, bar)", func() { + rest := SliceToArguments([]string{"--foo=bar1", "--foo=bar2"}, args) + Expect(rest).To(BeEmpty()) + Expect(args.Get("foo").Get(nil)).To(Equal([]string{"bar1", "bar2"})) + }) + + It("should split --foo=bar=baz into Append(foo, bar=baz)", func() { + rest := SliceToArguments([]string{"--vmodule=file.go=3", "--vmodule=other.go=4"}, args) + Expect(rest).To(BeEmpty()) + Expect(args.Get("vmodule").Get(nil)).To(Equal([]string{"file.go=3", "other.go=4"})) + }) + + It("should append to existing arguments", func() { + args.Append("foo", "barA") + rest := SliceToArguments([]string{"--foo=bar1", "--foo=bar2"}, args) + Expect(rest).To(BeEmpty()) + Expect(args.Get("foo").Get([]string{"barI"})).To(Equal([]string{"barI", "barA", "bar1", "bar2"})) + }) + }) }) type plainDefaults map[string][]string