Fix proxy-defaults incorrectly merging config on upstreams. (#16021)

hashicorp · Jan 25, 2023 · dbb948a · dbb948a
1 parent 2ddefcc
commit dbb948a
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 42 deletions.
diff --git a/agent/configentry/resolve.go b/agent/configentry/resolve.go
@@ -33,7 +33,6 @@ func ComputeResolvedServiceConfig(
 	// blocking query, this function will be rerun and these state store lookups will both be current.
 	// We use the default enterprise meta to look up the global proxy defaults because they are not namespaced.
 
-	var proxyConfGlobalProtocol string
 	proxyConf := entries.GetProxyDefaults(args.PartitionOrDefault())
 	if proxyConf != nil {
 		// Apply the proxy defaults to the sidecar's proxy config
@@ -50,21 +49,19 @@ func ComputeResolvedServiceConfig(
 		thisReply.EnvoyExtensions = proxyConf.EnvoyExtensions
 		thisReply.AccessLogs = proxyConf.AccessLogs
 
-		parsed, err := structs.ParseUpstreamConfigNoDefaults(mapCopy.(map[string]interface{}))
-		if err != nil {
-			return nil, fmt.Errorf("failed to parse upstream config map for proxy-defaults: %v", err)
-		}
-		proxyConfGlobalProtocol = parsed.Protocol
-
+		// Only MeshGateway and Protocol should affect upstreams.
 		// MeshGateway is strange. It's marshaled into UpstreamConfigs via the arbitrary map, but it
 		// uses concrete fields everywhere else. We always take the explicit definition here for
 		// wildcard upstreams and discard the user setting it via arbitrary map in proxy-defaults.
-		if err := mergo.MergeWithOverwrite(&wildcardUpstreamDefaults, mapCopy); err != nil {
-			return nil, fmt.Errorf("failed to merge upstream config map for proxy-defaults: %v", err)
+		if mgw, ok := thisReply.ProxyConfig["mesh_gateway"]; ok {
+			wildcardUpstreamDefaults["mesh_gateway"] = mgw
 		}
 		if !proxyConf.MeshGateway.IsZero() {
 			wildcardUpstreamDefaults["mesh_gateway"] = proxyConf.MeshGateway
 		}
+		if protocol, ok := thisReply.ProxyConfig["protocol"]; ok {
+			wildcardUpstreamDefaults["protocol"] = protocol
+		}
 	}
 
 	serviceConf := entries.GetServiceDefaults(
@@ -149,9 +146,7 @@ func ComputeResolvedServiceConfig(
 
 	// First store all upstreams that were provided in the request
 	for _, sid := range upstreamIDs {
-		if _, ok := seenUpstreams[sid]; !ok {
-			seenUpstreams[sid] = struct{}{}
-		}
+		seenUpstreams[sid] = struct{}{}
 	}
 
 	// Then store upstreams inferred from service-defaults and mapify the overrides.
@@ -204,21 +199,19 @@ func ComputeResolvedServiceConfig(
 		// 2. Protocol for upstream service defined in its service-defaults (how the upstream wants to be addressed)
 		// 3. Protocol defined for the upstream in the service-defaults.(upstream_config.defaults|upstream_config.overrides) of the downstream
 		// 	  (how the downstream wants to address it)
-		protocol := proxyConfGlobalProtocol
+		if err := mergo.MergeWithOverwrite(&resolvedCfg, wildcardUpstreamDefaults); err != nil {
+			return nil, fmt.Errorf("failed to merge wildcard defaults into upstream: %v", err)
+		}
 
 		upstreamSvcDefaults := entries.GetServiceDefaults(
 			structs.NewServiceID(upstream.ID, &upstream.EnterpriseMeta),
 		)
 		if upstreamSvcDefaults != nil {
 			if upstreamSvcDefaults.Protocol != "" {
-				protocol = upstreamSvcDefaults.Protocol
+				resolvedCfg["protocol"] = upstreamSvcDefaults.Protocol
 			}
 		}
 
-		if protocol != "" {
-			resolvedCfg["protocol"] = protocol
-		}
-
 		// When dialing an upstream, the goal is to flatten the mesh gateway mode in this order
 		// (larger number wins):
 		//  1. Value from the proxy-defaults

diff --git a/agent/configentry/resolve_test.go b/agent/configentry/resolve_test.go
@@ -141,8 +141,10 @@ func Test_ComputeResolvedServiceConfig(t *testing.T) {
 			name: "proxy inherits kitchen sink from proxy-defaults",
 			args: args{
 				scReq: &structs.ServiceConfigRequest{
-					Name: "sid",
+					Name:        "sid",
+					UpstreamIDs: uids,
 				},
+				upstreamIDs: uids,
 				entries: &ResolvedServiceConfigSet{
 					ProxyDefaults: map[string]*structs.ProxyConfigEntry{
 						acl.DefaultEnterpriseMeta().PartitionOrDefault(): {
@@ -195,7 +197,12 @@ func Test_ComputeResolvedServiceConfig(t *testing.T) {
 					{
 						Upstream: wildcard,
 						Config: map[string]interface{}{
-							"foo":          "bar",
+							"mesh_gateway": remoteMeshGW,
+						},
+					},
+					{
+						Upstream: uid,
+						Config: map[string]interface{}{
 							"mesh_gateway": remoteMeshGW,
 						},
 					},

diff --git a/agent/consul/config_endpoint_test.go b/agent/consul/config_endpoint_test.go
@@ -1025,8 +1025,9 @@ func TestConfigEntry_ResolveServiceConfig(t *testing.T) {
 	// Create a dummy proxy/service config in the state store to look up.
 	state := s1.fsm.State()
 	require.NoError(t, state.EnsureConfigEntry(1, &structs.ProxyConfigEntry{
-		Kind: structs.ProxyDefaults,
-		Name: structs.ProxyConfigGlobal,
+		Kind:        structs.ProxyDefaults,
+		Name:        structs.ProxyConfigGlobal,
+		MeshGateway: structs.MeshGatewayConfig{Mode: structs.MeshGatewayModeLocal},
 		Config: map[string]interface{}{
 			"foo": 1,
 		},
@@ -1056,12 +1057,25 @@ func TestConfigEntry_ResolveServiceConfig(t *testing.T) {
 			"foo":      int64(1),
 			"protocol": "http",
 		},
+		MeshGateway: structs.MeshGatewayConfig{
+			Mode: structs.MeshGatewayModeLocal,
+		},
 		UpstreamConfigs: map[string]map[string]interface{}{
 			"*": {
-				"foo": int64(1),
+				"mesh_gateway": map[string]interface{}{
+					"Mode": "local",
+				},
 			},
 			"bar": {
 				"protocol": "grpc",
+				"mesh_gateway": map[string]interface{}{
+					"Mode": "local",
+				},
+			},
+			"baz": {
+				"mesh_gateway": map[string]interface{}{
+					"Mode": "local",
+				},
 			},
 		},
 		Meta: map[string]string{"foo": "bar"},

diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go
@@ -11,7 +11,6 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/consul/testrpc"
@@ -421,12 +420,6 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 				"protocol": "http",
 			},
 			UpstreamIDConfigs: structs.OpaqueUpstreamConfigs{
-				{
-					Upstream: structs.NewServiceID(structs.WildcardSpecifier, acl.DefaultEnterpriseMeta().WithWildcardNamespace()),
-					Config: map[string]interface{}{
-						"foo": int64(1),
-					},
-				},
 				{
 					Upstream: structs.NewServiceID("redis", nil),
 					Config: map[string]interface{}{
@@ -475,12 +468,6 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 				"protocol": "http",
 			},
 			UpstreamIDConfigs: structs.OpaqueUpstreamConfigs{
-				{
-					Upstream: structs.NewServiceID(structs.WildcardSpecifier, acl.DefaultEnterpriseMeta().WithWildcardNamespace()),
-					Config: map[string]interface{}{
-						"foo": int64(1),
-					},
-				},
 				{
 					Upstream: structs.NewServiceID("redis", nil),
 					Config: map[string]interface{}{
@@ -660,12 +647,6 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) {
 				"protocol": "http",
 			},
 			UpstreamIDConfigs: structs.OpaqueUpstreamConfigs{
-				{
-					Upstream: structs.NewServiceID(structs.WildcardSpecifier, acl.DefaultEnterpriseMeta().WithWildcardNamespace()),
-					Config: map[string]interface{}{
-						"foo": int64(1),
-					},
-				},
 				{
 					Upstream: structs.NewServiceID("redis", nil),
 					Config: map[string]interface{}{