From 47c573fba64379c242b2031305091f124d10bf85 Mon Sep 17 00:00:00 2001
From: Michael Taufen <mtaufen@google.com>
Date: Thu, 19 Oct 2017 15:42:07 -0700
Subject: [PATCH 1/2] 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).

Kubernetes-commit: 1085b6f7304d46697ab9ed2131774ae9d4550ce2
---
 pkg/util/flag/BUILD                           |  10 +-
 .../langle_separated_map_string_string.go     |  87 +++++++++++
 ...langle_separated_map_string_string_test.go | 144 ++++++++++++++++++
 pkg/util/flag/map_string_bool.go              |  42 ++++-
 pkg/util/flag/map_string_bool_test.go         | 125 ++++++++++++---
 pkg/util/flag/map_string_string.go            |  87 +++++++++++
 pkg/util/flag/map_string_string_test.go       | 144 ++++++++++++++++++
 7 files changed, 604 insertions(+), 35 deletions(-)
 create mode 100644 pkg/util/flag/langle_separated_map_string_string.go
 create mode 100644 pkg/util/flag/langle_separated_map_string_string_test.go
 create mode 100644 pkg/util/flag/map_string_string.go
 create mode 100644 pkg/util/flag/map_string_string_test.go

diff --git a/pkg/util/flag/BUILD b/pkg/util/flag/BUILD
index 9bf629d1a..9cf2715c6 100644
--- a/pkg/util/flag/BUILD
+++ b/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,7 +26,9 @@ 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",
         "string_flag.go",
         "tristate.go",
diff --git a/pkg/util/flag/langle_separated_map_string_string.go b/pkg/util/flag/langle_separated_map_string_string.go
new file mode 100644
index 000000000..c2b950e77
--- /dev/null
+++ b/pkg/util/flag/langle_separated_map_string_string.go
@@ -0,0 +1,87 @@
+/*
+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 {
+	if *m.Map == nil {
+		return "nil"
+	}
+
+	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)")
+	}
+	// allow explicit nil map
+	if value == "nil" {
+		*m.Map = nil
+		m.initialized = true
+		return nil
+	}
+	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"
+}
diff --git a/pkg/util/flag/langle_separated_map_string_string_test.go b/pkg/util/flag/langle_separated_map_string_string_test.go
new file mode 100644
index 000000000..fd2ca25a5
--- /dev/null
+++ b/pkg/util/flag/langle_separated_map_string_string_test.go
@@ -0,0 +1,144 @@
+/*
+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), "nil"},
+		{"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{},
+			}, ""},
+		{"explicitly nil", []string{"nil"},
+			NewLangleSeparatedMapStringString(&map[string]string{"default": ""}),
+			&LangleSeparatedMapStringString{
+				initialized: true,
+				Map:         &nilMap,
+			}, ""},
+		// 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)
+			}
+		})
+	}
+}
diff --git a/pkg/util/flag/map_string_bool.go b/pkg/util/flag/map_string_bool.go
index 662feb162..73410d9c9 100644
--- a/pkg/util/flag/map_string_bool.go
+++ b/pkg/util/flag/map_string_bool.go
@@ -23,12 +23,28 @@ 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 {
+	if *m.Map == nil {
+		return "nil"
+	}
+
 	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 +52,21 @@ 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)")
+	}
+	// allow explicit nil map
+	if value == "nil" {
+		*m.Map = nil
+		m.initialized = true
+		return nil
+	}
+	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 +81,12 @@ 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"
 }
diff --git a/pkg/util/flag/map_string_bool_test.go b/pkg/util/flag/map_string_bool_test.go
index 38a671c12..80e0ce597 100644
--- a/pkg/util/flag/map_string_bool_test.go
+++ b/pkg/util/flag/map_string_bool_test.go
@@ -17,55 +17,132 @@ 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), "nil"},
+		{"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{},
+			}, ""},
+		{"explicitly nil", []string{"nil"},
+			NewMapStringBool(&map[string]bool{"default": true}),
+			&MapStringBool{
+				initialized: true,
+				Map:         &nilMap,
+			}, ""},
+		// 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)
 			}
-			assert.Equal(t, c.expect, m)
 		})
 	}
 }
diff --git a/pkg/util/flag/map_string_string.go b/pkg/util/flag/map_string_string.go
new file mode 100644
index 000000000..85d36f8a9
--- /dev/null
+++ b/pkg/util/flag/map_string_string.go
@@ -0,0 +1,87 @@
+/*
+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 {
+	if *m.Map == nil {
+		return "nil"
+	}
+
+	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)")
+	}
+	// allow explicit nil map
+	if value == "nil" {
+		*m.Map = nil
+		m.initialized = true
+		return nil
+	}
+	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"
+}
diff --git a/pkg/util/flag/map_string_string_test.go b/pkg/util/flag/map_string_string_test.go
new file mode 100644
index 000000000..b71777a8a
--- /dev/null
+++ b/pkg/util/flag/map_string_string_test.go
@@ -0,0 +1,144 @@
+/*
+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), "nil"},
+		{"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{},
+			}, ""},
+		{"explicitly nil", []string{"nil"},
+			NewMapStringString(&map[string]string{"default": ""}),
+			&MapStringString{
+				initialized: true,
+				Map:         &nilMap,
+			}, ""},
+		// 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)
+			}
+		})
+	}
+}

From b96eecee1644224674119782e4c810deb24a2ba2 Mon Sep 17 00:00:00 2001
From: Michael Taufen <mtaufen@google.com>
Date: Thu, 16 Nov 2017 14:47:39 -0800
Subject: [PATCH 2/2] omitempty

Kubernetes-commit: 617b49858fad83a72ce072356bb2808a259943c5
---
 pkg/util/flag/BUILD                           |  1 +
 .../langle_separated_map_string_string.go     | 15 ++++------
 ...langle_separated_map_string_string_test.go | 29 ++++++++++++++-----
 pkg/util/flag/map_string_bool.go              | 15 ++++------
 pkg/util/flag/map_string_bool_test.go         | 29 ++++++++++++++-----
 pkg/util/flag/map_string_string.go            | 15 ++++------
 pkg/util/flag/map_string_string_test.go       | 29 ++++++++++++++-----
 pkg/util/flag/omitempty.go                    | 24 +++++++++++++++
 8 files changed, 106 insertions(+), 51 deletions(-)
 create mode 100644 pkg/util/flag/omitempty.go

diff --git a/pkg/util/flag/BUILD b/pkg/util/flag/BUILD
index 9cf2715c6..57ab4058d 100644
--- a/pkg/util/flag/BUILD
+++ b/pkg/util/flag/BUILD
@@ -30,6 +30,7 @@ go_library(
         "map_string_bool.go",
         "map_string_string.go",
         "namedcertkey_flag.go",
+        "omitempty.go",
         "string_flag.go",
         "tristate.go",
     ],
diff --git a/pkg/util/flag/langle_separated_map_string_string.go b/pkg/util/flag/langle_separated_map_string_string.go
index c2b950e77..bf8dbfb9b 100644
--- a/pkg/util/flag/langle_separated_map_string_string.go
+++ b/pkg/util/flag/langle_separated_map_string_string.go
@@ -38,10 +38,6 @@ func NewLangleSeparatedMapStringString(m *map[string]string) *LangleSeparatedMap
 
 // String implements github.com/spf13/pflag.Value
 func (m *LangleSeparatedMapStringString) String() string {
-	if *m.Map == nil {
-		return "nil"
-	}
-
 	pairs := []string{}
 	for k, v := range *m.Map {
 		pairs = append(pairs, fmt.Sprintf("%s<%s", k, v))
@@ -55,12 +51,6 @@ func (m *LangleSeparatedMapStringString) Set(value string) error {
 	if m.Map == nil {
 		return fmt.Errorf("no target (nil pointer to map[string]string)")
 	}
-	// allow explicit nil map
-	if value == "nil" {
-		*m.Map = nil
-		m.initialized = true
-		return nil
-	}
 	if !m.initialized || *m.Map == nil {
 		// clear default values, or allocate if no existing map
 		*m.Map = make(map[string]string)
@@ -85,3 +75,8 @@ func (m *LangleSeparatedMapStringString) Set(value string) error {
 func (*LangleSeparatedMapStringString) Type() string {
 	return "mapStringString"
 }
+
+// Empty implements OmitEmpty
+func (m *LangleSeparatedMapStringString) Empty() bool {
+	return len(*m.Map) == 0
+}
diff --git a/pkg/util/flag/langle_separated_map_string_string_test.go b/pkg/util/flag/langle_separated_map_string_string_test.go
index fd2ca25a5..80c67c32b 100644
--- a/pkg/util/flag/langle_separated_map_string_string_test.go
+++ b/pkg/util/flag/langle_separated_map_string_string_test.go
@@ -28,7 +28,7 @@ func TestStringLangleSeparatedMapStringString(t *testing.T) {
 		m      *LangleSeparatedMapStringString
 		expect string
 	}{
-		{"nil", NewLangleSeparatedMapStringString(&nilMap), "nil"},
+		{"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"},
@@ -59,12 +59,6 @@ func TestSetLangleSeparatedMapStringString(t *testing.T) {
 				initialized: true,
 				Map:         &map[string]string{},
 			}, ""},
-		{"explicitly nil", []string{"nil"},
-			NewLangleSeparatedMapStringString(&map[string]string{"default": ""}),
-			&LangleSeparatedMapStringString{
-				initialized: true,
-				Map:         &nilMap,
-			}, ""},
 		// 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},
@@ -142,3 +136,24 @@ func TestSetLangleSeparatedMapStringString(t *testing.T) {
 		})
 	}
 }
+
+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)
+			}
+		})
+	}
+}
diff --git a/pkg/util/flag/map_string_bool.go b/pkg/util/flag/map_string_bool.go
index 73410d9c9..46ed43836 100644
--- a/pkg/util/flag/map_string_bool.go
+++ b/pkg/util/flag/map_string_bool.go
@@ -39,10 +39,6 @@ func NewMapStringBool(m *map[string]bool) *MapStringBool {
 
 // String implements github.com/spf13/pflag.Value
 func (m *MapStringBool) String() string {
-	if *m.Map == nil {
-		return "nil"
-	}
-
 	pairs := []string{}
 	for k, v := range *m.Map {
 		pairs = append(pairs, fmt.Sprintf("%s=%t", k, v))
@@ -56,12 +52,6 @@ func (m *MapStringBool) Set(value string) error {
 	if m.Map == nil {
 		return fmt.Errorf("no target (nil pointer to map[string]bool)")
 	}
-	// allow explicit nil map
-	if value == "nil" {
-		*m.Map = nil
-		m.initialized = true
-		return nil
-	}
 	if !m.initialized || *m.Map == nil {
 		// clear default values, or allocate if no existing map
 		*m.Map = make(map[string]bool)
@@ -90,3 +80,8 @@ func (m *MapStringBool) Set(value string) error {
 func (*MapStringBool) Type() string {
 	return "mapStringBool"
 }
+
+// Empty implements OmitEmpty
+func (m *MapStringBool) Empty() bool {
+	return len(*m.Map) == 0
+}
diff --git a/pkg/util/flag/map_string_bool_test.go b/pkg/util/flag/map_string_bool_test.go
index 80e0ce597..f23b240a1 100644
--- a/pkg/util/flag/map_string_bool_test.go
+++ b/pkg/util/flag/map_string_bool_test.go
@@ -28,7 +28,7 @@ func TestStringMapStringBool(t *testing.T) {
 		m      *MapStringBool
 		expect string
 	}{
-		{"nil", NewMapStringBool(&nilMap), "nil"},
+		{"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"},
@@ -59,12 +59,6 @@ func TestSetMapStringBool(t *testing.T) {
 				initialized: true,
 				Map:         &map[string]bool{},
 			}, ""},
-		{"explicitly nil", []string{"nil"},
-			NewMapStringBool(&map[string]bool{"default": true}),
-			&MapStringBool{
-				initialized: true,
-				Map:         &nilMap,
-			}, ""},
 		// 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},
@@ -146,3 +140,24 @@ func TestSetMapStringBool(t *testing.T) {
 		})
 	}
 }
+
+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)
+			}
+		})
+	}
+}
diff --git a/pkg/util/flag/map_string_string.go b/pkg/util/flag/map_string_string.go
index 85d36f8a9..48468a8dd 100644
--- a/pkg/util/flag/map_string_string.go
+++ b/pkg/util/flag/map_string_string.go
@@ -38,10 +38,6 @@ func NewMapStringString(m *map[string]string) *MapStringString {
 
 // String implements github.com/spf13/pflag.Value
 func (m *MapStringString) String() string {
-	if *m.Map == nil {
-		return "nil"
-	}
-
 	pairs := []string{}
 	for k, v := range *m.Map {
 		pairs = append(pairs, fmt.Sprintf("%s=%s", k, v))
@@ -55,12 +51,6 @@ func (m *MapStringString) Set(value string) error {
 	if m.Map == nil {
 		return fmt.Errorf("no target (nil pointer to map[string]string)")
 	}
-	// allow explicit nil map
-	if value == "nil" {
-		*m.Map = nil
-		m.initialized = true
-		return nil
-	}
 	if !m.initialized || *m.Map == nil {
 		// clear default values, or allocate if no existing map
 		*m.Map = make(map[string]string)
@@ -85,3 +75,8 @@ func (m *MapStringString) Set(value string) error {
 func (*MapStringString) Type() string {
 	return "mapStringString"
 }
+
+// Empty implements OmitEmpty
+func (m *MapStringString) Empty() bool {
+	return len(*m.Map) == 0
+}
diff --git a/pkg/util/flag/map_string_string_test.go b/pkg/util/flag/map_string_string_test.go
index b71777a8a..8feb62524 100644
--- a/pkg/util/flag/map_string_string_test.go
+++ b/pkg/util/flag/map_string_string_test.go
@@ -28,7 +28,7 @@ func TestStringMapStringString(t *testing.T) {
 		m      *MapStringString
 		expect string
 	}{
-		{"nil", NewMapStringString(&nilMap), "nil"},
+		{"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"},
@@ -59,12 +59,6 @@ func TestSetMapStringString(t *testing.T) {
 				initialized: true,
 				Map:         &map[string]string{},
 			}, ""},
-		{"explicitly nil", []string{"nil"},
-			NewMapStringString(&map[string]string{"default": ""}),
-			&MapStringString{
-				initialized: true,
-				Map:         &nilMap,
-			}, ""},
 		// 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},
@@ -142,3 +136,24 @@ func TestSetMapStringString(t *testing.T) {
 		})
 	}
 }
+
+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)
+			}
+		})
+	}
+}
diff --git a/pkg/util/flag/omitempty.go b/pkg/util/flag/omitempty.go
new file mode 100644
index 000000000..c354754ea
--- /dev/null
+++ b/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
+}