kube-proxy: fix combination of --config and logging command line flags

When parsing a config file, all settings derived from command line flags are
discarded because only the config settings are used. That has been the
traditional behavior for non-logging flags.

But `--config ... -v=4` used to work until
71ef0dafa7 added logging to the configuration.
To restore the original behavior, kube-proxy now:
- parses flags
- reads the config file
- applies logging settings from the flags to the config loaded from file
- uses that merged config

cmd/kube-proxy/app/server.go

@@ -235,7 +235,7 @@ func NewOptions() *Options {
 }
 
 // Complete completes all the required options.
-func (o *Options) Complete() error {
+func (o *Options) Complete(fs *pflag.FlagSet) error {
 	if len(o.ConfigFile) == 0 && len(o.WriteConfigTo) == 0 {
 		o.config.HealthzBindAddress = addressFromDeprecatedFlags(o.config.HealthzBindAddress, o.healthzPort)
 		o.config.MetricsBindAddress = addressFromDeprecatedFlags(o.config.MetricsBindAddress, o.metricsPort)
@@ -247,6 +247,14 @@ func (o *Options) Complete() error {
 		if err != nil {
 			return err
 		}
+
+		// Before we overwrite the config which holds the parsed
+		// command line parameters, we need to copy all modified
+		// logging settings over to the loaded config (i.e.  logging
+		// command line flags have priority). Otherwise `--config
+		// ... -v=5` doesn't work (config resets verbosity even
+		// when it contains no logging settings).
+		copyLogsFromFlags(fs, &c.Logging)
 		o.config = c
 
 		if err := o.initWatcher(); err != nil {
@@ -263,6 +271,39 @@ func (o *Options) Complete() error {
 	return utilfeature.DefaultMutableFeatureGate.SetFromMap(o.config.FeatureGates)
 }
 
+// copyLogsFromFlags applies the logging flags from the given flag set to the given
+// configuration. Fields for which the corresponding flag was not used are left
+// unmodified. For fields that have multiple values (like vmodule), the values from
+// the flags get joined so that the command line flags have priority.
+//
+// TODO (pohly): move this to logsapi
+func copyLogsFromFlags(from *pflag.FlagSet, to *logsapi.LoggingConfiguration) error {
+	var cloneFS pflag.FlagSet
+	logsapi.AddFlags(to, &cloneFS)
+	vmodule := to.VModule
+	to.VModule = nil
+	var err error
+	cloneFS.VisitAll(func(f *pflag.Flag) {
+		if err != nil {
+			return
+		}
+		fsFlag := from.Lookup(f.Name)
+		if fsFlag == nil {
+			err = fmt.Errorf("logging flag %s not found in flag set", f.Name)
+			return
+		}
+		if !fsFlag.Changed {
+			return
+		}
+		if setErr := f.Value.Set(fsFlag.Value.String()); setErr != nil {
+			err = fmt.Errorf("copying flag %s value: %v", f.Name, setErr)
+			return
+		}
+	})
+	to.VModule = append(to.VModule, vmodule...)
+	return err
+}
+
 // Creates a new filesystem watcher and adds watches for the config file.
 func (o *Options) initWatcher() error {
 	fswatcher := filesystem.NewFsnotifyWatcher()
@@ -476,7 +517,7 @@ with the apiserver API to configure the proxy.`,
 				return fmt.Errorf("failed os init: %w", err)
 			}
 
-			if err := opts.Complete(); err != nil {
+			if err := opts.Complete(cmd.Flags()); err != nil {
 				return fmt.Errorf("failed complete: %w", err)
 			}
 

cmd/kube-proxy/app/server_others_test.go

@@ -30,6 +30,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/spf13/pflag"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -563,7 +564,7 @@ detectLocalMode: "BridgeInterface"`)
 
 		opt := NewOptions()
 		opt.ConfigFile = file.Name()
-		err = opt.Complete()
+		err = opt.Complete(new(pflag.FlagSet))
 		if err != nil {
 			t.Fatal(err)
 		}

cmd/kube-proxy/app/server_test.go

@@ -19,13 +19,18 @@ package app
 import (
 	"errors"
 	"fmt"
+	"io/ioutil"
+	"path"
 	"testing"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clientsetfake "k8s.io/client-go/kubernetes/fake"
 	componentbaseconfig "k8s.io/component-base/config"
@@ -364,6 +369,125 @@ func TestProcessHostnameOverrideFlag(t *testing.T) {
 	}
 }
 
+// TestOptionsComplete checks that command line flags are combined with a
+// config properly.
+func TestOptionsComplete(t *testing.T) {
+	header := `apiVersion: kubeproxy.config.k8s.io/v1alpha1
+kind: KubeProxyConfiguration
+`
+
+	// Determine default config (depends on platform defaults).
+	o := NewOptions()
+	require.NoError(t, o.Complete(new(pflag.FlagSet)))
+	expected := o.config
+
+	config := header + `logging:
+  format: json
+  flushFrequency: 1s
+  verbosity: 10
+  vmodule:
+  - filePattern: foo.go
+    verbosity: 6
+  - filePattern: bar.go
+    verbosity: 8
+`
+	expectedLoggingConfig := logsapi.LoggingConfiguration{
+		Format:         "json",
+		FlushFrequency: logsapi.TimeOrMetaDuration{Duration: metav1.Duration{Duration: time.Second}, SerializeAsString: true},
+		Verbosity:      10,
+		VModule: []logsapi.VModuleItem{
+			{
+				FilePattern: "foo.go",
+				Verbosity:   6,
+			},
+			{
+				FilePattern: "bar.go",
+				Verbosity:   8,
+			},
+		},
+		Options: logsapi.FormatOptions{
+			JSON: logsapi.JSONOptions{
+				InfoBufferSize: resource.QuantityValue{Quantity: resource.MustParse("0")},
+			},
+		},
+	}
+
+	for name, tc := range map[string]struct {
+		config   string
+		flags    []string
+		expected *kubeproxyconfig.KubeProxyConfiguration
+	}{
+		"empty": {
+			expected: expected,
+		},
+		"empty-config": {
+			config:   header,
+			expected: expected,
+		},
+		"logging-config": {
+			config: config,
+			expected: func() *kubeproxyconfig.KubeProxyConfiguration {
+				c := expected.DeepCopy()
+				c.Logging = *expectedLoggingConfig.DeepCopy()
+				return c
+			}(),
+		},
+		"flags": {
+			flags: []string{
+				"-v=7",
+				"--vmodule", "goo.go=8",
+			},
+			expected: func() *kubeproxyconfig.KubeProxyConfiguration {
+				c := expected.DeepCopy()
+				c.Logging.Verbosity = 7
+				c.Logging.VModule = append(c.Logging.VModule, logsapi.VModuleItem{
+					FilePattern: "goo.go",
+					Verbosity:   8,
+				})
+				return c
+			}(),
+		},
+		"both": {
+			config: config,
+			flags: []string{
+				"-v=7",
+				"--vmodule", "goo.go=8",
+				"--ipvs-scheduler", "some-scheduler", // Overwritten by config.
+			},
+			expected: func() *kubeproxyconfig.KubeProxyConfiguration {
+				c := expected.DeepCopy()
+				c.Logging = *expectedLoggingConfig.DeepCopy()
+				// Flag wins.
+				c.Logging.Verbosity = 7
+				// Flag and config get merged with command line flags first.
+				c.Logging.VModule = append([]logsapi.VModuleItem{
+					{
+						FilePattern: "goo.go",
+						Verbosity:   8,
+					},
+				}, c.Logging.VModule...)
+				return c
+			}(),
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			options := NewOptions()
+			fs := new(pflag.FlagSet)
+			options.AddFlags(fs)
+			flags := tc.flags
+			if len(tc.config) > 0 {
+				tmp := t.TempDir()
+				configFile := path.Join(tmp, "kube-proxy.conf")
+				require.NoError(t, ioutil.WriteFile(configFile, []byte(tc.config), 0666))
+				flags = append(flags, "--config", configFile)
+			}
+			require.NoError(t, fs.Parse(flags))
+			require.NoError(t, options.Complete(fs))
+			assert.Equal(t, tc.expected, options.config)
+		})
+	}
+}
+
 type fakeProxyServerLongRun struct{}
 
 // Run runs the specified ProxyServer.