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 all 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
62 changes: 5 additions & 57 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,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)
}

Expand All @@ -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{}
},
Expand All @@ -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)
})
}
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{}
}
6 changes: 1 addition & 5 deletions sdk/filter/noop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
30 changes: 9 additions & 21 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,33 +93,22 @@ func wrapHandler(

setSchemeAttributes(ctx, 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)
}

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)
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
37 changes: 8 additions & 29 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,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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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]
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)
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
Expand Down
Loading
Loading