Skip to content

Commit

Permalink
tracing/tracingtest: deprecate Tracer and Span types
Browse files Browse the repository at this point in the history 
Deprecate hand-rolled test tracer implementation in favour of
MockTracer added by #3322 that wraps github.com/opentracing/opentracing-go/mocktracer
and waits for finished spans.

Follow up on #684, see #684 (comment)

Closes #2084
Updates #2104

Signed-off-by: Alexander Yastrebov <[email protected]>
  • Loading branch information
@AlexanderYastrebov
AlexanderYastrebov committed Jan 14, 2025
1 parent 35195d8 commit 436df67
Show file tree
Hide file tree
Showing 11 changed files with 124 additions and 169 deletions.
26 changes: 13 additions & 13 deletions filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,14 +464,14 @@ func TestAuthorizeRequestFilter(t *testing.T) {
allow_runtime_environment {
opa.runtime().config.labels.environment == "test"
}
default allow_object := {
"allowed": false,
"headers": {"x-ext-auth-allow": "no"},
"body": "Unauthorized Request",
"http_status": 401
}
allow_object := response {
input.parsed_path == [ "allow", "structured" ]
response := {
Expand Down Expand Up @@ -506,13 +506,13 @@ func TestAuthorizeRequestFilter(t *testing.T) {
allow_body {
input.parsed_body.target_id == "123456"
}
}
decision_id := input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id
allow_object_decision_id_in_header := response {
input.parsed_path = ["allow", "structured"]
decision_id
decision_id
response := {
"allowed": true,
"response_headers_to_add": {
Expand Down Expand Up @@ -541,9 +541,9 @@ func TestAuthorizeRequestFilter(t *testing.T) {
"environment": "test"
},
"plugins": {
"envoy_ext_authz_grpc": {
"envoy_ext_authz_grpc": {
"path": %q,
"dry-run": false
"dry-run": false
}
}
}`, opaControlPlane.URL(), ti.regoQuery))
Expand All @@ -561,7 +561,7 @@ func TestAuthorizeRequestFilter(t *testing.T) {
openpolicyagent.WithConfigTemplate(config),
openpolicyagent.WithEnvoyMetadataBytes(envoyMetaDataConfig))

opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{}))
opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(tracingtest.NewTracer()))
ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, opts...)
fr.Register(ftSpec)
ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, opts...)
Expand Down Expand Up @@ -769,11 +769,11 @@ func BenchmarkAuthorizeRequest(b *testing.B) {
"cert": public_key_cert,
"aud": "nqz3xhorr5"
})
valid
payload.sub == "5974934733"
}
}
`, publicKey),
}),
)
Expand Down Expand Up @@ -863,9 +863,9 @@ func generateConfig(opaControlPlane *opasdktest.Server, path string) []byte {
"environment": "test"
},
"plugins": {
"envoy_ext_authz_grpc": {
"envoy_ext_authz_grpc": {
"path": %q,
"dry-run": false
"dry-run": false
}
}
}`, opaControlPlane.URL(), path))
Expand Down
12 changes: 6 additions & 6 deletions filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,15 @@ func TestServerResponseFilter(t *testing.T) {
allow {
input.parsed_path == [ "allow" ]
}
}
default allow_object := {
"allowed": false,
"headers": {"x-ext-auth-allow": "no"},
"body": "Unauthorized Request",
"http_status": 403
}
allow_object := response {
input.parsed_path == [ "allow", "structured" ]
response := {
Expand Down Expand Up @@ -216,7 +216,7 @@ func TestServerResponseFilter(t *testing.T) {
"http_status": 200
}
}
allow_object := response {
input.parsed_path == [ "allow", "production" ]
opa.runtime().config.labels.environment == "production"
Expand Down Expand Up @@ -289,7 +289,7 @@ func TestServerResponseFilter(t *testing.T) {
}
},
"plugins": {
"envoy_ext_authz_grpc": {
"envoy_ext_authz_grpc": {
"path": %q,
"dry-run": false,
"skip-request-body-parse": false
Expand All @@ -300,7 +300,7 @@ func TestServerResponseFilter(t *testing.T) {
}
}`, opaControlPlane.URL(), ti.regoQuery))

opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{}))
opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(tracingtest.NewTracer()))
ftSpec := NewOpaServeResponseSpec(opaFactory, openpolicyagent.WithConfigTemplate(config))
fr.Register(ftSpec)
ftSpec = NewOpaServeResponseWithReqBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config))
Expand Down
21 changes: 11 additions & 10 deletions filters/openpolicyagent/openpolicyagent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/open-policy-agent/opa/storage/inmem"
"github.com/opentracing/opentracing-go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/zalando/skipper/filters"
"github.com/zalando/skipper/filters/filtertest"
"github.com/zalando/skipper/filters/openpolicyagent/internal/envoy"
Expand Down Expand Up @@ -385,15 +386,15 @@ func TestTracing(t *testing.T) {
inst, err := registry.NewOpenPolicyAgentInstance("test", *cfg, "testfilter")
assert.NoError(t, err)

tracer := &tracingtest.Tracer{}
tracer := tracingtest.NewTracer()
parent := tracer.StartSpan("start_span")
ctx := opentracing.ContextWithSpan(context.Background(), parent)
span, _ := inst.StartSpanFromContext(ctx)
span.Finish()
parent.Finish()

recspan, ok := tracer.FindSpan("open-policy-agent")
assert.True(t, ok, "No span was created for open policy agent")
recspan := tracer.FindSpan("open-policy-agent")
require.NotNil(t, recspan, "No span was created for open policy agent")
assert.Equal(t, map[string]interface{}{"opa.bundle_name": "test", "opa.label.id": inst.manager.Labels()["id"], "opa.label.version": inst.manager.Labels()["version"]}, recspan.Tags)
}

Expand All @@ -408,7 +409,7 @@ func TestEval(t *testing.T) {
inst, err := registry.NewOpenPolicyAgentInstance("test", *cfg, "testfilter")
assert.NoError(t, err)

tracer := &tracingtest.Tracer{}
tracer := tracingtest.NewTracer()
span := tracer.StartSpan("open-policy-agent")
ctx := opentracing.ContextWithSpan(context.Background(), span)

Expand All @@ -426,9 +427,9 @@ func TestEval(t *testing.T) {
assert.False(t, allowed)

span.Finish()
testspan, ok := tracer.FindSpan("open-policy-agent")
assert.True(t, ok)
assert.Equal(t, result.DecisionID, testspan.Tags["opa.decision_id"])
testspan := tracer.FindSpan("open-policy-agent")
require.NotNil(t, testspan)
assert.Equal(t, result.DecisionID, testspan.Tags()["opa.decision_id"])
}

func TestResponses(t *testing.T) {
Expand All @@ -442,7 +443,7 @@ func TestResponses(t *testing.T) {
inst, err := registry.NewOpenPolicyAgentInstance("test", *cfg, "testfilter")
assert.NoError(t, err)

tracer := &tracingtest.Tracer{}
tracer := tracingtest.NewTracer()
span := tracer.StartSpan("open-policy-agent")
metrics := &metricstest.MockMetrics{}

Expand All @@ -455,8 +456,8 @@ func TestResponses(t *testing.T) {
assert.Equal(t, int64(1), counters["decision.err.test"])
})
span.Finish()
testspan, ok := tracer.FindSpan("open-policy-agent")
assert.True(t, ok, "span not found")
testspan := tracer.FindSpan("open-policy-agent")
require.NotNil(t, testspan, "span not found")
assert.Contains(t, testspan.Tags, "error")

fc = &filtertest.Context{FMetrics: metrics}
Expand Down
57 changes: 30 additions & 27 deletions filters/openpolicyagent/tracing_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/open-policy-agent/opa/plugins"
"github.com/opentracing/opentracing-go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/zalando/skipper/proxy"
"github.com/zalando/skipper/tracing/tracingtest"
)
Expand All @@ -24,13 +25,13 @@ func (t *MockTransport) RoundTrip(*http.Request) (*http.Response, error) {
}

func TestTracingFactory(t *testing.T) {
tracer := &tracingtest.Tracer{}
tracer := tracingtest.NewTracer()

testCases := []struct {
name string
req *http.Request
tracer opentracing.Tracer
parentSpan opentracing.Span
parentSpan string
resp *http.Response
resperr error
}{
Expand All @@ -43,19 +44,19 @@ func TestTracingFactory(t *testing.T) {
URL: &url.URL{Path: "/test", Scheme: "http", Host: "example.com"},
},
tracer: nil,
parentSpan: tracer.StartSpan("open-policy-agent"),
parentSpan: "open-policy-agent",
resp: &http.Response{StatusCode: http.StatusOK},
},
{
name: "Sub-span created with parent span without tracer set",
name: "Sub-span created with parent span with 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"),
parentSpan: "open-policy-agent",
resp: &http.Response{StatusCode: http.StatusOK},
},
{
Expand Down Expand Up @@ -96,7 +97,15 @@ func TestTracingFactory(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
f := &tracingFactory{}
tracer.Reset("")
tracer.Reset()

if tc.parentSpan != "" {
parentSpan := tracer.StartSpan(tc.parentSpan)
defer parentSpan.Finish()

ctx := opentracing.ContextWithSpan(context.Background(), parentSpan)
tc.req = tc.req.WithContext(ctx)
}

tr := f.NewTransport(&MockTransport{tc.resp, tc.resperr}, buildTracingOptions(tc.tracer, "bundle", &plugins.Manager{
ID: "manager-id",
Expand All @@ -105,41 +114,35 @@ func TestTracingFactory(t *testing.T) {
},
}))

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()
}

createdSpan, ok := tracer.FindSpan("open-policy-agent.http")
assert.True(t, ok, "No span was created")
createdSpan := tracer.FindSpan("open-policy-agent.http")
require.NotNil(t, createdSpan, "No span was created")

createdSpanTags := createdSpan.Tags()

if tc.resperr == nil {
assert.NoError(t, err)
if tc.resp.StatusCode > 399 {
assert.Equal(t, true, createdSpan.Tags["error"], "Error tag was not set")
assert.Equal(t, true, createdSpanTags["error"], "Error tag was not set")
}

assert.Equal(t, tc.resp.StatusCode, createdSpan.Tags[proxy.HTTPStatusCodeTag], "http status tag was not set")
assert.Equal(t, tc.resp.StatusCode, createdSpanTags[proxy.HTTPStatusCodeTag], "http status tag was not set")
} else {
assert.Equal(t, true, createdSpan.Tags["error"], "Error tag was not set")
assert.Equal(t, true, createdSpanTags["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"])
assert.Equal(t, tc.req.Method, createdSpanTags["http.method"])
assert.Equal(t, tc.req.URL.String(), createdSpanTags["http.url"])
assert.Equal(t, tc.req.Host, createdSpanTags["hostname"])
assert.Equal(t, tc.req.URL.Path, createdSpanTags["http.path"])
assert.Equal(t, "skipper", createdSpanTags["component"])
assert.Equal(t, "client", createdSpanTags["span.kind"])
assert.Equal(t, "bundle", createdSpanTags["opa.bundle_name"])
assert.Equal(t, "value", createdSpanTags["opa.label.label"])
})
}
}
14 changes: 10 additions & 4 deletions filters/tracing/baggagetotag_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ func TestBaggageItemNameToTag(t *testing.T) {
t.Run(ti.msg, func(t *testing.T) {
req := &http.Request{Header: http.Header{}}

span := tracingtest.NewSpan("start_span")
tracer := tracingtest.NewTracer()
span := tracer.StartSpan("start_span")
span.SetBaggageItem(ti.baggageItemName, ti.baggageItemValue)
req = req.WithContext(opentracing.ContextWithSpan(req.Context(), span))
ctx := &filtertest.Context{FRequest: req}
Expand All @@ -38,7 +39,9 @@ func TestBaggageItemNameToTag(t *testing.T) {

f.Request(ctx)

if tagValue := span.Tags[ti.tagName]; ti.baggageItemValue != tagValue {
span.Finish()

if tagValue := span.(*tracingtest.MockSpan).Tags()[ti.tagName]; ti.baggageItemValue != tagValue {
t.Error("couldn't set span tag from baggage item")
}
})
Expand Down Expand Up @@ -99,7 +102,8 @@ func TestFallbackToBaggageNameForTag(t *testing.T) {
t.Run(ti.msg, func(t *testing.T) {
req := &http.Request{Header: http.Header{}}

span := tracingtest.NewSpan("start_span")
tracer := tracingtest.NewTracer()
span := tracer.StartSpan("start_span")
span.SetBaggageItem(ti.baggageItemName, ti.baggageItemValue)
req = req.WithContext(opentracing.ContextWithSpan(req.Context(), span))
ctx := &filtertest.Context{FRequest: req}
Expand All @@ -112,7 +116,9 @@ func TestFallbackToBaggageNameForTag(t *testing.T) {

f.Request(ctx)

if tagValue := span.Tags[ti.baggageItemName]; ti.baggageItemValue != tagValue {
span.Finish()

if tagValue := span.(*tracingtest.MockSpan).Tags()[ti.baggageItemName]; ti.baggageItemValue != tagValue {
t.Error("couldn't set span tag from baggage item")
}
})
Expand Down
Loading

0 comments on commit 436df67

Please sign in to comment.