Merge pull request #53025 from mtaufen/feature-gate-map

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>.

Make feature gates loadable from a map[string]bool

Command line flag API remains the same. This allows ComponentConfig
structures (e.g. KubeletConfiguration) to express the map structure
behind feature gates in a natural way when written as JSON or YAML.

For example:

KubeletConfiguration Before:
```
apiVersion: kubeletconfig/v1alpha1
kind: KubeletConfiguration
featureGates: "DynamicKubeletConfig=true,Accelerators=true"
```

KubeletConfiguration After:
```
apiVersion: kubeletconfig/v1alpha1
kind: KubeletConfiguration
featureGates:
  DynamicKubeletConfig: true
  Accelerators: true
```

Fixes: #53024

```release-note
The Kubelet's feature gates are now specified as a map when provided via a JSON or YAML KubeletConfiguration, rather than as a string of key-value pairs.
```

/cc @mikedanese @jlowdermilk @smarterclayton

Kubernetes-commit: df072ca97ee0248968c043759f0016e19911389e

pkg/util/feature/feature_gate.go

@@ -77,6 +77,8 @@ type FeatureGate interface {
 	// Set parses and stores flag gates for known features
 	// from a string like feature1=true,feature2=false,...
 	Set(value string) error
+	// SetFromMap stores flag gates for known features from a map[string]bool or returns an error
+	SetFromMap(m map[string]bool) error
 	// Enabled returns true if the key is enabled.
 	Enabled(key Feature) bool
 	// Add adds features to the featureGate.
@@ -133,7 +135,7 @@ func NewFeatureGate() *featureGate {
 	return f
 }
 
-// Set Parses a string of the form "key1=value1,key2=value2,..." into a
+// Set parses a string of the form "key1=value1,key2=value2,..." into a
 // map[string]bool of known keys or returns an error.
 func (f *featureGate) Set(value string) error {
 	f.lock.Lock()
@@ -183,6 +185,42 @@ func (f *featureGate) Set(value string) error {
 	return nil
 }
 
+// SetFromMap stores flag gates for known features from a map[string]bool or returns an error
+func (f *featureGate) SetFromMap(m map[string]bool) error {
+	f.lock.Lock()
+	defer f.lock.Unlock()
+
+	// Copy existing state
+	known := map[Feature]FeatureSpec{}
+	for k, v := range f.known.Load().(map[Feature]FeatureSpec) {
+		known[k] = v
+	}
+	enabled := map[Feature]bool{}
+	for k, v := range f.enabled.Load().(map[Feature]bool) {
+		enabled[k] = v
+	}
+
+	for k, v := range m {
+		k := Feature(k)
+		_, ok := known[k]
+		if !ok {
+			return fmt.Errorf("unrecognized key: %s", k)
+		}
+		enabled[k] = v
+		// Handle "special" features like "all alpha gates"
+		if fn, found := f.special[k]; found {
+			fn(known, enabled, v)
+		}
+	}
+
+	// Persist changes
+	f.known.Store(known)
+	f.enabled.Store(enabled)
+
+	glog.Infof("feature gates: %v", f.enabled)
+	return nil
+}
+
 // String returns a string containing all enabled feature gates, formatted as "key1=value1,key2=value2,...".
 func (f *featureGate) String() string {
 	pairs := []string{}

pkg/util/flag/BUILD

@@ -8,9 +8,16 @@ load(
 
 go_test(
     name = "go_default_test",
-    srcs = ["namedcertkey_flag_test.go"],
+    srcs = [
+        "map_string_bool_test.go",
+        "namedcertkey_flag_test.go",
+    ],
     library = ":go_default_library",
-    deps = ["//vendor/github.com/spf13/pflag: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",
+    ],
 )
 
 go_library(
@@ -18,6 +25,7 @@ go_library(
     srcs = [
         "configuration_map.go",
         "flags.go",
+        "map_string_bool.go",
         "namedcertkey_flag.go",
         "string_flag.go",
         "tristate.go",

pkg/util/flag/map_string_bool.go

@@ -0,0 +1,62 @@
+/*
+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"
+	"strconv"
+	"strings"
+)
+
+type MapStringBool map[string]bool
+
+// String implements github.com/spf13/pflag.Value
+func (m MapStringBool) String() string {
+	pairs := []string{}
+	for k, v := range m {
+		pairs = append(pairs, fmt.Sprintf("%s=%t", k, v))
+	}
+	sort.Strings(pairs)
+	return strings.Join(pairs, ",")
+}
+
+// Set implements github.com/spf13/pflag.Value
+func (m MapStringBool) Set(value string) error {
+	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=bool")
+		}
+		k := strings.TrimSpace(arr[0])
+		v := strings.TrimSpace(arr[1])
+		boolValue, err := strconv.ParseBool(v)
+		if err != nil {
+			return fmt.Errorf("invalid value of %s: %s, err: %v", k, v, err)
+		}
+		m[k] = boolValue
+	}
+	return nil
+}
+
+// Type implements github.com/spf13/pflag.Value
+func (MapStringBool) Type() string {
+	return "mapStringBool"
+}

pkg/util/flag/map_string_bool_test.go

@@ -0,0 +1,71 @@
+/*
+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 (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestStringMapStringBool(t *testing.T) {
+	cases := []struct {
+		desc   string
+		m      MapStringBool
+		expect string
+	}{
+		{"empty", MapStringBool{}, ""},
+		{"one key", MapStringBool{"one": true}, "one=true"},
+		{"two keys", MapStringBool{"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)
+		})
+	}
+}
+
+func TestSetMapStringBool(t *testing.T) {
+	cases := []struct {
+		desc   string
+		val    string
+		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`},
+	}
+	for _, c := range cases {
+		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)
+			}
+			assert.Equal(t, c.expect, m)
+		})
+	}
+}