From e114232abd35da4b3313aa73a4e0da18f0fdf2ef Mon Sep 17 00:00:00 2001 From: Anthony Regeda Date: Mon, 6 Jan 2025 16:32:18 +0100 Subject: [PATCH] refact: avoid mem alloc with disabled decision log (#632) Signed-off-by: Anthony Regeda --- Makefile | 2 +- envoyauth/response.go | 10 ++++----- go.mod | 1 + internal/internal.go | 37 +++++++++++++++++---------------- internal/internal_bench_test.go | 23 +++++++++++++++++++- internal/internal_test.go | 4 ++-- opa/decisionlog/decision_log.go | 10 ++------- 7 files changed, 52 insertions(+), 35 deletions(-) diff --git a/Makefile b/Makefile index 348e99ecc..9926a11a2 100644 --- a/Makefile +++ b/Makefile @@ -175,7 +175,7 @@ deploy-ci: docker-login ensure-release-dir start-builder ci-build-linux ci-build .PHONY: test test: generate - $(DISABLE_CGO) $(GO) test -v -bench=. $(PACKAGES) + $(DISABLE_CGO) $(GO) test -v -bench=. -benchmem $(PACKAGES) .PHONY: test-e2e test-e2e: diff --git a/envoyauth/response.go b/envoyauth/response.go index e54c9de55..667d3d23a 100644 --- a/envoyauth/response.go +++ b/envoyauth/response.go @@ -34,6 +34,10 @@ type StopFunc = func() // TransactionCloser should be called to abort the transaction type TransactionCloser func(ctx context.Context, err error) error +func noopTransactionCloser(context.Context, error) error { + return nil // no-op default +} + // NewEvalResult creates a new EvalResult and a StopFunc that is used to stop the timer for metrics func NewEvalResult(opts ...func(*EvalResult)) (*EvalResult, StopFunc, error) { var err error @@ -67,13 +71,9 @@ func NewEvalResult(opts ...func(*EvalResult)) (*EvalResult, StopFunc, error) { func (result *EvalResult) GetTxn(ctx context.Context, store storage.Store) (storage.Transaction, TransactionCloser, error) { params := storage.TransactionParams{} - noopCloser := func(ctx context.Context, err error) error { - return nil // no-op default - } - txn, err := store.NewTransaction(ctx, params) if err != nil { - return nil, noopCloser, err + return nil, noopTransactionCloser, err } // Setup a closer function that will abort the transaction. diff --git a/go.mod b/go.mod index 7ec25914a..fd66d34cb 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,7 @@ module github.com/open-policy-agent/opa-envoy-plugin go 1.22.0 + toolchain go1.23.1 require ( diff --git a/internal/internal.go b/internal/internal.go index 0fcbaf91a..02ad82be2 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -33,11 +33,13 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/reflection" "google.golang.org/protobuf/reflect/protoregistry" + "google.golang.org/protobuf/types/known/structpb" "github.com/open-policy-agent/opa/ast" "github.com/open-policy-agent/opa/config" "github.com/open-policy-agent/opa/logging" "github.com/open-policy-agent/opa/plugins" + "github.com/open-policy-agent/opa/plugins/logs" "github.com/open-policy-agent/opa/rego" "github.com/open-policy-agent/opa/server" "github.com/open-policy-agent/opa/storage" @@ -48,8 +50,6 @@ import ( "go.opentelemetry.io/otel/trace" - _structpb "github.com/golang/protobuf/ptypes/struct" - "github.com/open-policy-agent/opa-envoy-plugin/envoyauth" internal_util "github.com/open-policy-agent/opa-envoy-plugin/internal/util" "github.com/open-policy-agent/opa-envoy-plugin/opa/decisionlog" @@ -385,7 +385,7 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* var evalErr error var internalErr *Error start := time.Now() - logger := p.manager.Logger() + logger := p.Logger() result, stopeval, err := envoyauth.NewEvalResult() if err != nil { @@ -417,10 +417,10 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* p.metricErrorCounter.With(prometheus.Labels{"reason": internalErr.Code}).Inc() } } - logErr := p.log(ctx, input, result, err) + logErr := p.logDecision(ctx, input, result, err) if logErr != nil { _ = txnClose(ctx, logErr) // Ignore error - p.Logger().WithFields(map[string]interface{}{"err": logErr}).Debug("Error when logging event") + logger.WithFields(map[string]interface{}{"err": logErr}).Debug("Error when logging event") if p.cfg.EnablePerformanceMetrics { p.metricErrorCounter.With(prometheus.Labels{"reason": "unknown_log_error"}).Inc() } @@ -472,8 +472,6 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* return nil, stop, internalErr } - resp := &ext_authz_v3.CheckResponse{} - var allowed bool allowed, err = result.IsAllowed() if err != nil { @@ -482,6 +480,8 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* return nil, stop, internalErr } + resp := &ext_authz_v3.CheckResponse{} + status := int32(code.Code_PERMISSION_DENIED) if allowed { status = int32(code.Code_OK) @@ -498,7 +498,7 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* return nil, stop, internalErr } - var dynamicMetadata *_structpb.Struct + var dynamicMetadata *structpb.Struct dynamicMetadata, err = result.GetDynamicMetadata() if err != nil { err = errors.Wrap(err, "failed to get dynamic metadata") @@ -578,7 +578,7 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* Observe(float64(totalDecisionTime.Seconds())) } - p.manager.Logger().WithFields(map[string]interface{}{ + logger.WithFields(map[string]interface{}{ "query": p.cfg.parsedQuery.String(), "dry-run": p.cfg.DryRun, "decision": result.Decision, @@ -601,21 +601,22 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* // Add decision_id to dynamic metadata if resp.DynamicMetadata == nil { - resp.DynamicMetadata = &_structpb.Struct{ - Fields: map[string]*_structpb.Value{}, + resp.DynamicMetadata = &structpb.Struct{ + Fields: make(map[string]*structpb.Value, 1), } } - resp.DynamicMetadata.Fields["decision_id"] = &_structpb.Value{ - Kind: &_structpb.Value_StringValue{ - StringValue: result.DecisionID, - }, - } + resp.DynamicMetadata.Fields["decision_id"] = structpb.NewStringValue(result.DecisionID) return resp, stop, nil } -func (p *envoyExtAuthzGrpcServer) log(ctx context.Context, input interface{}, result *envoyauth.EvalResult, err error) error { +func (p *envoyExtAuthzGrpcServer) logDecision(ctx context.Context, input interface{}, result *envoyauth.EvalResult, err error) error { + plugin := logs.Lookup(p.manager) + if plugin == nil { + return nil + } + info := &server.Info{ Timestamp: time.Now(), Input: &input, @@ -643,7 +644,7 @@ func (p *envoyExtAuthzGrpcServer) log(ctx context.Context, input interface{}, re info.NDBuiltinCache = &x } - return decisionlog.LogDecision(ctx, p.manager, info, result, err) + return decisionlog.LogDecision(ctx, plugin, info, result, err) } func stringPathToDataRef(s string) (r ast.Ref) { diff --git a/internal/internal_bench_test.go b/internal/internal_bench_test.go index 03a97c941..2337ede9a 100644 --- a/internal/internal_bench_test.go +++ b/internal/internal_bench_test.go @@ -17,7 +17,28 @@ import ( func BenchmarkCheck(b *testing.B) { var req ext_authz.CheckRequest if err := util.Unmarshal([]byte(exampleAllowedRequest), &req); err != nil { - panic(err) + b.Fatal(err) + } + + server := testAuthzServer(nil) + ctx := context.Background() + + b.ResetTimer() + for i := 0; i < b.N; i++ { + output, err := server.Check(ctx, &req) + if err != nil { + b.Fatal(err) + } + if output.Status.Code != int32(code.Code_OK) { + b.Fatal("Expected request to be allowed but got:", output) + } + } +} + +func BenchmarkCheck_withCustomLogger(b *testing.B) { + var req ext_authz.CheckRequest + if err := util.Unmarshal([]byte(exampleAllowedRequest), &req); err != nil { + b.Fatal(err) } server := testAuthzServer(nil, withCustomLogger(&testPlugin{})) diff --git a/internal/internal_test.go b/internal/internal_test.go index 852a554df..0d4bc860f 100644 --- a/internal/internal_test.go +++ b/internal/internal_test.go @@ -2138,7 +2138,7 @@ func TestPrometheusMetrics(t *testing.T) { func TestLogWithASTError(t *testing.T) { server := testAuthzServer(nil, withCustomLogger(&testPlugin{})) - err := server.log(context.Background(), nil, &envoyauth.EvalResult{}, &ast.Error{Code: "foo"}) + err := server.logDecision(context.Background(), nil, &envoyauth.EvalResult{}, &ast.Error{Code: "foo"}) if err != nil { panic(err) } @@ -2149,7 +2149,7 @@ func TestLogWithCancelError(t *testing.T) { customLogger := &testPlugin{} server := testAuthzServer(nil, withCustomLogger(customLogger)) - err := server.log(context.Background(), nil, &envoyauth.EvalResult{}, &topdown.Error{ + err := server.logDecision(context.Background(), nil, &envoyauth.EvalResult{}, &topdown.Error{ Code: topdown.CancelErr, Message: "caller cancelled query execution", }) diff --git a/opa/decisionlog/decision_log.go b/opa/decisionlog/decision_log.go index 9c459fbba..1d5afa437 100644 --- a/opa/decisionlog/decision_log.go +++ b/opa/decisionlog/decision_log.go @@ -5,7 +5,6 @@ import ( "github.com/open-policy-agent/opa-envoy-plugin/envoyauth" "github.com/open-policy-agent/opa/ast" - "github.com/open-policy-agent/opa/plugins" "github.com/open-policy-agent/opa/plugins/logs" "github.com/open-policy-agent/opa/server" "github.com/open-policy-agent/opa/storage" @@ -21,15 +20,10 @@ func (e *internalError) Error() string { } // LogDecision - Logs a decision log event -func LogDecision(ctx context.Context, manager *plugins.Manager, info *server.Info, result *envoyauth.EvalResult, err error) error { - plugin := logs.Lookup(manager) - if plugin == nil { - return nil - } - +func LogDecision(ctx context.Context, plugin *logs.Plugin, info *server.Info, result *envoyauth.EvalResult, err error) error { info.Revision = result.Revision - bundles := map[string]server.BundleInfo{} + bundles := make(map[string]server.BundleInfo, len(result.Revisions)) for name, rev := range result.Revisions { bundles[name] = server.BundleInfo{Revision: rev} }