Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Nov 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdc0ea1
updating filter interface
Nov 16, 2023
0d20569
backup
Nov 17, 2023
2f56eb9
making a unified call to filter
Nov 17, 2023
e912b5b
updating intrumentations
Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions sdk/filter/filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
26 changes: 2 additions & 24 deletions sdk/filter/multifilter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/filter/multifilter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@

func TestMultiFilterEmpty(t *testing.T) {
f := NewMultiFilter()
res := f.EvaluateURLAndHeaders(nil, "", nil)

Check failure on line 14 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

f.EvaluateURLAndHeaders undefined (type *MultiFilter has no field or method EvaluateURLAndHeaders)

Check failure on line 14 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

f.EvaluateURLAndHeaders undefined (type *MultiFilter has no field or method EvaluateURLAndHeaders)
assert.False(t, res.Block)
res = f.EvaluateBody(nil, nil, nil)

Check failure on line 16 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

f.EvaluateBody undefined (type *MultiFilter has no field or method EvaluateBody)

Check failure on line 16 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

f.EvaluateBody undefined (type *MultiFilter has no field or method EvaluateBody)
assert.False(t, res.Block)
res = f.Evaluate(nil, "", nil, nil)
res = f.Evaluate(nil)
assert.False(t, res.Block)
}

Expand All @@ -32,17 +32,17 @@
expectedFilterResult: false,
multiFilter: NewMultiFilter(
mock.Filter{
URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult {

Check failure on line 35 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

unknown field URLAndHeadersEvaluator in struct literal of type mock.Filter

Check failure on line 35 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

unknown field URLAndHeadersEvaluator in struct literal of type mock.Filter
return result.FilterResult{}
},
},
mock.Filter{
URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult {

Check failure on line 40 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

unknown field URLAndHeadersEvaluator in struct literal of type mock.Filter

Check failure on line 40 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

unknown field URLAndHeadersEvaluator in struct literal of type mock.Filter
return result.FilterResult{Block: true, ResponseStatusCode: 403}
},
},
mock.Filter{
URLAndHeadersEvaluator: func(span sdk.Span, url string, headers map[string][]string) result.FilterResult {

Check failure on line 45 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

unknown field URLAndHeadersEvaluator in struct literal of type mock.Filter

Check failure on line 45 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

unknown field URLAndHeadersEvaluator in struct literal of type mock.Filter
assert.Fail(t, "should not be called")
return result.FilterResult{}
},
Expand All @@ -53,17 +53,17 @@
expectedBodyFilterResult: true,
multiFilter: NewMultiFilter(
mock.Filter{
BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {

Check failure on line 56 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

unknown field BodyEvaluator in struct literal of type mock.Filter

Check failure on line 56 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

unknown field BodyEvaluator in struct literal of type mock.Filter
return result.FilterResult{}
},
},
mock.Filter{
BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {

Check failure on line 61 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

unknown field BodyEvaluator in struct literal of type mock.Filter

Check failure on line 61 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

unknown field BodyEvaluator in struct literal of type mock.Filter
return result.FilterResult{Block: true, ResponseStatusCode: 403}
},
},
mock.Filter{
BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {

Check failure on line 66 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

unknown field BodyEvaluator in struct literal of type mock.Filter

Check failure on line 66 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

unknown field BodyEvaluator in struct literal of type mock.Filter
assert.Fail(t, "should not be called")
return result.FilterResult{}
},
Expand All @@ -74,12 +74,12 @@
expectedFilterResult: true,
multiFilter: NewMultiFilter(
mock.Filter{
Evaluator: func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {

Check failure on line 77 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

cannot use func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {…} (value of type func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult) as func(span sdk.Span) result.FilterResult value in struct literal

Check failure on line 77 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

cannot use func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {…} (value of type func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult) as func(span sdk.Span) result.FilterResult value in struct literal
return result.FilterResult{}
},
},
mock.Filter{
Evaluator: func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {

Check failure on line 82 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.20) 

cannot use func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {…} (value of type func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult) as func(span sdk.Span) result.FilterResult value in struct literal

Check failure on line 82 in sdk/filter/multifilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.21) 

cannot use func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult {…} (value of type func(span sdk.Span, url string, body []byte, headers map[string][]string) result.FilterResult) as func(span sdk.Span) result.FilterResult value in struct literal
return result.FilterResult{Block: true, ResponseStatusCode: 403}
},
},
Expand All @@ -99,7 +99,7 @@
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)
})
}
Expand Down
12 changes: 1 addition & 11 deletions sdk/filter/noop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
}
2 changes: 1 addition & 1 deletion sdk/filter/noop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ func TestNoopFilter(t *testing.T) {
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)
}
31 changes: 12 additions & 19 deletions sdk/instrumentation/google.golang.org/grpc/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -94,32 +93,26 @@ func wrapHandler(

setSchemeAttributes(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 dataCaptureConfig.RpcMetadata.Request.Value {
Copy link
Contributor Author

@varkey98 varkey98 Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanericson @puneet-traceable @tim-mwangi This reordering was required because we now only have a single method in filter interface and the scenario of blocking based on headers was failing since the only call to filter was done on body first.

I think we should make similar changes to all instrumentations so that our issue of allow rule not taking precedence over blocking rules also will be solved.
Or we can pass flowvariable to Evaluate method to specify if we're calling for header eval or body eval

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So libtraceable interface only has one method called Evaluate now? Do the conditions that had it split up into various methods no longer apply?

Copy link
Contributor Author

@varkey98 varkey98 Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, libtraceable interface has only one method Evaluate now.

Do the conditions that had it split up into various methods no longer apply?

This I'm not really sure. I looked through the code and couldn't find any reason why we had separate calls (maybe for consistency across agents). In both the cases we call the delegate after the body capture. I'll test with actual apps over the weekend.

Copy link
Contributor Author

@varkey98 varkey98 Nov 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tim-mwangi I tested with both a net/http app and and a grpc app, both of them are blocking with and w/o body, testing the remaining instrumentations as well

setAttributesFromRequestIncomingMetadata(ctx, 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)
// TODO: decide what should be passed as URL in GRPC
if !dataCaptureConfig.RpcBody.Request.Value {
filterResult := filter.Evaluate(span)
if filterResult.Block {
return nil, status.Error(StatusCode(int(filterResult.ResponseStatusCode)), StatusText(int(filterResult.ResponseStatusCode)))
}
}
}

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 {
// 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)))
}
filterResult := filter.Evaluate(span)
if filterResult.Block {
return nil, status.Error(StatusCode(int(filterResult.ResponseStatusCode)), StatusText(int(filterResult.ResponseStatusCode)))
}
}

Expand Down
20 changes: 11 additions & 9 deletions sdk/instrumentation/google.golang.org/grpc/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package grpc

import (
"context"
"fmt"
"testing"

config "github.com/hypertrace/agent-config/gen/go/v1"
Expand Down Expand Up @@ -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}
},
}),
Expand All @@ -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}
},
}),
Expand All @@ -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}
},
}),
Expand Down Expand Up @@ -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()
Expand All @@ -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),
Expand Down
17 changes: 2 additions & 15 deletions sdk/instrumentation/net/http/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package http // import "github.com/hypertrace/goagent/sdk/instrumentation/net/ht

import (
"bytes"
"encoding/base64"
"io"
"io/ioutil"
"net/http"
Expand Down Expand Up @@ -72,14 +71,13 @@ 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)
filterResult := h.filter.Evaluate(span)
if filterResult.Block {
w.WriteHeader(int(filterResult.ResponseStatusCode))
return
Expand All @@ -103,19 +101,8 @@ 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]
Copy link
Contributor Author

@varkey98 varkey98 Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataCaptureConfig.BodyMaxProcessingSizeBytes config is also becoming obsolete as we are using the body stored as span attribute for filter evaluation which uses BodyMaxSizeBytes

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm ok with that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help open a ticket to deprecate body max processing size throughout? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, :https://traceableai.atlassian.net/browse/ENG-37604

}
// 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)
filterResult := h.filter.Evaluate(span)
if filterResult.Block {
w.WriteHeader(int(filterResult.ResponseStatusCode))
return
Expand Down
22 changes: 3 additions & 19 deletions sdk/internal/mock/filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Loading