From 84ad4a0d9d4d35ee973403308574fb678fc1779d Mon Sep 17 00:00:00 2001 From: Janardhan Sharma Date: Thu, 25 Apr 2024 21:11:23 +0200 Subject: [PATCH 01/14] Facilitate OPA decision correlation with business flows Signed-off-by: Janardhan Sharma --- docs/tutorials/auth.md | 9 +++ filters/openpolicyagent/evaluation.go | 48 ++++++++++++++- .../opaauthorizerequest_test.go | 59 +++++++++++++++++-- .../openpolicyagent/openpolicyagent_test.go | 12 +++- 4 files changed, 119 insertions(+), 9 deletions(-) diff --git a/docs/tutorials/auth.md b/docs/tutorials/auth.md index 14bf04e1f7..92ce9884ca 100644 --- a/docs/tutorials/auth.md +++ b/docs/tutorials/auth.md @@ -466,6 +466,15 @@ The second argument is parsed as YAML, cannot be nested and values need to be st In Rego this can be used like this `input.attributes.contextExtensions["com.mycompany.myprop"] == "my value"` +### Decision's Identity + +Each evaluation yields a distinct decision, identifiable by its unique decision ID. +This decision ID can be located within the input at: + +`input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id` + +Leveraging this ID enables seamless correlation of decisions with the flow ID of the originating request. + ### Quick Start Rego Playground A quick way without setting up Backend APIs is to use the [Rego Playground](https://play.openpolicyagent.org/). diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go index 29f1cd5415..7ff0a8f354 100644 --- a/filters/openpolicyagent/evaluation.go +++ b/filters/openpolicyagent/evaluation.go @@ -2,9 +2,9 @@ package openpolicyagent import ( "context" + "encoding/json" "fmt" - "time" - + ext_authz_v3_core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" ext_authz_v3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" "github.com/open-policy-agent/opa-envoy-plugin/envoyauth" "github.com/open-policy-agent/opa-envoy-plugin/opa/decisionlog" @@ -13,6 +13,8 @@ import ( "github.com/open-policy-agent/opa/server" "github.com/open-policy-agent/opa/tracing" "github.com/opentracing/opentracing-go" + _struct "google.golang.org/protobuf/types/known/structpb" + "time" ) func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3.CheckRequest) (*envoyauth.EvalResult, error) { @@ -23,6 +25,8 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3. return nil, err } + setDecisionIdInRequest(req, decisionId) + result, stopeval, err := envoyauth.NewEvalResult(withDecisionID(decisionId)) if err != nil { opa.Logger().WithFields(map[string]interface{}{"err": err}).Error("Unable to generate new result with decision ID.") @@ -66,6 +70,46 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3. return result, nil } +func setDecisionIdInRequest(req *ext_authz_v3.CheckRequest, decisionId string) { + if metaDataContextDoesNotExist(req) { + req.Attributes.MetadataContext = formFilterMetadata(decisionId) + } else { + req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = formOpenPolicyAgentMetaDataObject(decisionId) + } +} + +func metaDataContextDoesNotExist(req *ext_authz_v3.CheckRequest) bool { + return req.Attributes.MetadataContext == nil +} + +func formFilterMetadata(decisionId string) *ext_authz_v3_core.Metadata { + metaData := &ext_authz_v3_core.Metadata{ + FilterMetadata: map[string]*_struct.Struct{ + "open_policy_agent": { + Fields: map[string]*_struct.Value{ + "decision_id": { + Kind: &_struct.Value_StringValue{StringValue: decisionId}, + }, + }, + }, + }, + } + return metaData +} + +func formOpenPolicyAgentMetaDataObject(decisionId string) *_struct.Struct { + nestedStruct := &_struct.Struct{} + nestedStruct.Fields = make(map[string]*_struct.Value) + + innerFields := make(map[string]interface{}) + innerFields["decision_id"] = decisionId + + innerBytes, _ := json.Marshal(innerFields) + _ = json.Unmarshal(innerBytes, &nestedStruct) + + return nestedStruct +} + func (opa *OpenPolicyAgentInstance) logDecision(ctx context.Context, input interface{}, result *envoyauth.EvalResult, err error) error { info := &server.Info{ Timestamp: time.Now(), diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index e95836c3ec..5b8f825ff0 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -254,6 +254,21 @@ func TestAuthorizeRequestFilter(t *testing.T) { backendHeaders: make(http.Header), removeHeaders: make(http.Header), }, + { + msg: "Decision id in request header", + filterName: "opaAuthorizeRequest", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow_object_decision_id_in_header", + requestMethod: "POST", + body: `{ "target_id" : "123456" }`, + requestHeaders: map[string][]string{"content-type": {"application/json"}}, + requestPath: "/allow/structured", + expectedStatus: http.StatusOK, + expectedBody: "Welcome!", + expectedHeaders: map[string][]string{"Decision-Id": {"some-random-decision-id-generated-during-evaluation"}}, + backendHeaders: make(http.Header), + removeHeaders: make(http.Header), + }, } { t.Run(ti.msg, func(t *testing.T) { t.Logf("Running test for %v", ti) @@ -330,8 +345,23 @@ func TestAuthorizeRequestFilter(t *testing.T) { allow_body { input.parsed_body.target_id == "123456" - } - `, + } + + decisionIdPresent { + input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id + } + + allow_object_decision_id_in_header = response { + input.parsed_path = [ "allow", "structured" ] + decisionIdPresent + response := { + "allowed": true, + "response_headers_to_add": { + "decision-id": input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id + } + } + } + `, }), ) @@ -359,10 +389,23 @@ func TestAuthorizeRequestFilter(t *testing.T) { } }`, opaControlPlane.URL(), ti.regoQuery)) + envoyMetaDataConfig := []byte(`{ + "filter_metadata": { + "envoy.filters.http.header_to_metadata": { + "policy_type": "ingress" + } + } + }`) + + opts := make([]func(*openpolicyagent.OpenPolicyAgentInstanceConfig) error, 0) + opts = append(opts, + openpolicyagent.WithConfigTemplate(config), + openpolicyagent.WithEnvoyMetadataBytes(envoyMetaDataConfig)) + opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry() - ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) + ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, opts...) fr.Register(ftSpec) - ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) + ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, opts...) fr.Register(ftSpec) r := eskip.MustParse(fmt.Sprintf(`* -> %s("%s", "%s") %s -> "%s"`, ti.filterName, ti.bundleName, ti.contextExtensions, ti.extraeskip, clientServer.URL)) @@ -420,8 +463,12 @@ func isHeadersPresent(t *testing.T, expectedHeaders http.Header, headers http.He if !headerFound { return false } - - assert.ElementsMatch(t, expectedValues, actualValues) + // since decision id is randomly generated we are just checking for not nil + if headerName == "Decision-Id" { + assert.NotNil(t, actualValues) + } else { + assert.ElementsMatch(t, expectedValues, actualValues) + } } return true } diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go index 117e8522bb..ffe5695322 100644 --- a/filters/openpolicyagent/openpolicyagent_test.go +++ b/filters/openpolicyagent/openpolicyagent_test.go @@ -63,11 +63,14 @@ func TestInterpolateTemplate(t *testing.T) { func TestLoadEnvoyMetadata(t *testing.T) { cfg := &OpenPolicyAgentInstanceConfig{} - WithEnvoyMetadataBytes([]byte(` + err := WithEnvoyMetadataBytes([]byte(` { "filter_metadata": { "envoy.filters.http.header_to_metadata": { "policy_type": "ingress" + }, + "open_policy_agent" : { + "decision_id" : "3b567656-bf28-4a63-a4c4-14407fbd9544" } } } @@ -82,6 +85,13 @@ func TestLoadEnvoyMetadata(t *testing.T) { }, }, }, + "open_policy_agent": { + Fields: map[string]*_struct.Value{ + "decision_id": { + Kind: &_struct.Value_StringValue{StringValue: "3b567656-bf28-4a63-a4c4-14407fbd9544"}, + }, + }, + }, }, }) From e5786f6bdf0845e5f18d694942048df74fbed7d3 Mon Sep 17 00:00:00 2001 From: JanardhanSharma Date: Fri, 26 Apr 2024 09:13:27 +0200 Subject: [PATCH 02/14] review comment Signed-off-by: Janardhan Sharma --- .../opaauthorizerequest_test.go | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index 5b8f825ff0..8c16b791f1 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -347,19 +347,17 @@ func TestAuthorizeRequestFilter(t *testing.T) { input.parsed_body.target_id == "123456" } - decisionIdPresent { - input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id - } + decision_id := input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id allow_object_decision_id_in_header = response { - input.parsed_path = [ "allow", "structured" ] - decisionIdPresent - response := { - "allowed": true, - "response_headers_to_add": { - "decision-id": input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id - } - } + input.parsed_path = ["allow", "structured"] + decision_id + response := { + "allowed": true, + "response_headers_to_add": { + "decision-id": decision_id + } + } } `, }), From db3b2ea27fd6c9b6dc3db41862b76353483f65da Mon Sep 17 00:00:00 2001 From: JanardhanSharma Date: Fri, 26 Apr 2024 13:55:23 +0200 Subject: [PATCH 03/14] review comment - import variable renamed Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/evaluation.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go index 7ff0a8f354..117766a807 100644 --- a/filters/openpolicyagent/evaluation.go +++ b/filters/openpolicyagent/evaluation.go @@ -13,7 +13,7 @@ import ( "github.com/open-policy-agent/opa/server" "github.com/open-policy-agent/opa/tracing" "github.com/opentracing/opentracing-go" - _struct "google.golang.org/protobuf/types/known/structpb" + pbstruct "google.golang.org/protobuf/types/known/structpb" "time" ) @@ -84,11 +84,11 @@ func metaDataContextDoesNotExist(req *ext_authz_v3.CheckRequest) bool { func formFilterMetadata(decisionId string) *ext_authz_v3_core.Metadata { metaData := &ext_authz_v3_core.Metadata{ - FilterMetadata: map[string]*_struct.Struct{ + FilterMetadata: map[string]*pbstruct.Struct{ "open_policy_agent": { - Fields: map[string]*_struct.Value{ + Fields: map[string]*pbstruct.Value{ "decision_id": { - Kind: &_struct.Value_StringValue{StringValue: decisionId}, + Kind: &pbstruct.Value_StringValue{StringValue: decisionId}, }, }, }, @@ -97,9 +97,9 @@ func formFilterMetadata(decisionId string) *ext_authz_v3_core.Metadata { return metaData } -func formOpenPolicyAgentMetaDataObject(decisionId string) *_struct.Struct { - nestedStruct := &_struct.Struct{} - nestedStruct.Fields = make(map[string]*_struct.Value) +func formOpenPolicyAgentMetaDataObject(decisionId string) *pbstruct.Struct { + nestedStruct := &pbstruct.Struct{} + nestedStruct.Fields = make(map[string]*pbstruct.Value) innerFields := make(map[string]interface{}) innerFields["decision_id"] = decisionId From 059e727988f4780b8dda51c55b086c56c9e79453 Mon Sep 17 00:00:00 2001 From: Magnus Jungsbluth Date: Fri, 26 Apr 2024 09:55:30 +0200 Subject: [PATCH 04/14] OPA: Add tracing for outbound http calls (#3034) Signed-off-by: Magnus Jungsbluth Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/evaluation.go | 3 +- .../opaauthorizerequest_test.go | 3 +- .../opaserveresponse/opaserveresponse_test.go | 3 +- filters/openpolicyagent/openpolicyagent.go | 54 ++++++-- filters/openpolicyagent/tracing.go | 71 ++++++++++- filters/openpolicyagent/tracing_test.go | 116 +++++++++++++++--- skipper.go | 3 +- 7 files changed, 218 insertions(+), 35 deletions(-) diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go index 117766a807..9501becced 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" pbstruct "google.golang.org/protobuf/types/known/structpb" "time" @@ -62,7 +61,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 8c16b791f1..b8c286ae5d 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" @@ -400,7 +401,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { openpolicyagent.WithConfigTemplate(config), openpolicyagent.WithEnvoyMetadataBytes(envoyMetaDataConfig)) - opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry() + opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{})) ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, opts...) fr.Register(ftSpec) ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, opts...) 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) From bb88e1ff9366cc65832f3f86374758004b555c29 Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Fri, 26 Apr 2024 12:37:38 +0200 Subject: [PATCH 05/14] Specify tracing span kind on creation (#3039) OpenTelemetry-OpenTracing bridge span kind can not be changed after creation, see https://github.com/open-telemetry/opentelemetry-go/issues/3953 The workaround is to specify span kind on creation which works for both Open Tracing and Open Telemetry bridge spans. Note that this change removes non-standard "shedder" kind from spans created by `admissionControl` filter. Use operation name "admission_control" to query its spans instead if needed. For #2104 Signed-off-by: Alexander Yastrebov Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/tracing.go | 26 +++++++++-------- filters/shedder/admission.go | 1 - net/httpclient.go | 21 ++++++-------- proxy/proxy.go | 45 +++++++++++++++++++----------- ratelimit/leakybucket.go | 15 +++++----- ratelimit/redis.go | 22 +++++++-------- tracing/tracingtest/testtracer.go | 3 ++ 7 files changed, 71 insertions(+), 62 deletions(-) diff --git a/filters/openpolicyagent/tracing.go b/filters/openpolicyagent/tracing.go index bc63c780b9..531e563913 100644 --- a/filters/openpolicyagent/tracing.go +++ b/filters/openpolicyagent/tracing.go @@ -71,25 +71,27 @@ 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(spanNameHttpOut, opentracing.ChildOf(parentSpan.Context())) + spanOpts := []opentracing.StartSpanOption{opentracing.Tags{ + proxy.HTTPMethodTag: req.Method, + proxy.HTTPUrlTag: req.URL.String(), + proxy.HostnameTag: req.Host, + proxy.HTTPPathTag: req.URL.Path, + proxy.ComponentTag: "skipper", + proxy.SpanKindTag: proxy.SpanKindClient, + }} + + var span opentracing.Span + if parentSpan := opentracing.SpanFromContext(ctx); parentSpan != nil { + spanOpts = append(spanOpts, opentracing.ChildOf(parentSpan.Context())) + span = parentSpan.Tracer().StartSpan(spanNameHttpOut, spanOpts...) } else if tr.tracer != nil { - span = tr.tracer.StartSpan(spanNameHttpOut) + span = tr.tracer.StartSpan(spanNameHttpOut, spanOpts...) } 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)) diff --git a/filters/shedder/admission.go b/filters/shedder/admission.go index c963330d8c..80d63e49d0 100644 --- a/filters/shedder/admission.go +++ b/filters/shedder/admission.go @@ -456,7 +456,6 @@ func (ac *admissionControl) startSpan(ctx context.Context) (span opentracing.Spa if parent != nil { span = ac.tracer.StartSpan(admissionControlSpanName, opentracing.ChildOf(parent.Context())) ext.Component.Set(span, "skipper") - ext.SpanKind.Set(span, "shedder") span.SetTag("mode", ac.mode.String()) } return diff --git a/net/httpclient.go b/net/httpclient.go index bf9c7d7aa5..833bae6203 100644 --- a/net/httpclient.go +++ b/net/httpclient.go @@ -381,20 +381,17 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { } func (t *Transport) injectSpan(req *http.Request) (*http.Request, opentracing.Span) { - parentSpan := opentracing.SpanFromContext(req.Context()) - var span opentracing.Span - if parentSpan != nil { + spanOpts := []opentracing.StartSpanOption{opentracing.Tags{ + string(ext.Component): t.componentName, + string(ext.SpanKind): "client", + string(ext.HTTPMethod): req.Method, + string(ext.HTTPUrl): req.URL.String(), + }} + if parentSpan := opentracing.SpanFromContext(req.Context()); parentSpan != nil { req = req.WithContext(opentracing.ContextWithSpan(req.Context(), parentSpan)) - span = t.tracer.StartSpan(t.spanName, opentracing.ChildOf(parentSpan.Context())) - } else { - span = t.tracer.StartSpan(t.spanName) + spanOpts = append(spanOpts, opentracing.ChildOf(parentSpan.Context())) } - - // add Tags - ext.Component.Set(span, t.componentName) - ext.HTTPUrl.Set(span, req.URL.String()) - ext.HTTPMethod.Set(span, req.Method) - ext.SpanKind.Set(span, "client") + span := t.tracer.StartSpan(t.spanName, spanOpts...) _ = t.tracer.Inject(span.Context(), opentracing.HTTPHeaders, opentracing.HTTPHeadersCarrier(req.Header)) diff --git a/proxy/proxy.go b/proxy/proxy.go index 442ea71324..549cc6778a 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -958,14 +958,19 @@ func (p *Proxy) makeBackendRequest(ctx *context, requestContext stdlibcontext.Co if !ok { spanName = "proxy" } - ctx.proxySpan = tracing.CreateSpan(spanName, req.Context(), p.tracing.tracer) + + proxySpanOpts := []ot.StartSpanOption{ot.Tags{ + SpanKindTag: SpanKindClient, + SkipperRouteIDTag: ctx.route.Id, + }} + if parentSpan := ot.SpanFromContext(req.Context()); parentSpan != nil { + proxySpanOpts = append(proxySpanOpts, ot.ChildOf(parentSpan.Context())) + } + ctx.proxySpan = p.tracing.tracer.StartSpan(spanName, proxySpanOpts...) u := cloneURL(req.URL) u.RawQuery = "" - p.tracing. - setTag(ctx.proxySpan, SpanKindTag, SpanKindClient). - setTag(ctx.proxySpan, SkipperRouteIDTag, ctx.route.Id). - setTag(ctx.proxySpan, HTTPUrlTag, u.String()) + p.tracing.setTag(ctx.proxySpan, HTTPUrlTag, u.String()) p.setCommonSpanInfo(u, req, ctx.proxySpan) carrier := ot.HTTPHeadersCarrier(req.Header) @@ -1181,10 +1186,16 @@ func (p *Proxy) do(ctx *context, parentSpan ot.Span) (err error) { ctx.ensureDefaultResponse() } else if ctx.route.BackendType == eskip.LoopBackend { loopCTX := ctx.clone() - loopSpan := tracing.CreateSpan("loopback", ctx.request.Context(), p.tracing.tracer) - p.tracing. - setTag(loopSpan, SpanKindTag, SpanKindServer). - setTag(loopSpan, SkipperRouteIDTag, ctx.route.Id) + + loopSpanOpts := []ot.StartSpanOption{ot.Tags{ + SpanKindTag: SpanKindServer, + SkipperRouteIDTag: ctx.route.Id, + }} + if parentSpan := ot.SpanFromContext(ctx.request.Context()); parentSpan != nil { + loopSpanOpts = append(loopSpanOpts, ot.ChildOf(parentSpan.Context())) + } + loopSpan := p.tracing.tracer.StartSpan("loopback", loopSpanOpts...) + p.setCommonSpanInfo(ctx.Request().URL, ctx.Request(), loopSpan) ctx.parentSpan = loopSpan @@ -1481,12 +1492,15 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { p.metrics.IncCounter("incoming." + r.Proto) var ctx *context - var span ot.Span - if wireContext, err := p.tracing.tracer.Extract(ot.HTTPHeaders, ot.HTTPHeadersCarrier(r.Header)); err != nil { - span = p.tracing.tracer.StartSpan(p.tracing.initialOperationName) - } else { - span = p.tracing.tracer.StartSpan(p.tracing.initialOperationName, ext.RPCServerOption(wireContext)) + spanOpts := []ot.StartSpanOption{ot.Tags{ + SpanKindTag: SpanKindServer, + HTTPRemoteIPTag: stripPort(r.RemoteAddr), + }} + if wireContext, err := p.tracing.tracer.Extract(ot.HTTPHeaders, ot.HTTPHeadersCarrier(r.Header)); err == nil { + spanOpts = append(spanOpts, ext.RPCServerOption(wireContext)) } + span := p.tracing.tracer.StartSpan(p.tracing.initialOperationName, spanOpts...) + defer func() { if ctx != nil && ctx.proxySpan != nil { ctx.proxySpan.Finish() @@ -1533,9 +1547,6 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { r.URL.Path = rfc.PatchPath(r.URL.Path, r.URL.RawPath) } - p.tracing. - setTag(span, SpanKindTag, SpanKindServer). - setTag(span, HTTPRemoteIPTag, stripPort(r.RemoteAddr)) p.setCommonSpanInfo(r.URL, r, span) r = r.WithContext(ot.ContextWithSpan(r.Context(), span)) r = r.WithContext(routing.NewContext(r.Context())) diff --git a/ratelimit/leakybucket.go b/ratelimit/leakybucket.go index 7a432d1fac..a1253605e4 100644 --- a/ratelimit/leakybucket.go +++ b/ratelimit/leakybucket.go @@ -112,13 +112,12 @@ func (b *ClusterLeakyBucket) getBucketId(label string) string { } func (b *ClusterLeakyBucket) startSpan(ctx context.Context) (span opentracing.Span) { - parent := opentracing.SpanFromContext(ctx) - if parent != nil { - span = b.ringClient.StartSpan(leakyBucketSpanName, opentracing.ChildOf(parent.Context())) - } else { - span = opentracing.NoopTracer{}.StartSpan("") + spanOpts := []opentracing.StartSpanOption{opentracing.Tags{ + string(ext.Component): "skipper", + string(ext.SpanKind): "client", + }} + if parent := opentracing.SpanFromContext(ctx); parent != nil { + spanOpts = append(spanOpts, opentracing.ChildOf(parent.Context())) } - ext.Component.Set(span, "skipper") - ext.SpanKind.Set(span, "client") - return + return b.ringClient.StartSpan(leakyBucketSpanName, spanOpts...) } diff --git a/ratelimit/redis.go b/ratelimit/redis.go index 1c6804c0b2..7ac45c0dd3 100644 --- a/ratelimit/redis.go +++ b/ratelimit/redis.go @@ -84,14 +84,14 @@ func parentSpan(ctx context.Context) opentracing.Span { return opentracing.SpanFromContext(ctx) } -func (c *clusterLimitRedis) setCommonTags(span opentracing.Span) { - if span != nil { - ext.Component.Set(span, "skipper") - ext.SpanKind.Set(span, "client") - span.SetTag("ratelimit_type", c.typ) - span.SetTag("group", c.group) - span.SetTag("max_hits", c.maxHits) - span.SetTag("window", c.window.String()) +func (c *clusterLimitRedis) commonTags() opentracing.Tags { + return opentracing.Tags{ + string(ext.Component): "skipper", + string(ext.SpanKind): "client", + "ratelimit_type": c.typ, + "group": c.group, + "max_hits": c.maxHits, + "window": c.window.String(), } } @@ -114,10 +114,9 @@ func (c *clusterLimitRedis) Allow(ctx context.Context, clearText string) bool { var span opentracing.Span if parentSpan := parentSpan(ctx); parentSpan != nil { - span = c.ringClient.StartSpan(allowSpanName, opentracing.ChildOf(parentSpan.Context())) + span = c.ringClient.StartSpan(allowSpanName, opentracing.ChildOf(parentSpan.Context()), c.commonTags()) defer span.Finish() } - c.setCommonTags(span) allow, err := c.allow(ctx, clearText) failed := err != nil @@ -227,10 +226,9 @@ func (c *clusterLimitRedis) oldest(ctx context.Context, clearText string) (time. var span opentracing.Span if parentSpan := parentSpan(ctx); parentSpan != nil { - span = c.ringClient.StartSpan(oldestScoreSpanName, opentracing.ChildOf(parentSpan.Context())) + span = c.ringClient.StartSpan(oldestScoreSpanName, opentracing.ChildOf(parentSpan.Context()), c.commonTags()) defer span.Finish() } - c.setCommonTags(span) res, err := c.ringClient.ZRangeByScoreWithScoresFirst(ctx, key, 0.0, float64(now.UnixNano()), 0, 1) diff --git a/tracing/tracingtest/testtracer.go b/tracing/tracingtest/testtracer.go index 40fc5a742f..4da0a6d4ba 100644 --- a/tracing/tracingtest/testtracer.go +++ b/tracing/tracingtest/testtracer.go @@ -102,6 +102,9 @@ func (t *Tracer) StartSpan(operationName string, opts ...tracing.StartSpanOption s := t.createSpanBase() s.operationName = operationName s.Refs = sso.References + for k, v := range sso.Tags { + s.Tags[k] = v + } return s } From bd738f99e414d944f3254075fca3f46ffa9689b8 Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Fri, 26 Apr 2024 13:20:40 +0200 Subject: [PATCH 06/14] config: refactor test defaultConfig (#3042) Make `defaultConfig` return configuration equal to one created from default flags and modified by a helper function for defining expected test case values. Signed-off-by: Alexander Yastrebov Signed-off-by: Janardhan Sharma --- config/config_test.go | 106 +++++++++++++++++++++----------------- config/testdata/test.yaml | 2 + 2 files changed, 60 insertions(+), 48 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 5d740c9e93..6ce6c76cb3 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -62,15 +62,14 @@ func TestEnvOverrides_SwarmRedisPassword(t *testing.T) { } } -func defaultConfig() *Config { - return &Config{ - ConfigFile: "testdata/test.yaml", +func defaultConfig(with func(*Config)) *Config { + cfg := &Config{ Flags: nil, - Address: "localhost:8080", - StatusChecks: nil, + Address: ":9090", + StatusChecks: commaListFlag(), ExpectedBytesPerRequest: 50 * 1024, SupportListener: ":9911", - MaxLoopbacks: 12, + MaxLoopbacks: proxy.DefaultMaxLoopbacks, DefaultHTTPStatus: 404, MaxAuditBody: 1024, MaxMatcherBufferSize: 2097152, @@ -92,7 +91,7 @@ func defaultConfig() *Config { ApplicationLogLevelString: "INFO", ApplicationLogPrefix: "[APP]", EtcdPrefix: "/skipper", - EtcdTimeout: 2 * time.Second, + EtcdTimeout: time.Second, AppendFilters: &defaultFiltersFlags{}, PrependFilters: &defaultFiltersFlags{}, DisabledFilters: commaListFlag(), @@ -138,7 +137,6 @@ func defaultConfig() *Config { ServeMethodMetric: true, ServeStatusCodeMetric: true, SwarmRedisURLs: commaListFlag(), - SwarmRedisPassword: "set_from_file", SwarmRedisDialTimeout: 25 * time.Millisecond, SwarmRedisReadTimeout: 25 * time.Millisecond, SwarmRedisWriteTimeout: 25 * time.Millisecond, @@ -156,7 +154,6 @@ func defaultConfig() *Config { ForwardedHeadersList: commaListFlag(), ForwardedHeadersExcludeCIDRList: commaListFlag(), ClusterRatelimitMaxGroupShards: 1, - RefusePayload: multiFlag{"foo", "bar", "baz"}, ValidateQuery: true, ValidateQueryLog: true, LuaModules: commaListFlag(), @@ -166,44 +163,40 @@ func defaultConfig() *Config { OpenPolicyAgentMaxRequestBodySize: openpolicyagent.DefaultMaxRequestBodySize, OpenPolicyAgentMaxMemoryBodyParsing: openpolicyagent.DefaultMaxMemoryBodyParsing, } -} - -func defaultConfigWithoutNil() *Config { - cfg := defaultConfig() - cfg.StatusChecks = newListFlag("", "") + with(cfg) return cfg } func TestToOptions(t *testing.T) { - c := defaultConfigWithoutNil() + c := defaultConfig(func(c *Config) { + // ProxyFlags + c.Insecure = true // 1 + c.ProxyPreserveHost = true // 4 + c.RemoveHopHeaders = true // 16 + c.RfcPatchPath = true // 32 - // ProxyFlags - c.Insecure = true // 1 - c.ProxyPreserveHost = true // 4 - c.RemoveHopHeaders = true // 16 - c.RfcPatchPath = true // 32 - - // config - c.EtcdUrls = "127.0.0.1:5555" - c.WhitelistedHealthCheckCIDR = "127.0.0.0/8,10.0.0.0/8" - c.ForwardedHeadersList = commaListFlag("X-Forwarded-For,X-Forwarded-Host,X-Forwarded-Method,X-Forwarded-Uri,X-Forwarded-Port=,X-Forwarded-Proto=http") - c.ForwardedHeadersList.Set("X-Forwarded-For,X-Forwarded-Host,X-Forwarded-Method,X-Forwarded-Uri,X-Forwarded-Port=,X-Forwarded-Proto=http") - c.HostPatch = net.HostPatch{ - ToLower: true, - RemoteTrailingDot: true, - } - c.RefusePayload = append(c.RefusePayload, "refuse") - c.ValidateQuery = true - c.ValidateQueryLog = true + // config + c.EtcdUrls = "127.0.0.1:5555" + c.WhitelistedHealthCheckCIDR = "127.0.0.0/8,10.0.0.0/8" + c.ForwardedHeadersList = commaListFlag("X-Forwarded-For,X-Forwarded-Host,X-Forwarded-Method,X-Forwarded-Uri,X-Forwarded-Port=,X-Forwarded-Proto=http") + c.ForwardedHeadersList.Set("X-Forwarded-For,X-Forwarded-Host,X-Forwarded-Method,X-Forwarded-Uri,X-Forwarded-Port=,X-Forwarded-Proto=http") + c.HostPatch = net.HostPatch{ + ToLower: true, + RemoteTrailingDot: true, + } + c.RefusePayload = append(c.RefusePayload, "refuse") + c.ValidateQuery = true + c.ValidateQueryLog = true - c.CloneRoute = routeChangerConfig{} - if err := c.CloneRoute.Set("/foo/bar/"); err != nil { - t.Fatalf("Failed to set: %v", err) - } - c.EditRoute = routeChangerConfig{} - if err := c.EditRoute.Set("/foo/bar/"); err != nil { - t.Fatalf("Failed to set: %v", err) - } + c.CloneRoute = routeChangerConfig{} + if err := c.CloneRoute.Set("/foo/bar/"); err != nil { + t.Fatalf("Failed to set: %v", err) + } + c.EditRoute = routeChangerConfig{} + if err := c.EditRoute.Set("/foo/bar/"); err != nil { + t.Fatalf("Failed to set: %v", err) + } + }) if err := validate(c); err != nil { t.Fatalf("Failed to validate config: %v", err) @@ -322,22 +315,39 @@ func Test_NewConfigWithArgs(t *testing.T) { wantErr: true, }, { - name: "test only valid flag overwrite yaml file", - args: []string{"skipper", "-config-file=testdata/test.yaml", "-address=localhost:8080", "-refuse-payload=baz"}, - want: defaultConfig(), - wantErr: false, + name: "test only valid flag overwrite yaml file", + args: []string{"skipper", "-config-file=testdata/test.yaml", "-address=localhost:8080", "-refuse-payload=baz"}, + want: defaultConfig(func(c *Config) { + c.ConfigFile = "testdata/test.yaml" + c.Address = "localhost:8080" + c.MaxLoopbacks = 12 + c.EtcdTimeout = 2 * time.Second + c.StatusChecks = &listFlag{ + sep: ",", + allowed: map[string]bool{}, + value: "http://foo.test/bar,http://baz.test/qux", + values: []string{"http://foo.test/bar", "http://baz.test/qux"}, + } + c.SwarmRedisPassword = "set_from_file" + c.RefusePayload = multiFlag{"foo", "bar", "baz"} + }), }, } { t.Run(tt.name, func(t *testing.T) { cfg := NewConfig() err := cfg.ParseArgs(tt.args[0], tt.args[1:]) + if (err != nil) != tt.wantErr { - t.Errorf("config.NewConfig() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("config.NewConfig() error: %v, wantErr: %v", err, tt.wantErr) } if !tt.wantErr { - if cmp.Equal(cfg, tt.want, cmp.AllowUnexported(listFlag{}, pluginFlag{}, defaultFiltersFlags{}, mapFlags{}), cmpopts.IgnoreUnexported(Config{}), cmpopts.IgnoreFields(Config{}, "Flags")) == false { - t.Errorf("config.NewConfig() got vs. want:\n%v", cmp.Diff(cfg, tt.want, cmp.AllowUnexported(listFlag{}, pluginFlag{}, defaultFiltersFlags{}, mapFlags{}), cmpopts.IgnoreUnexported(Config{}), cmpopts.IgnoreFields(Config{}, "Flags"))) + d := cmp.Diff(cfg, tt.want, + cmp.AllowUnexported(listFlag{}, pluginFlag{}, defaultFiltersFlags{}, mapFlags{}), + cmpopts.IgnoreUnexported(Config{}), cmpopts.IgnoreFields(Config{}, "Flags"), + ) + if d != "" { + t.Errorf("config.NewConfig() want vs got:\n%s", d) } } }) diff --git a/config/testdata/test.yaml b/config/testdata/test.yaml index 76410e66b4..26714a9871 100644 --- a/config/testdata/test.yaml +++ b/config/testdata/test.yaml @@ -2,6 +2,8 @@ address: localhost:9090 max-loopbacks: 12 etcd-timeout: 2s status-checks: + - http://foo.test/bar + - http://baz.test/qux swarm-redis-password: set_from_file refuse-payload: - foo From 727c27fd8d5e2090b9d0ea49e5caa427d7ef356a Mon Sep 17 00:00:00 2001 From: JanardhanSharma Date: Fri, 26 Apr 2024 18:39:38 +0200 Subject: [PATCH 07/14] Fix static analysis error Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/openpolicyagent_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go index ffe5695322..1f3fc6e27d 100644 --- a/filters/openpolicyagent/openpolicyagent_test.go +++ b/filters/openpolicyagent/openpolicyagent_test.go @@ -63,7 +63,7 @@ func TestInterpolateTemplate(t *testing.T) { func TestLoadEnvoyMetadata(t *testing.T) { cfg := &OpenPolicyAgentInstanceConfig{} - err := WithEnvoyMetadataBytes([]byte(` + - = WithEnvoyMetadataBytes([]byte(` { "filter_metadata": { "envoy.filters.http.header_to_metadata": { From 372e51e1cbb384161c7db48fc7d0d5c21e3506c4 Mon Sep 17 00:00:00 2001 From: JanardhanSharma Date: Fri, 26 Apr 2024 18:43:02 +0200 Subject: [PATCH 08/14] fix static analysis error. Signed-off-by: Janardhan Sharma Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/openpolicyagent_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go index 1f3fc6e27d..69c492c5bf 100644 --- a/filters/openpolicyagent/openpolicyagent_test.go +++ b/filters/openpolicyagent/openpolicyagent_test.go @@ -63,7 +63,7 @@ func TestInterpolateTemplate(t *testing.T) { func TestLoadEnvoyMetadata(t *testing.T) { cfg := &OpenPolicyAgentInstanceConfig{} - - = WithEnvoyMetadataBytes([]byte(` + _ = WithEnvoyMetadataBytes([]byte(` { "filter_metadata": { "envoy.filters.http.header_to_metadata": { From eda16cae6feaf2f9b8146576fa99a1cbd8054de3 Mon Sep 17 00:00:00 2001 From: Janardhan Sharma Date: Mon, 29 Apr 2024 23:28:29 +0200 Subject: [PATCH 09/14] test faoilure fix and rename the import variable Signed-off-by: Janardhan Sharma --- .../openpolicyagent/openpolicyagent_test.go | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go index 69c492c5bf..bc2e71fff1 100644 --- a/filters/openpolicyagent/openpolicyagent_test.go +++ b/filters/openpolicyagent/openpolicyagent_test.go @@ -16,6 +16,7 @@ import ( ext_authz_v3_core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" authv3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" + ext_authz_v3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" "github.com/open-policy-agent/opa-envoy-plugin/envoyauth" opaconf "github.com/open-policy-agent/opa/config" opasdktest "github.com/open-policy-agent/opa/sdk/test" @@ -29,7 +30,7 @@ import ( "github.com/zalando/skipper/routing" "github.com/zalando/skipper/tracing/tracingtest" "google.golang.org/protobuf/encoding/protojson" - _struct "google.golang.org/protobuf/types/known/structpb" + pbstruct "google.golang.org/protobuf/types/known/structpb" ) type MockOpenPolicyAgentFilter struct { @@ -77,18 +78,18 @@ func TestLoadEnvoyMetadata(t *testing.T) { `))(cfg) expectedBytes, err := protojson.Marshal(&ext_authz_v3_core.Metadata{ - FilterMetadata: map[string]*_struct.Struct{ + FilterMetadata: map[string]*pbstruct.Struct{ "envoy.filters.http.header_to_metadata": { - Fields: map[string]*_struct.Value{ + Fields: map[string]*pbstruct.Value{ "policy_type": { - Kind: &_struct.Value_StringValue{StringValue: "ingress"}, + Kind: &pbstruct.Value_StringValue{StringValue: "ingress"}, }, }, }, "open_policy_agent": { - Fields: map[string]*_struct.Value{ + Fields: map[string]*pbstruct.Value{ "decision_id": { - Kind: &_struct.Value_StringValue{StringValue: "3b567656-bf28-4a63-a4c4-14407fbd9544"}, + Kind: &pbstruct.Value_StringValue{StringValue: "3b567656-bf28-4a63-a4c4-14407fbd9544"}, }, }, }, @@ -421,7 +422,13 @@ func TestEval(t *testing.T) { span := tracer.StartSpan("open-policy-agent") ctx := opentracing.ContextWithSpan(context.Background(), span) - result, err := inst.Eval(ctx, &authv3.CheckRequest{}) + result, err := inst.Eval(ctx, &authv3.CheckRequest{ + Attributes: &ext_authz_v3.AttributeContext{ + Request: nil, + ContextExtensions: nil, + MetadataContext: nil, + }, + }) assert.NoError(t, err) allowed, err := result.IsAllowed() From d2b0b31e765968ec2efa8d66b2f8b8d467ddec79 Mon Sep 17 00:00:00 2001 From: Janardhan Sharma Date: Sun, 5 May 2024 23:29:08 +0200 Subject: [PATCH 10/14] review comments - refactored code and updated documentation Signed-off-by: Janardhan Sharma --- docs/tutorials/auth.md | 4 +- filters/openpolicyagent/evaluation.go | 37 ++++--------------- .../openpolicyagent/openpolicyagent_test.go | 10 ----- 3 files changed, 9 insertions(+), 42 deletions(-) diff --git a/docs/tutorials/auth.md b/docs/tutorials/auth.md index 92ce9884ca..0ac9363792 100644 --- a/docs/tutorials/auth.md +++ b/docs/tutorials/auth.md @@ -466,14 +466,14 @@ The second argument is parsed as YAML, cannot be nested and values need to be st In Rego this can be used like this `input.attributes.contextExtensions["com.mycompany.myprop"] == "my value"` -### Decision's Identity +### Decision ID in Policies Each evaluation yields a distinct decision, identifiable by its unique decision ID. This decision ID can be located within the input at: `input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id` -Leveraging this ID enables seamless correlation of decisions with the flow ID of the originating request. +Typical use cases are either propagation of the decision ID to downstream systems or returning it as part of the response. As an example this can allow to trouble shoot deny requests by looking up details using the full decision in a control plane. ### Quick Start Rego Playground diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go index 9501becced..67254538c5 100644 --- a/filters/openpolicyagent/evaluation.go +++ b/filters/openpolicyagent/evaluation.go @@ -2,7 +2,6 @@ package openpolicyagent import ( "context" - "encoding/json" "fmt" ext_authz_v3_core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" ext_authz_v3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" @@ -70,43 +69,21 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3. } func setDecisionIdInRequest(req *ext_authz_v3.CheckRequest, decisionId string) { - if metaDataContextDoesNotExist(req) { - req.Attributes.MetadataContext = formFilterMetadata(decisionId) - } else { - req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = formOpenPolicyAgentMetaDataObject(decisionId) - } -} - -func metaDataContextDoesNotExist(req *ext_authz_v3.CheckRequest) bool { - return req.Attributes.MetadataContext == nil -} - -func formFilterMetadata(decisionId string) *ext_authz_v3_core.Metadata { - metaData := &ext_authz_v3_core.Metadata{ - FilterMetadata: map[string]*pbstruct.Struct{ - "open_policy_agent": { - Fields: map[string]*pbstruct.Value{ - "decision_id": { - Kind: &pbstruct.Value_StringValue{StringValue: decisionId}, - }, - }, - }, - }, + if req.Attributes.MetadataContext == nil { + req.Attributes.MetadataContext = &ext_authz_v3_core.Metadata{ + FilterMetadata: map[string]*pbstruct.Struct{}, + } } - return metaData + req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = formOpenPolicyAgentMetaDataObject(decisionId) } func formOpenPolicyAgentMetaDataObject(decisionId string) *pbstruct.Struct { - nestedStruct := &pbstruct.Struct{} - nestedStruct.Fields = make(map[string]*pbstruct.Value) innerFields := make(map[string]interface{}) innerFields["decision_id"] = decisionId - innerBytes, _ := json.Marshal(innerFields) - _ = json.Unmarshal(innerBytes, &nestedStruct) - - return nestedStruct + openPolicyAgentMetaDataObject, _ := pbstruct.NewStruct(innerFields) + return openPolicyAgentMetaDataObject } func (opa *OpenPolicyAgentInstance) logDecision(ctx context.Context, input interface{}, result *envoyauth.EvalResult, err error) error { diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go index bc2e71fff1..1b7f850c5f 100644 --- a/filters/openpolicyagent/openpolicyagent_test.go +++ b/filters/openpolicyagent/openpolicyagent_test.go @@ -70,9 +70,6 @@ func TestLoadEnvoyMetadata(t *testing.T) { "envoy.filters.http.header_to_metadata": { "policy_type": "ingress" }, - "open_policy_agent" : { - "decision_id" : "3b567656-bf28-4a63-a4c4-14407fbd9544" - } } } `))(cfg) @@ -86,13 +83,6 @@ func TestLoadEnvoyMetadata(t *testing.T) { }, }, }, - "open_policy_agent": { - Fields: map[string]*pbstruct.Value{ - "decision_id": { - Kind: &pbstruct.Value_StringValue{StringValue: "3b567656-bf28-4a63-a4c4-14407fbd9544"}, - }, - }, - }, }, }) From 3c12e4b733689c988b9b59b4f241eb4674a262a7 Mon Sep 17 00:00:00 2001 From: Janardhan Sharma Date: Mon, 6 May 2024 13:58:15 +0200 Subject: [PATCH 11/14] duplicate import fixed Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/openpolicyagent_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go index 1b7f850c5f..d6bf877940 100644 --- a/filters/openpolicyagent/openpolicyagent_test.go +++ b/filters/openpolicyagent/openpolicyagent_test.go @@ -16,7 +16,6 @@ import ( ext_authz_v3_core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" authv3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" - ext_authz_v3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" "github.com/open-policy-agent/opa-envoy-plugin/envoyauth" opaconf "github.com/open-policy-agent/opa/config" opasdktest "github.com/open-policy-agent/opa/sdk/test" @@ -413,7 +412,7 @@ func TestEval(t *testing.T) { ctx := opentracing.ContextWithSpan(context.Background(), span) result, err := inst.Eval(ctx, &authv3.CheckRequest{ - Attributes: &ext_authz_v3.AttributeContext{ + Attributes: &authv3.AttributeContext{ Request: nil, ContextExtensions: nil, MetadataContext: nil, From aef72d0fa5e1ae55c427f1c79a5b26ad213a1fbd Mon Sep 17 00:00:00 2001 From: Janardhan Sharma Date: Wed, 8 May 2024 14:53:59 +0200 Subject: [PATCH 12/14] handled error Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/evaluation.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go index 67254538c5..5ac6888176 100644 --- a/filters/openpolicyagent/evaluation.go +++ b/filters/openpolicyagent/evaluation.go @@ -23,7 +23,11 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3. return nil, err } - setDecisionIdInRequest(req, decisionId) + err = setDecisionIdInRequest(req, decisionId) + if err != nil { + opa.Logger().WithFields(map[string]interface{}{"err": err}).Error("Unable to set decision ID in Request.") + return nil, err + } result, stopeval, err := envoyauth.NewEvalResult(withDecisionID(decisionId)) if err != nil { @@ -68,22 +72,28 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3. return result, nil } -func setDecisionIdInRequest(req *ext_authz_v3.CheckRequest, decisionId string) { +func setDecisionIdInRequest(req *ext_authz_v3.CheckRequest, decisionId string) error { if req.Attributes.MetadataContext == nil { req.Attributes.MetadataContext = &ext_authz_v3_core.Metadata{ FilterMetadata: map[string]*pbstruct.Struct{}, } } - req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = formOpenPolicyAgentMetaDataObject(decisionId) + filterMeta, err := formOpenPolicyAgentMetaDataObject(decisionId) + + if err == nil { + req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = filterMeta + } + return err } -func formOpenPolicyAgentMetaDataObject(decisionId string) *pbstruct.Struct { +func formOpenPolicyAgentMetaDataObject(decisionId string) (*pbstruct.Struct, error) { innerFields := make(map[string]interface{}) innerFields["decision_id"] = decisionId - openPolicyAgentMetaDataObject, _ := pbstruct.NewStruct(innerFields) - return openPolicyAgentMetaDataObject + openPolicyAgentMetaDataObject, err := pbstruct.NewStruct(innerFields) + + return openPolicyAgentMetaDataObject, err } func (opa *OpenPolicyAgentInstance) logDecision(ctx context.Context, input interface{}, result *envoyauth.EvalResult, err error) error { From 3307de35b1fb31d069bf417b35dce87bbef40f7b Mon Sep 17 00:00:00 2001 From: Janardhan Sharma Date: Fri, 10 May 2024 13:07:47 +0200 Subject: [PATCH 13/14] review comments Signed-off-by: Janardhan Sharma --- filters/openpolicyagent/evaluation.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go index 5ac6888176..7d3b5a578b 100644 --- a/filters/openpolicyagent/evaluation.go +++ b/filters/openpolicyagent/evaluation.go @@ -78,12 +78,13 @@ func setDecisionIdInRequest(req *ext_authz_v3.CheckRequest, decisionId string) e FilterMetadata: map[string]*pbstruct.Struct{}, } } - filterMeta, err := formOpenPolicyAgentMetaDataObject(decisionId) - if err == nil { - req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = filterMeta + filterMeta, err := formOpenPolicyAgentMetaDataObject(decisionId) + if err != nil { + return err } - return err + req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = filterMeta + return nil } func formOpenPolicyAgentMetaDataObject(decisionId string) (*pbstruct.Struct, error) { @@ -91,9 +92,7 @@ func formOpenPolicyAgentMetaDataObject(decisionId string) (*pbstruct.Struct, err innerFields := make(map[string]interface{}) innerFields["decision_id"] = decisionId - openPolicyAgentMetaDataObject, err := pbstruct.NewStruct(innerFields) - - return openPolicyAgentMetaDataObject, err + return pbstruct.NewStruct(innerFields) } func (opa *OpenPolicyAgentInstance) logDecision(ctx context.Context, input interface{}, result *envoyauth.EvalResult, err error) error { From 44f51050ca0c23eea7d17a7c537005c33b54837d Mon Sep 17 00:00:00 2001 From: Janardhan Sharma Date: Thu, 20 Jun 2024 12:04:39 +0200 Subject: [PATCH 14/14] formatting fix Signed-off-by: Janardhan Sharma --- .../opaauthorizerequest/opaauthorizerequest_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index 82616e0bd4..4dff05ecfa 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -269,8 +269,8 @@ func TestAuthorizeRequestFilter(t *testing.T) { expectedHeaders: map[string][]string{"Decision-Id": {"some-random-decision-id-generated-during-evaluation"}}, backendHeaders: make(http.Header), removeHeaders: make(http.Header), - }, - { + }, + { msg: "Invalid UTF-8 in Path", filterName: "opaAuthorizeRequest", bundleName: "somebundle.tar.gz",