From 51371d25e25e1199336a5a831530506313628ff3 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Fri, 5 Jul 2024 14:25:34 -0400 Subject: [PATCH] fix: invalid scoped-sync responses for empty flags (#1352) Fixes an issue where invalid flag payloads were returned on sync requests with scopes if the flag set was empty. Below is an example of the bug. ```shell $ grpcurl -import-path /home/todd/temp -proto sync.proto -plaintext localhost:8015 flagd.sync.v1.FlagSyncService/FetchAllFlags { "flagConfiguration": "{\"flags\":{}}" } $ grpcurl -import-path /home/todd/temp -proto sync.proto -plaintext -d '{"selector":"../config/samples/example_flags.flagd.json"}' localhost:8015 flagd.sync.v1.FlagSyncService/FetchAllFlags {} ``` --------- Signed-off-by: Todd Baert --- .../pkg/service/flag-sync/sync-multiplexer.go | 12 +++- .../flag-sync/sync-multiplexer_test.go | 62 +++++++++++++------ .../service/flag-sync/sync_service_test.go | 2 +- flagd/pkg/service/flag-sync/util_test.go | 4 +- 4 files changed, 58 insertions(+), 22 deletions(-) diff --git a/flagd/pkg/service/flag-sync/sync-multiplexer.go b/flagd/pkg/service/flag-sync/sync-multiplexer.go index 4d680712c..3191e0dd4 100644 --- a/flagd/pkg/service/flag-sync/sync-multiplexer.go +++ b/flagd/pkg/service/flag-sync/sync-multiplexer.go @@ -12,6 +12,11 @@ import ( "github.com/open-feature/flagd/core/pkg/store" ) +//nolint:errchkjson +var emptyConfigBytes, _ = json.Marshal(map[string]map[string]string{ + "flags": {}, +}) + // Multiplexer abstract subscription handling and storage processing. // Flag configurations will be lazy loaded using reFill logic upon the calls to publish. type Multiplexer struct { @@ -162,6 +167,10 @@ func (r *Multiplexer) SourcesAsMetadata() string { // reFill local configuration values func (r *Multiplexer) reFill() error { clear(r.selectorFlags) + // start all sources with empty config + for _, source := range r.sources { + r.selectorFlags[source] = string(emptyConfigBytes) + } all, err := r.store.GetAll(context.Background()) if err != nil { @@ -170,7 +179,7 @@ func (r *Multiplexer) reFill() error { bytes, err := json.Marshal(map[string]interface{}{"flags": all}) if err != nil { - return fmt.Errorf("error from marshallin: %w", err) + return fmt.Errorf("error marshalling: %w", err) } r.allFlags = string(bytes) @@ -188,6 +197,7 @@ func (r *Multiplexer) reFill() error { } } + // for all flags, sort them into their correct selector for source, flags := range collector { bytes, err := json.Marshal(map[string]interface{}{"flags": flags}) if err != nil { diff --git a/flagd/pkg/service/flag-sync/sync-multiplexer_test.go b/flagd/pkg/service/flag-sync/sync-multiplexer_test.go index 08e416bf6..2afcd1e24 100644 --- a/flagd/pkg/service/flag-sync/sync-multiplexer_test.go +++ b/flagd/pkg/service/flag-sync/sync-multiplexer_test.go @@ -6,8 +6,12 @@ import ( "strings" "testing" "time" + + "github.com/stretchr/testify/assert" ) +const emptyConfigString = "{\"flags\":{}}" + func TestRegistration(t *testing.T) { // given mux, err := NewMux(getSimpleFlagStore()) @@ -17,11 +21,12 @@ func TestRegistration(t *testing.T) { } tests := []struct { - testName string - id interface{} - source string - connection chan payload - expectError bool + testName string + id interface{} + source string + flagStringValidator func(flagString string, testSource string, testName string) + connection chan payload + expectError bool }{ { testName: "subscribe to all flags", @@ -29,21 +34,38 @@ func TestRegistration(t *testing.T) { connection: make(chan payload, 1), }, { - testName: "subscribe to source A", - id: context.Background(), - source: "A", + testName: "subscribe to source A", + id: context.Background(), + source: "A", + flagStringValidator: func(flagString string, testSource string, testName string) { + assert.Contains(t, flagString, fmt.Sprintf("\"source\":\"%s\"", testSource)) + }, + connection: make(chan payload, 1), + }, + { + testName: "subscribe to source B", + id: context.Background(), + source: "B", + flagStringValidator: func(flagString string, testSource string, testName string) { + assert.Contains(t, flagString, fmt.Sprintf("\"source\":\"%s\"", testSource)) + }, connection: make(chan payload, 1), }, + { - testName: "subscribe to source B", + testName: "subscribe to empty", id: context.Background(), - source: "B", + source: "C", connection: make(chan payload, 1), + flagStringValidator: func(flagString string, testSource string, testName string) { + assert.Equal(t, flagString, emptyConfigString) + }, + expectError: false, }, { testName: "subscribe to non-existing", id: context.Background(), - source: "C", + source: "D", connection: make(chan payload, 1), expectError: true, }, @@ -75,13 +97,8 @@ func TestRegistration(t *testing.T) { break } - if initSync.flags == "" { - t.Fatal("expected non empty flag payload, but got empty") - } - - // validate source of flag - if test.source != "" && !strings.Contains(initSync.flags, fmt.Sprintf("\"source\":\"%s\"", test.source)) { - t.Fatal("expected initial flag response to contain flags from source, but failed to find source") + if test.flagStringValidator != nil { + test.flagStringValidator(initSync.flags, test.source, test.testName) } }) } @@ -173,4 +190,13 @@ func TestGetAllFlags(t *testing.T) { t.Fatal("expected flags to be scoped") return } + + // when - get all for a flagless-scope + flags, err = mux.GetAllFlags("C") + if err != nil { + t.Fatal("error when retrieving all flags") + return + } + + assert.Equal(t, flags, emptyConfigString) } diff --git a/flagd/pkg/service/flag-sync/sync_service_test.go b/flagd/pkg/service/flag-sync/sync_service_test.go index 5f524c643..81cfb46c0 100644 --- a/flagd/pkg/service/flag-sync/sync_service_test.go +++ b/flagd/pkg/service/flag-sync/sync_service_test.go @@ -128,7 +128,7 @@ func TestSyncServiceEndToEnd(t *testing.T) { t.Fatal("expected sources entry in the metadata, but got nil") } - if asMap["sources"] != "A,B" { + if asMap["sources"] != "A,B,C" { t.Fatal("incorrect sources entry in metadata") } diff --git a/flagd/pkg/service/flag-sync/util_test.go b/flagd/pkg/service/flag-sync/util_test.go index 4baa99fcd..339379fd9 100644 --- a/flagd/pkg/service/flag-sync/util_test.go +++ b/flagd/pkg/service/flag-sync/util_test.go @@ -5,7 +5,7 @@ import ( "github.com/open-feature/flagd/core/pkg/store" ) -// getSimpleFlagStore returns a flag store pre-filled with flags from sources A & B +// getSimpleFlagStore returns a flag store pre-filled with flags from sources A & B & C, which C empty func getSimpleFlagStore() (*store.Flags, []string) { variants := map[string]any{ "true": true, @@ -28,5 +28,5 @@ func getSimpleFlagStore() (*store.Flags, []string) { Source: "B", }) - return flagStore, []string{"A", "B"} + return flagStore, []string{"A", "B", "C"} }