From 9c72425d94bfb317ac9690bd63d2d4dbf7da71d9 Mon Sep 17 00:00:00 2001 From: Patrick East Date: Thu, 23 Jan 2020 13:41:33 -0800 Subject: [PATCH] decision logger: Leave the path unchanged for decisions The logger was swapping `.`'s with `/`'s but this isn't safe when a valid path should be /foo/a.b.c/main. The server was already doing the right thing by passing in the url path where applicable, or only specifying a query instead of the path. This might affect anyone using the decision logger golang API passing in something in dot-notation and expecting it to come out with paths. Anyone using the HTTP server should be unaffected. Fixes: #2031 Signed-off-by: Patrick East --- CHANGELOG.md | 12 ++++++++++++ plugins/logs/plugin.go | 4 +--- plugins/logs/plugin_test.go | 29 +++++++++++++++++------------ server/server.go | 2 +- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93189691f1..96399b1845 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ project adheres to [Semantic Versioning](http://semver.org/). ## Unreleased +### Compatibility Notes + +- Related to the fix for [#2031](https://github.com/open-policy-agent/opa/issues/2031) + and the changes with OPA v0.16.0 to use `/` separated `path`'s with + the decision log plugin API. The decision logger will no longer modify + the `server.Info#Path` field. Older versions would substitute `.` for + `/` but this was causing incorrect results. As of v0.16.0 the server has + been updated to provide the correct paths so REST API users are unaffected. + Golang API users of the `plugins.log.Logger#Log` interface may be impacted + if passing `ast.Ref` style strings as a path as it will no longer be changed + to `/` separated. Callers need to do any transformation beforehand. + ## 0.16.2 This release includes an important bugfix for users that enable diff --git a/plugins/logs/plugin.go b/plugins/logs/plugin.go index 94633e954d..43738b59f3 100644 --- a/plugins/logs/plugin.go +++ b/plugins/logs/plugin.go @@ -266,8 +266,6 @@ func (p *Plugin) Stop(ctx context.Context) { // Log appends a decision log event to the buffer for uploading. func (p *Plugin) Log(ctx context.Context, decision *server.Info) error { - path := strings.Replace(strings.TrimPrefix(decision.Path, "data."), ".", "/", -1) - bundles := map[string]BundleInfoV1{} for name, info := range decision.Bundles { bundles[name] = BundleInfoV1{Revision: info.Revision} @@ -278,7 +276,7 @@ func (p *Plugin) Log(ctx context.Context, decision *server.Info) error { DecisionID: decision.DecisionID, Revision: decision.Revision, Bundles: bundles, - Path: path, + Path: decision.Path, Query: decision.Query, Input: decision.Input, Result: decision.Results, diff --git a/plugins/logs/plugin_test.go b/plugins/logs/plugin_test.go index acfd776df0..1959571f05 100644 --- a/plugins/logs/plugin_test.go +++ b/plugins/logs/plugin_test.go @@ -144,21 +144,26 @@ func TestPluginQueriesAndPaths(t *testing.T) { } plugin := New(config, manager) - plugin.Log(ctx, &server.Info{Path: "data.foo"}) - plugin.Log(ctx, &server.Info{Path: "data.foo.bar"}) + plugin.Log(ctx, &server.Info{Path: "/"}) + plugin.Log(ctx, &server.Info{Path: "/data"}) // /v1/data/data case + plugin.Log(ctx, &server.Info{Path: "/foo"}) + plugin.Log(ctx, &server.Info{Path: "foo"}) + plugin.Log(ctx, &server.Info{Path: "/foo/bar"}) + plugin.Log(ctx, &server.Info{Path: "a.b.c"}) + plugin.Log(ctx, &server.Info{Path: "/foo/a.b.c/bar"}) plugin.Log(ctx, &server.Info{Query: "a = data.foo"}) exp := []struct { query string path string }{ - // TODO(tsandall): we need to fix how POST /v1/data (and - // friends) are represented here. Currently we can't tell the - // difference between /v1/data and /v1/data/data. The decision - // log event paths should be slash prefixed to avoid ambiguity. - // {path: "data"}, + {path: "/"}, + {path: "/data"}, + {path: "/foo"}, {path: "foo"}, - {path: "foo/bar"}, + {path: "/foo/bar"}, + {path: "a.b.c"}, + {path: "/foo/a.b.c/bar"}, {query: "a = data.foo"}, } @@ -168,7 +173,7 @@ func TestPluginQueriesAndPaths(t *testing.T) { for i, e := range exp { if e.query != backend.events[i].Query || e.path != backend.events[i].Path { - t.Fatalf("Unexpected event %d, want %v but got %v", i, e, backend.events[i]) + t.Fatalf("Unexpected event %d, want %+v but got %+v", i, e, backend.events[i]) } } } @@ -196,7 +201,7 @@ func TestPluginStartSameInput(t *testing.T) { fixture.plugin.Log(ctx, &server.Info{ Revision: fmt.Sprint(i), DecisionID: fmt.Sprint(i), - Path: "data.tda.bar", + Path: "tda/bar", Input: &input, Results: &result, RemoteAddr: "test", @@ -274,7 +279,7 @@ func TestPluginStartChangingInputValues(t *testing.T) { fixture.plugin.Log(ctx, &server.Info{ Revision: fmt.Sprint(i), DecisionID: fmt.Sprint(i), - Path: "data.foo.bar", + Path: "foo/bar", Input: &input, Results: &result, RemoteAddr: "test", @@ -345,7 +350,7 @@ func TestPluginStartChangingInputKeysAndValues(t *testing.T) { fixture.plugin.Log(ctx, &server.Info{ Revision: fmt.Sprint(i), DecisionID: fmt.Sprint(i), - Path: "data.foo.bar", + Path: "foo/bar", Input: &input, Results: &result, RemoteAddr: "test", diff --git a/server/server.go b/server/server.go index 08024bc482..fade60b99f 100644 --- a/server/server.go +++ b/server/server.go @@ -816,7 +816,7 @@ func (s *Server) v0QueryPath(w http.ResponseWriter, r *http.Request, urlPath str ) pq, err := rego.PrepareForEval(ctx) if err != nil { - _ = logger.Log(ctx, txn, decisionID, r.RemoteAddr, path.String(), "", goInput, nil, err, m) + _ = logger.Log(ctx, txn, decisionID, r.RemoteAddr, urlPath, "", goInput, nil, err, m) writer.ErrorAuto(w, err) return }