From a3fccf5e5b1058a1a515cdea7c27624934b9a180 Mon Sep 17 00:00:00 2001 From: Paul Glass Date: Mon, 10 Oct 2022 12:40:27 -0500 Subject: [PATCH] Merge central config for GetEnvoyBootstrapParams (#14869) This fixes GetEnvoyBootstrapParams to merge in proxy-defaults and service-defaults. Co-authored-by: Dan Upton --- .changelog/14869.txt | 3 + .../merge_service_config.go | 17 +-- .../merge_service_config_test.go | 2 +- agent/consul/catalog_endpoint.go | 5 +- agent/consul/health_endpoint.go | 3 +- .../dataplane/get_envoy_bootstrap_params.go | 24 +++- .../get_envoy_bootstrap_params_test.go | 103 +++++++++++++++--- .../services/dataplane/server.go | 3 + agent/service_manager.go | 6 +- 9 files changed, 136 insertions(+), 30 deletions(-) create mode 100644 .changelog/14869.txt rename agent/{consul => configentry}/merge_service_config.go (94%) rename agent/{consul => configentry}/merge_service_config_test.go (99%) diff --git a/.changelog/14869.txt b/.changelog/14869.txt new file mode 100644 index 000000000..58dafb62e --- /dev/null +++ b/.changelog/14869.txt @@ -0,0 +1,3 @@ +```release-note:bug +grpc: Merge proxy-defaults and service-defaults in GetEnvoyBootstrapParams response. +``` diff --git a/agent/consul/merge_service_config.go b/agent/configentry/merge_service_config.go similarity index 94% rename from agent/consul/merge_service_config.go rename to agent/configentry/merge_service_config.go index 706e24f4a..f11d96fcc 100644 --- a/agent/consul/merge_service_config.go +++ b/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/agent/consul/state" + "github.com/hashicorp/consul/acl" "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, diff --git a/agent/consul/merge_service_config_test.go b/agent/configentry/merge_service_config_test.go similarity index 99% rename from agent/consul/merge_service_config_test.go rename to agent/configentry/merge_service_config_test.go index a4b88308e..eb5e97d42 100644 --- a/agent/consul/merge_service_config_test.go +++ b/agent/configentry/merge_service_config_test.go @@ -1,4 +1,4 @@ -package consul +package configentry import ( "testing" diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 696ae314a..bf0492a7b 100644 --- a/agent/consul/catalog_endpoint.go +++ b/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 } diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go index ee8f32888..28823d958 100644 --- a/agent/consul/health_endpoint.go +++ b/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 } diff --git a/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params.go b/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params.go index 4456e361b..08f53578f 100644 --- a/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params.go +++ b/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") diff --git a/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go b/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go index 230f95e81..55aeca0e3 100644 --- a/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go +++ b/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" - resolver "github.com/hashicorp/consul/acl/resolver" + "github.com/hashicorp/consul/acl" + "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 + 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) - require.Equal(t, structpb.NewStringValue(proxyConfigValue), resp.Config.Fields[proxyConfigKey]) + requireConfigField(t, resp, proxyConfigKey, structpb.NewStringValue(proxyConfigValue)) 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 { diff --git a/agent/grpc-external/services/dataplane/server.go b/agent/grpc-external/services/dataplane/server.go index 4b4aef061..b665b368a 100644 --- a/agent/grpc-external/services/dataplane/server.go +++ b/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 diff --git a/agent/service_manager.go b/agent/service_manager.go index f9f449874..290102cb1 100644 --- a/agent/service_manager.go +++ b/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 }