internal/remoteconfig: client error handling (backport of release-v1.…

…45.x fixes) (#1627) Properly handle many error cases that were not in the remote config client. Especially the "no remote config update" case {} that is leading to a json parsing error in the repository Update(). Backport of release-v1.45.x changes Co-authored-by: François Mazeau <francois.mazeau@datadoghq.com>
DataDog · Dec 28, 2022 · f7a009a · f7a009a
1 parent 2d25957
commit f7a009a
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 35 deletions.
diff --git a/internal/appsec/remoteconfig.go b/internal/appsec/remoteconfig.go
@@ -74,8 +74,7 @@ func (a *appsec) asmFeaturesCallback(u remoteconfig.ProductUpdate) map[string]rc
 			a.stop()
 		}
 		if err != nil {
-			status.State = rc.ApplyStateError
-			status.Error = err.Error()
+			status = genApplyStatus(false, err)
 		}
 		statuses[path] = status
 	}
@@ -97,6 +96,15 @@ func (h *wafHandleWrapper) asmDataCallback(u remoteconfig.ProductUpdate) map[str
 
 	for path, raw := range u {
 		log.Debug("appsec: Remote config: processing %s", path)
+
+		// A nil config means ASM_DATA was disabled, and we stopped receiving the config file
+		// Don't ack the config in this case
+		if raw == nil {
+			log.Debug("appsec: remote config: %s disabled", path)
+			statuses[path] = genApplyStatus(false, nil)
+			continue
+		}
+
 		var rulesData rc.ASMDataRulesData
 		if err := json.Unmarshal(raw, &rulesData); err != nil {
 			log.Debug("appsec: Remote config: error while unmarshalling payload for %s: %v. Configuration won't be applied.", path, err)

diff --git a/internal/remoteconfig/remoteconfig.go b/internal/remoteconfig/remoteconfig.go
@@ -21,6 +21,7 @@ import (
 	rc "github.com/DataDog/datadog-agent/pkg/remoteconfig/state"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig"
+	"gopkg.in/DataDog/dd-trace-go.v1/internal/log"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/version"
 )
 
@@ -148,19 +149,19 @@ func (c *Client) Stop() {
 func (c *Client) updateState() {
 	data, err := c.newUpdateRequest()
 	if err != nil {
-		c.lastError = err
+		log.Error("remoteconfig: unexpected error while creating a new update request payload: %v", err)
 		return
 	}
 
 	req, err := http.NewRequest(http.MethodGet, c.endpoint, &data)
 	if err != nil {
-		c.lastError = err
+		log.Error("remoteconfig: unexpected error while creating a new http request: %v", err)
 		return
 	}
 
 	resp, err := c.HTTP.Do(req)
 	if err != nil {
-		c.lastError = err
+		log.Debug("remoteconfig: http request error: %v", err)
 		return
 	}
 	// Flush and close the response body when returning (cf. https://pkg.go.dev/net/http#Client.Do)
@@ -169,15 +170,28 @@ func (c *Client) updateState() {
 		resp.Body.Close()
 	}()
 
-	var update clientGetConfigsResponse
-	err = json.NewDecoder(resp.Body).Decode(&update)
+	if sc := resp.StatusCode; sc != http.StatusOK {
+		log.Debug("remoteconfig: http request error: response status code is not 200 (OK) but %s", http.StatusText(sc))
+		return
+	}
+
+	respBody, err := io.ReadAll(resp.Body)
 	if err != nil {
-		c.lastError = err
+		log.Error("remoteconfig: http request error: could not read the response body: %v", err)
 		return
 	}
 
-	err = c.applyUpdate(&update)
-	c.lastError = err
+	if body := string(respBody); body == `{}` || body == `null` {
+		return
+	}
+
+	var update clientGetConfigsResponse
+	if err := json.Unmarshal(respBody, &update); err != nil {
+		log.Error("remoteconfig: http request error: could not parse the json response body: %v", err)
+		return
+	}
+
+	c.lastError = c.applyUpdate(&update)
 }
 
 // RegisterCallback allows registering a callback that will be invoked when the client
@@ -199,13 +213,6 @@ func (c *Client) applyUpdate(pbUpdate *clientGetConfigsResponse) error {
 		}
 	}
 
-	update := rc.Update{
-		TUFRoots:      pbUpdate.Roots,
-		TUFTargets:    pbUpdate.Targets,
-		TargetFiles:   fileMap,
-		ClientConfigs: pbUpdate.ClientConfigs,
-	}
-
 	mapify := func(s *rc.RepositoryState) map[string]string {
 		m := make(map[string]string)
 		for i := range s.Configs {
@@ -219,30 +226,43 @@ func (c *Client) applyUpdate(pbUpdate *clientGetConfigsResponse) error {
 	// Check the repository state before and after the update to detect which configs are not being sent anymore.
 	// This is needed because some products can stop sending configurations, and we want to make sure that the subscribers
 	// are provided with this information in this case
-	stateBefore, _ := c.repository.CurrentState()
-	products, err := c.repository.Update(update)
-	stateAfter, _ := c.repository.CurrentState()
+	stateBefore, err := c.repository.CurrentState()
+	if err != nil {
+		return fmt.Errorf("repository current state error: %v", err)
+	}
+	products, err := c.repository.Update(rc.Update{
+		TUFRoots:      pbUpdate.Roots,
+		TUFTargets:    pbUpdate.Targets,
+		TargetFiles:   fileMap,
+		ClientConfigs: pbUpdate.ClientConfigs,
+	})
+	if err != nil {
+		return fmt.Errorf("repository update error: %v", err)
+	}
+	stateAfter, err := c.repository.CurrentState()
+	if err != nil {
+		return fmt.Errorf("repository current state error after update: %v", err)
+	}
 
 	// Create a config files diff between before/after the update to see which config files are missing
 	mBefore := mapify(&stateBefore)
-	mAfter := mapify(&stateAfter)
-	for k := range mAfter {
+	for k := range mapify(&stateAfter) {
 		delete(mBefore, k)
 	}
 
 	// Set the payload data to nil for missing config files. The callbacks then can handle the nil config case to detect
 	// that this config will not be updated anymore.
-	updatedProducts := make(map[string]bool)
+	updatedProducts := make(map[string]struct{})
 	for path, product := range mBefore {
 		if productUpdates[product] == nil {
 			productUpdates[product] = make(ProductUpdate)
 		}
 		productUpdates[product][path] = nil
-		updatedProducts[product] = true
+		updatedProducts[product] = struct{}{}
 	}
 	// Aggregate updated products and missing products so that callbacks get called for both
 	for _, p := range products {
-		updatedProducts[p] = true
+		updatedProducts[p] = struct{}{}
 	}
 
 	// Performs the callbacks registered for all updated products and update the application status in the repository
@@ -255,7 +275,7 @@ func (c *Client) applyUpdate(pbUpdate *clientGetConfigsResponse) error {
 		}
 	}
 
-	return err
+	return nil
 }
 
 func (c *Client) newUpdateRequest() (bytes.Buffer, error) {
@@ -290,15 +310,18 @@ func (c *Client) newUpdateRequest() (bytes.Buffer, error) {
 		errMsg = c.lastError.Error()
 	}
 
-	pbConfigState := make([]*configState, 0, len(state.Configs))
-	for _, f := range state.Configs {
-		pbConfigState = append(pbConfigState, &configState{
-			ID:         f.ID,
-			Version:    f.Version,
-			Product:    f.Product,
-			ApplyState: f.ApplyStatus.State,
-			ApplyError: f.ApplyStatus.Error,
-		})
+	var pbConfigState []*configState
+	if !hasError {
+		pbConfigState = make([]*configState, 0, len(state.Configs))
+		for _, f := range state.Configs {
+			pbConfigState = append(pbConfigState, &configState{
+				ID:         f.ID,
+				Version:    f.Version,
+				Product:    f.Product,
+				ApplyState: f.ApplyStatus.State,
+				ApplyError: f.ApplyStatus.Error,
+			})
+		}
 	}
 
 	cap := big.NewInt(0)