Merge 1.5.1 bugfix into master (#669)

* Fix: code:3 - proto: (line 1:102): invalid value for string null (#665) * WIP reproduce: code:3 - proto: (line 1:102): invalid value for string type: null #664 * pipefail smh * Replace github.com/golang/protobuf with google.golang.org/protobuf * just eo pipefail * fix shakedown trap * got failing test * move around * Add v1toV2MarshallerAdapter to fix backwards compatability issue * Update Changelog * Dont need to suffix here * Use node 16 * Oops forgot print body * RM this for now until come up with a better solution * Fix changelog
flipt-io · Jan 27, 2022 · f6a532f · f6a532f
1 parent a252b64
commit f6a532f
Show file tree

Hide file tree

Showing 21 changed files with 150 additions and 124 deletions.
diff --git a/.tool-versions b/.tool-versions
@@ -1,2 +1,2 @@
 golang 1.17.6
-nodejs 16.13.2
+nodejs 16.13.2
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,12 @@
 This format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [v1.5.1](https://github.com/markphelps/flipt/releases/tag/v1.5.1) - 2022-01-26
+
+### Fixed
+
+- Backwards compatability issue with using `null` as a string field in the `context` map for `Evaluate`. [https://github.com/markphelps/flipt/issues/664](https://github.com/markphelps/flipt/issues/664)
+
 ## [v1.5.0](https://github.com/markphelps/flipt/releases/tag/v1.5.0) - 2022-01-11
 
 ### Changed

diff --git a/cmd/flipt/main.go b/cmd/flipt/main.go
@@ -300,11 +300,6 @@ func run(_ []string) error {
 			_ = lis.Close()
 		}()
 
-		var (
-			grpcOpts []grpc.ServerOption
-			srv      *server.Server
-		)
-
 		db, driver, err := sql.Open(*cfg)
 		if err != nil {
 			return fmt.Errorf("opening db: %w", err)
@@ -336,8 +331,6 @@ func run(_ []string) error {
 
 		logger = logger.WithField("store", store.String())
 
-		srv = server.New(logger, store)
-
 		var tracer opentracing.Tracer = &opentracing.NoopTracer{}
 
 		if cfg.Tracing.Jaeger.Enabled {
@@ -366,6 +359,11 @@ func run(_ []string) error {
 
 		opentracing.SetGlobalTracer(tracer)
 
+		var (
+			grpcOpts []grpc.ServerOption
+			srv      = server.New(logger, store)
+		)
+
 		grpcOpts = append(grpcOpts, grpc_middleware.WithUnaryServerChain(
 			grpc_recovery.UnaryServerInterceptor(),
 			grpc_ctxtags.UnaryServerInterceptor(),
@@ -399,8 +397,18 @@ func run(_ []string) error {
 		logger := l.WithField("server", cfg.Server.Protocol.String())
 
 		var (
+			// This is required to fix a backwards compatibility issue with the v2 marshaller where `null` map values
+			// cause an error because they are not allowed by the proto spec, but they were handled by the v1 marshaller.
+			//
+			// See: rpc/flipt/marshal.go
+			//
+			// See: https://github.com/markphelps/flipt/issues/664
+			muxOpts = []grpc_gateway.ServeMuxOption{
+				grpc_gateway.WithMarshalerOption(grpc_gateway.MIMEWildcard, pb.NewV1toV2MarshallerAdapter()),
+			}
+
 			r        = chi.NewRouter()
-			api      = grpc_gateway.NewServeMux()
+			api      = grpc_gateway.NewServeMux(muxOpts...)
 			opts     = []grpc.DialOption{grpc.WithBlock()}
 			httpPort int
 		)

diff --git a/go.mod b/go.mod
@@ -17,10 +17,10 @@ require (
 	github.com/go-sql-driver/mysql v1.6.0
 	github.com/gofrs/uuid v4.2.0+incompatible
 	github.com/golang-migrate/migrate v3.5.4+incompatible
-	github.com/golang/protobuf v1.5.2
 	github.com/google/go-github/v32 v32.1.0
 	github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
+	github.com/grpc-ecosystem/grpc-gateway v1.16.0
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.3
 	github.com/kr/text v0.2.0 // indirect
 	github.com/lib/pq v1.10.4

diff --git a/rpc/flipt/flipt.pb.go b/rpc/flipt/flipt.pb.go
diff --git a/rpc/flipt/flipt_grpc.pb.go b/rpc/flipt/flipt_grpc.pb.go
diff --git a/rpc/flipt/marshaller.go b/rpc/flipt/marshaller.go
@@ -0,0 +1,43 @@
+package flipt
+
+import (
+	"io"
+
+	grpc_gateway_v1 "github.com/grpc-ecosystem/grpc-gateway/runtime"
+	grpc_gateway_v2 "github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
+)
+
+var _ grpc_gateway_v2.Marshaler = &V1toV2MarshallerAdapter{}
+
+// V1toV2MarshallerAdapter is a V1 to V2 marshaller adapter to be able to use the v1 marshaller
+// from grpc-gateway v2.
+//
+// This is required to fix a backwards compatibility issue with the v2 marshaller where `null` map values
+// cause an error because they are not allowed by the proto spec, but they were handled by the v1 marshaller.
+//
+// See: https://github.com/markphelps/flipt/issues/664
+//
+// TODO: remove this custom marshaller for Flipt API v2 as we want to use the default v2 marshaller directly.
+type V1toV2MarshallerAdapter struct {
+	*grpc_gateway_v1.JSONPb
+}
+
+func NewV1toV2MarshallerAdapter() *V1toV2MarshallerAdapter {
+	return &V1toV2MarshallerAdapter{&grpc_gateway_v1.JSONPb{OrigName: false, EmitDefaults: true}}
+}
+
+func (m *V1toV2MarshallerAdapter) ContentType(_ interface{}) string {
+	return m.JSONPb.ContentType()
+}
+
+func (m *V1toV2MarshallerAdapter) Marshal(v interface{}) ([]byte, error) {
+	return m.JSONPb.Marshal(v)
+}
+
+func (m *V1toV2MarshallerAdapter) NewDecoder(r io.Reader) grpc_gateway_v2.Decoder {
+	return m.JSONPb.NewDecoder(r)
+}
+
+func (m *V1toV2MarshallerAdapter) NewEncoder(w io.Writer) grpc_gateway_v2.Encoder {
+	return m.JSONPb.NewEncoder(w)
+}
diff --git a/server/evaluator.go b/server/evaluator.go
@@ -11,10 +11,10 @@ import (
 	"time"
 
 	"github.com/gofrs/uuid"
-	"github.com/golang/protobuf/ptypes"
 	errs "github.com/markphelps/flipt/errors"
 	flipt "github.com/markphelps/flipt/rpc/flipt"
 	"github.com/markphelps/flipt/storage"
+	timestamp "google.golang.org/protobuf/types/known/timestamppb"
 )
 
 // Evaluate evaluates a request for a given flag and entity
@@ -89,8 +89,8 @@ func (s *Server) batchEvaluate(ctx context.Context, r *flipt.BatchEvaluationRequ
 
 func (s *Server) evaluate(ctx context.Context, r *flipt.EvaluationRequest) (*flipt.EvaluationResponse, error) {
 	var (
-		ts, _ = ptypes.TimestampProto(time.Now().UTC())
-		resp  = &flipt.EvaluationResponse{
+		ts   = timestamp.New(time.Now().UTC())
+		resp = &flipt.EvaluationResponse{
 			RequestId:      r.RequestId,
 			EntityId:       r.EntityId,
 			RequestContext: r.Context,

diff --git a/server/flag.go b/server/flag.go
@@ -3,9 +3,9 @@ package server
 import (
 	"context"
 
-	"github.com/golang/protobuf/ptypes/empty"
 	flipt "github.com/markphelps/flipt/rpc/flipt"
 	"github.com/markphelps/flipt/storage"
+	empty "google.golang.org/protobuf/types/known/emptypb"
 )
 
 // GetFlag gets a flag

diff --git a/server/rule.go b/server/rule.go
@@ -3,9 +3,9 @@ package server
 import (
 	"context"
 
-	"github.com/golang/protobuf/ptypes/empty"
 	flipt "github.com/markphelps/flipt/rpc/flipt"
 	"github.com/markphelps/flipt/storage"
+	empty "google.golang.org/protobuf/types/known/emptypb"
 )
 
 // GetRule gets a rule

diff --git a/server/segment.go b/server/segment.go
@@ -3,9 +3,9 @@ package server
 import (
 	"context"
 
-	"github.com/golang/protobuf/ptypes/empty"
 	flipt "github.com/markphelps/flipt/rpc/flipt"
 	"github.com/markphelps/flipt/storage"
+	empty "google.golang.org/protobuf/types/known/emptypb"
 )
 
 // GetSegment gets a segment

diff --git a/storage/sql/common/flag.go b/storage/sql/common/flag.go
@@ -8,10 +8,10 @@ import (
 	sq "github.com/Masterminds/squirrel"
 	"github.com/gofrs/uuid"
 
-	proto "github.com/golang/protobuf/ptypes"
 	errs "github.com/markphelps/flipt/errors"
 	flipt "github.com/markphelps/flipt/rpc/flipt"
 	"github.com/markphelps/flipt/storage"
+	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
 // GetFlag gets a flag
@@ -121,7 +121,7 @@ func (s *Store) ListFlags(ctx context.Context, opts ...storage.QueryOption) ([]*
 // CreateFlag creates a flag
 func (s *Store) CreateFlag(ctx context.Context, r *flipt.CreateFlagRequest) (*flipt.Flag, error) {
 	var (
-		now  = proto.TimestampNow()
+		now  = timestamppb.Now()
 		flag = &flipt.Flag{
 			Key:         r.Key,
 			Name:        r.Name,
@@ -148,7 +148,7 @@ func (s *Store) UpdateFlag(ctx context.Context, r *flipt.UpdateFlagRequest) (*fl
 		Set("name", r.Name).
 		Set("description", r.Description).
 		Set("enabled", r.Enabled).
-		Set("updated_at", &timestamp{proto.TimestampNow()}).
+		Set("updated_at", &timestamp{timestamppb.Now()}).
 		Where(sq.Eq{"\"key\"": r.Key})
 
 	res, err := query.ExecContext(ctx)
@@ -180,7 +180,7 @@ func (s *Store) DeleteFlag(ctx context.Context, r *flipt.DeleteFlagRequest) erro
 // CreateVariant creates a variant
 func (s *Store) CreateVariant(ctx context.Context, r *flipt.CreateVariantRequest) (*flipt.Variant, error) {
 	var (
-		now = proto.TimestampNow()
+		now = timestamppb.Now()
 		v   = &flipt.Variant{
 			Id:          uuid.Must(uuid.NewV4()).String(),
 			FlagKey:     r.FlagKey,
@@ -208,7 +208,7 @@ func (s *Store) UpdateVariant(ctx context.Context, r *flipt.UpdateVariantRequest
 		Set("\"key\"", r.Key).
 		Set("name", r.Name).
 		Set("description", r.Description).
-		Set("updated_at", &timestamp{proto.TimestampNow()}).
+		Set("updated_at", &timestamp{timestamppb.Now()}).
 		Where(sq.And{sq.Eq{"id": r.Id}, sq.Eq{"flag_key": r.FlagKey}})
 
 	res, err := query.ExecContext(ctx)

diff --git a/storage/sql/common/rule.go b/storage/sql/common/rule.go
@@ -7,7 +7,8 @@ import (
 
 	sq "github.com/Masterminds/squirrel"
 	"github.com/gofrs/uuid"
-	proto "github.com/golang/protobuf/ptypes"
+	"google.golang.org/protobuf/types/known/timestamppb"
+
 	errs "github.com/markphelps/flipt/errors"
 	flipt "github.com/markphelps/flipt/rpc/flipt"
 	"github.com/markphelps/flipt/storage"
@@ -115,7 +116,7 @@ func (s *Store) ListRules(ctx context.Context, flagKey string, opts ...storage.Q
 // CreateRule creates a rule
 func (s *Store) CreateRule(ctx context.Context, r *flipt.CreateRuleRequest) (*flipt.Rule, error) {
 	var (
-		now  = proto.TimestampNow()
+		now  = timestamppb.Now()
 		rule = &flipt.Rule{
 			Id:         uuid.Must(uuid.NewV4()).String(),
 			FlagKey:    r.FlagKey,
@@ -141,7 +142,7 @@ func (s *Store) CreateRule(ctx context.Context, r *flipt.CreateRuleRequest) (*fl
 func (s *Store) UpdateRule(ctx context.Context, r *flipt.UpdateRuleRequest) (*flipt.Rule, error) {
 	query := s.builder.Update("rules").
 		Set("segment_key", r.SegmentKey).
-		Set("updated_at", &timestamp{proto.TimestampNow()}).
+		Set("updated_at", &timestamp{timestamppb.Now()}).
 		Where(sq.And{sq.Eq{"id": r.Id}, sq.Eq{"flag_key": r.FlagKey}})
 
 	res, err := query.ExecContext(ctx)
@@ -236,7 +237,7 @@ func (s *Store) OrderRules(ctx context.Context, r *flipt.OrderRulesRequest) erro
 }
 
 func (s *Store) orderRules(ctx context.Context, runner sq.BaseRunner, flagKey string, ruleIDs []string) error {
-	updatedAt := proto.TimestampNow()
+	updatedAt := timestamppb.Now()
 
 	for i, id := range ruleIDs {
 		_, err := s.builder.Update("rules").
@@ -256,7 +257,7 @@ func (s *Store) orderRules(ctx context.Context, runner sq.BaseRunner, flagKey st
 // CreateDistribution creates a distribution
 func (s *Store) CreateDistribution(ctx context.Context, r *flipt.CreateDistributionRequest) (*flipt.Distribution, error) {
 	var (
-		now = proto.TimestampNow()
+		now = timestamppb.Now()
 		d   = &flipt.Distribution{
 			Id:        uuid.Must(uuid.NewV4()).String(),
 			RuleId:    r.RuleId,
@@ -282,7 +283,7 @@ func (s *Store) CreateDistribution(ctx context.Context, r *flipt.CreateDistribut
 func (s *Store) UpdateDistribution(ctx context.Context, r *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) {
 	query := s.builder.Update("distributions").
 		Set("rollout", r.Rollout).
-		Set("updated_at", &timestamp{proto.TimestampNow()}).
+		Set("updated_at", &timestamp{timestamppb.Now()}).
 		Where(sq.And{sq.Eq{"id": r.Id}, sq.Eq{"rule_id": r.RuleId}, sq.Eq{"variant_id": r.VariantId}})
 
 	res, err := query.ExecContext(ctx)

diff --git a/storage/sql/common/segment.go b/storage/sql/common/segment.go
@@ -9,10 +9,10 @@ import (
 	sq "github.com/Masterminds/squirrel"
 	"github.com/gofrs/uuid"
 
-	proto "github.com/golang/protobuf/ptypes"
 	errs "github.com/markphelps/flipt/errors"
 	flipt "github.com/markphelps/flipt/rpc/flipt"
 	"github.com/markphelps/flipt/storage"
+	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
 // GetSegment gets a segment
@@ -121,7 +121,7 @@ func (s *Store) ListSegments(ctx context.Context, opts ...storage.QueryOption) (
 // CreateSegment creates a segment
 func (s *Store) CreateSegment(ctx context.Context, r *flipt.CreateSegmentRequest) (*flipt.Segment, error) {
 	var (
-		now     = proto.TimestampNow()
+		now     = timestamppb.Now()
 		segment = &flipt.Segment{
 			Key:         r.Key,
 			Name:        r.Name,
@@ -148,7 +148,7 @@ func (s *Store) UpdateSegment(ctx context.Context, r *flipt.UpdateSegmentRequest
 		Set("name", r.Name).
 		Set("description", r.Description).
 		Set("match_type", r.MatchType).
-		Set("updated_at", &timestamp{proto.TimestampNow()}).
+		Set("updated_at", &timestamp{timestamppb.Now()}).
 		Where(sq.Eq{"\"key\"": r.Key})
 
 	res, err := query.ExecContext(ctx)
@@ -181,7 +181,7 @@ func (s *Store) DeleteSegment(ctx context.Context, r *flipt.DeleteSegmentRequest
 func (s *Store) CreateConstraint(ctx context.Context, r *flipt.CreateConstraintRequest) (*flipt.Constraint, error) {
 	var (
 		operator = strings.ToLower(r.Operator)
-		now      = proto.TimestampNow()
+		now      = timestamppb.Now()
 		c        = &flipt.Constraint{
 			Id:         uuid.Must(uuid.NewV4()).String(),
 			SegmentKey: r.SegmentKey,
@@ -223,7 +223,7 @@ func (s *Store) UpdateConstraint(ctx context.Context, r *flipt.UpdateConstraintR
 		Set("property", r.Property).
 		Set("operator", operator).
 		Set("value", r.Value).
-		Set("updated_at", &timestamp{proto.TimestampNow()}).
+		Set("updated_at", &timestamp{timestamppb.Now()}).
 		Where(sq.And{sq.Eq{"id": r.Id}, sq.Eq{"segment_key": r.SegmentKey}}).
 		ExecContext(ctx)
 	if err != nil {

diff --git a/storage/sql/common/timestamp.go b/storage/sql/common/timestamp.go
@@ -4,19 +4,17 @@ import (
 	"database/sql/driver"
 	"time"
 
-	"github.com/golang/protobuf/ptypes"
-
-	proto "github.com/golang/protobuf/ptypes/timestamp"
+	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
 type timestamp struct {
-	*proto.Timestamp
+	*timestamppb.Timestamp
 }
 
 func (t *timestamp) Scan(value interface{}) error {
 	if v, ok := value.(time.Time); ok {
-		val, err := ptypes.TimestampProto(v)
-		if err != nil {
+		val := timestamppb.New(v)
+		if err := val.CheckValid(); err != nil {
 			return err
 		}
 
@@ -27,5 +25,5 @@ func (t *timestamp) Scan(value interface{}) error {
 }
 
 func (t *timestamp) Value() (driver.Value, error) {
-	return ptypes.Timestamp(t.Timestamp)
+	return t.Timestamp.AsTime(), t.Timestamp.CheckValid()
 }