From 08b9173812d25fe6bcd5cf3000e3e723a0f426ba Mon Sep 17 00:00:00 2001 From: Varkeychan Jacob <53667828+varkey98@users.noreply.github.com> Date: Thu, 23 Nov 2023 01:05:55 +0530 Subject: [PATCH] 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 --- sdk/filter/filter.go | 8 +-- sdk/filter/multifilter.go | 26 +------- sdk/filter/multifilter_test.go | 62 ++----------------- sdk/filter/noop.go | 12 +--- sdk/filter/noop_test.go | 6 +- .../google.golang.org/grpc/server.go | 30 +++------ .../google.golang.org/grpc/server_test.go | 20 +++--- sdk/instrumentation/net/http/handler.go | 37 +++-------- sdk/instrumentation/net/http/handler_test.go | 46 +++++++------- sdk/internal/mock/filter.go | 22 +------ 10 files changed, 62 insertions(+), 207 deletions(-) diff --git a/sdk/filter/filter.go b/sdk/filter/filter.go index 6a9e672..f1fa31d 100644 --- 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 index 38add9c..55242e1 100644 --- 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 index 8309694..afdb557 100644 --- 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 index 1e165dc..12372b7 100644 --- 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 index db58b82..c2373fa 100644 --- 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 index cc406e2..a3eeaed 100644 --- 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 index 5707035..c192f50 100644 --- 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 index 646530b..a454d7a 100644 --- 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 diff --git a/sdk/instrumentation/net/http/handler_test.go b/sdk/instrumentation/net/http/handler_test.go index 32b059c..36147eb 100644 --- a/sdk/instrumentation/net/http/handler_test.go +++ b/sdk/instrumentation/net/http/handler_test.go @@ -370,14 +370,10 @@ func TestServerRequestFilter(t *testing.T) { body: "haha", options: &Options{ Filter: mock.Filter{ - URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult { - assert.Equal(t, "http://localhost/foo", url) - assert.Equal(t, 1, len(headers)) - assert.Equal(t, []string{"application/json"}, headers["Content-Type"]) - return result.FilterResult{} - }, - BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult { - assert.Equal(t, []byte("haha"), body) + Evaluator: func(span sdk.Span) result.FilterResult { + assert.Equal(t, "http://localhost/foo", span.GetAttributes().GetValue("http.url")) + assert.Equal(t, "application/json", span.GetAttributes().GetValue("http.request.header.content-type")) + assert.Equal(t, "haha", span.GetAttributes().GetValue("http.request.body")) return result.FilterResult{} }, }, @@ -390,14 +386,12 @@ func TestServerRequestFilter(t *testing.T) { body: "haha", options: &Options{ Filter: mock.Filter{ - URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult { - assert.Equal(t, "http://localhost/foo", url) - assert.Equal(t, 1, len(headers)) - assert.Equal(t, []string{"multipart/form-data"}, headers["Content-Type"]) - return result.FilterResult{} - }, - BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult { - assert.Equal(t, []byte(base64.RawStdEncoding.EncodeToString([]byte("haha"))), body) + Evaluator: func(span sdk.Span) result.FilterResult { + assert.Equal(t, "http://localhost/foo", span.GetAttributes().GetValue("http.url")) + assert.Equal(t, "multipart/form-data", span.GetAttributes().GetValue("http.request.header.content-type")) + assert.Equal(t, base64.RawStdEncoding.EncodeToString([]byte("haha")), + span.GetAttributes().GetValue("http.request.body.base64")) + return result.FilterResult{} }, }, @@ -411,10 +405,11 @@ func TestServerRequestFilter(t *testing.T) { body: "haha", options: &Options{ Filter: mock.Filter{ - URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult { - assert.Equal(t, "http://localhost/foo", url) - assert.Equal(t, 1, len(headers)) - assert.Equal(t, []string{"multipart/form-data"}, headers["Content-Type"]) + Evaluator: func(span sdk.Span) result.FilterResult { + assert.Equal(t, "http://localhost/foo", span.GetAttributes().GetValue("http.url")) + assert.Equal(t, "multipart/form-data", span.GetAttributes().GetValue("http.request.header.content-type")) + assert.Nil(t, span.GetAttributes().GetValue("http.request.body")) + assert.Nil(t, span.GetAttributes().GetValue("http.request.body.base64")) return result.FilterResult{} }, }, @@ -425,7 +420,7 @@ func TestServerRequestFilter(t *testing.T) { url: "http://localhost/foo", options: &Options{ Filter: mock.Filter{ - URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult { + Evaluator: func(span sdk.Span) result.FilterResult { return result.FilterResult{Block: true, ResponseStatusCode: 403} }, }, @@ -436,7 +431,7 @@ func TestServerRequestFilter(t *testing.T) { url: "http://localhost/foo", options: &Options{ Filter: mock.Filter{ - URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult { + Evaluator: func(span sdk.Span) result.FilterResult { return result.FilterResult{Block: true, ResponseStatusCode: 403} }, }, @@ -450,7 +445,7 @@ func TestServerRequestFilter(t *testing.T) { body: "haha", options: &Options{ Filter: mock.Filter{ - BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult { + Evaluator: func(span sdk.Span) result.FilterResult { return result.FilterResult{Block: true, ResponseStatusCode: 403} }, }, @@ -501,8 +496,8 @@ func TestProcessingBodyIsTrimmed(t *testing.T) { wh, _ := WrapHandler(h, 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, "{", span.GetAttributes().GetValue("http.request.body")) // body is truncated return result.FilterResult{Block: true, ResponseStatusCode: 403} }, }, @@ -510,6 +505,7 @@ func TestProcessingBodyIsTrimmed(t *testing.T) { wh.dataCaptureConfig = emptyTestConfig wh.dataCaptureConfig.HttpBody.Request = config.Bool(true) wh.dataCaptureConfig.BodyMaxProcessingSizeBytes = config.Int32(int32(bodyMaxProcessingSizeBytes)) + wh.dataCaptureConfig.BodyMaxSizeBytes = config.Int32(int32(bodyMaxProcessingSizeBytes)) ih := &mockHandler{baseHandler: wh} diff --git a/sdk/internal/mock/filter.go b/sdk/internal/mock/filter.go index 2b4aef1..79eeec3 100644 --- a/sdk/internal/mock/filter.go +++ b/sdk/internal/mock/filter.go @@ -6,28 +6,12 @@ import ( ) type Filter struct { - URLAndHeadersEvaluator func(span sdk.Span, url string, headers map[string][]string) result.FilterResult - BodyEvaluator func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult - Evaluator func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult + Evaluator func(span sdk.Span) result.FilterResult } -func (f Filter) EvaluateURLAndHeaders(span sdk.Span, url string, headers map[string][]string) result.FilterResult { - if f.URLAndHeadersEvaluator == nil { - return result.FilterResult{} - } - return f.URLAndHeadersEvaluator(span, url, headers) -} - -func (f Filter) EvaluateBody(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult { - if f.BodyEvaluator == nil { - return result.FilterResult{} - } - return f.BodyEvaluator(span, body, headers) -} - -func (f Filter) Evaluate(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult { +func (f Filter) Evaluate(span sdk.Span) result.FilterResult { if f.Evaluator == nil { return result.FilterResult{} } - return f.Evaluator(span, url, body, headers) + return f.Evaluator(span) }