From 1640cdb044056924383f182d1afd62449f4455c5 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Mon, 19 Mar 2018 02:00:20 -0700 Subject: [PATCH] pkg/flags: clean up, add "SelectiveStringsValue" Signed-off-by: Gyuho Lee --- etcdmain/config.go | 20 +-- pkg/flags/flag.go | 50 +------- .../{string_slice_test.go => ignored.go} | 39 +++--- pkg/flags/selective_string.go | 114 ++++++++++++++++++ pkg/flags/selective_string_test.go | 70 +++++++++++ pkg/flags/string_slice.go | 44 ------- pkg/flags/strings.go | 55 +++++---- pkg/flags/strings_test.go | 35 ++---- pkg/flags/urls.go | 9 ++ 9 files changed, 264 insertions(+), 172 deletions(-) rename pkg/flags/{string_slice_test.go => ignored.go} (53%) create mode 100644 pkg/flags/selective_string.go create mode 100644 pkg/flags/selective_string_test.go delete mode 100644 pkg/flags/string_slice.go diff --git a/etcdmain/config.go b/etcdmain/config.go index afa54d9cd..4bef82781 100644 --- a/etcdmain/config.go +++ b/etcdmain/config.go @@ -86,9 +86,9 @@ type config struct { // configFlags has the set of flags used for command line parsing a Config type configFlags struct { flagSet *flag.FlagSet - clusterState *flags.StringsFlag - fallback *flags.StringsFlag - proxy *flags.StringsFlag + clusterState *flags.SelectiveStringValue + fallback *flags.SelectiveStringValue + proxy *flags.SelectiveStringValue } func newConfig() *config { @@ -105,15 +105,15 @@ func newConfig() *config { } cfg.cf = configFlags{ flagSet: flag.NewFlagSet("etcd", flag.ContinueOnError), - clusterState: flags.NewStringsFlag( + clusterState: flags.NewSelectiveStringValue( embed.ClusterStateFlagNew, embed.ClusterStateFlagExisting, ), - fallback: flags.NewStringsFlag( + fallback: flags.NewSelectiveStringValue( fallbackFlagProxy, fallbackFlagExit, ), - proxy: flags.NewStringsFlag( + proxy: flags.NewSelectiveStringValue( proxyFlagOff, proxyFlagReadonly, proxyFlagOn, @@ -151,7 +151,7 @@ func newConfig() *config { fs.Var(flags.NewURLsValue(embed.DefaultInitialAdvertisePeerURLs), "initial-advertise-peer-urls", "List of this member's peer URLs to advertise to the rest of the cluster.") fs.Var(flags.NewURLsValue(embed.DefaultAdvertiseClientURLs), "advertise-client-urls", "List of this member's client URLs to advertise to the public.") fs.StringVar(&cfg.ec.Durl, "discovery", cfg.ec.Durl, "Discovery URL used to bootstrap the cluster.") - fs.Var(cfg.cf.fallback, "discovery-fallback", fmt.Sprintf("Valid values include %s", strings.Join(cfg.cf.fallback.Values, ", "))) + fs.Var(cfg.cf.fallback, "discovery-fallback", fmt.Sprintf("Valid values include %q", cfg.cf.fallback.Valids())) fs.StringVar(&cfg.ec.Dproxy, "discovery-proxy", cfg.ec.Dproxy, "HTTP proxy to use for traffic to discovery service.") fs.StringVar(&cfg.ec.DNSCluster, "discovery-srv", cfg.ec.DNSCluster, "DNS domain used to bootstrap initial cluster.") @@ -165,7 +165,7 @@ func newConfig() *config { fs.StringVar(&cfg.ec.ExperimentalEnableV2V3, "experimental-enable-v2v3", cfg.ec.ExperimentalEnableV2V3, "v3 prefix for serving emulated v2 state.") // proxy - fs.Var(cfg.cf.proxy, "proxy", fmt.Sprintf("Valid values include %s", strings.Join(cfg.cf.proxy.Values, ", "))) + fs.Var(cfg.cf.proxy, "proxy", fmt.Sprintf("Valid values include %q", cfg.cf.proxy.Valids())) fs.UintVar(&cfg.cp.ProxyFailureWaitMs, "proxy-failure-wait", cfg.cp.ProxyFailureWaitMs, "Time (in milliseconds) an endpoint will be held in a failed state.") fs.UintVar(&cfg.cp.ProxyRefreshIntervalMs, "proxy-refresh-interval", cfg.cp.ProxyRefreshIntervalMs, "Time (in milliseconds) of the endpoints refresh interval.") @@ -189,7 +189,7 @@ func newConfig() *config { fs.BoolVar(&cfg.ec.PeerAutoTLS, "peer-auto-tls", false, "Peer TLS using generated certificates") fs.StringVar(&cfg.ec.PeerTLSInfo.CRLFile, "peer-crl-file", "", "Path to the peer certificate revocation list file.") fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedCN, "peer-cert-allowed-cn", "", "Allowed CN for inter peer authentication.") - fs.Var(flags.NewStringSlice(""), "host-whitelist", "Comma-separated acceptable hostnames from HTTP client requests, if server is not secure (empty means allow all).") + fs.Var(flags.NewStringsValue(""), "host-whitelist", "Comma-separated acceptable hostnames from HTTP client requests, if server is not secure (empty means allow all).") // logging fs.BoolVar(&cfg.ec.Debug, "debug", false, "Enable debug-level logging for etcd.") @@ -268,7 +268,7 @@ func (cfg *config) configFromCmdLine() error { cfg.ec.APUrls = flags.URLsFromFlag(cfg.cf.flagSet, "initial-advertise-peer-urls") cfg.ec.LCUrls = flags.URLsFromFlag(cfg.cf.flagSet, "listen-client-urls") cfg.ec.ACUrls = flags.URLsFromFlag(cfg.cf.flagSet, "advertise-client-urls") - cfg.ec.HostWhitelist = flags.StringSliceFromFlag(cfg.cf.flagSet, "host-whitelist") + cfg.ec.HostWhitelist = flags.StringsFromFlag(cfg.cf.flagSet, "host-whitelist") cfg.ec.ListenMetricsUrls = flags.URLsFromFlag(cfg.cf.flagSet, "listen-metrics-urls") cfg.ec.ClusterState = cfg.cf.clusterState.String() diff --git a/pkg/flags/flag.go b/pkg/flags/flag.go index 3e4924e4a..9f3c02aba 100644 --- a/pkg/flags/flag.go +++ b/pkg/flags/flag.go @@ -18,7 +18,6 @@ package flags import ( "flag" "fmt" - "net/url" "os" "strings" @@ -26,44 +25,7 @@ import ( "github.com/spf13/pflag" ) -var ( - plog = capnslog.NewPackageLogger("github.com/coreos/etcd", "pkg/flags") -) - -// DeprecatedFlag encapsulates a flag that may have been previously valid but -// is now deprecated. If a DeprecatedFlag is set, an error occurs. -type DeprecatedFlag struct { - Name string -} - -func (f *DeprecatedFlag) Set(_ string) error { - return fmt.Errorf(`flag "-%s" is no longer supported.`, f.Name) -} - -func (f *DeprecatedFlag) String() string { - return "" -} - -// IgnoredFlag encapsulates a flag that may have been previously valid but is -// now ignored. If an IgnoredFlag is set, a warning is printed and -// operation continues. -type IgnoredFlag struct { - Name string -} - -// IsBoolFlag is defined to allow the flag to be defined without an argument -func (f *IgnoredFlag) IsBoolFlag() bool { - return true -} - -func (f *IgnoredFlag) Set(s string) error { - plog.Warningf(`flag "-%s" is no longer supported - ignoring.`, f.Name) - return nil -} - -func (f *IgnoredFlag) String() string { - return "" -} +var plog = capnslog.NewPackageLogger("github.com/coreos/etcd", "pkg/flags") // SetFlagsFromEnv parses all registered flags in the given flagset, // and if they are not already set it attempts to set their values from @@ -148,16 +110,6 @@ func setFlagFromEnv(fs flagSetter, prefix, fname string, usedEnvKey, alreadySet return nil } -// URLsFromFlag returns a slices from url got from the flag. -func URLsFromFlag(fs *flag.FlagSet, urlsFlagName string) []url.URL { - return []url.URL(*fs.Lookup(urlsFlagName).Value.(*URLsValue)) -} - -// StringSliceFromFlag returns a string slice from the flag. -func StringSliceFromFlag(fs *flag.FlagSet, flagName string) []string { - return []string(*fs.Lookup(flagName).Value.(*StringSlice)) -} - func IsSet(fs *flag.FlagSet, name string) bool { set := false fs.Visit(func(f *flag.Flag) { diff --git a/pkg/flags/string_slice_test.go b/pkg/flags/ignored.go similarity index 53% rename from pkg/flags/string_slice_test.go rename to pkg/flags/ignored.go index 9f96ede03..995304900 100644 --- a/pkg/flags/string_slice_test.go +++ b/pkg/flags/ignored.go @@ -14,24 +14,23 @@ package flags -import ( - "reflect" - "testing" -) - -func TestStringSlice(t *testing.T) { - tests := []struct { - s string - exp []string - }{ - {s: "a,b,c", exp: []string{"a", "b", "c"}}, - {s: "a, b,c", exp: []string{"a", " b", "c"}}, - {s: "", exp: []string{}}, - } - for i := range tests { - ss := []string(*NewStringSlice(tests[i].s)) - if !reflect.DeepEqual(tests[i].exp, ss) { - t.Fatalf("#%d: expected %q, got %q", i, tests[i].exp, ss) - } - } +// IgnoredFlag encapsulates a flag that may have been previously valid but is +// now ignored. If an IgnoredFlag is set, a warning is printed and +// operation continues. +type IgnoredFlag struct { + Name string +} + +// IsBoolFlag is defined to allow the flag to be defined without an argument +func (f *IgnoredFlag) IsBoolFlag() bool { + return true +} + +func (f *IgnoredFlag) Set(s string) error { + plog.Warningf(`flag "-%s" is no longer supported - ignoring.`, f.Name) + return nil +} + +func (f *IgnoredFlag) String() string { + return "" } diff --git a/pkg/flags/selective_string.go b/pkg/flags/selective_string.go new file mode 100644 index 000000000..4b90fbf4b --- /dev/null +++ b/pkg/flags/selective_string.go @@ -0,0 +1,114 @@ +// Copyright 2018 The etcd 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 flags + +import ( + "errors" + "fmt" + "sort" + "strings" +) + +// SelectiveStringValue implements the flag.Value interface. +type SelectiveStringValue struct { + v string + valids map[string]struct{} +} + +// Set verifies the argument to be a valid member of the allowed values +// before setting the underlying flag value. +func (ss *SelectiveStringValue) Set(s string) error { + if _, ok := ss.valids[s]; ok { + ss.v = s + return nil + } + return errors.New("invalid value") +} + +// String returns the set value (if any) of the SelectiveStringValue +func (ss *SelectiveStringValue) String() string { + return ss.v +} + +// Valids returns the list of valid strings. +func (ss *SelectiveStringValue) Valids() []string { + s := make([]string, 0, len(ss.valids)) + for k := range ss.valids { + s = append(s, k) + } + sort.Strings(s) + return s +} + +// NewSelectiveStringValue creates a new string flag +// for which any one of the given strings is a valid value, +// and any other value is an error. +// +// valids[0] will be default value. Caller must be sure +// len(valids) != 0 or it will panic. +func NewSelectiveStringValue(valids ...string) *SelectiveStringValue { + vm := make(map[string]struct{}) + for _, v := range valids { + vm[v] = struct{}{} + } + return &SelectiveStringValue{valids: vm, v: valids[0]} +} + +// SelectiveStringsValue implements the flag.Value interface. +type SelectiveStringsValue struct { + vs []string + valids map[string]struct{} +} + +// Set verifies the argument to be a valid member of the allowed values +// before setting the underlying flag value. +func (ss *SelectiveStringsValue) Set(s string) error { + vs := strings.Split(s, ",") + for i := range vs { + if _, ok := ss.valids[vs[i]]; ok { + ss.vs = append(ss.vs, vs[i]) + } else { + return fmt.Errorf("invalid value %q", vs[i]) + } + } + sort.Strings(ss.vs) + return nil +} + +// String returns the set value (if any) of the SelectiveStringsValue. +func (ss *SelectiveStringsValue) String() string { + return strings.Join(ss.vs, ",") +} + +// Valids returns the list of valid strings. +func (ss *SelectiveStringsValue) Valids() []string { + s := make([]string, 0, len(ss.valids)) + for k := range ss.valids { + s = append(s, k) + } + sort.Strings(s) + return s +} + +// NewSelectiveStringsValue creates a new string slice flag +// for which any one of the given strings is a valid value, +// and any other value is an error. +func NewSelectiveStringsValue(valids ...string) *SelectiveStringsValue { + vm := make(map[string]struct{}) + for _, v := range valids { + vm[v] = struct{}{} + } + return &SelectiveStringsValue{valids: vm, vs: []string{}} +} diff --git a/pkg/flags/selective_string_test.go b/pkg/flags/selective_string_test.go new file mode 100644 index 000000000..cc310ed63 --- /dev/null +++ b/pkg/flags/selective_string_test.go @@ -0,0 +1,70 @@ +// Copyright 2018 The etcd 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 flags + +import ( + "testing" +) + +func TestSelectiveStringValue(t *testing.T) { + tests := []struct { + vals []string + + val string + pass bool + }{ + // known values + {[]string{"abc", "def"}, "abc", true}, + {[]string{"on", "off", "false"}, "on", true}, + + // unrecognized values + {[]string{"abc", "def"}, "ghi", false}, + {[]string{"on", "off"}, "", false}, + } + for i, tt := range tests { + sf := NewSelectiveStringValue(tt.vals...) + if sf.v != tt.vals[0] { + t.Errorf("#%d: want default val=%v,but got %v", i, tt.vals[0], sf.v) + } + err := sf.Set(tt.val) + if tt.pass != (err == nil) { + t.Errorf("#%d: want pass=%t, but got err=%v", i, tt.pass, err) + } + } +} + +func TestSelectiveStringsValue(t *testing.T) { + tests := []struct { + vals []string + + val string + pass bool + }{ + {[]string{"abc", "def"}, "abc", true}, + {[]string{"abc", "def"}, "abc,def", true}, + {[]string{"abc", "def"}, "abc, def", false}, + {[]string{"on", "off", "false"}, "on,false", true}, + {[]string{"abc", "def"}, "ghi", false}, + {[]string{"on", "off"}, "", false}, + {[]string{"a", "b", "c", "d", "e"}, "a,c,e", true}, + } + for i, tt := range tests { + sf := NewSelectiveStringsValue(tt.vals...) + err := sf.Set(tt.val) + if tt.pass != (err == nil) { + t.Errorf("#%d: want pass=%t, but got err=%v", i, tt.pass, err) + } + } +} diff --git a/pkg/flags/string_slice.go b/pkg/flags/string_slice.go deleted file mode 100644 index 9a19b86a7..000000000 --- a/pkg/flags/string_slice.go +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright 2018 The etcd 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 flags - -import ( - "sort" - "strings" -) - -// StringSlice wraps "sort.StringSlice". -type StringSlice sort.StringSlice - -// Set parses a command line set of strings, separated by comma. -func (ss *StringSlice) Set(s string) error { - *ss = strings.Split(s, ",") - return nil -} - -func (ss *StringSlice) String() string { return strings.Join(*ss, ",") } - -// NewStringSlice implements string slice as flag.Value interface. -// Given value is to be separated by comma. -func NewStringSlice(s string) (ss *StringSlice) { - if s == "" { - return &StringSlice{} - } - ss = new(StringSlice) - if err := ss.Set(s); err != nil { - plog.Panicf("new StringSlice should never fail: %v", err) - } - return ss -} diff --git a/pkg/flags/strings.go b/pkg/flags/strings.go index 40ee43253..3e47fb38e 100644 --- a/pkg/flags/strings.go +++ b/pkg/flags/strings.go @@ -1,4 +1,4 @@ -// Copyright 2015 The etcd Authors +// Copyright 2018 The etcd Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -14,36 +14,39 @@ package flags -import "errors" +import ( + "flag" + "sort" + "strings" +) -// NewStringsFlag creates a new string flag for which any one of the given -// strings is a valid value, and any other value is an error. -// -// valids[0] will be default value. Caller must be sure len(valids)!=0 or -// it will panic. -func NewStringsFlag(valids ...string) *StringsFlag { - return &StringsFlag{Values: valids, val: valids[0]} +// StringsValue wraps "sort.StringSlice". +type StringsValue sort.StringSlice + +// Set parses a command line set of strings, separated by comma. +// Implements "flag.Value" interface. +func (ss *StringsValue) Set(s string) error { + *ss = strings.Split(s, ",") + return nil } -// StringsFlag implements the flag.Value interface. -type StringsFlag struct { - Values []string - val string -} +// String implements "flag.Value" interface. +func (ss *StringsValue) String() string { return strings.Join(*ss, ",") } -// Set verifies the argument to be a valid member of the allowed values -// before setting the underlying flag value. -func (ss *StringsFlag) Set(s string) error { - for _, v := range ss.Values { - if s == v { - ss.val = s - return nil - } +// NewStringsValue implements string slice as "flag.Value" interface. +// Given value is to be separated by comma. +func NewStringsValue(s string) (ss *StringsValue) { + if s == "" { + return &StringsValue{} } - return errors.New("invalid value") + ss = new(StringsValue) + if err := ss.Set(s); err != nil { + plog.Panicf("new StringsValue should never fail: %v", err) + } + return ss } -// String returns the set value (if any) of the StringsFlag -func (ss *StringsFlag) String() string { - return ss.val +// StringsFromFlag returns a string slice from the flag. +func StringsFromFlag(fs *flag.FlagSet, flagName string) []string { + return []string(*fs.Lookup(flagName).Value.(*StringsValue)) } diff --git a/pkg/flags/strings_test.go b/pkg/flags/strings_test.go index b699961e3..3835612b0 100644 --- a/pkg/flags/strings_test.go +++ b/pkg/flags/strings_test.go @@ -1,4 +1,4 @@ -// Copyright 2015 The etcd Authors +// Copyright 2018 The etcd Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,34 +15,23 @@ package flags import ( + "reflect" "testing" ) -func TestStringsSet(t *testing.T) { +func TestStringsValue(t *testing.T) { tests := []struct { - vals []string - - val string - pass bool + s string + exp []string }{ - // known values - {[]string{"abc", "def"}, "abc", true}, - {[]string{"on", "off", "false"}, "on", true}, - - // unrecognized values - {[]string{"abc", "def"}, "ghi", false}, - {[]string{"on", "off"}, "", false}, + {s: "a,b,c", exp: []string{"a", "b", "c"}}, + {s: "a, b,c", exp: []string{"a", " b", "c"}}, + {s: "", exp: []string{}}, } - - for i, tt := range tests { - sf := NewStringsFlag(tt.vals...) - if sf.val != tt.vals[0] { - t.Errorf("#%d: want default val=%v,but got %v", i, tt.vals[0], sf.val) - } - - err := sf.Set(tt.val) - if tt.pass != (err == nil) { - t.Errorf("#%d: want pass=%t, but got err=%v", i, tt.pass, err) + for i := range tests { + ss := []string(*NewStringsValue(tests[i].s)) + if !reflect.DeepEqual(tests[i].exp, ss) { + t.Fatalf("#%d: expected %q, got %q", i, tests[i].exp, ss) } } } diff --git a/pkg/flags/urls.go b/pkg/flags/urls.go index 9d2818774..180a8d6f4 100644 --- a/pkg/flags/urls.go +++ b/pkg/flags/urls.go @@ -15,6 +15,8 @@ package flags import ( + "flag" + "net/url" "strings" "github.com/coreos/etcd/pkg/types" @@ -25,6 +27,7 @@ type URLsValue types.URLs // Set parses a command line set of URLs formatted like: // http://127.0.0.1:2380,http://10.1.1.2:80 +// Implements "flag.Value" interface. func (us *URLsValue) Set(s string) error { ss, err := types.NewURLs(strings.Split(s, ",")) if err != nil { @@ -34,6 +37,7 @@ func (us *URLsValue) Set(s string) error { return nil } +// String implements "flag.Value" interface. func (us *URLsValue) String() string { all := make([]string, len(*us)) for i, u := range *us { @@ -54,3 +58,8 @@ func NewURLsValue(s string) *URLsValue { } return v } + +// URLsFromFlag returns a slices from url got from the flag. +func URLsFromFlag(fs *flag.FlagSet, urlsFlagName string) []url.URL { + return []url.URL(*fs.Lookup(urlsFlagName).Value.(*URLsValue)) +}