feat: update filter interface to process only based on spans (#223)

* updating filter interface * backup * making a unified call to filter * updating intrumentations --------- Co-authored-by: Varkeychan Jacob <[email protected]>
hypertrace · Nov 22, 2023 · 08b9173 · 08b9173
1 parent ce4d775
commit 08b9173
Show file tree

Hide file tree

Showing 10 changed files with 62 additions and 207 deletions.
diff --git a/sdk/filter/filter.go b/sdk/filter/filter.go
@@ -7,12 +7,6 @@ import (
 
 // Filter evaluates whether request should be blocked, `true` blocks the request and `false` continues it.
 type Filter interface {
-	// EvaluateURLAndHeaders can be used to evaluate both URL and headers
-	EvaluateURLAndHeaders(span sdk.Span, url string, headers map[string][]string) result.FilterResult
-
-	// EvaluateBody can be used to evaluate the body content
-	EvaluateBody(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult
-
 	// Evaluate can be used to evaluate URL, headers and body content in one call
-	Evaluate(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult
+	Evaluate(span sdk.Span) result.FilterResult
 }
diff --git a/sdk/filter/multifilter.go b/sdk/filter/multifilter.go
@@ -17,32 +17,10 @@ func NewMultiFilter(filter ...Filter) *MultiFilter {
 	return &MultiFilter{filters: filter}
 }
 
-// EvaluateURLAndHeaders runs URL and headers evaluation for each filter until one returns true
-func (m *MultiFilter) EvaluateURLAndHeaders(span sdk.Span, url string, headers map[string][]string) result.FilterResult {
-	for _, f := range (*m).filters {
-		filterResult := f.EvaluateURLAndHeaders(span, url, headers)
-		if filterResult.Block {
-			return filterResult
-		}
-	}
-	return result.FilterResult{}
-}
-
-// EvaluateBody runs body evaluators for each filter until one returns true
-func (m *MultiFilter) EvaluateBody(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-	for _, f := range (*m).filters {
-		filterResult := f.EvaluateBody(span, body, headers)
-		if filterResult.Block {
-			return filterResult
-		}
-	}
-	return result.FilterResult{}
-}
-
 // Evaluate runs body evaluators for each filter until one returns true
-func (m *MultiFilter) Evaluate(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {
+func (m *MultiFilter) Evaluate(span sdk.Span) result.FilterResult {
 	for _, f := range (*m).filters {
-		filterResult := f.Evaluate(span, url, body, headers)
+		filterResult := f.Evaluate(span)
 		if filterResult.Block {
 			return filterResult
 		}

diff --git a/sdk/filter/multifilter_test.go b/sdk/filter/multifilter_test.go
@@ -11,11 +11,7 @@ import (
 
 func TestMultiFilterEmpty(t *testing.T) {
 	f := NewMultiFilter()
-	res := f.EvaluateURLAndHeaders(nil, "", nil)
-	assert.False(t, res.Block)
-	res = f.EvaluateBody(nil, nil, nil)
-	assert.False(t, res.Block)
-	res = f.Evaluate(nil, "", nil, nil)
+	res := f.Evaluate(nil)
 	assert.False(t, res.Block)
 }
 
@@ -26,65 +22,21 @@ func TestMultiFilterStopsAfterTrue(t *testing.T) {
 		expectedFilterResult              bool
 		multiFilter                       *MultiFilter
 	}{
-		"URL and Headers multi filter": {
-			expectedURLAndHeadersFilterResult: true,
-			expectedBodyFilterResult:          false,
-			expectedFilterResult:              false,
-			multiFilter: NewMultiFilter(
-				mock.Filter{
-					URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult {
-						return result.FilterResult{}
-					},
-				},
-				mock.Filter{
-					URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult {
-						return result.FilterResult{Block: true, ResponseStatusCode: 403}
-					},
-				},
-				mock.Filter{
-					URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult {
-						assert.Fail(t, "should not be called")
-						return result.FilterResult{}
-					},
-				},
-			),
-		},
-		"Body multi filter": {
-			expectedBodyFilterResult: true,
-			multiFilter: NewMultiFilter(
-				mock.Filter{
-					BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-						return result.FilterResult{}
-					},
-				},
-				mock.Filter{
-					BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-						return result.FilterResult{Block: true, ResponseStatusCode: 403}
-					},
-				},
-				mock.Filter{
-					BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-						assert.Fail(t, "should not be called")
-						return result.FilterResult{}
-					},
-				},
-			),
-		},
 		"Evaluate multi filter": {
 			expectedFilterResult: true,
 			multiFilter: NewMultiFilter(
 				mock.Filter{
-					Evaluator: func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {
+					Evaluator: func(span sdk.Span) result.FilterResult {
 						return result.FilterResult{}
 					},
 				},
 				mock.Filter{
-					Evaluator: func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {
+					Evaluator: func(span sdk.Span) result.FilterResult {
 						return result.FilterResult{Block: true, ResponseStatusCode: 403}
 					},
 				},
 				mock.Filter{
-					Evaluator: func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {
+					Evaluator: func(span sdk.Span) result.FilterResult {
 						assert.Fail(t, "should not be called")
 						return result.FilterResult{}
 					},
@@ -95,11 +47,7 @@ func TestMultiFilterStopsAfterTrue(t *testing.T) {
 
 	for name, tCase := range tCases {
 		t.Run(name, func(t *testing.T) {
-			res := tCase.multiFilter.EvaluateURLAndHeaders(nil, "", nil)
-			assert.Equal(t, tCase.expectedURLAndHeadersFilterResult, res.Block)
-			res = tCase.multiFilter.EvaluateBody(nil, nil, nil)
-			assert.Equal(t, tCase.expectedBodyFilterResult, res.Block)
-			res = tCase.multiFilter.Evaluate(nil, "", nil, nil)
+			res := tCase.multiFilter.Evaluate(nil)
 			assert.Equal(t, tCase.expectedFilterResult, res.Block)
 		})
 	}

diff --git a/sdk/filter/noop.go b/sdk/filter/noop.go
@@ -10,17 +10,7 @@ type NoopFilter struct{}
 
 var _ Filter = NoopFilter{}
 
-// EvaluateURLAndHeaders that always returns false
-func (NoopFilter) EvaluateURLAndHeaders(span sdk.Span, url string, headers map[string][]string) result.FilterResult {
-	return result.FilterResult{}
-}
-
-// EvaluateBody that always returns false
-func (NoopFilter) EvaluateBody(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-	return result.FilterResult{}
-}
-
 // Evaluate that always returns false
-func (NoopFilter) Evaluate(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {
+func (NoopFilter) Evaluate(span sdk.Span) result.FilterResult {
 	return result.FilterResult{}
 }
diff --git a/sdk/filter/noop_test.go b/sdk/filter/noop_test.go
@@ -8,10 +8,6 @@ import (
 
 func TestNoopFilter(t *testing.T) {
 	f := NoopFilter{}
-	res := f.EvaluateURLAndHeaders(nil, "", nil)
-	assert.False(t, res.Block)
-	res = f.EvaluateBody(nil, nil, nil)
-	assert.False(t, res.Block)
-	res = f.Evaluate(nil, "", nil, nil)
+	res := f.Evaluate(nil)
 	assert.False(t, res.Block)
 }
diff --git a/sdk/instrumentation/google.golang.org/grpc/server.go b/sdk/instrumentation/google.golang.org/grpc/server.go
@@ -12,7 +12,6 @@ import (
 	internalconfig "github.com/hypertrace/goagent/sdk/internal/config"
 	"github.com/hypertrace/goagent/sdk/internal/container"
 	"google.golang.org/grpc"
-	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/peer"
 	"google.golang.org/grpc/stats"
 	"google.golang.org/grpc/status"
@@ -94,33 +93,22 @@ func wrapHandler(
 
 		setSchemeAttributes(ctx, span)
 
+		if dataCaptureConfig.RpcMetadata.Request.Value {
+			setAttributesFromRequestIncomingMetadata(ctx, span)
+		}
+
 		reqBody, err := marshalMessageableJSON(req)
 		if dataCaptureConfig.RpcBody.Request.Value &&
 			len(reqBody) > 0 && err == nil {
 			setTruncatedBodyAttribute("request", reqBody, int(dataCaptureConfig.BodyMaxSizeBytes.Value), span)
 
-			if md, ok := metadata.FromIncomingContext(ctx); ok {
-				processingBody := reqBody
-				if int(dataCaptureConfig.BodyMaxProcessingSizeBytes.Value) < len(reqBody) {
-					processingBody = reqBody[:dataCaptureConfig.BodyMaxProcessingSizeBytes.Value]
-				}
-				filterResult := filter.EvaluateBody(span, processingBody, md)
-				if filterResult.Block {
-					return nil, status.Error(StatusCode(int(filterResult.ResponseStatusCode)), StatusText(int(filterResult.ResponseStatusCode)))
-				}
-			}
 		}
 
-		if dataCaptureConfig.RpcMetadata.Request.Value {
-			setAttributesFromRequestIncomingMetadata(ctx, span)
-
-			if md, ok := metadata.FromIncomingContext(ctx); ok {
-				// TODO: decide what should be passed as URL in GRPC
-				filterResult := filter.EvaluateURLAndHeaders(span, "", md)
-				if filterResult.Block {
-					return nil, status.Error(StatusCode(int(filterResult.ResponseStatusCode)), StatusText(int(filterResult.ResponseStatusCode)))
-				}
-			}
+		// TODO: decide what should be passed as URL in GRPC
+		// single evaluation call to filter after capturing the configured parameters
+		filterResult := filter.Evaluate(span)
+		if filterResult.Block {
+			return nil, status.Error(StatusCode(int(filterResult.ResponseStatusCode)), StatusText(int(filterResult.ResponseStatusCode)))
 		}
 
 		res, err := delegateHandler(ctx, req)

diff --git a/sdk/instrumentation/google.golang.org/grpc/server_test.go b/sdk/instrumentation/google.golang.org/grpc/server_test.go
@@ -2,6 +2,7 @@ package grpc
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	config "github.com/hypertrace/agent-config/gen/go/v1"
@@ -116,8 +117,8 @@ func TestServerInterceptorFilter(t *testing.T) {
 			expectedFilterResult: true,
 			expectedStatusCode:   codes.PermissionDenied,
 			multiFilter: filter.NewMultiFilter(mock.Filter{
-				URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult {
-					assert.Equal(t, []string{"test_value"}, headers["test_key"])
+				Evaluator: func(span sdk.Span) result.FilterResult {
+					assert.Equal(t, "test_value", fmt.Sprintf("%s", span.GetAttributes().GetValue("rpc.request.metadata.test_key")))
 					return result.FilterResult{Block: true, ResponseStatusCode: 403}
 				},
 			}),
@@ -126,8 +127,8 @@ func TestServerInterceptorFilter(t *testing.T) {
 			expectedFilterResult: true,
 			expectedStatusCode:   codes.PermissionDenied,
 			multiFilter: filter.NewMultiFilter(mock.Filter{
-				BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-					assert.Equal(t, "{\"name\":\"Pupo\"}", string(body))
+				Evaluator: func(span sdk.Span) result.FilterResult {
+					assert.Equal(t, "{\"name\":\"Pupo\"}", span.GetAttributes().GetValue("rpc.request.body"))
 					return result.FilterResult{Block: true, ResponseStatusCode: 403}
 				},
 			}),
@@ -136,8 +137,8 @@ func TestServerInterceptorFilter(t *testing.T) {
 			expectedFilterResult: true,
 			expectedStatusCode:   codes.FailedPrecondition,
 			multiFilter: filter.NewMultiFilter(mock.Filter{
-				BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-					assert.Equal(t, "{\"name\":\"Pupo\"}", string(body))
+				Evaluator: func(span sdk.Span) result.FilterResult {
+					assert.Equal(t, "{\"name\":\"Pupo\"}", span.GetAttributes().GetValue("rpc.request.body"))
 					return result.FilterResult{Block: true, ResponseStatusCode: 412}
 				},
 			}),
@@ -198,7 +199,7 @@ func TestServerInterceptorFilterWithMaxProcessingBodyLen(t *testing.T) {
 			RpcBody: &config.Message{
 				Request: config.Bool(true),
 			},
-			BodyMaxProcessingSizeBytes: config.Int32(1),
+			BodyMaxSizeBytes: config.Int32(1),
 		},
 	}
 	cfg.LoadFromEnv()
@@ -210,8 +211,9 @@ func TestServerInterceptorFilterWithMaxProcessingBodyLen(t *testing.T) {
 	s := grpc.NewServer(
 		grpc.UnaryInterceptor(
 			WrapUnaryServerInterceptor(mockUnaryInterceptor, mock.SpanFromContext, &Options{Filter: mock.Filter{
-				BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
-					assert.Equal(t, "{", string(body)) // body is truncated
+				Evaluator: func(span sdk.Span) result.FilterResult {
+					assert.Equal(t, true, span.GetAttributes().GetValue("rpc.request.body.truncated"))
+					assert.Equal(t, "{", span.GetAttributes().GetValue("rpc.request.body")) // body is truncated
 					return result.FilterResult{}
 				},
 			}}, nil),

diff --git a/sdk/instrumentation/net/http/handler.go b/sdk/instrumentation/net/http/handler.go
@@ -2,9 +2,7 @@ package http // import "github.com/hypertrace/goagent/sdk/instrumentation/net/ht
 
 import (
 	"bytes"
-	"encoding/base64"
 	"io"
-	"io/ioutil"
 	"net/http"
 
 	config "github.com/hypertrace/agent-config/gen/go/v1"
@@ -72,23 +70,15 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	host := r.Host
 	span.SetAttribute("http.request.header.host", host)
 
-	headers := r.Header
 	// Sets an attribute per each request header.
 	if h.dataCaptureConfig.HttpHeaders.Request.Value {
 		SetAttributesFromHeaders("request", NewHeaderMapAccessor(r.Header), span)
 	}
 
-	// run filters on headers
-	filterResult := h.filter.EvaluateURLAndHeaders(span, url, headers)
-	if filterResult.Block {
-		w.WriteHeader(int(filterResult.ResponseStatusCode))
-		return
-	}
-
 	// nil check for body is important as this block turns the body into another
 	// object that isn't nil and that will leverage the "Observer effect".
 	if r.Body != nil && h.dataCaptureConfig.HttpBody.Request.Value && ShouldRecordBodyOfContentType(headerMapAccessor{r.Header}) {
-		body, err := ioutil.ReadAll(r.Body)
+		body, err := io.ReadAll(r.Body)
 		if err != nil {
 			return
 		}
@@ -103,25 +93,14 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 				isMultipartFormDataBody)
 		}
 
-		processingBody := body
-		if int(h.dataCaptureConfig.BodyMaxProcessingSizeBytes.Value) < len(body) {
-			processingBody = body[:h.dataCaptureConfig.BodyMaxProcessingSizeBytes.Value]
-		}
-		// if body is multipart/form-data, base64 encode it before passing it on to the filter
-		if isMultipartFormDataBody {
-			origProcessingBody := processingBody
-			processingBody = make([]byte, base64.RawStdEncoding.EncodedLen(len(origProcessingBody)))
-			base64.RawStdEncoding.Encode(processingBody, origProcessingBody)
-		}
-
-		// run body filters
-		filterResult := h.filter.EvaluateBody(span, processingBody, headers)
-		if filterResult.Block {
-			w.WriteHeader(int(filterResult.ResponseStatusCode))
-			return
-		}
+		r.Body = io.NopCloser(bytes.NewBuffer(body))
+	}
 
-		r.Body = ioutil.NopCloser(bytes.NewBuffer(body))
+	// single evaluation call to filter after capturing the configured parameters
+	filterResult := h.filter.Evaluate(span)
+	if filterResult.Block {
+		w.WriteHeader(int(filterResult.ResponseStatusCode))
+		return
 	}
 
 	// create http.ResponseWriter interceptor for tracking status code