From 29581eef24f690f6ee36341435931724bb337e32 Mon Sep 17 00:00:00 2001 From: Thomas Poignant Date: Tue, 9 Jul 2024 16:13:45 +0200 Subject: [PATCH] fix(relay-proxy): Report original error when retriever failed (#2092) * fix(relay-proxy): Report original error when retriever failed Signed-off-by: Thomas Poignant * fix: add tests to validate that we have information about the error in flag Signed-off-by: Thomas Poignant --------- Signed-off-by: Thomas Poignant --- feature_flag.go | 19 +++++++++----- feature_flag_test.go | 27 +++++++++++++++++-- internal/flag/internal_flag.go | 5 +++- testdata/invalid-flag-config.json | 43 +++++++++++++++++++++++++++++++ testdata/invalid-flag-config.yaml | 17 ++++++++++++ 5 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 testdata/invalid-flag-config.json create mode 100644 testdata/invalid-flag-config.yaml diff --git a/feature_flag.go b/feature_flag.go index a63416f56e1..5ead9efaa83 100644 --- a/feature_flag.go +++ b/feature_flag.go @@ -106,12 +106,19 @@ func New(config Config) (*GoFeatureFlag, error) { err = retrieveFlagsAndUpdateCache(goFF.config, goFF.cache, goFF.retrieverManager) if err != nil { - // if initial retrieval failed, we are trying to start the persistent local disk configuration (if enabled). - errPersist := retrievePersistentLocalDisk(config.Context, config, goFF) - if errPersist != nil && !config.StartWithRetrieverError { - return nil, - fmt.Errorf("impossible to retrieve the flags, please check your configuration: %v", - errPersist) + switch { + case config.PersistentFlagConfigurationFile != "": + errPersist := retrievePersistentLocalDisk(config.Context, config, goFF) + if errPersist != nil && !config.StartWithRetrieverError { + return nil, fmt.Errorf("impossible to use the persistent flag configuration file: %v "+ + "[original error: %v]", errPersist, err) + } + case !config.StartWithRetrieverError: + return nil, fmt.Errorf("impossible to retrieve the flags, please check your configuration: %v", err) + default: + // We accept to start with a retriever error, we will serve only default value + goFF.config.internalLogger.Error("Impossible to retrieve the flags, starting with the "+ + "retriever error", slog.Any("error", err)) } } diff --git a/feature_flag_test.go b/feature_flag_test.go index a33b93b75f9..23b7fdf95fe 100644 --- a/feature_flag_test.go +++ b/feature_flag_test.go @@ -349,7 +349,7 @@ test-flag: gff, err := ffclient.New(ffclient.Config{ PollingInterval: 1 * time.Second, Retriever: &fileretriever.Retriever{Path: flagFilePath}, - Logger: log.New(os.Stdout, "", 0), + LeveledLogger: slog.Default(), StartWithRetrieverError: true, }) defer gff.Close() @@ -359,13 +359,36 @@ test-flag: flagValue, _ := gff.StringVariation("test-flag", ffcontext.NewEvaluationContext("random-key"), "SDKdefault") assert.Equal(t, "SDKdefault", flagValue, "should use the SDK default value") - _ = os.WriteFile(flagFilePath, []byte(initialFileContent), os.ModePerm) + err = os.WriteFile(flagFilePath, []byte(initialFileContent), os.ModePerm) + assert.NoError(t, err) time.Sleep(2 * time.Second) flagValue, _ = gff.StringVariation("test-flag", ffcontext.NewEvaluationContext("random-key"), "SDKdefault") assert.Equal(t, "true", flagValue, "should use the true value") } +func TestInvalidConf(t *testing.T) { + gff, err := ffclient.New(ffclient.Config{ + PollingInterval: 1 * time.Second, + Retriever: &fileretriever.Retriever{Path: "testdata/invalid-flag-config.json"}, + LeveledLogger: slog.Default(), + }) + defer gff.Close() + assert.Error(t, err) + assert.Equal(t, err.Error(), "impossible to retrieve the flags, please check your configuration: yaml: line 43: did not find expected ',' or '}'") +} + +func TestInvalidConfAndRetrieverError(t *testing.T) { + gff, err := ffclient.New(ffclient.Config{ + PollingInterval: 1 * time.Second, + Retriever: &fileretriever.Retriever{Path: "testdata/invalid-flag-config.json"}, + LeveledLogger: slog.Default(), + StartWithRetrieverError: true, + }) + defer gff.Close() + assert.NoError(t, err) +} + func TestValidUseCaseBigFlagFile(t *testing.T) { // Valid use case gff, err := ffclient.New(ffclient.Config{ diff --git a/internal/flag/internal_flag.go b/internal/flag/internal_flag.go index dc13c0154fe..c73846a4841 100644 --- a/internal/flag/internal_flag.go +++ b/internal/flag/internal_flag.go @@ -231,7 +231,10 @@ func (f *InternalFlag) IsValid() error { // Check that all variation has the same types expectedVarType := "" - for _, value := range f.GetVariations() { + for name, value := range f.GetVariations() { + if value == nil { + return fmt.Errorf("nil value for variation: %s", name) + } if expectedVarType != "" { currentType, err := utils.JSONTypeExtractor(*value) if err != nil { diff --git a/testdata/invalid-flag-config.json b/testdata/invalid-flag-config.json new file mode 100644 index 00000000000..618fdc146c9 --- /dev/null +++ b/testdata/invalid-flag-config.json @@ -0,0 +1,43 @@ +{ + "test-flag": { + "variations": { + "Default": false, + "false": false, + "true": true + }, + "targeting": [ + { + "name": "legacyRuleV0", + "query": "key eq \"random-key\"", + "percentage": { + "false": 0, + "true": 100 + } + } + ], + "defaultRule": { + "name": "legacyDefaultRule", + "variation": "Default" + } + }, + "test-flag2": { + "variations": { + "Default": false, + "false": false, + "true": true + }, + "targeting": [ + { + "name": "legacyRuleV0", + "query": "key eq \"not-a-key\"", + "percentage": { + "false": 0, + "true": 100 + } + } + ], + "defaultRule": { + "name": "legacyDefaultRule", + "variation": "Default" + } + } diff --git a/testdata/invalid-flag-config.yaml b/testdata/invalid-flag-config.yaml new file mode 100644 index 00000000000..7f874aaa32f --- /dev/null +++ b/testdata/invalid-flag-config.yaml @@ -0,0 +1,17 @@ +test-flag: + variations: + Default: false + False: false + True: + targeting: + - name: legacyRuleV0 + query: key eq "random-key" + percentage: + False: 0 + True: 100 + defaultRule: + name: legacyDefaultRule + variation: Default + metadata: + description: this is a simple feature flag + issue-link: https://jira.xxx/GOFF-01 \ No newline at end of file