Separete test file and no stack trace in ret error

This commit is contained in:
Giulio Micheloni 2021-10-16 18:02:03 +01:00
parent ab75b4ee92
commit fecce25658
6 changed files with 88 additions and 86 deletions

View File

@ -1,7 +1,6 @@
package grpc package grpc
import ( import (
"bytes"
"context" "context"
"fmt" "fmt"
"net" "net"
@ -15,8 +14,6 @@ import (
"github.com/hashicorp/go-hclog" "github.com/hashicorp/go-hclog"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"github.com/hashicorp/consul/agent/grpc/internal/testservice" "github.com/hashicorp/consul/agent/grpc/internal/testservice"
"github.com/hashicorp/consul/agent/grpc/resolver" "github.com/hashicorp/consul/agent/grpc/resolver"
@ -444,46 +441,3 @@ func registerWithGRPC(t *testing.T, b *resolver.ServerResolverBuilder) {
resolver.Deregister(b.Authority()) resolver.Deregister(b.Authority())
}) })
} }
func TestRecoverMiddleware(t *testing.T) {
// Prepare a logger with output to a buffer
// so we can check what it writes.
var buf bytes.Buffer
logger := hclog.New(&hclog.LoggerOptions{
Output: &buf,
})
res := resolver.NewServerResolverBuilder(newConfig(t))
registerWithGRPC(t, res)
srv := newPanicTestServer(t, logger, "server-1", "dc1", nil)
res.AddServer(srv.Metadata())
t.Cleanup(srv.shutdown)
pool := NewClientConnPool(ClientConnPoolConfig{
Servers: res,
UseTLSForDC: useTLSForDcAlwaysTrue,
DialingFromServer: true,
DialingFromDatacenter: "dc1",
})
conn, err := pool.ClientConn("dc1")
require.NoError(t, err)
client := testservice.NewSimpleClient(conn)
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
t.Cleanup(cancel)
resp, err := client.Something(ctx, &testservice.Req{})
expectedErr := status.Errorf(codes.Internal, "grpc: panic serving request: panic from Something")
require.Equal(t, expectedErr, err)
require.Nil(t, resp)
// Read the log
strLog := buf.String()
// Checking the entire stack trace is not possible, let's
// make sure that it contains a couple of expected strings.
require.Contains(t, strLog, `[ERROR] panic serving grpc request: panic="panic from Something`)
require.Contains(t, strLog, `github.com/hashicorp/consul/agent/grpc.(*simplePanic).Something`)
}

View File

@ -4,7 +4,6 @@ Package grpc provides a Handler and client for agent gRPC connections.
package grpc package grpc
import ( import (
"context"
"fmt" "fmt"
"net" "net"
"time" "time"
@ -14,8 +13,8 @@ import (
"google.golang.org/grpc/keepalive" "google.golang.org/grpc/keepalive"
"google.golang.org/grpc/status" "google.golang.org/grpc/status"
middleware "github.com/grpc-ecosystem/go-grpc-middleware/v2" middleware "github.com/grpc-ecosystem/go-grpc-middleware"
"github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/recovery" recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery"
"github.com/hashicorp/go-hclog" "github.com/hashicorp/go-hclog"
) )
@ -23,13 +22,12 @@ import (
// The register function will be called with the grpc.Server to register // The register function will be called with the grpc.Server to register
// gRPC services with the server. // gRPC services with the server.
func NewHandler(logger Logger, addr net.Addr, register func(server *grpc.Server)) *Handler { func NewHandler(logger Logger, addr net.Addr, register func(server *grpc.Server)) *Handler {
recoveryOpts := []recovery.Option{
recovery.WithRecoveryHandlerContext(newPanicHandler(logger)),
}
metrics := defaultMetrics() metrics := defaultMetrics()
// We don't need to pass tls.Config to the server since it's multiplexed
// behind the RPC listener, which already has TLS configured. recoveryOpts := PanicHandlerMiddlewareOpts(logger)
srv := grpc.NewServer(
opts := []grpc.ServerOption{
grpc.StatsHandler(newStatsHandler(metrics)),
middleware.WithUnaryServerChain( middleware.WithUnaryServerChain(
// Add middlware interceptors to recover in case of panics. // Add middlware interceptors to recover in case of panics.
recovery.UnaryServerInterceptor(recoveryOpts...), recovery.UnaryServerInterceptor(recoveryOpts...),
@ -39,21 +37,32 @@ func NewHandler(logger Logger, addr net.Addr, register func(server *grpc.Server)
recovery.StreamServerInterceptor(recoveryOpts...), recovery.StreamServerInterceptor(recoveryOpts...),
(&activeStreamCounter{metrics: metrics}).Intercept, (&activeStreamCounter{metrics: metrics}).Intercept,
), ),
grpc.StatsHandler(newStatsHandler(metrics)),
grpc.KeepaliveEnforcementPolicy(keepalive.EnforcementPolicy{ grpc.KeepaliveEnforcementPolicy(keepalive.EnforcementPolicy{
MinTime: 15 * time.Second, MinTime: 15 * time.Second,
}), }),
) }
// We don't need to pass tls.Config to the server since it's multiplexed
// behind the RPC listener, which already has TLS configured.
srv := grpc.NewServer(opts...)
register(srv) register(srv)
lis := &chanListener{addr: addr, conns: make(chan net.Conn), done: make(chan struct{})} lis := &chanListener{addr: addr, conns: make(chan net.Conn), done: make(chan struct{})}
return &Handler{srv: srv, listener: lis} return &Handler{srv: srv, listener: lis}
} }
// newPanicHandler returns a recovery.RecoveryHandlerFuncContext closure function // PanicHandlerMiddlewareOpts returns the []recovery.Option containing
// recovery handler function.
func PanicHandlerMiddlewareOpts(logger Logger) []recovery.Option {
return []recovery.Option{
recovery.WithRecoveryHandler(NewPanicHandler(logger)),
}
}
// NewPanicHandler returns a recovery.RecoveryHandlerFunc closure function
// to handle panic in GRPC server's handlers. // to handle panic in GRPC server's handlers.
func newPanicHandler(logger Logger) recovery.RecoveryHandlerFuncContext { func NewPanicHandler(logger Logger) recovery.RecoveryHandlerFunc {
return func(ctx context.Context, p interface{}) (err error) { return func(p interface{}) (err error) {
// Log the panic and the stack trace of the Goroutine that caused the panic. // Log the panic and the stack trace of the Goroutine that caused the panic.
stacktrace := hclog.Stacktrace() stacktrace := hclog.Stacktrace()
logger.Error("panic serving grpc request", logger.Error("panic serving grpc request",
@ -61,7 +70,7 @@ func newPanicHandler(logger Logger) recovery.RecoveryHandlerFuncContext {
"stack", stacktrace, "stack", stacktrace,
) )
return status.Errorf(codes.Internal, "grpc: panic serving request: %v", p) return status.Errorf(codes.Internal, "grpc: panic serving request")
} }
} }

View File

@ -0,0 +1,59 @@
package grpc
import (
"bytes"
"context"
"testing"
"time"
"github.com/hashicorp/go-hclog"
"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"github.com/hashicorp/consul/agent/grpc/internal/testservice"
"github.com/hashicorp/consul/agent/grpc/resolver"
)
func TestHandler_PanicRecoveryInterceptor(t *testing.T) {
// Prepare a logger with output to a buffer
// so we can check what it writes.
var buf bytes.Buffer
logger := hclog.New(&hclog.LoggerOptions{
Output: &buf,
})
res := resolver.NewServerResolverBuilder(newConfig(t))
registerWithGRPC(t, res)
srv := newPanicTestServer(t, logger, "server-1", "dc1", nil)
res.AddServer(srv.Metadata())
t.Cleanup(srv.shutdown)
pool := NewClientConnPool(ClientConnPoolConfig{
Servers: res,
UseTLSForDC: useTLSForDcAlwaysTrue,
DialingFromServer: true,
DialingFromDatacenter: "dc1",
})
conn, err := pool.ClientConn("dc1")
require.NoError(t, err)
client := testservice.NewSimpleClient(conn)
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
t.Cleanup(cancel)
resp, err := client.Something(ctx, &testservice.Req{})
expectedErr := status.Errorf(codes.Internal, "grpc: panic serving request")
require.Equal(t, expectedErr, err)
require.Nil(t, resp)
// Read the log
strLog := buf.String()
// Checking the entire stack trace is not possible, let's
// make sure that it contains a couple of expected strings.
require.Contains(t, strLog, `[ERROR] panic serving grpc request: panic="panic from Something`)
require.Contains(t, strLog, `github.com/hashicorp/consul/agent/grpc.(*simplePanic).Something`)
}

View File

@ -10,11 +10,11 @@ import (
envoy_config_core_v3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" envoy_config_core_v3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
envoy_discovery_v2 "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v2" envoy_discovery_v2 "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v2"
envoy_discovery_v3 "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" envoy_discovery_v3 "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
middleware "github.com/grpc-ecosystem/go-grpc-middleware"
recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery"
"github.com/armon/go-metrics" "github.com/armon/go-metrics"
"github.com/armon/go-metrics/prometheus" "github.com/armon/go-metrics/prometheus"
middleware "github.com/grpc-ecosystem/go-grpc-middleware/v2"
"github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/recovery"
"github.com/hashicorp/go-hclog" "github.com/hashicorp/go-hclog"
"google.golang.org/grpc" "google.golang.org/grpc"
"google.golang.org/grpc/codes" "google.golang.org/grpc/codes"
@ -23,6 +23,7 @@ import (
"google.golang.org/grpc/status" "google.golang.org/grpc/status"
"github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/acl"
agentgrpc "github.com/hashicorp/consul/agent/grpc"
"github.com/hashicorp/consul/agent/proxycfg" "github.com/hashicorp/consul/agent/proxycfg"
"github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/logging" "github.com/hashicorp/consul/logging"
@ -550,27 +551,10 @@ func tokenFromContext(ctx context.Context) string {
return "" return ""
} }
// newPanicHandler returns a recovery.RecoveryHandlerFuncContext closure function
// to handle panic in GRPC server's handlers.
func newPanicHandler(logger hclog.Logger) recovery.RecoveryHandlerFuncContext {
return func(ctx context.Context, p interface{}) (err error) {
// Log the panic and the stack trace of the Goroutine that caused the panic.
stacktrace := hclog.Stacktrace()
logger.Error("panic serving grpc request",
"panic", p,
"stack", stacktrace,
)
return status.Errorf(codes.Internal, "grpc: panic serving request: %v", p)
}
}
// NewGRPCServer creates a grpc.Server, registers the Server, and then returns // NewGRPCServer creates a grpc.Server, registers the Server, and then returns
// the grpc.Server. // the grpc.Server.
func NewGRPCServer(s *Server, tlsConfigurator *tlsutil.Configurator) *grpc.Server { func NewGRPCServer(s *Server, tlsConfigurator *tlsutil.Configurator) *grpc.Server {
recoveryOpts := []recovery.Option{ recoveryOpts := agentgrpc.PanicHandlerMiddlewareOpts(s.Logger)
recovery.WithRecoveryHandlerContext(newPanicHandler(s.Logger)),
}
opts := []grpc.ServerOption{ opts := []grpc.ServerOption{
grpc.MaxConcurrentStreams(2048), grpc.MaxConcurrentStreams(2048),

2
go.mod
View File

@ -29,7 +29,7 @@ require (
github.com/google/gofuzz v1.2.0 github.com/google/gofuzz v1.2.0
github.com/google/pprof v0.0.0-20210601050228-01bbb1931b22 github.com/google/pprof v0.0.0-20210601050228-01bbb1931b22
github.com/google/tcpproxy v0.0.0-20180808230851-dfa16c61dad2 github.com/google/tcpproxy v0.0.0-20180808230851-dfa16c61dad2
github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.0.0-rc.2 github.com/grpc-ecosystem/go-grpc-middleware v1.0.0
github.com/hashicorp/consul/api v1.8.0 github.com/hashicorp/consul/api v1.8.0
github.com/hashicorp/consul/sdk v0.7.0 github.com/hashicorp/consul/sdk v0.7.0
github.com/hashicorp/go-bexpr v0.1.2 github.com/hashicorp/go-bexpr v0.1.2

4
go.sum
View File

@ -213,8 +213,6 @@ github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoA
github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA= github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:Iju5GlWwrvL6UBg4zJJt3btmonfrMlCDdsejg4CZE7c= github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:Iju5GlWwrvL6UBg4zJJt3btmonfrMlCDdsejg4CZE7c=
github.com/grpc-ecosystem/go-grpc-middleware v1.0.0/go.mod h1:FiyG127CGDf3tlThmgyCl78X/SZQqEOJBCDaAfeWzPs= github.com/grpc-ecosystem/go-grpc-middleware v1.0.0/go.mod h1:FiyG127CGDf3tlThmgyCl78X/SZQqEOJBCDaAfeWzPs=
github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.0.0-rc.2 h1:1aeRCnE2CkKYqyzBu0+B2lgTcZPc3ea2lGpijeHbI1c=
github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.0.0-rc.2/go.mod h1:GhphxcdlaRyAuBSvo6rV71BvQcvB/vuX8ugCyybuS2k=
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk= github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk=
github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY=
github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA= github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA=
@ -408,7 +406,6 @@ github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+W
github.com/onsi/ginkgo v1.11.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/ginkgo v1.11.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA= github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA=
github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o=
github.com/packethost/packngo v0.1.1-0.20180711074735-b9cb5096f54c h1:vwpFWvAO8DeIZfFeqASzZfsxuWPno9ncAebBEP0N3uE= github.com/packethost/packngo v0.1.1-0.20180711074735-b9cb5096f54c h1:vwpFWvAO8DeIZfFeqASzZfsxuWPno9ncAebBEP0N3uE=
github.com/packethost/packngo v0.1.1-0.20180711074735-b9cb5096f54c/go.mod h1:otzZQXgoO96RTzDB/Hycg0qZcXZsWJGJRSXbmEIJ+4M= github.com/packethost/packngo v0.1.1-0.20180711074735-b9cb5096f54c/go.mod h1:otzZQXgoO96RTzDB/Hycg0qZcXZsWJGJRSXbmEIJ+4M=
github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc=
@ -422,7 +419,6 @@ github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi
github.com/pierrec/lz4 v2.5.2+incompatible h1:WCjObylUIOlKy/+7Abdn34TLIkXiA4UWUMhxq9m9ZXI= github.com/pierrec/lz4 v2.5.2+incompatible h1:WCjObylUIOlKy/+7Abdn34TLIkXiA4UWUMhxq9m9ZXI=
github.com/pierrec/lz4 v2.5.2+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY= github.com/pierrec/lz4 v2.5.2+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY=
github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=