Apply comments

contrib/envoyproxy/envoy/envoy.go

@@ -17,6 +17,10 @@ import (
 	"strings"
 	"sync/atomic"
 
+	corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
+	v3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
+	extproc "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3"
+	v32 "github.com/envoyproxy/go-control-plane/envoy/type/v3"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
@@ -25,12 +29,6 @@ import (
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/waf/actions"
-	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/trace"
-
-	corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
-	v3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
-	extproc "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3"
-	v32 "github.com/envoyproxy/go-control-plane/envoy/type/v3"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
@@ -87,9 +85,7 @@ func StreamServerInterceptor(opts ...grpctrace.Option) grpc.StreamServerIntercep
 		}
 
 		// Close the span when the request is done processing
-		defer func() {
-			closeSpan(currentRequest)
-		}()
+		defer closeSpan(currentRequest)
 
 		for {
 			select {
@@ -104,8 +100,7 @@ func StreamServerInterceptor(opts ...grpctrace.Option) grpc.StreamServerIntercep
 			}
 
 			var req extproc.ProcessingRequest
-			err := ss.RecvMsg(&req)
-			if err != nil {
+			if err := ss.RecvMsg(&req); err != nil {
 				// Note: Envoy is inconsistent with the "end_of_stream" value of its headers responses,
 				// so we can't fully rely on it to determine when it will close (cancel) the stream.
 				if err == io.EOF || err.(interface{ GRPCStatus() *status.Status }).GRPCStatus().Code() == codes.Canceled {
@@ -222,7 +217,7 @@ func ProcessRequestHeaders(ctx context.Context, req *extproc.ProcessingRequest_R
 	currentRequest.op, currentRequest.blockAction, _ = httpsec.StartOperation(ctx, currentRequest.requestArgs)
 
 	// Block handling: If triggered, we need to block the request, return an immediate response
-	if blockPtr := currentRequest.blockAction.Load(); blockPtr != nil {
+	if blockPtr := currentRequest.blockAction.Swap(nil); blockPtr != nil {
 		response := doBlockRequest(currentRequest, blockPtr, headers)
 		return response, nil
 	}
@@ -244,7 +239,7 @@ func verifyRequestHttp2RequestHeaders(headers map[string][]string) (string, stri
 	// :authority, :scheme, :path, :method
 
 	for _, header := range []string{":authority", ":scheme", ":path", ":method"} {
-		if _, ok := headers[header]; !ok {
+		if _, ok := headers[header]; !ok || len(headers[header]) == 0 {
 			return "", "", "", "", status.Errorf(codes.InvalidArgument, "Missing required header: %v", header)
 		}
 	}
@@ -255,7 +250,7 @@ func verifyRequestHttp2RequestHeaders(headers map[string][]string) (string, stri
 func verifyRequestHttp2ResponseHeaders(headers map[string][]string) (string, error) {
 	// :status
 
-	if _, ok := headers[":status"]; !ok {
+	if _, ok := headers[":status"]; !ok || len(headers[":status"]) == 0 {
 		return "", status.Errorf(codes.InvalidArgument, "Missing required header: %v", ":status")
 	}
 
@@ -286,12 +281,10 @@ func ProcessResponseHeaders(res *extproc.ProcessingRequest_ResponseHeaders, curr
 	currentRequest.op = nil
 
 	// Block handling: If triggered, we need to block the request, return an immediate response
-	if blockPtr := currentRequest.blockAction.Load(); blockPtr != nil {
+	if blockPtr := currentRequest.blockAction.Swap(nil); blockPtr != nil {
 		return doBlockRequest(currentRequest, blockPtr, headers), nil
 	}
 
-	httpsec2.SetResponseHeadersTags(currentRequest.span, headers)
-
 	// Note: (cf. comment in the stream error handling)
 	// The end of stream bool value is not reliable
 	if res.ResponseHeaders.GetEndOfStream() {
@@ -311,7 +304,7 @@ func ProcessResponseHeaders(res *extproc.ProcessingRequest_ResponseHeaders, curr
 
 func createExternalProcessedSpan(ctx context.Context, headers map[string][]string, method string, host string, path string, remoteAddr string, ipTags map[string]string, parsedUrl *url.URL) tracer.Span {
 	userAgent := ""
-	if ua, ok := headers["User-Agent"]; ok {
+	if ua, ok := headers["User-Agent"]; ok || len(ua) > 0 {
 		userAgent = ua[0]
 	}
 
@@ -336,9 +329,6 @@ func createExternalProcessedSpan(ctx context.Context, headers map[string][]strin
 		}...,
 	)
 
-	httpsec2.SetRequestHeadersTags(span, headers)
-	trace.SetAppsecStaticTags(span)
-
 	return span
 }
 
@@ -350,6 +340,10 @@ func separateEnvoyHeaders(receivedHeaders []*corev3.HeaderValue) (map[string][]s
 	pseudoHeadersHttp2 := make(map[string][]string)
 	for _, v := range receivedHeaders {
 		key := v.GetKey()
+		if len(key) == 0 {
+			continue
+		}
+
 		if key[0] == ':' {
 			pseudoHeadersHttp2[key] = []string{string(v.GetRawValue())}
 		} else {
@@ -386,7 +380,6 @@ func doBlockRequest(currentRequest *CurrentRequest, blockAction *actions.BlockHT
 		})
 	}
 
-	httpsec2.SetResponseHeadersTags(currentRequest.span, headerToSet)
 	currentRequest.statusCode = blockAction.StatusCode
 	var int32StatusCode int32 = 0
 	if currentRequest.statusCode > 0 && currentRequest.statusCode <= math.MaxInt32 {

contrib/envoyproxy/envoy/envoy_test.go

@@ -385,7 +385,6 @@ func TestGeneratedSpan(t *testing.T) {
 		require.Equal(t, "GET", span.Tag("http.method"))
 		require.Equal(t, "datadoghq.com", span.Tag("http.host"))
 		require.Equal(t, "GET /resource-span", span.Tag("resource.name"))
-		require.Equal(t, "datadoghq.com", span.Tag("http.request.headers.host"))
 		require.Equal(t, "server", span.Tag("span.kind"))
 		require.Equal(t, "Mistake Not...", span.Tag("http.useragent"))
 	})

internal/appsec/emitter/httpsec/http.go

@@ -208,7 +208,6 @@ func MakeHandlerOperationArgs(headers map[string][]string, method string, host s
 		PathParams:  map[string]string{},
 	}
 
-	args.Headers["host"] = []string{host}
 	return args
 }
 

internal/appsec/listener/httpsec/http.go

@@ -59,7 +59,7 @@ func (feature *Feature) OnRequest(op *httpsec.HandlerOperation, args httpsec.Han
 	headers := headersRemoveCookies(args.Headers)
 	headers["host"] = []string{args.Host}
 
-	SetRequestHeadersTags(op, headers)
+	setRequestHeadersTags(op, headers)
 
 	op.Run(op,
 		addresses.NewAddressesBuilder().
@@ -76,7 +76,7 @@ func (feature *Feature) OnRequest(op *httpsec.HandlerOperation, args httpsec.Han
 
 func (feature *Feature) OnResponse(op *httpsec.HandlerOperation, resp httpsec.HandlerOperationRes) {
 	headers := headersRemoveCookies(resp.Headers)
-	SetResponseHeadersTags(op, headers)
+	setResponseHeadersTags(op, headers)
 
 	builder := addresses.NewAddressesBuilder().
 		WithResponseHeadersNoCookies(headers).

internal/appsec/listener/httpsec/request.go

@@ -149,13 +149,13 @@ func readMonitoredClientIPHeadersConfig() {
 	}
 }
 
-// SetRequestHeadersTags sets the AppSec-specific request headers span tags.
-func SetRequestHeadersTags(span trace.TagSetter, headers map[string][]string) {
+// setRequestHeadersTags sets the AppSec-specific request headers span tags.
+func setRequestHeadersTags(span trace.TagSetter, headers map[string][]string) {
 	setHeadersTags(span, "http.request.headers.", headers)
 }
 
-// SetResponseHeadersTags sets the AppSec-specific response headers span tags.
-func SetResponseHeadersTags(span trace.TagSetter, headers map[string][]string) {
+// setResponseHeadersTags sets the AppSec-specific response headers span tags.
+func setResponseHeadersTags(span trace.TagSetter, headers map[string][]string) {
 	setHeadersTags(span, "http.response.headers.", headers)
 }
 

internal/appsec/listener/httpsec/request_test.go

@@ -215,8 +215,8 @@ func TestTags(t *testing.T) {
 						return
 					}
 					require.NoError(t, err)
-					SetRequestHeadersTags(&span, reqHeadersCase.headers)
-					SetResponseHeadersTags(&span, respHeadersCase.headers)
+					setRequestHeadersTags(&span, reqHeadersCase.headers)
+					setResponseHeadersTags(&span, respHeadersCase.headers)
 
 					if eventCase.events != nil {
 						require.Subset(t, span.Tags, map[string]interface{}{

internal/appsec/listener/trace/trace.go

@@ -6,7 +6,6 @@
 package trace
 
 import (
-	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/config"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/trace"
@@ -20,12 +19,6 @@ var staticAppsecTags = map[string]any{
 	"_dd.runtime_family": "go",
 }
 
-func SetAppsecStaticTags(span ddtrace.Span) {
-	for key, value := range staticAppsecTags {
-		span.SetTag(key, value)
-	}
-}
-
 type AppsecSpanTransport struct{}
 
 func (*AppsecSpanTransport) String() string {