From e58a502b232ad0d22fa6a6384095a33d7f766be0 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Tue, 31 Oct 2023 09:54:02 -0700 Subject: [PATCH 1/7] Add []byte as base64 flag type --- dyngeneric.go | 5 ++++- dyngeneric_test.go | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/dyngeneric.go b/dyngeneric.go index f724ada..bb7b5b8 100644 --- a/dyngeneric.go +++ b/dyngeneric.go @@ -3,6 +3,7 @@ package dflag import ( + "encoding/base64" "flag" "fmt" "strconv" @@ -74,7 +75,7 @@ func ValidateDynSliceMinElements[T any](count int) func([]T) error { // DynValueTypes are the types currently supported by Parse[T] and thus by Dyn[T]. // DynJSON is special. type DynValueTypes interface { - bool | time.Duration | float64 | int64 | string | []string | sets.Set[string] + bool | time.Duration | float64 | int64 | string | []string | sets.Set[string] | []byte } type DynValue[T any] struct { @@ -189,6 +190,8 @@ func parse[T any](input string) (val T, err error) { *v, err = strconv.ParseFloat(strings.TrimSpace(input), 64) case *time.Duration: *v, err = time.ParseDuration(input) + case *[]byte: + *v, err = base64.StdEncoding.DecodeString(input) case *string: *v = input case *[]string: diff --git a/dyngeneric_test.go b/dyngeneric_test.go index 64535ed..7fdc8fb 100644 --- a/dyngeneric_test.go +++ b/dyngeneric_test.go @@ -70,3 +70,12 @@ func TestRemoveCommon(t *testing.T) { setBB.Remove("c") assert.False(t, setBB.Has("c")) } + +func TestBinary(t *testing.T) { + set := flag.NewFlagSet("foobar", flag.ContinueOnError) + dynFlag := Dyn(set, "some_binary", []byte{2, 1, 0}, "some binary values") + assert.Equal(t, []byte{2, 1, 0}, dynFlag.Get(), "value must be default after create") + err := set.Set("some_binary", "AAEC\n") + assert.NoError(t, err, "setting value must succeed") + assert.Equal(t, []byte{0, 1, 2}, dynFlag.Get(), "value must be set after update") +} From a1c382bb40c0b76c2197e34d173433a20f082b38 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Tue, 31 Oct 2023 09:57:23 -0700 Subject: [PATCH 2/7] add error case --- dyngeneric_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dyngeneric_test.go b/dyngeneric_test.go index 7fdc8fb..d264289 100644 --- a/dyngeneric_test.go +++ b/dyngeneric_test.go @@ -75,7 +75,9 @@ func TestBinary(t *testing.T) { set := flag.NewFlagSet("foobar", flag.ContinueOnError) dynFlag := Dyn(set, "some_binary", []byte{2, 1, 0}, "some binary values") assert.Equal(t, []byte{2, 1, 0}, dynFlag.Get(), "value must be default after create") - err := set.Set("some_binary", "AAEC\n") + err := set.Set("some_binary", "\nAAEC\n") // extra newlines are fine assert.NoError(t, err, "setting value must succeed") assert.Equal(t, []byte{0, 1, 2}, dynFlag.Get(), "value must be set after update") + err = set.Set("some_binary", "foo bar") + assert.Error(t, err, "setting bogus base64 should fail") } From cb031a5698211744bcbfba6e5d1dd9aeac90c552 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Tue, 31 Oct 2023 10:13:36 -0700 Subject: [PATCH 3/7] fix the tostring to not be [ bytes...] but base64 too, add a binary to http example flags --- dyngeneric.go | 4 +++- dyngeneric_test.go | 2 ++ examples/server_kube/http.go | 5 +++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/dyngeneric.go b/dyngeneric.go index bb7b5b8..5849f4e 100644 --- a/dyngeneric.go +++ b/dyngeneric.go @@ -278,8 +278,10 @@ func (d *DynValue[T]) String() string { switch v := any(d.Get()).(type) { case []string: return strings.Join(v, ",") + case []byte: + return base64.StdEncoding.EncodeToString(v) default: - return fmt.Sprintf("%v", d.Get()) + return fmt.Sprintf("%v", v) } } diff --git a/dyngeneric_test.go b/dyngeneric_test.go index d264289..13efc53 100644 --- a/dyngeneric_test.go +++ b/dyngeneric_test.go @@ -78,6 +78,8 @@ func TestBinary(t *testing.T) { err := set.Set("some_binary", "\nAAEC\n") // extra newlines are fine assert.NoError(t, err, "setting value must succeed") assert.Equal(t, []byte{0, 1, 2}, dynFlag.Get(), "value must be set after update") + str := dynFlag.String() + assert.Equal(t, "AAEC", str, "value when printed must be base64 encoded") err = set.Set("some_binary", "foo bar") assert.Error(t, err, "setting bogus base64 should fail") } diff --git a/examples/server_kube/http.go b/examples/server_kube/http.go index c140501..7234f44 100644 --- a/examples/server_kube/http.go +++ b/examples/server_kube/http.go @@ -49,8 +49,9 @@ var ( }, }, "An arbitrary JSON struct.") - dynArray = dflag.New([]string{"z", "b", "a"}, "An array of strings (comma separated)") - dynSet = dflag.New(sets.New("z", "b", "a"), "An set of strings (comma separated)") + dynArray = dflag.New([]string{"z", "b", "a"}, "An array of strings (comma separated)") + dynSet = dflag.New(sets.New("z", "b", "a"), "An set of strings (comma separated)") + dynBinary = dflag.Dyn(flag.CommandLine, "example_binary", []byte{0x00, 0x01, 0x02}, "A binary value") ) func main() { From 24e309f20f2d26cbbd5d9ddb8e5f70d8423b18a6 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Tue, 31 Oct 2023 13:21:56 -0700 Subject: [PATCH 4/7] handle binary when reading from file --- configmap/loglevel_change_test.go | 17 +++++++++++++++-- configmap/updater.go | 10 +++++++++- dyngeneric.go | 9 +++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/configmap/loglevel_change_test.go b/configmap/loglevel_change_test.go index 366f7ea..f6b69a3 100644 --- a/configmap/loglevel_change_test.go +++ b/configmap/loglevel_change_test.go @@ -21,11 +21,14 @@ import ( "testing" "time" + "fortio.org/assert" + "fortio.org/dflag" "fortio.org/dflag/configmap" "fortio.org/log" ) -func TestDynamicLogLevel(t *testing.T) { +func TestDynamicLogLevelAndBinaryFlag(t *testing.T) { + binF := dflag.Dyn(flag.CommandLine, "binary_flag", []byte{}, "a test binary flag") log.SetDefaultsForClientTools() tmpDir, err := os.MkdirTemp("", "fortio-logger-test") if err != nil { @@ -40,6 +43,10 @@ func TestDynamicLogLevel(t *testing.T) { if err = os.WriteFile(fName, []byte("ignored"), 0o644); err != nil { t.Fatalf("unable to write %v: %v", fName, err) } + binaryFlag := path.Join(pDir, "binary_flag") + if err = os.WriteFile(binaryFlag, []byte{0, 1, 2, 3}, 0o644); err != nil { + t.Fatalf("unable to write %v: %v", binaryFlag, err) + } var u *configmap.Updater log.SetLogLevel(log.Debug) if u, err = configmap.Setup(flag.CommandLine, pDir); err != nil { @@ -49,8 +56,13 @@ func TestDynamicLogLevel(t *testing.T) { if u.Warnings() != 1 { t.Errorf("Expected exactly 1 warning (extra flag), got %d", u.Warnings()) } + assert.Equal(t, binF.Get(), []byte{0, 1, 2, 3}) + // Now update that flag (and the loglevel) + if err = os.WriteFile(binaryFlag, []byte{1, 0}, 0o644); err != nil { + t.Fatalf("unable to write %v: %v", binaryFlag, err) + } fName = path.Join(pDir, "loglevel") - // Test also the new normalization (space trimmimg and captitalization) + // Test also the new normalization (space trimming and capitalization) if err = os.WriteFile(fName, []byte(" InFO\n\n"), 0o644); err != nil { t.Fatalf("unable to write %v: %v", fName, err) } @@ -59,6 +71,7 @@ func TestDynamicLogLevel(t *testing.T) { if newLevel != log.Info { t.Errorf("Loglevel didn't change as expected, still %v %v", newLevel, newLevel.String()) } + assert.Equal(t, binF.Get(), []byte{1, 0}) // put back debug log.SetLogLevel(log.Debug) } diff --git a/configmap/updater.go b/configmap/updater.go index 024577c..379093b 100644 --- a/configmap/updater.go +++ b/configmap/updater.go @@ -161,8 +161,16 @@ func (u *Updater) readFlagFile(fullPath string, dynamicOnly bool) error { if err != nil { return err } + if v := dflag.IsBinary(flag); v != nil { + log.Infof("Updating binary %q to new blob (len %d)", flagName, len(content)) + err = v.SetV(content) + if err != nil { + return err + } + return nil + } str := string(content) - log.Infof("updating %v to %q", flagName, str) + log.Infof("Updating %q to %q", flagName, str) // do not call flag.Value.Set, instead go through flagSet.Set to change "changed" state. return u.flagSet.Set(flagName, str) } diff --git a/dyngeneric.go b/dyngeneric.go index 5849f4e..11105c4 100644 --- a/dyngeneric.go +++ b/dyngeneric.go @@ -44,6 +44,15 @@ func IsFlagDynamic(f *flag.Flag) bool { return df.IsDynamicFlag() // will clearly return true if it exists } +// IsBinary returns the binary flag or nil depending on if the given Flag +// is a []byte dynamic value or not (for confimap/file based setting). +func IsBinary(f *flag.Flag) *DynValue[[]byte] { + if v, ok := f.Value.(*DynValue[[]byte]); ok { + return v + } + return nil +} + type DynamicBoolValueTag struct{} func (*DynamicBoolValueTag) IsBoolFlag() bool { From 7cfb95e7a81b7c54d4d5e3ee5691735304a2c396 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Tue, 31 Oct 2023 13:34:11 -0700 Subject: [PATCH 5/7] add test for IsBinary() in same package as definition so coverage sees it's used --- dyngeneric_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/dyngeneric_test.go b/dyngeneric_test.go index 13efc53..23687ac 100644 --- a/dyngeneric_test.go +++ b/dyngeneric_test.go @@ -44,11 +44,16 @@ func TestArrayToString(t *testing.T) { s := []string{"z", "a", "c", "b"} f := New(s, "test array") Flag("testing123", f) - defValue := flag.CommandLine.Lookup("testing123").DefValue + flag := flag.CommandLine.Lookup("testing123") + defValue := flag.DefValue // order preserved unlike for sets.Set where we sort str := f.String() assert.Equal(t, "z,a,c,b", str) assert.Equal(t, "z,a,c,b", defValue) + b := IsBinary(flag) + if b != nil { + t.Errorf("flag %v isn't binary yet got non nil: %v", flag, b) + } } func TestRemoveCommon(t *testing.T) { @@ -82,4 +87,9 @@ func TestBinary(t *testing.T) { assert.Equal(t, "AAEC", str, "value when printed must be base64 encoded") err = set.Set("some_binary", "foo bar") assert.Error(t, err, "setting bogus base64 should fail") + flag := set.Lookup("some_binary") + assert.True(t, IsFlagDynamic(flag), "flag must be dynamic") + if IsBinary(flag) == nil { + t.Errorf("flag %v isn't binary yet it should", flag) + } } From 9917767d866b5830fad1666220e3587e73b45602 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Tue, 31 Oct 2023 13:44:20 -0700 Subject: [PATCH 6/7] also test for errors, add Errors() count --- configmap/loglevel_change_test.go | 16 +++++++++++++++- configmap/updater.go | 8 ++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/configmap/loglevel_change_test.go b/configmap/loglevel_change_test.go index f6b69a3..84046e4 100644 --- a/configmap/loglevel_change_test.go +++ b/configmap/loglevel_change_test.go @@ -16,6 +16,7 @@ package configmap_test // this used to be in fortio.org/fortio/fnet/dyanmic_logg import ( "flag" + "fmt" "os" "path" "testing" @@ -28,7 +29,13 @@ import ( ) func TestDynamicLogLevelAndBinaryFlag(t *testing.T) { - binF := dflag.Dyn(flag.CommandLine, "binary_flag", []byte{}, "a test binary flag") + binF := dflag.Dyn(flag.CommandLine, "binary_flag", []byte{}, "a test binary flag").WithValidator(func(data []byte) error { + l := len(data) + if l > 4 { + return fmt.Errorf("generating error for binary flag len %d", l) + } + return nil + }) log.SetDefaultsForClientTools() tmpDir, err := os.MkdirTemp("", "fortio-logger-test") if err != nil { @@ -74,4 +81,11 @@ func TestDynamicLogLevelAndBinaryFlag(t *testing.T) { assert.Equal(t, binF.Get(), []byte{1, 0}) // put back debug log.SetLogLevel(log.Debug) + assert.Equal(t, u.Errors(), 0) + // Now create validation error on binary flag: + if err = os.WriteFile(binaryFlag, []byte{1, 2, 3, 4, 5}, 0o644); err != nil { + t.Fatalf("unable to write %v: %v", binaryFlag, err) + } + time.Sleep(1 * time.Second) + assert.Equal(t, u.Errors(), 1) } diff --git a/configmap/updater.go b/configmap/updater.go index 379093b..6ac13e8 100644 --- a/configmap/updater.go +++ b/configmap/updater.go @@ -40,6 +40,7 @@ type Updater struct { flagSet *flag.FlagSet done chan bool warnings atomic.Int32 // Count of unknown flags that have been logged (increases at each iteration). + errors atomic.Int32 // Count of validation errors that have been logged (increases at each iteration). } // Setup is a combination/shortcut for New+Initialize+Start. @@ -133,6 +134,7 @@ func (u *Updater) readAll(dynamicOnly bool) error { u.warnings.Add(1) } else if !(errors.Is(err, errFlagNotDynamic) && dynamicOnly) { errorStrings = append(errorStrings, fmt.Sprintf("flag %v: %v", f.Name(), err.Error())) + u.errors.Add(1) } } } @@ -148,6 +150,11 @@ func (u *Updater) Warnings() int { return int(u.warnings.Load()) } +// Return the errors count. +func (u *Updater) Errors() int { + return int(u.errors.Load()) +} + func (u *Updater) readFlagFile(fullPath string, dynamicOnly bool) error { flagName := path.Base(fullPath) flag := u.flagSet.Lookup(flagName) @@ -201,6 +208,7 @@ func (u *Updater) watchForUpdates() { flagName := path.Base(event.Name) if err := u.readFlagFile(event.Name, true); err != nil { log.Errf("dflag: failed setting flag %s: %v", flagName, err.Error()) + u.errors.Add(1) } case fsnotify.Chmod: } From b1ccad6edf9d2b74df5b015e273a78b7b82c942f Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Tue, 31 Oct 2023 14:04:25 -0700 Subject: [PATCH 7/7] hopefully passes, but is brittle/timing related --- configmap/loglevel_change_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/configmap/loglevel_change_test.go b/configmap/loglevel_change_test.go index 84046e4..96fb38f 100644 --- a/configmap/loglevel_change_test.go +++ b/configmap/loglevel_change_test.go @@ -54,6 +54,8 @@ func TestDynamicLogLevelAndBinaryFlag(t *testing.T) { if err = os.WriteFile(binaryFlag, []byte{0, 1, 2, 3}, 0o644); err != nil { t.Fatalf("unable to write %v: %v", binaryFlag, err) } + // Time based tests aren't great, specially when ran on (slow) CI try to have notification not get events for above. + time.Sleep(1 * time.Second) var u *configmap.Updater log.SetLogLevel(log.Debug) if u, err = configmap.Setup(flag.CommandLine, pDir); err != nil { @@ -73,7 +75,8 @@ func TestDynamicLogLevelAndBinaryFlag(t *testing.T) { if err = os.WriteFile(fName, []byte(" InFO\n\n"), 0o644); err != nil { t.Fatalf("unable to write %v: %v", fName, err) } - time.Sleep(1 * time.Second) + // Time based tests aren't great, specially when ran on (slow) CI but... + time.Sleep(2 * time.Second) newLevel := log.GetLogLevel() if newLevel != log.Info { t.Errorf("Loglevel didn't change as expected, still %v %v", newLevel, newLevel.String()) @@ -81,11 +84,11 @@ func TestDynamicLogLevelAndBinaryFlag(t *testing.T) { assert.Equal(t, binF.Get(), []byte{1, 0}) // put back debug log.SetLogLevel(log.Debug) - assert.Equal(t, u.Errors(), 0) + assert.Equal(t, u.Errors(), 0, "should have 0 errors so far") // Now create validation error on binary flag: if err = os.WriteFile(binaryFlag, []byte{1, 2, 3, 4, 5}, 0o644); err != nil { t.Fatalf("unable to write %v: %v", binaryFlag, err) } - time.Sleep(1 * time.Second) - assert.Equal(t, u.Errors(), 1) + time.Sleep(2 * time.Second) + assert.Equal(t, u.Errors(), 1, "should have 1 error picked up as we wrote > 4 bytes") }