From 6ddcdef29ae9bfb631ba4598711ae1d1d3e60c98 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 9 Aug 2023 18:17:30 +0200 Subject: [PATCH] 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 71ef0dafa72ca8cf2aa26fe1a75d3379a551771a 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 | 45 +++++++- cmd/kube-proxy/app/server_others_test.go | 3 +- cmd/kube-proxy/app/server_test.go | 124 +++++++++++++++++++++++ 3 files changed, 169 insertions(+), 3 deletions(-) diff --git a/cmd/kube-proxy/app/server.go b/cmd/kube-proxy/app/server.go index d019212db28..fee3d2f56db 100644 --- a/cmd/kube-proxy/app/server.go +++ b/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) } diff --git a/cmd/kube-proxy/app/server_others_test.go b/cmd/kube-proxy/app/server_others_test.go index 32311315fc8..cbb131e6df3 100644 --- a/cmd/kube-proxy/app/server_others_test.go +++ b/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) } diff --git a/cmd/kube-proxy/app/server_test.go b/cmd/kube-proxy/app/server_test.go index 9e63e6f1f02..74f05b182f3 100644 --- a/cmd/kube-proxy/app/server_test.go +++ b/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.