Merge pull request #52597 from smarterclayton/roundtrip_options

Automatic merge from submit-queue (batch tested with PRs 52485, 52443, 52597, 52450, 51971). 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>..

Some kubelet flags do not accept their default values

Correct the flags and add a round trip test that ensure these do not
break again in the future.

@deads2k as observed when we tried to turn flags into args.

cmd/kubelet/app/options/BUILD

@@ -3,6 +3,7 @@ package(default_visibility = ["//visibility:public"])
 load(
     "@io_bazel_rules_go//go:def.bzl",
     "go_library",
+    "go_test",
 )
 
 go_library(
@@ -38,3 +39,14 @@ filegroup(
     srcs = [":package-srcs"],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["options_test.go"],
+    library = ":go_default_library",
+    deps = [
+        "//vendor/github.com/spf13/pflag:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/diff:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/util/flag:go_default_library",
+    ],
+)

cmd/kubelet/app/options/options.go

@@ -22,6 +22,8 @@ import (
 	_ "net/http/pprof"
 	"strings"
 
+	"github.com/spf13/pflag"
+
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/apiserver/pkg/util/flag"
 	"k8s.io/kubernetes/pkg/apis/componentconfig"
@@ -31,8 +33,6 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig/v1alpha1"
 	kubeletconfigvalidation "k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig/validation"
 	utiltaints "k8s.io/kubernetes/pkg/util/taints"
-
-	"github.com/spf13/pflag"
 )
 
 const (

cmd/kubelet/app/options/options_test.go

@@ -0,0 +1,143 @@
+/*
+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 options
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	"github.com/spf13/pflag"
+
+	"k8s.io/apimachinery/pkg/util/diff"
+	utilflag "k8s.io/apiserver/pkg/util/flag"
+)
+
+func newKubeletServerOrDie() *KubeletServer {
+	s, err := NewKubeletServer()
+	if err != nil {
+		panic(err)
+	}
+	return s
+}
+
+func cleanFlags(s *KubeletServer) {
+	s.KubeConfig = utilflag.NewStringFlag(s.KubeConfig.Value())
+	s.DynamicConfigDir = utilflag.NewStringFlag(s.DynamicConfigDir.Value())
+	s.InitConfigDir = utilflag.NewStringFlag(s.InitConfigDir.Value())
+}
+
+// TestRoundTrip ensures that flag values from the Kubelet can be serialized
+// to arguments and then read back and have the same value. Also catches cases
+// where the default value reported by the flag is not actually allowed to be
+// specified.
+func TestRoundTrip(t *testing.T) {
+	testCases := []struct {
+		name          string
+		inputFlags    func() *KubeletServer
+		outputFlags   func() *KubeletServer
+		flagDefaulter func(*pflag.FlagSet)
+		skipDefault   bool
+		err           bool
+		expectArgs    bool
+	}{
+		{
+			name:          "default flags are eliminated",
+			inputFlags:    newKubeletServerOrDie,
+			outputFlags:   newKubeletServerOrDie,
+			flagDefaulter: newKubeletServerOrDie().AddFlags,
+			err:           false,
+			expectArgs:    false,
+		},
+		{
+			name:          "default flag values round trip",
+			inputFlags:    newKubeletServerOrDie,
+			outputFlags:   newKubeletServerOrDie,
+			flagDefaulter: func(*pflag.FlagSet) {},
+			err:           false,
+			expectArgs:    true,
+		},
+		{
+			name: "nil address does not fail for optional argument",
+			inputFlags: func() *KubeletServer {
+				s := newKubeletServerOrDie()
+				s.HealthzBindAddress = ""
+				return s
+			},
+			outputFlags: func() *KubeletServer {
+				s := newKubeletServerOrDie()
+				s.HealthzBindAddress = ""
+				return s
+			},
+			flagDefaulter: func(*pflag.FlagSet) {},
+			err:           false,
+			expectArgs:    true,
+		},
+	}
+	for _, testCase := range testCases {
+		modifiedFlags := testCase.inputFlags()
+		args := asArgs(modifiedFlags.AddFlags, testCase.flagDefaulter)
+		if testCase.expectArgs != (len(args) > 0) {
+			t.Errorf("%s: unexpected args: %v", testCase.name, args)
+			continue
+		}
+		t.Logf("%s: args: %v", testCase.name, args)
+		flagSet := pflag.NewFlagSet("output", pflag.ContinueOnError)
+		outputFlags := testCase.outputFlags()
+		outputFlags.AddFlags(flagSet)
+		if err := flagSet.Parse(args); err != nil {
+			if !testCase.err {
+				t.Errorf("%s: unexpected flag error: %v", testCase.name, err)
+			}
+			continue
+		}
+		cleanFlags(outputFlags)
+		if !reflect.DeepEqual(modifiedFlags, outputFlags) {
+			t.Errorf("%s: flags did not round trip: %s", testCase.name, diff.ObjectReflectDiff(modifiedFlags, outputFlags))
+			continue
+		}
+	}
+}
+
+func asArgs(fn, defaultFn func(*pflag.FlagSet)) []string {
+	fs := pflag.NewFlagSet("extended", pflag.ContinueOnError)
+	fn(fs)
+	defaults := pflag.NewFlagSet("defaults", pflag.ContinueOnError)
+	defaultFn(defaults)
+	var args []string
+	fs.VisitAll(func(flag *pflag.Flag) {
+		s := flag.Value.String()
+		var defaultValue string
+		if defaultFlag := defaults.Lookup(flag.Name); defaultFlag != nil {
+			defaultValue = defaultFlag.Value.String()
+			if s == defaultValue {
+				return
+			}
+		}
+		if values, err := fs.GetStringSlice(flag.Name); err == nil {
+			for _, s := range values {
+				args = append(args, fmt.Sprintf("--%s=%s", flag.Name, s))
+			}
+		} else {
+			if len(s) == 0 {
+				s = defaultValue
+			}
+			args = append(args, fmt.Sprintf("--%s=%s", flag.Name, s))
+		}
+	})
+	return args
+}

pkg/apis/componentconfig/helpers.go

@@ -35,6 +35,10 @@ type IPVar struct {
 }
 
 func (v IPVar) Set(s string) error {
+	if len(s) == 0 {
+		v.Val = nil
+		return nil
+	}
 	if net.ParseIP(s) == nil {
 		return fmt.Errorf("%q is not a valid IP address", s)
 	}

pkg/util/taints/taints.go

@@ -76,6 +76,10 @@ type taintsVar struct {
 }
 
 func (t taintsVar) Set(s string) error {
+	if len(s) == 0 {
+		*t.ptr = nil
+		return nil
+	}
 	sts := strings.Split(s, ",")
 	var taints []api.Taint
 	for _, st := range sts {
@@ -91,7 +95,7 @@ func (t taintsVar) Set(s string) error {
 
 func (t taintsVar) String() string {
 	if len(*t.ptr) == 0 {
-		return "<nil>"
+		return ""
 	}
 	var taints []string
 	for _, taint := range *t.ptr {