From 8bcf26b575b1c6dc09e4c1acb41a130cec180ec3 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 23 Dec 2022 17:10:42 +0100 Subject: [PATCH 1/3] k8s.io/component-base/logs: fix usage through Go flag package api/v1.AddFlags only supports a pflag.FlagSet. The assumption was that code which wants to use flag.FlagSet can use VisitAll to copy the flags. That works, with one caveat: the flag.FlagSet help implementation will call String for the zero value to determine whether the flag has a non-default value. This currently leads to additional warnings at the end of the -help output: panic calling String method on zero v1.verbosityLevelPflag for flag v: runtime error: invalid memory address or nil pointer dereference panic calling String method on zero v1.vmoduleConfigurationPFlag for flag vmodule: runtime error: invalid memory address or nil pointer dereference Supporting usage of methods with the zero value is good practice anyway and thus gets added. This then avoids these panics. --- staging/src/k8s.io/component-base/logs/api/v1/pflags.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/staging/src/k8s.io/component-base/logs/api/v1/pflags.go b/staging/src/k8s.io/component-base/logs/api/v1/pflags.go index 36a98cc81cf..b74e132a722 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/pflags.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/pflags.go @@ -36,6 +36,9 @@ type vmoduleConfigurationPFlag struct { // String returns the -vmodule parameter (comma-separated list of pattern=N). func (wrapper vmoduleConfigurationPFlag) String() string { + if wrapper.value == nil { + return "" + } var patterns []string for _, item := range *wrapper.value { patterns = append(patterns, fmt.Sprintf("%s=%d", item.FilePattern, item.Verbosity)) @@ -82,10 +85,16 @@ type verbosityLevelPflag struct { } func (wrapper verbosityLevelPflag) String() string { + if wrapper.value == nil { + return "0" + } return strconv.FormatInt(int64(*wrapper.value), 10) } func (wrapper verbosityLevelPflag) Get() interface{} { + if wrapper.value == nil { + return VerbosityLevel(0) + } return *wrapper.value } From 8251a632692ad6d996bf3fff49002c5ead23c318 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 17 Jan 2023 17:32:50 +0100 Subject: [PATCH 2/3] k8s.io/component-base/logs: unit test for command line help output Both pflag and standard FlagSet are covered. --- .../logs/api/v1/options_test.go | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/staging/src/k8s.io/component-base/logs/api/v1/options_test.go b/staging/src/k8s.io/component-base/logs/api/v1/options_test.go index fc38c64dfef..475f7a46eb4 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/options_test.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/options_test.go @@ -19,6 +19,7 @@ package v1 import ( "bytes" "context" + "flag" "testing" "github.com/go-logr/logr" @@ -100,6 +101,45 @@ func TestOptions(t *testing.T) { } } +func TestFlagSet(t *testing.T) { + t.Run("pflag", func(t *testing.T) { + newOptions := NewLoggingConfiguration() + var fs pflag.FlagSet + AddFlags(newOptions, &fs) + var buffer bytes.Buffer + fs.SetOutput(&buffer) + fs.PrintDefaults() + assert.Equal(t, ` --logging-format string Sets the log format. Permitted formats: "text". (default "text") + --log-flush-frequency duration Maximum number of seconds between log flushes (default 5s) + -v, --v Level number for the log level verbosity + --vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) +`, buffer.String()) + }) + + t.Run("flag", func(t *testing.T) { + newOptions := NewLoggingConfiguration() + var pfs pflag.FlagSet + AddFlags(newOptions, &pfs) + var fs flag.FlagSet + pfs.VisitAll(func(f *pflag.Flag) { + fs.Var(f.Value, f.Name, f.Usage) + }) + var buffer bytes.Buffer + fs.SetOutput(&buffer) + fs.PrintDefaults() + assert.Equal(t, ` -log-flush-frequency value + Maximum number of seconds between log flushes (default 5s) + -logging-format value + Sets the log format. Permitted formats: "text". (default text) + -v value + number for the log level verbosity + -vmodule value + comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) +`, buffer.String()) + }) + +} + func TestContextualLogging(t *testing.T) { t.Run("enabled", func(t *testing.T) { testContextualLogging(t, true) From 4add08dcceda536ce89c0d69954a6e60c0058206 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 18 Jan 2023 14:45:54 +0100 Subject: [PATCH 3/3] k8s.io/component-base/logs: relax flagset unit tests A full string comparison might fail when the underlying libraries change how they format the help text. A regex match is less strict, but still makes some assumptions about the implementation. --- .../logs/api/v1/options_test.go | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/component-base/logs/api/v1/options_test.go b/staging/src/k8s.io/component-base/logs/api/v1/options_test.go index 475f7a46eb4..2597d8797a0 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/options_test.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/options_test.go @@ -109,10 +109,15 @@ func TestFlagSet(t *testing.T) { var buffer bytes.Buffer fs.SetOutput(&buffer) fs.PrintDefaults() - assert.Equal(t, ` --logging-format string Sets the log format. Permitted formats: "text". (default "text") - --log-flush-frequency duration Maximum number of seconds between log flushes (default 5s) - -v, --v Level number for the log level verbosity - --vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) + // Expected (Go 1.19, pflag v1.0.5): + // --logging-format string Sets the log format. Permitted formats: "text". (default "text") + // --log-flush-frequency duration Maximum number of seconds between log flushes (default 5s) + // -v, --v Level number for the log level verbosity + // --vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) + assert.Regexp(t, `.*--logging-format.*default.*text.* +.*--log-flush-frequency.*default 5s.* +.*-v.*--v.* +.*--vmodule.*pattern=N.* `, buffer.String()) }) @@ -127,14 +132,23 @@ func TestFlagSet(t *testing.T) { var buffer bytes.Buffer fs.SetOutput(&buffer) fs.PrintDefaults() - assert.Equal(t, ` -log-flush-frequency value - Maximum number of seconds between log flushes (default 5s) - -logging-format value - Sets the log format. Permitted formats: "text". (default text) - -v value - number for the log level verbosity - -vmodule value - comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) + // Expected (Go 1.19): + // -log-flush-frequency value + // Maximum number of seconds between log flushes (default 5s) + // -logging-format value + // Sets the log format. Permitted formats: "text". (default text) + // -v value + // number for the log level verbosity + // -vmodule value + // comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) + assert.Regexp(t, `.*-log-flush-frequency.* +.*default 5s.* +.*-logging-format.* +.*default.*text.* +.*-v.* +.* +.*-vmodule.* +.* `, buffer.String()) })