fix(relay-proxy): Report original error when retriever failed (#2092)

* fix(relay-proxy): Report original error when retriever failed Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org> * fix: add tests to validate that we have information about the error in flag Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org> --------- Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
thomaspoignant · Jul 9, 2024 · 29581ee · 29581ee
1 parent b727998
commit 29581ee
Show file tree

Hide file tree

Showing 5 changed files with 102 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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