From 06766e50d9ba9e16d91895b735bdf856bc8ea9b4 Mon Sep 17 00:00:00 2001 From: Dario Meloni Date: Fri, 24 Nov 2023 13:09:07 +0100 Subject: [PATCH] Trace-agent doesn't forward 404s to tracers (#21016) * Trace-agent forward 404s to tracers When remote configuration is in a failed state the trace agent always returned 500s * Simplify code that can't file * Added release note * Removed log message * Update releasenotes/notes/fix-tracer-agent-forwards-remote-config-errors-1e939be195e6db67.yaml Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com> * Added Unit Test * Close body * Remove returned error handling --------- Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com> --- cmd/agent/subcommands/run/command.go | 4 +-- .../subcommands/start/command.go | 4 +-- cmd/trace-agent/config/remote/config.go | 11 ++++++- cmd/trace-agent/config/remote/config_test.go | 29 ++++++++++++++++++- pkg/config/remote/service/service.go | 3 +- ...remote-config-errors-1e939be195e6db67.yaml | 5 ++++ 6 files changed, 47 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/fix-tracer-agent-forwards-remote-config-errors-1e939be195e6db67.yaml diff --git a/cmd/agent/subcommands/run/command.go b/cmd/agent/subcommands/run/command.go index b679a7fbd2d7a..54650cee453a3 100644 --- a/cmd/agent/subcommands/run/command.go +++ b/cmd/agent/subcommands/run/command.go @@ -481,8 +481,8 @@ func startAgent( configService, err = remoteconfig.NewService() if err != nil { log.Errorf("Failed to initialize config management service: %s", err) - } else if err := configService.Start(context.Background()); err != nil { - log.Errorf("Failed to start config management service: %s", err) + } else { + configService.Start(context.Background()) } if err := rcclient.Start("core-agent"); err != nil { diff --git a/cmd/cluster-agent/subcommands/start/command.go b/cmd/cluster-agent/subcommands/start/command.go index 2284f94a0f2fd..85dfb0523e0a5 100644 --- a/cmd/cluster-agent/subcommands/start/command.go +++ b/cmd/cluster-agent/subcommands/start/command.go @@ -444,9 +444,7 @@ func initializeRemoteConfig(ctx context.Context) (*remote.Client, error) { return nil, fmt.Errorf("unable to create remote-config service: %w", err) } - if err := configService.Start(ctx); err != nil { - return nil, fmt.Errorf("unable to start remote-config service: %w", err) - } + configService.Start(ctx) rcClient, err := remote.NewClient("cluster-agent", configService, version.AgentVersion, []data.Product{data.ProductAPMTracing}, time.Second*5) if err != nil { diff --git a/cmd/trace-agent/config/remote/config.go b/cmd/trace-agent/config/remote/config.go index 315e879f5fcbe..9f6e61854abfc 100644 --- a/cmd/trace-agent/config/remote/config.go +++ b/cmd/trace-agent/config/remote/config.go @@ -22,6 +22,8 @@ import ( "github.com/DataDog/datadog-agent/pkg/trace/metrics/timing" "github.com/DataDog/datadog-agent/pkg/trace/traceutil" "github.com/DataDog/datadog-agent/pkg/util/log" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) var bufferPool = sync.Pool{ @@ -74,7 +76,14 @@ func ConfigHandler(r *api.HTTPReceiver, client remote.ConfigUpdater, cfg *config } cfg, err := client.ClientGetConfigs(req.Context(), &configsRequest) if err != nil { - statusCode = http.StatusInternalServerError + if e, ok := status.FromError(err); ok { + switch e.Code() { + case codes.Unimplemented, codes.NotFound: + statusCode = http.StatusNotFound + } + } else { + statusCode = http.StatusInternalServerError + } http.Error(w, err.Error(), statusCode) return } diff --git a/cmd/trace-agent/config/remote/config_test.go b/cmd/trace-agent/config/remote/config_test.go index 4e56731211994..6c605bea2989b 100644 --- a/cmd/trace-agent/config/remote/config_test.go +++ b/cmd/trace-agent/config/remote/config_test.go @@ -22,6 +22,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func TestConfigEndpoint(t *testing.T) { @@ -153,6 +156,26 @@ func TestUpstreamRequest(t *testing.T) { } } +func TestForwardErrors(t *testing.T) { + assert := assert.New(t) + grpc := agentGRPCConfigFetcher{} + rcv := api.NewHTTPReceiver(config.New(), sampler.NewDynamicConfig(), make(chan *api.Payload, 5000), nil, telemetry.NewNoopCollector()) + + grpc.On("ClientGetConfigs", mock.Anything, mock.Anything, mock.Anything). + Return(nil, status.Error(codes.Unimplemented, "not implemented")) + + mux := http.NewServeMux() + mux.Handle("/v0.7/config", ConfigHandler(rcv, &grpc, &config.AgentConfig{})) + server := httptest.NewServer(mux) + + req, _ := http.NewRequest("POST", server.URL+"/v0.7/config", strings.NewReader(`{"client":{"id":"test_client","is_tracer":true,"client_tracer":{"service":"test","tags":["foo:bar"]}}}`)) + req.Header.Set("Datadog-Container-ID", "cid") + r, err := http.DefaultClient.Do(req) + assert.NoError(err) + assert.Equal(404, r.StatusCode) + r.Body.Close() +} + type agentGRPCConfigFetcher struct { pbgo.AgentSecureClient mock.Mock @@ -160,5 +183,9 @@ type agentGRPCConfigFetcher struct { func (a *agentGRPCConfigFetcher) ClientGetConfigs(ctx context.Context, in *pbgo.ClientGetConfigsRequest) (*pbgo.ClientGetConfigsResponse, error) { args := a.Called(ctx, in) - return args.Get(0).(*pbgo.ClientGetConfigsResponse), args.Error(1) + var maybeResponse *pbgo.ClientGetConfigsResponse + if args.Get(0) != nil { + maybeResponse = args.Get(0).(*pbgo.ClientGetConfigsResponse) + } + return maybeResponse, args.Error(1) } diff --git a/pkg/config/remote/service/service.go b/pkg/config/remote/service/service.go index a8d5b08d36c59..dfbf79feaa9b7 100644 --- a/pkg/config/remote/service/service.go +++ b/pkg/config/remote/service/service.go @@ -270,7 +270,7 @@ func newRCBackendOrgUUIDProvider(http api.API) uptane.OrgUUIDProvider { } // Start the remote configuration management service -func (s *Service) Start(ctx context.Context) error { +func (s *Service) Start(ctx context.Context) { ctx, cancel := context.WithCancel(ctx) s.cancel = cancel go func() { @@ -324,7 +324,6 @@ func (s *Service) Start(ctx context.Context) error { } } }() - return nil } func (s *Service) Stop() error { diff --git a/releasenotes/notes/fix-tracer-agent-forwards-remote-config-errors-1e939be195e6db67.yaml b/releasenotes/notes/fix-tracer-agent-forwards-remote-config-errors-1e939be195e6db67.yaml new file mode 100644 index 0000000000000..4d8884d5f042d --- /dev/null +++ b/releasenotes/notes/fix-tracer-agent-forwards-remote-config-errors-1e939be195e6db67.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + APM: Fixed trace-agent not forwarding errors from remote configuration and reporting them all as 500s +