Skip to content

Commit

Permalink
fix: invalid scoped-sync responses for empty flags (#1352)
Browse files Browse the repository at this point in the history
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 <todd.baert@dynatrace.com>
  • Loading branch information
toddbaert committed Jul 5, 2024
1 parent 450cbc8 commit 51371d2
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 22 deletions.
12 changes: 11 additions & 1 deletion flagd/pkg/service/flag-sync/sync-multiplexer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand All @@ -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 {
Expand Down
62 changes: 44 additions & 18 deletions flagd/pkg/service/flag-sync/sync-multiplexer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -17,33 +21,51 @@ 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",
id: context.Background(),
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,
},
Expand Down Expand Up @@ -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)
}
})
}
Expand Down Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion flagd/pkg/service/flag-sync/sync_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
4 changes: 2 additions & 2 deletions flagd/pkg/service/flag-sync/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -28,5 +28,5 @@ func getSimpleFlagStore() (*store.Flags, []string) {
Source: "B",
})

return flagStore, []string{"A", "B"}
return flagStore, []string{"A", "B", "C"}
}

0 comments on commit 51371d2

Please sign in to comment.