Merge pull request #54823 from mtaufen/structure-eviction-thresholds

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Lift embedded structure out of eviction-related KubeletConfiguration fields

- Changes the following KubeletConfiguration fields from `string` to
`map[string]string`:
  - `EvictionHard`
  - `EvictionSoft`
  - `EvictionSoftGracePeriod`
  - `EvictionMinimumReclaim`
- Adds flag parsing shims to maintain Kubelet's public flags API, while
enabling structured input in the file API.
- Also removes `kubeletconfig.ConfigurationMap`, which was an ad-hoc flag
parsing shim living in the kubeletconfig API group, and replaces it
with the `MapStringString` shim introduced in this PR. Flag parsing
shims belong in a common place, not in the kubeletconfig API.
I manually audited these to ensure that this wouldn't cause errors
parsing the command line for syntax that would have previously been
error free (`kubeletconfig.ConfigurationMap` was unique in that it
allowed keys to be provided on the CLI without values. I believe this was
done in `flags.ConfigurationMap` to facilitate the `--node-labels` flag,
which rightfully accepts value-free keys, and that this shim was then
just copied to `kubeletconfig`). Fortunately, the affected fields
(`ExperimentalQOSReserved`, `SystemReserved`, and `KubeReserved`) expect
non-empty strings in the values of the map, and as a result passing the
empty string is already an error. Thus requiring keys shouldn't break
anyone's scripts.
- Updates code and tests accordingly.

Regarding eviction operators, directionality is already implicit in the
signal type (for a given signal, the decision to evict will be made when
crossing the threshold from either above or below, never both). There is
no need to expose an operator, such as `<`, in the API. By changing
`EvictionHard` and `EvictionSoft` to `map[string]string`, this PR
simplifies the experience of working with these fields via the
`KubeletConfiguration` type. Again, flags stay the same.

Other things:
- There is another flag parsing shim, `flags.ConfigurationMap`, from the
shared flag utility. The `NodeLabels` field still uses
`flags.ConfigurationMap`. This PR moves the allocation of the
`map[string]string` for the `NodeLabels` field from
`AddKubeletConfigFlags` to the defaulter for the external
`KubeletConfiguration` type. Flags are layered on top of an internal
object that has undergone conversion from a defaulted external object,
which means that previously the mere registration of flags would have
overwritten any previously-defined defaults for `NodeLabels` (fortunately
there were none).

Related: #53833 (lifting embedded structures out of string fields is part of getting this API to beta)

```release-note
The EvictionHard, EvictionSoft, EvictionSoftGracePeriod, EvictionMinimumReclaim, SystemReserved, and KubeReserved fields in the KubeletConfiguration object (kubeletconfig/v1alpha1) are now of type map[string]string, which facilitates writing JSON and YAML files.
```

Kubernetes-commit: 00fe2cfe6cfa2513f8165b696a2570cbbd2498cf

pkg/util/flag/BUILD

@@ -10,16 +10,14 @@ go_test(
     name = "go_default_test",
     srcs = [
         "colon_separated_multimap_string_string_test.go",
+        "langle_separated_map_string_string_test.go",
         "map_string_bool_test.go",
+        "map_string_string_test.go",
         "namedcertkey_flag_test.go",
     ],
     importpath = "k8s.io/apiserver/pkg/util/flag",
     library = ":go_default_library",
-    deps = [
-        "//vendor/github.com/spf13/pflag:go_default_library",
-        "//vendor/github.com/stretchr/testify/assert:go_default_library",
-        "//vendor/github.com/stretchr/testify/require:go_default_library",
-    ],
+    deps = ["//vendor/github.com/spf13/pflag:go_default_library"],
 )
 
 go_library(
@@ -28,8 +26,11 @@ go_library(
         "colon_separated_multimap_string_string.go",
         "configuration_map.go",
         "flags.go",
+        "langle_separated_map_string_string.go",
         "map_string_bool.go",
+        "map_string_string.go",
         "namedcertkey_flag.go",
+        "omitempty.go",
         "string_flag.go",
         "tristate.go",
     ],

pkg/util/flag/langle_separated_map_string_string.go

@@ -0,0 +1,82 @@
+/*
+Copyright 2017 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 flag
+
+import (
+	"fmt"
+	"sort"
+	"strings"
+)
+
+// LangleSeparatedMapStringString can be set from the command line with the format `--flag "string<string"`.
+// Multiple comma-separated key-value pairs in a single invocation are supported. For example: `--flag "a<foo,b<bar"`.
+// Multiple flag invocations are supported. For example: `--flag "a<foo" --flag "b<foo"`.
+type LangleSeparatedMapStringString struct {
+	Map         *map[string]string
+	initialized bool // set to true after first Set call
+}
+
+// NewLangleSeparatedMapStringString takes a pointer to a map[string]string and returns the
+// LangleSeparatedMapStringString flag parsing shim for that map
+func NewLangleSeparatedMapStringString(m *map[string]string) *LangleSeparatedMapStringString {
+	return &LangleSeparatedMapStringString{Map: m}
+}
+
+// String implements github.com/spf13/pflag.Value
+func (m *LangleSeparatedMapStringString) String() string {
+	pairs := []string{}
+	for k, v := range *m.Map {
+		pairs = append(pairs, fmt.Sprintf("%s<%s", k, v))
+	}
+	sort.Strings(pairs)
+	return strings.Join(pairs, ",")
+}
+
+// Set implements github.com/spf13/pflag.Value
+func (m *LangleSeparatedMapStringString) Set(value string) error {
+	if m.Map == nil {
+		return fmt.Errorf("no target (nil pointer to map[string]string)")
+	}
+	if !m.initialized || *m.Map == nil {
+		// clear default values, or allocate if no existing map
+		*m.Map = make(map[string]string)
+		m.initialized = true
+	}
+	for _, s := range strings.Split(value, ",") {
+		if len(s) == 0 {
+			continue
+		}
+		arr := strings.SplitN(s, "<", 2)
+		if len(arr) != 2 {
+			return fmt.Errorf("malformed pair, expect string<string")
+		}
+		k := strings.TrimSpace(arr[0])
+		v := strings.TrimSpace(arr[1])
+		(*m.Map)[k] = v
+	}
+	return nil
+}
+
+// Type implements github.com/spf13/pflag.Value
+func (*LangleSeparatedMapStringString) Type() string {
+	return "mapStringString"
+}
+
+// Empty implements OmitEmpty
+func (m *LangleSeparatedMapStringString) Empty() bool {
+	return len(*m.Map) == 0
+}

pkg/util/flag/langle_separated_map_string_string_test.go

@@ -0,0 +1,159 @@
+/*
+Copyright 2017 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 flag
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestStringLangleSeparatedMapStringString(t *testing.T) {
+	var nilMap map[string]string
+	cases := []struct {
+		desc   string
+		m      *LangleSeparatedMapStringString
+		expect string
+	}{
+		{"nil", NewLangleSeparatedMapStringString(&nilMap), ""},
+		{"empty", NewLangleSeparatedMapStringString(&map[string]string{}), ""},
+		{"one key", NewLangleSeparatedMapStringString(&map[string]string{"one": "foo"}), "one<foo"},
+		{"two keys", NewLangleSeparatedMapStringString(&map[string]string{"one": "foo", "two": "bar"}), "one<foo,two<bar"},
+	}
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			str := c.m.String()
+			if c.expect != str {
+				t.Fatalf("expect %q but got %q", c.expect, str)
+			}
+		})
+	}
+}
+
+func TestSetLangleSeparatedMapStringString(t *testing.T) {
+	var nilMap map[string]string
+	cases := []struct {
+		desc   string
+		vals   []string
+		start  *LangleSeparatedMapStringString
+		expect *LangleSeparatedMapStringString
+		err    string
+	}{
+		// we initialize the map with a default key that should be cleared by Set
+		{"clears defaults", []string{""},
+			NewLangleSeparatedMapStringString(&map[string]string{"default": ""}),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{},
+			}, ""},
+		// make sure we still allocate for "initialized" maps where Map was initially set to a nil map
+		{"allocates map if currently nil", []string{""},
+			&LangleSeparatedMapStringString{initialized: true, Map: &nilMap},
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{},
+			}, ""},
+		// for most cases, we just reuse nilMap, which should be allocated by Set, and is reset before each test case
+		{"empty", []string{""},
+			NewLangleSeparatedMapStringString(&nilMap),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{},
+			}, ""},
+		{"one key", []string{"one<foo"},
+			NewLangleSeparatedMapStringString(&nilMap),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo"},
+			}, ""},
+		{"two keys", []string{"one<foo,two<bar"},
+			NewLangleSeparatedMapStringString(&nilMap),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo", "two": "bar"},
+			}, ""},
+		{"two keys, multiple Set invocations", []string{"one<foo", "two<bar"},
+			NewLangleSeparatedMapStringString(&nilMap),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo", "two": "bar"},
+			}, ""},
+		{"two keys with space", []string{"one<foo, two<bar"},
+			NewLangleSeparatedMapStringString(&nilMap),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo", "two": "bar"},
+			}, ""},
+		{"empty key", []string{"<foo"},
+			NewLangleSeparatedMapStringString(&nilMap),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &map[string]string{"": "foo"},
+			}, ""},
+		{"missing value", []string{"one"},
+			NewLangleSeparatedMapStringString(&nilMap),
+			nil,
+			"malformed pair, expect string<string"},
+		{"no target", []string{"a:foo"},
+			NewLangleSeparatedMapStringString(nil),
+			nil,
+			"no target (nil pointer to map[string]string)"},
+	}
+	for _, c := range cases {
+		nilMap = nil
+		t.Run(c.desc, func(t *testing.T) {
+			var err error
+			for _, val := range c.vals {
+				err = c.start.Set(val)
+				if err != nil {
+					break
+				}
+			}
+			if c.err != "" {
+				if err == nil || err.Error() != c.err {
+					t.Fatalf("expect error %s but got %v", c.err, err)
+				}
+				return
+			} else if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if !reflect.DeepEqual(c.expect, c.start) {
+				t.Fatalf("expect %#v but got %#v", c.expect, c.start)
+			}
+		})
+	}
+}
+
+func TestEmptyLangleSeparatedMapStringString(t *testing.T) {
+	var nilMap map[string]string
+	cases := []struct {
+		desc   string
+		val    *LangleSeparatedMapStringString
+		expect bool
+	}{
+		{"nil", NewLangleSeparatedMapStringString(&nilMap), true},
+		{"empty", NewLangleSeparatedMapStringString(&map[string]string{}), true},
+		{"populated", NewLangleSeparatedMapStringString(&map[string]string{"foo": ""}), false},
+	}
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			result := c.val.Empty()
+			if result != c.expect {
+				t.Fatalf("expect %t but got %t", c.expect, result)
+			}
+		})
+	}
+}

pkg/util/flag/map_string_bool.go

@@ -23,12 +23,24 @@ import (
 	"strings"
 )
 
-type MapStringBool map[string]bool
+// MapStringBool can be set from the command line with the format `--flag "string=bool"`.
+// Multiple comma-separated key-value pairs in a single invocation are supported. For example: `--flag "a=true,b=false"`.
+// Multiple flag invocations are supported. For example: `--flag "a=true" --flag "b=false"`.
+type MapStringBool struct {
+	Map         *map[string]bool
+	initialized bool
+}
+
+// NewMapStringBool takes a pointer to a map[string]string and returns the
+// MapStringBool flag parsing shim for that map
+func NewMapStringBool(m *map[string]bool) *MapStringBool {
+	return &MapStringBool{Map: m}
+}
 
 // String implements github.com/spf13/pflag.Value
-func (m MapStringBool) String() string {
+func (m *MapStringBool) String() string {
 	pairs := []string{}
-	for k, v := range m {
+	for k, v := range *m.Map {
 		pairs = append(pairs, fmt.Sprintf("%s=%t", k, v))
 	}
 	sort.Strings(pairs)
@@ -36,7 +48,15 @@ func (m MapStringBool) String() string {
 }
 
 // Set implements github.com/spf13/pflag.Value
-func (m MapStringBool) Set(value string) error {
+func (m *MapStringBool) Set(value string) error {
+	if m.Map == nil {
+		return fmt.Errorf("no target (nil pointer to map[string]bool)")
+	}
+	if !m.initialized || *m.Map == nil {
+		// clear default values, or allocate if no existing map
+		*m.Map = make(map[string]bool)
+		m.initialized = true
+	}
 	for _, s := range strings.Split(value, ",") {
 		if len(s) == 0 {
 			continue
@@ -51,12 +71,17 @@ func (m MapStringBool) Set(value string) error {
 		if err != nil {
 			return fmt.Errorf("invalid value of %s: %s, err: %v", k, v, err)
 		}
-		m[k] = boolValue
+		(*m.Map)[k] = boolValue
 	}
 	return nil
 }
 
 // Type implements github.com/spf13/pflag.Value
-func (MapStringBool) Type() string {
+func (*MapStringBool) Type() string {
 	return "mapStringBool"
 }
+
+// Empty implements OmitEmpty
+func (m *MapStringBool) Empty() bool {
+	return len(*m.Map) == 0
+}

pkg/util/flag/map_string_bool_test.go

@@ -17,55 +17,147 @@ limitations under the License.
 package flag
 
 import (
+	"reflect"
 	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestStringMapStringBool(t *testing.T) {
+	var nilMap map[string]bool
 	cases := []struct {
 		desc   string
-		m      MapStringBool
+		m      *MapStringBool
 		expect string
 	}{
-		{"empty", MapStringBool{}, ""},
-		{"one key", MapStringBool{"one": true}, "one=true"},
-		{"two keys", MapStringBool{"one": true, "two": false}, "one=true,two=false"},
+		{"nil", NewMapStringBool(&nilMap), ""},
+		{"empty", NewMapStringBool(&map[string]bool{}), ""},
+		{"one key", NewMapStringBool(&map[string]bool{"one": true}), "one=true"},
+		{"two keys", NewMapStringBool(&map[string]bool{"one": true, "two": false}), "one=true,two=false"},
 	}
 	for _, c := range cases {
 		t.Run(c.desc, func(t *testing.T) {
 			str := c.m.String()
-			assert.Equal(t, c.expect, str)
+			if c.expect != str {
+				t.Fatalf("expect %q but got %q", c.expect, str)
+			}
 		})
 	}
 }
 
 func TestSetMapStringBool(t *testing.T) {
+	var nilMap map[string]bool
 	cases := []struct {
 		desc   string
-		val    string
-		expect MapStringBool
+		vals   []string
+		start  *MapStringBool
+		expect *MapStringBool
 		err    string
 	}{
-		{"empty", "", MapStringBool{}, ""},
-		{"one key", "one=true", MapStringBool{"one": true}, ""},
-		{"two keys", "one=true,two=false", MapStringBool{"one": true, "two": false}, ""},
-		{"two keys with space", "one=true, two=false", MapStringBool{"one": true, "two": false}, ""},
-		{"empty key", "=true", MapStringBool{"": true}, ""},
-		{"missing value", "one", MapStringBool{}, "malformed pair, expect string=bool"},
-		{"non-boolean value", "one=foo", MapStringBool{}, `invalid value of one: foo, err: strconv.ParseBool: parsing "foo": invalid syntax`},
+		// we initialize the map with a default key that should be cleared by Set
+		{"clears defaults", []string{""},
+			NewMapStringBool(&map[string]bool{"default": true}),
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{},
+			}, ""},
+		// make sure we still allocate for "initialized" maps where Map was initially set to a nil map
+		{"allocates map if currently nil", []string{""},
+			&MapStringBool{initialized: true, Map: &nilMap},
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{},
+			}, ""},
+		// for most cases, we just reuse nilMap, which should be allocated by Set, and is reset before each test case
+		{"empty", []string{""},
+			NewMapStringBool(&nilMap),
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{},
+			}, ""},
+		{"one key", []string{"one=true"},
+			NewMapStringBool(&nilMap),
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{"one": true},
+			}, ""},
+		{"two keys", []string{"one=true,two=false"},
+			NewMapStringBool(&nilMap),
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{"one": true, "two": false},
+			}, ""},
+		{"two keys, multiple Set invocations", []string{"one=true", "two=false"},
+			NewMapStringBool(&nilMap),
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{"one": true, "two": false},
+			}, ""},
+		{"two keys with space", []string{"one=true, two=false"},
+			NewMapStringBool(&nilMap),
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{"one": true, "two": false},
+			}, ""},
+		{"empty key", []string{"=true"},
+			NewMapStringBool(&nilMap),
+			&MapStringBool{
+				initialized: true,
+				Map:         &map[string]bool{"": true},
+			}, ""},
+		{"missing value", []string{"one"},
+			NewMapStringBool(&nilMap),
+			nil,
+			"malformed pair, expect string=bool"},
+		{"non-boolean value", []string{"one=foo"},
+			NewMapStringBool(&nilMap),
+			nil,
+			`invalid value of one: foo, err: strconv.ParseBool: parsing "foo": invalid syntax`},
+		{"no target", []string{"one=true"},
+			NewMapStringBool(nil),
+			nil,
+			"no target (nil pointer to map[string]bool)"},
 	}
 	for _, c := range cases {
+		nilMap = nil
 		t.Run(c.desc, func(t *testing.T) {
-			m := MapStringBool{}
-			err := m.Set(c.val)
-			if c.err != "" {
-				require.EqualError(t, err, c.err)
-			} else {
-				require.NoError(t, err)
+			var err error
+			for _, val := range c.vals {
+				err = c.start.Set(val)
+				if err != nil {
+					break
+				}
+			}
+			if c.err != "" {
+				if err == nil || err.Error() != c.err {
+					t.Fatalf("expect error %s but got %v", c.err, err)
+				}
+				return
+			} else if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if !reflect.DeepEqual(c.expect, c.start) {
+				t.Fatalf("expect %#v but got %#v", c.expect, c.start)
+			}
+		})
+	}
+}
+
+func TestEmptyMapStringBool(t *testing.T) {
+	var nilMap map[string]bool
+	cases := []struct {
+		desc   string
+		val    *MapStringBool
+		expect bool
+	}{
+		{"nil", NewMapStringBool(&nilMap), true},
+		{"empty", NewMapStringBool(&map[string]bool{}), true},
+		{"populated", NewMapStringBool(&map[string]bool{"foo": true}), false},
+	}
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			result := c.val.Empty()
+			if result != c.expect {
+				t.Fatalf("expect %t but got %t", c.expect, result)
 			}
-			assert.Equal(t, c.expect, m)
 		})
 	}
 }

pkg/util/flag/map_string_string.go

@@ -0,0 +1,82 @@
+/*
+Copyright 2017 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 flag
+
+import (
+	"fmt"
+	"sort"
+	"strings"
+)
+
+// MapStringString can be set from the command line with the format `--flag "string=string"`.
+// Multiple comma-separated key-value pairs in a single invocation are supported. For example: `--flag "a=foo,b=bar"`.
+// Multiple flag invocations are supported. For example: `--flag "a=foo" --flag "b=bar"`.
+type MapStringString struct {
+	Map         *map[string]string
+	initialized bool
+}
+
+// NewMapStringString takes a pointer to a map[string]string and returns the
+// MapStringString flag parsing shim for that map
+func NewMapStringString(m *map[string]string) *MapStringString {
+	return &MapStringString{Map: m}
+}
+
+// String implements github.com/spf13/pflag.Value
+func (m *MapStringString) String() string {
+	pairs := []string{}
+	for k, v := range *m.Map {
+		pairs = append(pairs, fmt.Sprintf("%s=%s", k, v))
+	}
+	sort.Strings(pairs)
+	return strings.Join(pairs, ",")
+}
+
+// Set implements github.com/spf13/pflag.Value
+func (m *MapStringString) Set(value string) error {
+	if m.Map == nil {
+		return fmt.Errorf("no target (nil pointer to map[string]string)")
+	}
+	if !m.initialized || *m.Map == nil {
+		// clear default values, or allocate if no existing map
+		*m.Map = make(map[string]string)
+		m.initialized = true
+	}
+	for _, s := range strings.Split(value, ",") {
+		if len(s) == 0 {
+			continue
+		}
+		arr := strings.SplitN(s, "=", 2)
+		if len(arr) != 2 {
+			return fmt.Errorf("malformed pair, expect string=string")
+		}
+		k := strings.TrimSpace(arr[0])
+		v := strings.TrimSpace(arr[1])
+		(*m.Map)[k] = v
+	}
+	return nil
+}
+
+// Type implements github.com/spf13/pflag.Value
+func (*MapStringString) Type() string {
+	return "mapStringString"
+}
+
+// Empty implements OmitEmpty
+func (m *MapStringString) Empty() bool {
+	return len(*m.Map) == 0
+}

pkg/util/flag/map_string_string_test.go

@@ -0,0 +1,159 @@
+/*
+Copyright 2017 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 flag
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestStringMapStringString(t *testing.T) {
+	var nilMap map[string]string
+	cases := []struct {
+		desc   string
+		m      *MapStringString
+		expect string
+	}{
+		{"nil", NewMapStringString(&nilMap), ""},
+		{"empty", NewMapStringString(&map[string]string{}), ""},
+		{"one key", NewMapStringString(&map[string]string{"one": "foo"}), "one=foo"},
+		{"two keys", NewMapStringString(&map[string]string{"one": "foo", "two": "bar"}), "one=foo,two=bar"},
+	}
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			str := c.m.String()
+			if c.expect != str {
+				t.Fatalf("expect %q but got %q", c.expect, str)
+			}
+		})
+	}
+}
+
+func TestSetMapStringString(t *testing.T) {
+	var nilMap map[string]string
+	cases := []struct {
+		desc   string
+		vals   []string
+		start  *MapStringString
+		expect *MapStringString
+		err    string
+	}{
+		// we initialize the map with a default key that should be cleared by Set
+		{"clears defaults", []string{""},
+			NewMapStringString(&map[string]string{"default": ""}),
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{},
+			}, ""},
+		// make sure we still allocate for "initialized" maps where Map was initially set to a nil map
+		{"allocates map if currently nil", []string{""},
+			&MapStringString{initialized: true, Map: &nilMap},
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{},
+			}, ""},
+		// for most cases, we just reuse nilMap, which should be allocated by Set, and is reset before each test case
+		{"empty", []string{""},
+			NewMapStringString(&nilMap),
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{},
+			}, ""},
+		{"one key", []string{"one=foo"},
+			NewMapStringString(&nilMap),
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo"},
+			}, ""},
+		{"two keys", []string{"one=foo,two=bar"},
+			NewMapStringString(&nilMap),
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo", "two": "bar"},
+			}, ""},
+		{"two keys, multiple Set invocations", []string{"one=foo", "two=bar"},
+			NewMapStringString(&nilMap),
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo", "two": "bar"},
+			}, ""},
+		{"two keys with space", []string{"one=foo, two=bar"},
+			NewMapStringString(&nilMap),
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{"one": "foo", "two": "bar"},
+			}, ""},
+		{"empty key", []string{"=foo"},
+			NewMapStringString(&nilMap),
+			&MapStringString{
+				initialized: true,
+				Map:         &map[string]string{"": "foo"},
+			}, ""},
+		{"missing value", []string{"one"},
+			NewMapStringString(&nilMap),
+			nil,
+			"malformed pair, expect string=string"},
+		{"no target", []string{"a:foo"},
+			NewMapStringString(nil),
+			nil,
+			"no target (nil pointer to map[string]string)"},
+	}
+	for _, c := range cases {
+		nilMap = nil
+		t.Run(c.desc, func(t *testing.T) {
+			var err error
+			for _, val := range c.vals {
+				err = c.start.Set(val)
+				if err != nil {
+					break
+				}
+			}
+			if c.err != "" {
+				if err == nil || err.Error() != c.err {
+					t.Fatalf("expect error %s but got %v", c.err, err)
+				}
+				return
+			} else if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if !reflect.DeepEqual(c.expect, c.start) {
+				t.Fatalf("expect %#v but got %#v", c.expect, c.start)
+			}
+		})
+	}
+}
+
+func TestEmptyMapStringString(t *testing.T) {
+	var nilMap map[string]string
+	cases := []struct {
+		desc   string
+		val    *MapStringString
+		expect bool
+	}{
+		{"nil", NewMapStringString(&nilMap), true},
+		{"empty", NewMapStringString(&map[string]string{}), true},
+		{"populated", NewMapStringString(&map[string]string{"foo": ""}), false},
+	}
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			result := c.val.Empty()
+			if result != c.expect {
+				t.Fatalf("expect %t but got %t", c.expect, result)
+			}
+		})
+	}
+}

pkg/util/flag/omitempty.go

@@ -0,0 +1,24 @@
+/*
+Copyright 2017 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 flag
+
+// OmitEmpty is an interface for flags to report whether their underlying value
+// is "empty." If a flag implements OmitEmpty and returns true for a call to Empty(),
+// it is assumed that flag may be omitted from the command line.
+type OmitEmpty interface {
+	Empty() bool
+}