Merge central config for GetEnvoyBootstrapParams (#14869)

This fixes GetEnvoyBootstrapParams to merge in proxy-defaults and service-defaults.

Co-authored-by: Dan Upton <daniel@floppy.co>

.changelog/14869.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+grpc: Merge proxy-defaults and service-defaults in GetEnvoyBootstrapParams response.
+```

agent/configentry/merge_service_config.go

@@ -1,4 +1,4 @@
-package consul
+package configentry
 
 import (
 	"fmt"
@@ -8,18 +8,21 @@ import (
 	"github.com/imdario/mergo"
 	"github.com/mitchellh/copystructure"
 
-	"github.com/hashicorp/consul/agent/configentry"
+	"github.com/hashicorp/consul/acl"
-	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
 )
 
-// mergeNodeServiceWithCentralConfig merges a service instance (NodeService) with the
+type StateStore interface {
+	ReadResolvedServiceConfigEntries(memdb.WatchSet, string, *acl.EnterpriseMeta, []structs.ServiceID, structs.ProxyMode) (uint64, *ResolvedServiceConfigSet, error)
+}
+
+// MergeNodeServiceWithCentralConfig merges a service instance (NodeService) with the
 // proxy-defaults/global and service-defaults/:service config entries.
 // This common helper is used by the blocking query function of different RPC endpoints
 // that need to return a fully resolved service defintion.
-func mergeNodeServiceWithCentralConfig(
+func MergeNodeServiceWithCentralConfig(
 	ws memdb.WatchSet,
-	state *state.Store,
+	state StateStore,
 	args *structs.ServiceSpecificRequest,
 	ns *structs.NodeService,
 	logger hclog.Logger) (uint64, *structs.NodeService, error) {
@@ -67,7 +70,7 @@ func mergeNodeServiceWithCentralConfig(
 			ns.ID, err)
 	}
 
-	defaults, err := configentry.ComputeResolvedServiceConfig(
+	defaults, err := ComputeResolvedServiceConfig(
 		configReq,
 		upstreams,
 		false,

agent/configentry/merge_service_config_test.go

@@ -1,4 +1,4 @@
-package consul
+package configentry
 
 import (
 	"testing"

agent/consul/catalog_endpoint.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/acl/resolver"
+	"github.com/hashicorp/consul/agent/configentry"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/ipaddr"
@@ -752,7 +753,7 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru
 					mergedsn := sn
 					ns := sn.ToNodeService()
 					if ns.IsSidecarProxy() || ns.IsGateway() {
-						cfgIndex, mergedns, err := mergeNodeServiceWithCentralConfig(ws, state, args, ns, c.logger)
+						cfgIndex, mergedns, err := configentry.MergeNodeServiceWithCentralConfig(ws, state, args, ns, c.logger)
 						if err != nil {
 							return err
 						}
@@ -960,7 +961,7 @@ func (c *Catalog) NodeServiceList(args *structs.NodeSpecificRequest, reply *stru
 							Datacenter:   args.Datacenter,
 							QueryOptions: args.QueryOptions,
 						}
-						cfgIndex, mergedns, err = mergeNodeServiceWithCentralConfig(ws, state, &serviceSpecificReq, ns, c.logger)
+						cfgIndex, mergedns, err = configentry.MergeNodeServiceWithCentralConfig(ws, state, &serviceSpecificReq, ns, c.logger)
 						if err != nil {
 							return err
 						}

agent/consul/health_endpoint.go

@@ -11,6 +11,7 @@ import (
 	hashstructure_v2 "github.com/mitchellh/hashstructure/v2"
 
 	"github.com/hashicorp/consul/acl"
+	"github.com/hashicorp/consul/agent/configentry"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
 )
@@ -256,7 +257,7 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc
 				for _, node := range resolvedNodes {
 					ns := node.Service
 					if ns.IsSidecarProxy() || ns.IsGateway() {
-						cfgIndex, mergedns, err := mergeNodeServiceWithCentralConfig(ws, state, args, ns, h.logger)
+						cfgIndex, mergedns, err := configentry.MergeNodeServiceWithCentralConfig(ws, state, args, ns, h.logger)
 						if err != nil {
 							return err
 						}

agent/grpc-external/services/dataplane/get_envoy_bootstrap_params.go

@@ -9,10 +9,11 @@ import (
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/structpb"
 
-	acl "github.com/hashicorp/consul/acl"
+	"github.com/hashicorp/consul/acl"
+	"github.com/hashicorp/consul/agent/configentry"
 	"github.com/hashicorp/consul/agent/consul/state"
 	external "github.com/hashicorp/consul/agent/grpc-external"
-	structs "github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/proto-public/pbdataplane"
 )
 
@@ -73,7 +74,24 @@ func (s *Server) GetEnvoyBootstrapParams(ctx context.Context, req *pbdataplane.G
 		NodeId:      string(svc.ID),
 	}
 
-	bootstrapConfig, err := structpb.NewStruct(svc.ServiceProxy.Config)
+	// This is awkward because it's designed for different requests, but
+	// this fakes the ServiceSpecificRequest so that we can reuse code.
+	_, ns, err := configentry.MergeNodeServiceWithCentralConfig(
+		nil,
+		store,
+		&structs.ServiceSpecificRequest{
+			Datacenter:   s.Datacenter,
+			QueryOptions: options,
+		},
+		svc.ToNodeService(),
+		logger,
+	)
+	if err != nil {
+		logger.Error("Error merging with central config", "error", err)
+		return nil, status.Errorf(codes.Unknown, "Error merging central config: %v", err)
+	}
+
+	bootstrapConfig, err := structpb.NewStruct(ns.Proxy.Config)
 	if err != nil {
 		logger.Error("Error creating the envoy boostrap params config", "error", err)
 		return nil, status.Error(codes.Unknown, "Error creating the envoy boostrap params config")

agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go

@@ -5,29 +5,39 @@ import (
 	"testing"
 
 	"github.com/hashicorp/go-hclog"
-	mock "github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/structpb"
 
-	acl "github.com/hashicorp/consul/acl"
+	"github.com/hashicorp/consul/acl"
-	resolver "github.com/hashicorp/consul/acl/resolver"
+	"github.com/hashicorp/consul/acl/resolver"
 	external "github.com/hashicorp/consul/agent/grpc-external"
 	"github.com/hashicorp/consul/agent/grpc-external/testutils"
-	structs "github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/proto-public/pbdataplane"
 	"github.com/hashicorp/consul/types"
 )
 
 const (
 	testToken        = "acl-token-get-envoy-bootstrap-params"
+	testServiceName  = "web"
 	proxyServiceID   = "web-proxy"
 	nodeName         = "foo"
 	nodeID           = "2980b72b-bd9d-9d7b-d4f9-951bf7508d95"
 	proxyConfigKey   = "envoy_dogstatsd_url"
 	proxyConfigValue = "udp://127.0.0.1:8125"
 	serverDC         = "dc1"
+
+	protocolKey       = "protocol"
+	connectTimeoutKey = "local_connect_timeout_ms"
+	requestTimeoutKey = "local_request_timeout_ms"
+
+	proxyDefaultsProtocol         = "http"
+	proxyDefaultsRequestTimeout   = 1111
+	serviceDefaultsProtocol       = "tcp"
+	serviceDefaultsConnectTimeout = 4444
 )
 
 func testRegisterRequestProxy(t *testing.T) *structs.RegisterRequest {
@@ -43,7 +53,7 @@ func testRegisterRequestProxy(t *testing.T) *structs.RegisterRequest {
 			Address: "127.0.0.2",
 			Port:    2222,
 			Proxy: structs.ConnectProxyConfig{
-				DestinationServiceName: "web",
+				DestinationServiceName: testServiceName,
 				Config: map[string]interface{}{
 					proxyConfigKey: proxyConfigValue,
 				},
@@ -63,18 +73,57 @@ func testRegisterIngressGateway(t *testing.T) *structs.RegisterRequest {
 	return registerReq
 }
 
+func testProxyDefaults(t *testing.T) structs.ConfigEntry {
+	return &structs.ProxyConfigEntry{
+		Kind: structs.ProxyDefaults,
+		Name: structs.ProxyConfigGlobal,
+		Config: map[string]interface{}{
+			protocolKey:       proxyDefaultsProtocol,
+			requestTimeoutKey: proxyDefaultsRequestTimeout,
+		},
+	}
+}
+
+func testServiceDefaults(t *testing.T) structs.ConfigEntry {
+	return &structs.ServiceConfigEntry{
+		Kind:                  structs.ServiceDefaults,
+		Name:                  testServiceName,
+		Protocol:              serviceDefaultsProtocol,
+		LocalConnectTimeoutMs: serviceDefaultsConnectTimeout,
+	}
+}
+
+func requireConfigField(t *testing.T, resp *pbdataplane.GetEnvoyBootstrapParamsResponse, key string, value interface{}) {
+	require.Contains(t, resp.Config.Fields, key)
+	require.Equal(t, value, resp.Config.Fields[key])
+}
+
 func TestGetEnvoyBootstrapParams_Success(t *testing.T) {
 	type testCase struct {
 		name            string
 		registerReq     *structs.RegisterRequest
 		nodeID          bool
+		proxyDefaults   structs.ConfigEntry
+		serviceDefaults structs.ConfigEntry
 	}
 
 	run := func(t *testing.T, tc testCase) {
 		store := testutils.TestStateStore(t, nil)
-		err := store.EnsureRegistration(1, tc.registerReq)
+		idx := uint64(1)
+		err := store.EnsureRegistration(idx, tc.registerReq)
 		require.NoError(t, err)
 
+		if tc.proxyDefaults != nil {
+			idx++
+			err := store.EnsureConfigEntry(idx, tc.proxyDefaults)
+			require.NoError(t, err)
+		}
+		if tc.serviceDefaults != nil {
+			idx++
+			err := store.EnsureConfigEntry(idx, tc.serviceDefaults)
+			require.NoError(t, err)
+		}
+
 		aclResolver := &MockACLResolver{}
 		aclResolver.On("ResolveTokenAndDefaultMeta", testToken, mock.Anything, mock.Anything).
 			Return(testutils.TestAuthorizerServiceRead(t, tc.registerReq.Service.ID), nil)
@@ -109,20 +158,33 @@ func TestGetEnvoyBootstrapParams_Success(t *testing.T) {
 		require.Equal(t, serverDC, resp.Datacenter)
 		require.Equal(t, tc.registerReq.EnterpriseMeta.PartitionOrDefault(), resp.Partition)
 		require.Equal(t, tc.registerReq.EnterpriseMeta.NamespaceOrDefault(), resp.Namespace)
-		require.Contains(t, resp.Config.Fields, proxyConfigKey)
+		requireConfigField(t, resp, proxyConfigKey, structpb.NewStringValue(proxyConfigValue))
-		require.Equal(t, structpb.NewStringValue(proxyConfigValue), resp.Config.Fields[proxyConfigKey])
 		require.Equal(t, convertToResponseServiceKind(tc.registerReq.Service.Kind), resp.ServiceKind)
 		require.Equal(t, tc.registerReq.Node, resp.NodeName)
 		require.Equal(t, string(tc.registerReq.ID), resp.NodeId)
+
+		if tc.serviceDefaults != nil && tc.proxyDefaults != nil {
+			// service-defaults take precedence over proxy-defaults
+			requireConfigField(t, resp, protocolKey, structpb.NewStringValue(serviceDefaultsProtocol))
+			requireConfigField(t, resp, connectTimeoutKey, structpb.NewNumberValue(serviceDefaultsConnectTimeout))
+			requireConfigField(t, resp, requestTimeoutKey, structpb.NewNumberValue(proxyDefaultsRequestTimeout))
+		} else if tc.serviceDefaults != nil {
+			requireConfigField(t, resp, protocolKey, structpb.NewStringValue(serviceDefaultsProtocol))
+			requireConfigField(t, resp, connectTimeoutKey, structpb.NewNumberValue(serviceDefaultsConnectTimeout))
+		} else if tc.proxyDefaults != nil {
+			requireConfigField(t, resp, protocolKey, structpb.NewStringValue(proxyDefaultsProtocol))
+			requireConfigField(t, resp, requestTimeoutKey, structpb.NewNumberValue(proxyDefaultsRequestTimeout))
+		}
+
 	}
 
 	testCases := []testCase{
 		{
-			name:        "lookup service side car proxy by node name",
+			name:        "lookup service sidecar proxy by node name",
 			registerReq: testRegisterRequestProxy(t),
 		},
 		{
-			name:        "lookup service side car proxy by node ID",
+			name:        "lookup service sidecar proxy by node ID",
 			registerReq: testRegisterRequestProxy(t),
 			nodeID:      true,
 		},
@@ -135,6 +197,21 @@ func TestGetEnvoyBootstrapParams_Success(t *testing.T) {
 			registerReq: testRegisterIngressGateway(t),
 			nodeID:      true,
 		},
+		{
+			name:          "merge proxy defaults for sidecar proxy",
+			registerReq:   testRegisterRequestProxy(t),
+			proxyDefaults: testProxyDefaults(t),
+		},
+		{
+			name:            "merge service defaults for sidecar proxy",
+			registerReq:     testRegisterRequestProxy(t),
+			serviceDefaults: testServiceDefaults(t),
+		},
+		{
+			name:            "merge proxy defaults and service defaults for sidecar proxy",
+			registerReq:     testRegisterRequestProxy(t),
+			serviceDefaults: testServiceDefaults(t),
+		},
 	}
 
 	for _, tc := range testCases {

agent/grpc-external/services/dataplane/server.go

@@ -4,9 +4,11 @@ import (
 	"google.golang.org/grpc"
 
 	"github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/go-memdb"
 
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/acl/resolver"
+	"github.com/hashicorp/consul/agent/configentry"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/proto-public/pbdataplane"
 )
@@ -25,6 +27,7 @@ type Config struct {
 
 type StateStore interface {
 	ServiceNode(string, string, string, *acl.EnterpriseMeta, string) (uint64, *structs.ServiceNode, error)
+	ReadResolvedServiceConfigEntries(memdb.WatchSet, string, *acl.EnterpriseMeta, []structs.ServiceID, structs.ProxyMode) (uint64, *configentry.ResolvedServiceConfigSet, error)
 }
 
 //go:generate mockery --name ACLResolver --inpackage

agent/service_manager.go

@@ -8,7 +8,7 @@ import (
 
 	"github.com/hashicorp/consul/agent/cache"
 	cachetype "github.com/hashicorp/consul/agent/cache-types"
-	"github.com/hashicorp/consul/agent/consul"
+	"github.com/hashicorp/consul/agent/configentry"
 	"github.com/hashicorp/consul/agent/structs"
 )
 
@@ -145,7 +145,7 @@ func (w *serviceConfigWatch) register(ctx context.Context) error {
 
 	// Merge the local registration with the central defaults and update this service
 	// in the local state.
-	merged, err := consul.MergeServiceConfig(serviceDefaults, w.registration.Service)
+	merged, err := configentry.MergeServiceConfig(serviceDefaults, w.registration.Service)
 	if err != nil {
 		return err
 	}
@@ -275,7 +275,7 @@ func (w *serviceConfigWatch) handleUpdate(ctx context.Context, event cache.Updat
 
 	// Merge the local registration with the central defaults and update this service
 	// in the local state.
-	merged, err := consul.MergeServiceConfig(serviceDefaults, w.registration.Service)
+	merged, err := configentry.MergeServiceConfig(serviceDefaults, w.registration.Service)
 	if err != nil {
 		return err
 	}