Skip to content

Commit

Permalink
[backport] Fix trace agent error forwarding (#21024)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>

* Added Unit Test

* Close body

* Remove returned error handling

---------

Co-authored-by: Ahmed Mezghani <[email protected]>
  • Loading branch information
mellon85 and ahmed-mez authored Nov 22, 2023
1 parent 478f523 commit 412a286
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 9 deletions.
4 changes: 2 additions & 2 deletions cmd/agent/subcommands/run/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,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 {
Expand Down
4 changes: 1 addition & 3 deletions cmd/cluster-agent/subcommands/start/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,9 +430,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 {
Expand Down
11 changes: 10 additions & 1 deletion cmd/trace-agent/config/remote/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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
}
Expand Down
29 changes: 28 additions & 1 deletion cmd/trace-agent/config/remote/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -153,12 +156,36 @@ 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
}

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)
}
3 changes: 1 addition & 2 deletions pkg/config/remote/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -324,7 +324,6 @@ func (s *Service) Start(ctx context.Context) error {
}
}
}()
return nil
}

func (s *Service) Stop() error {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
fixes:
- |
APM: Fixed trace-agent not forwarding errors from remote configuration and reporting them all as 500s

0 comments on commit 412a286

Please sign in to comment.