Skip to content

Commit

Permalink
Merge pull request #2043 from authzed/2036-enforce-usage-of-VT
Browse files Browse the repository at this point in the history
Add analyzer to enforce usage of VT versions of marshalling and unmarshalling
  • Loading branch information
tstirrat15 authored Aug 30, 2024
2 parents d8906b5 + 0dd3667 commit 506248a
Show file tree
Hide file tree
Showing 34 changed files with 510 additions and 87 deletions.
2 changes: 1 addition & 1 deletion e2e/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ require (
github.com/jzelinskie/stringz v0.0.3 // indirect
github.com/mattn/go-colorable v0.1.13 // indirect
github.com/mattn/go-isatty v0.0.20 // indirect
github.com/planetscale/vtprotobuf v0.6.0 // indirect
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca // indirect
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
github.com/rs/zerolog v1.33.0 // indirect
github.com/samber/lo v1.46.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions e2e/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ github.com/ngrok/sqlmw v0.0.0-20220520173518-97c9c04efc79/go.mod h1:E26fwEtRNigB
github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/planetscale/vtprotobuf v0.6.0 h1:nBeETjudeJ5ZgBHUz1fVHvbqUKnYOXNhsIEabROxmNA=
github.com/planetscale/vtprotobuf v0.6.0/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8=
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca h1:ujRGEVWJEoaxQ+8+HMl8YEpGaDAgohgZxJ5S+d2TTFQ=
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
Expand Down
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ require (
cloud.google.com/go/spanner v1.65.0
contrib.go.opencensus.io/exporter/prometheus v0.4.2
github.com/IBM/pgxpoolprometheus v1.1.1
github.com/KimMachineGun/automemlimit v0.6.1 // indirect
github.com/KimMachineGun/automemlimit v0.6.1
github.com/Masterminds/squirrel v1.5.4
github.com/authzed/authzed-go v0.14.0

Expand Down Expand Up @@ -67,7 +67,7 @@ require (
github.com/ory/dockertest/v3 v3.10.0
github.com/outcaste-io/ristretto v0.2.3
github.com/pbnjay/memory v0.0.0-20210728143218-7b4eea64cf58
github.com/planetscale/vtprotobuf v0.6.0
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca
github.com/prometheus/client_golang v1.19.1
github.com/prometheus/client_model v0.6.1
github.com/prometheus/common v0.55.0
Expand Down
8 changes: 2 additions & 6 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1246,10 +1246,6 @@ github.com/julz/importas v0.1.0 h1:F78HnrsjY3cR7j0etXy5+TU1Zuy7Xt08X/1aJnH5xXY=
github.com/julz/importas v0.1.0/go.mod h1:oSFU2R4XK/P7kNBrnL/FEQlDGN1/6WoxXEjSSXO0DV0=
github.com/jung-kurt/gofpdf v1.0.0/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes=
github.com/jung-kurt/gofpdf v1.0.3-0.20190309125859-24315acbbda5/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes=
github.com/jzelinskie/cobrautil/v2 v2.0.0-20240813173937-98b79ae0b499 h1:dXbwn1pwooxn2DAnPF3SR7tNPqC6N4VmwftMrapCYng=
github.com/jzelinskie/cobrautil/v2 v2.0.0-20240813173937-98b79ae0b499/go.mod h1:jsl6cEF6BT3UeQoSLreA7G0sZXemoI5XNqyxzWCohbE=
github.com/jzelinskie/cobrautil/v2 v2.0.0-20240816002907-ef0e64d7f25b h1:dUjc3twJXVQ7FILS1+KhHilbM7LQwIvVgH4E7h0AwTA=
github.com/jzelinskie/cobrautil/v2 v2.0.0-20240816002907-ef0e64d7f25b/go.mod h1:jsl6cEF6BT3UeQoSLreA7G0sZXemoI5XNqyxzWCohbE=
github.com/jzelinskie/cobrautil/v2 v2.0.0-20240819150235-f7fe73942d0f h1:+WgAZQQXj+X8lcJdwcrQpD89Zd9ekdauOK3hWl3FkPU=
github.com/jzelinskie/cobrautil/v2 v2.0.0-20240819150235-f7fe73942d0f/go.mod h1:jsl6cEF6BT3UeQoSLreA7G0sZXemoI5XNqyxzWCohbE=
github.com/jzelinskie/persistent v0.0.0-20230816160542-1205ef8f0e15 h1:lFr5Krrc4LESaXK9yW15IQMZ4p2bZGw/+71Z1dV6tuk=
Expand Down Expand Up @@ -1433,8 +1429,8 @@ github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/sftp v1.10.1/go.mod h1:lYOWFsE0bwd1+KfKJaKeuokY15vzFx25BLbzYYoAxZI=
github.com/pkg/sftp v1.13.1/go.mod h1:3HaPG6Dq1ILlpPZRO0HVMrsydcdLt6HRDccSgb87qRg=
github.com/planetscale/vtprotobuf v0.6.0 h1:nBeETjudeJ5ZgBHUz1fVHvbqUKnYOXNhsIEabROxmNA=
github.com/planetscale/vtprotobuf v0.6.0/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8=
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca h1:ujRGEVWJEoaxQ+8+HMl8YEpGaDAgohgZxJ5S+d2TTFQ=
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
Expand Down
3 changes: 1 addition & 2 deletions internal/datastore/crdb/readwrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/jackc/pgx/v5"
"github.com/jzelinskie/stringz"
"google.golang.org/protobuf/proto"

pgxcommon "github.com/authzed/spicedb/internal/datastore/postgres/common"
"github.com/authzed/spicedb/internal/datastore/revisions"
Expand Down Expand Up @@ -378,7 +377,7 @@ func (rwt *crdbReadWriteTXN) WriteNamespaces(ctx context.Context, newConfigs ...
for _, newConfig := range newConfigs {
rwt.addOverlapKey(newConfig.Name)

serialized, err := proto.Marshal(newConfig)
serialized, err := newConfig.MarshalVT()
if err != nil {
return fmt.Errorf(errUnableToWriteConfig, err)
}
Expand Down
3 changes: 1 addition & 2 deletions internal/datastore/memdb/readwrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/hashicorp/go-memdb"
"github.com/jzelinskie/stringz"
"google.golang.org/protobuf/proto"

"github.com/authzed/spicedb/internal/datastore/common"
"github.com/authzed/spicedb/pkg/datastore"
Expand Down Expand Up @@ -265,7 +264,7 @@ func (rwt *memdbReadWriteTx) WriteNamespaces(_ context.Context, newConfigs ...*c
}

for _, newConfig := range newConfigs {
serialized, err := proto.Marshal(newConfig)
serialized, err := newConfig.MarshalVT()
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions internal/datastore/mysql/readwrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/go-sql-driver/mysql"
"github.com/jzelinskie/stringz"
"google.golang.org/protobuf/proto"

"github.com/authzed/spicedb/internal/datastore/common"
"github.com/authzed/spicedb/internal/datastore/revisions"
Expand Down Expand Up @@ -387,7 +386,7 @@ func (rwt *mysqlReadWriteTXN) WriteNamespaces(ctx context.Context, newNamespaces
writeQuery := rwt.WriteNamespaceQuery

for _, newNamespace := range newNamespaces {
serialized, err := proto.Marshal(newNamespace)
serialized, err := newNamespace.MarshalVT()
if err != nil {
return fmt.Errorf(errUnableToWriteConfig, err)
}
Expand Down
3 changes: 1 addition & 2 deletions internal/datastore/postgres/readwrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/jackc/pgx/v5"
"github.com/jzelinskie/stringz"
"google.golang.org/protobuf/proto"

pgxcommon "github.com/authzed/spicedb/internal/datastore/postgres/common"
"github.com/authzed/spicedb/pkg/datastore"
Expand Down Expand Up @@ -496,7 +495,7 @@ func (rwt *pgReadWriteTXN) WriteNamespaces(ctx context.Context, newConfigs ...*c
writeQuery := writeNamespace

for _, newNamespace := range newConfigs {
serialized, err := proto.Marshal(newNamespace)
serialized, err := newNamespace.MarshalVT()
if err != nil {
return fmt.Errorf(errUnableToWriteConfig, err)
}
Expand Down
3 changes: 1 addition & 2 deletions internal/datastore/spanner/readwrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
sq "github.com/Masterminds/squirrel"
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/jzelinskie/stringz"
"google.golang.org/protobuf/proto"

"github.com/authzed/spicedb/internal/datastore/revisions"
log "github.com/authzed/spicedb/internal/logging"
Expand Down Expand Up @@ -326,7 +325,7 @@ func caveatVals(r *core.RelationTuple) []any {
func (rwt spannerReadWriteTXN) WriteNamespaces(_ context.Context, newConfigs ...*core.NamespaceDefinition) error {
mutations := make([]*spanner.Mutation, 0, len(newConfigs))
for _, newConfig := range newConfigs {
serialized, err := proto.Marshal(newConfig)
serialized, err := newConfig.MarshalVT()
if err != nil {
return fmt.Errorf(errUnableToWriteConfig, err)
}
Expand Down
142 changes: 142 additions & 0 deletions internal/namespace/test/serialization_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
package namespace

import (
"fmt"
"os"
"strings"
"testing"

"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

"github.com/authzed/spicedb/pkg/genutil/mapz"
core "github.com/authzed/spicedb/pkg/proto/core/v1"
"github.com/authzed/spicedb/pkg/schemadsl/compiler"
"github.com/authzed/spicedb/pkg/schemadsl/input"
"github.com/authzed/spicedb/pkg/testutil"
)

/*
NOTE: this test exists because we found a place where MarshalVT
was producing a different serialization than proto.Marshal. The
idea is that each time we regenerate our _vtproto.pb.go files,
we run this generation, and then the serialization_test will
use these snapshots to assert that nothing has changed.
*/

type serializationTest struct {
name string
schema string
}

var serializationTests = []serializationTest{
{"Basic serialization test", "basic"},
}

type definitionInterface interface {
protoreflect.ProtoMessage
UnmarshalVT([]byte) error
}

func assertParity(t *testing.T, filenames []string, emptyProto definitionInterface) {
vtProtoDefinitions := make(map[string]definitionInterface)
standardProtoDefinitions := make(map[string]definitionInterface)
definitions := mapz.NewSet[string]()
for _, filename := range filenames {
definition := strings.Split(filename, ".")[0]
definitions.Add(definition)
bytes, err := os.ReadFile(fmt.Sprintf("testdata/proto/%s", filename))
require.NoError(t, err)

standardRepresentation := proto.Clone(emptyProto).(definitionInterface)
err = proto.Unmarshal(bytes, standardRepresentation)
require.NoError(t, err)
standardProtoDefinitions[filename] = standardRepresentation

vtRepresentation := proto.Clone(emptyProto).(definitionInterface)
err = vtRepresentation.UnmarshalVT(bytes)
require.NoError(t, err)
vtProtoDefinitions[filename] = vtRepresentation
}

// For each namespace, we want to assert that all of the following are equivalent:
// standard serialization -> standard deserialization
// vt serialization -> standard deserialization
// standard serialization -> vt deserialization
// This is to validate that the vt serialization/deserialization isn't doing anything unexpected
// compared to the "official" serialization/deserialization

for _, definition := range definitions.AsSlice() {
vtFilename := fmt.Sprintf("%s.vtproto", definition)
standardFilename := fmt.Sprintf("%s.proto", definition)

testutil.RequireProtoEqual(t, standardProtoDefinitions[vtFilename], standardProtoDefinitions[standardFilename], "vt and standard serializations disagree")
testutil.RequireProtoEqual(t, standardProtoDefinitions[standardFilename], vtProtoDefinitions[standardFilename], "vt and standard deserializations of standard proto disagree")
testutil.RequireProtoEqual(t, vtProtoDefinitions[standardFilename], vtProtoDefinitions[vtFilename], "vt and standard deserializations of vt proto disagree")
}
}

func TestSerialization(t *testing.T) {
for _, test := range serializationTests {
test := test
t.Run(test.name, func(t *testing.T) {
require := require.New(t)

if os.Getenv("REGEN") == "true" {
schema, err := os.ReadFile(fmt.Sprintf("testdata/schema/%s.zed", test.schema))
require.NoError(err)
compiled, _ := compiler.Compile(compiler.InputSchema{
Source: input.Source("schema"),
SchemaString: string(schema),
}, compiler.AllowUnprefixedObjectType())

for _, objectDef := range compiled.ObjectDefinitions {
protoSerialized, err := proto.Marshal(objectDef)
require.NoError(err)
err = os.WriteFile(fmt.Sprintf("testdata/proto/%s-definition-%s.proto", test.schema, objectDef.Name), protoSerialized, 0o600)
require.NoError(err)

vtSerialized, err := objectDef.MarshalVT()
require.NoError(err)
err = os.WriteFile(fmt.Sprintf("testdata/proto/%s-definition-%s.vtproto", test.schema, objectDef.Name), vtSerialized, 0o600)
require.NoError(err)
}
for _, caveatDef := range compiled.CaveatDefinitions {
protoSerialized, err := proto.Marshal(caveatDef)
require.NoError(err)
err = os.WriteFile(fmt.Sprintf("testdata/proto/%s-caveat-%s.proto", test.schema, caveatDef.Name), protoSerialized, 0o600)
require.NoError(err)

vtSerialized, err := caveatDef.MarshalVT()
require.NoError(err)
err = os.WriteFile(fmt.Sprintf("testdata/proto/%s-caveat-%s.vtproto", test.schema, caveatDef.Name), vtSerialized, 0o600)
require.NoError(err)
}
} else {
files, err := os.ReadDir("testdata/proto")
require.NoError(err)

definitionFiles := mapz.NewSet[string]()
caveatFiles := mapz.NewSet[string]()
for _, file := range files {
filename := file.Name()
if strings.Contains(filename, test.schema) {
// NOTE: this makes some assumptions about the names of the files,
// namely that a schema file name will not have either of these
// keywords.
if strings.Contains(filename, "definition") {
definitionFiles.Add(filename)
}
if strings.Contains(filename, "caveat") {
caveatFiles.Add(filename)
}
}
}

assertParity(t, definitionFiles.AsSlice(), &core.NamespaceDefinition{})
assertParity(t, caveatFiles.AsSlice(), &core.CaveatDefinition{})
}
})
}
}
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
14 changes: 14 additions & 0 deletions internal/namespace/test/testdata/schema/basic.zed
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
caveat foo(someParam int) {
someParam == 42
}

definition document {
relation viewer: user | user:*
relation editor: user | group#member with foo
relation parent: organization
permission edit = editor
permission view = viewer + edit + parent->view
permission other = viewer - edit
permission intersect = viewer & edit
permission with_nil = (viewer - edit) & parent->view & nil
}
2 changes: 1 addition & 1 deletion magefiles/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ require (
github.com/ecordell/optgen v0.0.9
github.com/envoyproxy/protoc-gen-validate v1.0.4
github.com/magefile/mage v1.15.0
github.com/planetscale/vtprotobuf v0.5.1-0.20231212170721-e7d721933795
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca
golang.org/x/tools v0.22.0
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.3.0
google.golang.org/protobuf v1.34.2
Expand Down
2 changes: 2 additions & 0 deletions magefiles/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,8 @@ github.com/pkg/profile v1.7.0/go.mod h1:8Uer0jas47ZQMJ7VD+OHknK4YDY07LPUC6dEvqDj
github.com/pkg/sftp v1.13.1/go.mod h1:3HaPG6Dq1ILlpPZRO0HVMrsydcdLt6HRDccSgb87qRg=
github.com/planetscale/vtprotobuf v0.5.1-0.20231212170721-e7d721933795 h1:pH+U6pJP0BhxqQ4njBUjOg0++WMMvv3eByWzB+oATBY=
github.com/planetscale/vtprotobuf v0.5.1-0.20231212170721-e7d721933795/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8=
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca h1:ujRGEVWJEoaxQ+8+HMl8YEpGaDAgohgZxJ5S+d2TTFQ=
github.com/planetscale/vtprotobuf v0.6.1-0.20240409071808-615f978279ca/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
Expand Down
8 changes: 8 additions & 0 deletions magefiles/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ func (Lint) Analyzers() error {
"-paniccheck.skip-files=_test,zz_",
"-zerologmarshalcheck",
"-zerologmarshalcheck.skip-files=_test,zz_",
"-protomarshalcheck",
// Skip generated protobuf files for this check
// Also skip test where we're explicitly using proto.Marshal to assert
// that the proto.Marshal behavior matches foo.MarshalVT()
"-protomarshalcheck.skip-files=.pb,serialization_test.go",
// Skip our dispatch codec logic that explicitly calls MarshalVT with proto.Marshal as a fallback
// Skip our internal telemetry reporter which uses a prometheus proto definition that we don't control
"-protomarshalcheck.skip-pkg=github.com/authzed/spicedb/pkg/proto/dispatch/v1,github.com/authzed/spicedb/internal/telemetry",
"github.com/authzed/spicedb/...",
)
}
Expand Down
Loading

0 comments on commit 506248a

Please sign in to comment.