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 <east.patrick@gmail.com>

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

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,

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",

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
 		}