Merge pull request #105 from mailgun/thrawn/develop

JSON responses are now back to their original camel_case form

CHANGELOG

@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
 The 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).
 
+## [2.0.0-rc.6] - 2021-08-20
+## Changes
+* JSON responses are now back to their original camel_case form
+* Fixed reporting of number of peers in health check
+
 ## [2.0.0-rc.5] - 2021-06-03
 ## Changes
 * Implemented performance recommendations reported in Issue #74

cluster/cluster.go

@@ -36,7 +36,7 @@ const (
 var daemons []*gubernator.Daemon
 var peers []gubernator.PeerInfo
 
-// Returns a random peer from the cluster
+// GetRandomPeer returns a random peer from the cluster
 func GetRandomPeer(dc string) gubernator.PeerInfo {
 	var local []gubernator.PeerInfo
 
@@ -53,27 +53,27 @@ func GetRandomPeer(dc string) gubernator.PeerInfo {
 	return local[rand.Intn(len(local))]
 }
 
-// Returns a list of all peers in the cluster
+// GetPeers returns a list of all peers in the cluster
 func GetPeers() []gubernator.PeerInfo {
 	return peers
 }
 
-// Returns a list of all deamons in the cluster
+// GetDaemons returns a list of all daemons in the cluster
 func GetDaemons() []*gubernator.Daemon {
 	return daemons
 }
 
-// Returns a specific peer
+// PeerAt returns a specific peer
 func PeerAt(idx int) gubernator.PeerInfo {
 	return peers[idx]
 }
 
-// Returns a specific daemon
+// DaemonAt returns a specific daemon
 func DaemonAt(idx int) *gubernator.Daemon {
 	return daemons[idx]
 }
 
-// Returns the number of instances
+// NumOfDaemons returns the number of instances
 func NumOfDaemons() int {
 	return len(daemons)
 }
@@ -84,15 +84,19 @@ func Start(numInstances int) error {
 	return StartWith(peers)
 }
 
-func Restart(ctx context.Context) {
+// Restart the cluster
+func Restart(ctx context.Context) error {
 	for i := 0; i < len(daemons); i++ {
 		daemons[i].Close()
-		daemons[i].Start(ctx)
+		if err := daemons[i].Start(ctx); err != nil {
+			return err
+		}
 		daemons[i].SetPeers(peers)
 	}
+	return nil
 }
 
-// Start a local cluster with specific addresses
+// StartWith a local cluster with specific addresses
 func StartWith(localPeers []gubernator.PeerInfo) error {
 	for _, peer := range localPeers {
 		ctx, cancel := context.WithTimeout(context.Background(), clock.Second*10)
@@ -130,6 +134,7 @@ func StartWith(localPeers []gubernator.PeerInfo) error {
 	return nil
 }
 
+// Stop all daemons in the cluster
 func Stop() {
 	for _, d := range daemons {
 		d.Close()

daemon.go

@@ -35,6 +35,7 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/keepalive"
+	"google.golang.org/protobuf/encoding/protojson"
 )
 
 type Daemon struct {
@@ -191,8 +192,25 @@ func (s *Daemon) Start(ctx context.Context) error {
 		}
 	}
 
+	// We override the default Marshaller to enable the `UseProtoNames` option.
+	// We do this is because the default JSONPb in 2.5.0 marshals proto structs using
+	// `camelCase`, while all the JSON annotations are `under_score`.
+	// Our protobuf files follow convention described here
+	// https://developers.google.com/protocol-buffers/docs/style#message-and-field-names
+	// Camel case breaks unmarshalling our GRPC gateway responses with protobuf structs.
+	gateway := runtime.NewServeMux(
+		runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.JSONPb{
+			MarshalOptions: protojson.MarshalOptions{
+				UseProtoNames:   true,
+				EmitUnpopulated: true,
+			},
+			UnmarshalOptions: protojson.UnmarshalOptions{
+				DiscardUnknown: true,
+			},
+		}),
+	)
+
 	// Setup an JSON Gateway API for our GRPC methods
-	gateway := runtime.NewServeMux()
 	var gwCtx context.Context
 	gwCtx, s.gwCancel = context.WithCancel(context.Background())
 	err = RegisterV1HandlerFromEndpoint(gwCtx, gateway, gatewayAddr, []grpc.DialOption{grpc.WithInsecure()})

functional_test.go

@@ -18,8 +18,10 @@ package gubernator_test
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net/http"
 	"os"
 	"strings"
@@ -885,7 +887,7 @@ func TestHealthCheck(t *testing.T) {
 	// Restart stopped instances
 	ctx, cancel := context.WithTimeout(context.Background(), clock.Second*15)
 	defer cancel()
-	cluster.Restart(ctx)
+	require.NoError(t, cluster.Restart(ctx))
 }
 
 func TestLeakyBucketDivBug(t *testing.T) {
@@ -944,6 +946,17 @@ func TestGRPCGateway(t *testing.T) {
 	resp, err := http.DefaultClient.Get("http://" + cluster.GetRandomPeer(cluster.DataCenterNone).HTTPAddress + "/v1/HealthCheck")
 	require.NoError(t, err)
 	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	b, err := ioutil.ReadAll(resp.Body)
+
+	// This test ensures future upgrades don't accidentally change `under_score` to `camelCase` again.
+	assert.Contains(t, string(b), "peer_count")
+
+	// Should unmarshall JSON correctly
+	var hc guber.HealthCheckResp
+	require.NoError(t, json.Unmarshal(b, &hc))
+	assert.Equal(t, int32(10), hc.PeerCount)
+
+	require.NoError(t, err)
 }
 
 // TODO: Add a test for sending no rate limits RateLimitReqList.RateLimits = nil

gubernator.go

@@ -41,7 +41,6 @@ const (
 type V1Instance struct {
 	UnimplementedV1Server
 	UnimplementedPeersV1Server
-	health      HealthCheckResp
 	global      *globalManager
 	mutliRegion *mutliRegionManager
 	peerMutex   sync.RWMutex
@@ -358,15 +357,18 @@ func (s *V1Instance) HealthCheck(ctx context.Context, r *HealthCheckReq) (*Healt
 		}
 	}
 
-	s.health.Status = Healthy
+	health := HealthCheckResp{
+		PeerCount: int32(len(localPeers) + len(regionPeers)),
+		Status:    Healthy,
+	}
+
 	if len(errs) != 0 {
-		s.health.Status = UnHealthy
-		s.health.Message = strings.Join(errs, "|")
-		s.health.PeerCount = int32(s.conf.LocalPicker.Size())
+		health.Status = UnHealthy
+		health.Message = strings.Join(errs, "|")
 	}
 
 	defer s.peerMutex.RUnlock()
-	return &s.health, nil
+	return &health, nil
 }
 
 func (s *V1Instance) getRateLimit(r *RateLimitReq) (*RateLimitResp, error) {

scripts/proto.sh

@@ -25,12 +25,13 @@ PYTHON_DST_DIR=$REPO_ROOT/python/gubernator
 GOLANG_DST_DIR=$REPO_ROOT
 
 # Build Golang stabs
-go get google.golang.org/protobuf/cmd/protoc-gen-go
-go install google.golang.org/protobuf/cmd/protoc-gen-go
-go get google.golang.org/grpc/cmd/protoc-gen-go-grpc
-go install google.golang.org/grpc/cmd/protoc-gen-go-grpc
-go get github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway
-go install github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway
+
+
+go install \
+  google.golang.org/protobuf/cmd/protoc-gen-go \
+  google.golang.org/grpc/cmd/protoc-gen-go-grpc \
+  github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway \
+  github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2
 GOPATH=$(go env GOPATH)
 export PATH=$PATH:$GOPATH/bin
 
@@ -64,7 +65,7 @@ protoc -I=$PROTO_DIR \
 mkdir -p "$PYTHON_DST_DIR"
 pip install grpcio
 pip install grpcio-tools
-python -m grpc.tools.protoc \
+python3 -m grpc.tools.protoc \
     -I=$PROTO_DIR \
     -I=$GOOGLE_APIS_DIR \
     --python_out=$PYTHON_DST_DIR \

tls_test.go

@@ -287,5 +287,5 @@ func TestHTTPSClientAuth(t *testing.T) {
 	require.NoError(t, err)
 	b, err := ioutil.ReadAll(resp.Body)
 	require.NoError(t, err)
-	assert.Equal(t, `{"status":"healthy","message":"","peerCount":0}`, strings.ReplaceAll(string(b), " ", ""))
+	assert.Equal(t, `{"status":"healthy","message":"","peer_count":1}`, strings.ReplaceAll(string(b), " ", ""))
 }