diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go index 29f1cd5415..f01b1be948 100644 --- a/filters/openpolicyagent/evaluation.go +++ b/filters/openpolicyagent/evaluation.go @@ -11,7 +11,6 @@ import ( "github.com/open-policy-agent/opa/ast" "github.com/open-policy-agent/opa/rego" "github.com/open-policy-agent/opa/server" - "github.com/open-policy-agent/opa/tracing" "github.com/opentracing/opentracing-go" ) @@ -58,7 +57,7 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3. return nil, err } - err = envoyauth.Eval(ctx, opa, inputValue, result, rego.DistributedTracingOpts(tracing.Options{opa})) + err = envoyauth.Eval(ctx, opa, inputValue, result, rego.DistributedTracingOpts(opa.DistributedTracing())) if err != nil { return nil, err } diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index e95836c3ec..f0850dec79 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -19,6 +19,7 @@ import ( "github.com/zalando/skipper/filters" "github.com/zalando/skipper/metrics/metricstest" "github.com/zalando/skipper/proxy/proxytest" + "github.com/zalando/skipper/tracing/tracingtest" "github.com/zalando/skipper/filters/filtertest" "github.com/zalando/skipper/filters/openpolicyagent" @@ -359,7 +360,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { } }`, opaControlPlane.URL(), ti.regoQuery)) - opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry() + opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{})) ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) fr.Register(ftSpec) ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) diff --git a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go index 5ffe0b41a1..0951cdf3c7 100644 --- a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go +++ b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go @@ -14,6 +14,7 @@ import ( "github.com/zalando/skipper/eskip" "github.com/zalando/skipper/filters" "github.com/zalando/skipper/proxy/proxytest" + "github.com/zalando/skipper/tracing/tracingtest" "github.com/zalando/skipper/filters/openpolicyagent" ) @@ -239,7 +240,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { } }`, opaControlPlane.URL(), ti.regoQuery)) - opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry() + opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{})) ftSpec := NewOpaServeResponseSpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) fr.Register(ftSpec) ftSpec = NewOpaServeResponseWithReqBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) diff --git a/filters/openpolicyagent/openpolicyagent.go b/filters/openpolicyagent/openpolicyagent.go index 20e7dd269e..2970860493 100644 --- a/filters/openpolicyagent/openpolicyagent.go +++ b/filters/openpolicyagent/openpolicyagent.go @@ -47,6 +47,8 @@ const ( DefaultMaxRequestBodySize = 1 << 20 // 1 MB DefaultMaxMemoryBodyParsing = 100 * DefaultMaxRequestBodySize defaultBodyBufferSize = 8192 * 1024 + + spanNameEval = "open-policy-agent" ) type OpenPolicyAgentRegistry struct { @@ -69,6 +71,8 @@ type OpenPolicyAgentRegistry struct { maxMemoryBodyParsingSem *semaphore.Weighted maxRequestBodyBytes int64 bodyReadBufferSize int64 + + tracer opentracing.Tracer } type OpenPolicyAgentFilter interface { @@ -110,6 +114,13 @@ func WithCleanInterval(interval time.Duration) func(*OpenPolicyAgentRegistry) er } } +func WithTracer(tracer opentracing.Tracer) func(*OpenPolicyAgentRegistry) error { + return func(cfg *OpenPolicyAgentRegistry) error { + cfg.tracer = tracer + return nil + } +} + func NewOpenPolicyAgentRegistry(opts ...func(*OpenPolicyAgentRegistry) error) *OpenPolicyAgentRegistry { registry := &OpenPolicyAgentRegistry{ reuseDuration: defaultReuseDuration, @@ -395,6 +406,22 @@ func interpolateConfigTemplate(configTemplate []byte, bundleName string) ([]byte return buf.Bytes(), nil } +func buildTracingOptions(tracer opentracing.Tracer, bundleName string, manager *plugins.Manager) opatracing.Options { + return opatracing.NewOptions(WithTracingOptTracer(tracer), WithTracingOptBundleName(bundleName), WithTracingOptManager(manager)) +} + +func (registry *OpenPolicyAgentRegistry) withTracingOptions(bundleName string) func(*plugins.Manager) { + return func(m *plugins.Manager) { + options := buildTracingOptions( + registry.tracer, + bundleName, + m, + ) + + plugins.WithDistributedTracingOpts(options)(m) + } +} + // new returns a new OPA object. func (registry *OpenPolicyAgentRegistry) new(store storage.Store, configBytes []byte, instanceConfig OpenPolicyAgentInstanceConfig, filterName string, bundleName string, maxBodyBytes int64, bodyReadBufferSize int64) (*OpenPolicyAgentInstance, error) { id := uuid.New().String() @@ -412,7 +439,8 @@ func (registry *OpenPolicyAgentRegistry) new(store storage.Store, configBytes [] var logger logging.Logger = &QuietLogger{target: logging.Get()} logger = logger.WithFields(map[string]interface{}{"skipper-filter": filterName}) - manager, err := plugins.New(configBytes, id, store, configLabelsInfo(*opaConfig), plugins.Logger(logger)) + manager, err := plugins.New(configBytes, id, store, configLabelsInfo(*opaConfig), plugins.Logger(logger), registry.withTracingOptions(bundleName)) + if err != nil { return nil, err } @@ -544,20 +572,28 @@ func (opa *OpenPolicyAgentInstance) EnvoyPluginConfig() envoy.PluginConfig { return defaultConfig } +func setSpanTags(span opentracing.Span, bundleName string, manager *plugins.Manager) { + if bundleName != "" { + span.SetTag("opa.bundle_name", bundleName) + } + + if manager != nil { + for label, value := range manager.Labels() { + span.SetTag("opa.label."+label, value) + } + } +} + func (opa *OpenPolicyAgentInstance) startSpanFromContextWithTracer(tr opentracing.Tracer, parent opentracing.Span, ctx context.Context) (opentracing.Span, context.Context) { var span opentracing.Span if parent != nil { - span = tr.StartSpan("open-policy-agent", opentracing.ChildOf(parent.Context())) + span = tr.StartSpan(spanNameEval, opentracing.ChildOf(parent.Context())) } else { - span = tracing.CreateSpan("open-policy-agent", ctx, tr) + span = tracing.CreateSpan(spanNameEval, ctx, tr) } - span.SetTag("opa.bundle_name", opa.bundleName) - - for label, value := range opa.manager.Labels() { - span.SetTag("opa.label."+label, value) - } + setSpanTags(span, opa.bundleName, opa.manager) return span, opentracing.ContextWithSpan(ctx, span) } @@ -730,7 +766,7 @@ func (opa *OpenPolicyAgentInstance) Config() *config.Config { return opa.opaConf // DistributedTracing is an implementation of the envoyauth.EvalContext interface func (opa *OpenPolicyAgentInstance) DistributedTracing() opatracing.Options { - return opatracing.NewOptions(opa) + return buildTracingOptions(opa.registry.tracer, opa.bundleName, opa.manager) } // logging.Logger that does not pollute info with debug logs diff --git a/filters/openpolicyagent/tracing.go b/filters/openpolicyagent/tracing.go index d0b6b60244..bc63c780b9 100644 --- a/filters/openpolicyagent/tracing.go +++ b/filters/openpolicyagent/tracing.go @@ -3,8 +3,15 @@ package openpolicyagent import ( "net/http" + "github.com/open-policy-agent/opa/plugins" opatracing "github.com/open-policy-agent/opa/tracing" "github.com/opentracing/opentracing-go" + "github.com/zalando/skipper/logging" + "github.com/zalando/skipper/proxy" +) + +const ( + spanNameHttpOut = "open-policy-agent.http" ) func init() { @@ -14,15 +21,48 @@ func init() { type tracingFactory struct{} type transport struct { - opa *OpenPolicyAgentInstance + tracer opentracing.Tracer + bundleName string + manager *plugins.Manager + wrapped http.RoundTripper } +func WithTracingOptTracer(tracer opentracing.Tracer) func(*transport) { + return func(t *transport) { + t.tracer = tracer + } +} + +func WithTracingOptBundleName(bundleName string) func(*transport) { + return func(t *transport) { + t.bundleName = bundleName + } +} + +func WithTracingOptManager(manager *plugins.Manager) func(*transport) { + return func(t *transport) { + t.manager = manager + } +} + func (*tracingFactory) NewTransport(tr http.RoundTripper, opts opatracing.Options) http.RoundTripper { - return &transport{ - opa: opts[0].(*OpenPolicyAgentInstance), + log := &logging.DefaultLog{} + + wrapper := &transport{ wrapped: tr, } + + for _, o := range opts { + opt, ok := o.(func(*transport)) + if !ok { + log.Warnf("invalid type for OPA tracing option, expected func(*transport) got %T, tracing information might be incomplete", o) + } else { + opt(wrapper) + } + } + + return wrapper } func (*tracingFactory) NewHandler(f http.Handler, label string, opts opatracing.Options) http.Handler { @@ -32,15 +72,36 @@ func (*tracingFactory) NewHandler(f http.Handler, label string, opts opatracing. func (tr *transport) RoundTrip(req *http.Request) (*http.Response, error) { ctx := req.Context() parentSpan := opentracing.SpanFromContext(ctx) + var span opentracing.Span if parentSpan != nil { - span := parentSpan.Tracer().StartSpan("http.send", opentracing.ChildOf(parentSpan.Context())) + span = parentSpan.Tracer().StartSpan(spanNameHttpOut, opentracing.ChildOf(parentSpan.Context())) + } else if tr.tracer != nil { + span = tr.tracer.StartSpan(spanNameHttpOut) + } + + if span != nil { defer span.Finish() + + span.SetTag(proxy.HTTPMethodTag, req.Method) + span.SetTag(proxy.HTTPUrlTag, req.URL.String()) + span.SetTag(proxy.HostnameTag, req.Host) + span.SetTag(proxy.HTTPPathTag, req.URL.Path) + span.SetTag(proxy.ComponentTag, "skipper") + span.SetTag(proxy.SpanKindTag, proxy.SpanKindClient) + + setSpanTags(span, tr.bundleName, tr.manager) req = req.WithContext(opentracing.ContextWithSpan(ctx, span)) carrier := opentracing.HTTPHeadersCarrier(req.Header) span.Tracer().Inject(span.Context(), opentracing.HTTPHeaders, carrier) } - return tr.wrapped.RoundTrip(req) + resp, err := tr.wrapped.RoundTrip(req) + if err != nil && span != nil { + span.SetTag("error", true) + span.LogKV("event", "error", "message", err.Error()) + } + + return resp, err } diff --git a/filters/openpolicyagent/tracing_test.go b/filters/openpolicyagent/tracing_test.go index aad4f77ad4..dc5bd33370 100644 --- a/filters/openpolicyagent/tracing_test.go +++ b/filters/openpolicyagent/tracing_test.go @@ -3,39 +3,123 @@ package openpolicyagent import ( "context" "net/http" + "net/url" "testing" - opatracing "github.com/open-policy-agent/opa/tracing" + "github.com/open-policy-agent/opa/config" + "github.com/open-policy-agent/opa/plugins" "github.com/opentracing/opentracing-go" "github.com/stretchr/testify/assert" "github.com/zalando/skipper/tracing/tracingtest" ) type MockTransport struct { + resp *http.Response + err error } func (t *MockTransport) RoundTrip(*http.Request) (*http.Response, error) { - return &http.Response{}, nil + return t.resp, t.err } func TestTracingFactory(t *testing.T) { - f := &tracingFactory{} - - tr := f.NewTransport(&MockTransport{}, opatracing.Options{&OpenPolicyAgentInstance{}}) - tracer := &tracingtest.Tracer{} - span := tracer.StartSpan("open-policy-agent") - ctx := opentracing.ContextWithSpan(context.Background(), span) - req := &http.Request{ - Header: map[string][]string{}, + testCases := []struct { + name string + req *http.Request + tracer opentracing.Tracer + parentSpan opentracing.Span + resp *http.Response + resperr error + }{ + { + name: "Sub-span created with parent span without tracer set", + req: &http.Request{ + Header: map[string][]string{}, + Method: "GET", + Host: "example.com", + URL: &url.URL{Path: "/test", Scheme: "http", Host: "example.com"}, + }, + tracer: nil, + parentSpan: tracer.StartSpan("open-policy-agent"), + }, + { + name: "Sub-span created with parent span without tracer set", + req: &http.Request{ + Header: map[string][]string{}, + Method: "GET", + Host: "example.com", + URL: &url.URL{Path: "/test", Scheme: "http", Host: "example.com"}, + }, + tracer: tracer, + parentSpan: tracer.StartSpan("open-policy-agent"), + }, + { + name: "Sub-span created without parent span", + req: &http.Request{ + Header: map[string][]string{}, + Method: "GET", + Host: "example.com", + URL: &url.URL{Path: "/test", Scheme: "http", Host: "example.com"}, + }, + tracer: tracer, + }, + { + name: "Span contains error information", + req: &http.Request{ + Header: map[string][]string{}, + Method: "GET", + Host: "example.com", + URL: &url.URL{Path: "/test", Scheme: "http", Host: "example.com"}, + }, + tracer: tracer, + resperr: assert.AnError, + }, } - req = req.WithContext(ctx) - _, err := tr.RoundTrip(req) - assert.NoError(t, err) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := &tracingFactory{} + tracer.Reset("") + + tr := f.NewTransport(&MockTransport{tc.resp, tc.resperr}, buildTracingOptions(tc.tracer, "bundle", &plugins.Manager{ + ID: "manager-id", + Config: &config.Config{ + Labels: map[string]string{"label": "value"}, + }, + })) + + if tc.parentSpan != nil { + ctx := opentracing.ContextWithSpan(context.Background(), tc.parentSpan) + tc.req = tc.req.WithContext(ctx) + } + + resp, err := tr.RoundTrip(tc.req) + if tc.parentSpan != nil { + tc.parentSpan.Finish() + } - span.Finish() - _, ok := tracer.FindSpan("http.send") - assert.True(t, ok, "No http.send span was created") + createdSpan, ok := tracer.FindSpan("open-policy-agent.http") + assert.True(t, ok, "No span was created") + + if tc.resperr == nil { + assert.NoError(t, err) + } else { + assert.Equal(t, true, createdSpan.Tags["error"], "Error tag was not set") + assert.Equal(t, tc.resperr, err, "Error was not propagated") + } + + assert.Equal(t, tc.resp, resp, "Response was not propagated") + + assert.Equal(t, tc.req.Method, createdSpan.Tags["http.method"]) + assert.Equal(t, tc.req.URL.String(), createdSpan.Tags["http.url"]) + assert.Equal(t, tc.req.Host, createdSpan.Tags["hostname"]) + assert.Equal(t, tc.req.URL.Path, createdSpan.Tags["http.path"]) + assert.Equal(t, "skipper", createdSpan.Tags["component"]) + assert.Equal(t, "client", createdSpan.Tags["span.kind"]) + assert.Equal(t, "bundle", createdSpan.Tags["opa.bundle_name"]) + assert.Equal(t, "value", createdSpan.Tags["opa.label.label"]) + }) + } } diff --git a/skipper.go b/skipper.go index c7e9aa2d71..f8b6d30130 100644 --- a/skipper.go +++ b/skipper.go @@ -1835,7 +1835,8 @@ func run(o Options, sig chan os.Signal, idleConnsCH chan struct{}) error { opaRegistry = openpolicyagent.NewOpenPolicyAgentRegistry( openpolicyagent.WithMaxRequestBodyBytes(o.OpenPolicyAgentMaxRequestBodySize), openpolicyagent.WithMaxMemoryBodyParsing(o.OpenPolicyAgentMaxMemoryBodyParsing), - openpolicyagent.WithCleanInterval(o.OpenPolicyAgentCleanerInterval)) + openpolicyagent.WithCleanInterval(o.OpenPolicyAgentCleanerInterval), + openpolicyagent.WithTracer(tracer)) defer opaRegistry.Close() opts := make([]func(*openpolicyagent.OpenPolicyAgentInstanceConfig) error, 0)