From f7a009a065f74ba608acaf065ca6b31236307883 Mon Sep 17 00:00:00 2001 From: Julio Guerra <julio@datadog.com> Date: Wed, 28 Dec 2022 14:57:27 +0100 Subject: [PATCH] internal/remoteconfig: client error handling (backport of release-v1.45.x fixes) (#1627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- internal/appsec/remoteconfig.go | 12 +++- internal/remoteconfig/remoteconfig.go | 89 +++++++++++++++++---------- 2 files changed, 66 insertions(+), 35 deletions(-) diff --git a/internal/appsec/remoteconfig.go b/internal/appsec/remoteconfig.go index b177910c00..840c5e2a22 100644 --- 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 index 0503b34780..0626466ecd 100644 --- 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)