From fe5c145cddd89da804d0de7d19bdc5d0d276df3b Mon Sep 17 00:00:00 2001 From: Derek Menteer Date: Tue, 27 Jun 2023 10:10:01 -0500 Subject: [PATCH] Fix incorrect protocol for transparent proxy upstreams. This PR fixes a bug that was introduced in: https://github.com/hashicorp/consul/pull/16021 A user setting a protocol in proxy-defaults would cause tproxy implicit upstreams to not honor the upstream service's protocol set in its `ServiceDefaults.Protocol` field, and would instead always use the proxy-defaults value. Due to the fact that upstreams configured with "tcp" can successfully contact upstream "http" services, this issue was not recognized until recently (a proxy-defaults with "tcp" and a listening service with "http" would make successful requests, but not the opposite). As a temporary work-around, users experiencing this issue can explicitly set the protocol on the `ServiceDefaults.UpstreamConfig.Overrides`, which should take precedence. The fix in this PR removes the proxy-defaults protocol from the wildcard upstream that tproxy uses to configure implicit upstreams. When the protocol was included, it would always overwrite the value during discovery chain compilation, which was not correct. The discovery chain compiler also consumes proxy defaults to determine the protocol, so simply excluding it from the wildcard upstream config map resolves the issue. --- .changelog/17894.txt | 3 +++ agent/configentry/resolve.go | 30 ++++++++++++++++++++-- agent/consul/config_endpoint_test.go | 37 ---------------------------- 3 files changed, 31 insertions(+), 39 deletions(-) create mode 100644 .changelog/17894.txt diff --git a/.changelog/17894.txt b/.changelog/17894.txt new file mode 100644 index 000000000000..5749f995f71a --- /dev/null +++ b/.changelog/17894.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect: Fix incorrect protocol config merging for transparent proxy implicit upstreams. +``` diff --git a/agent/configentry/resolve.go b/agent/configentry/resolve.go index de6165d5cc44..882f1d16b548 100644 --- a/agent/configentry/resolve.go +++ b/agent/configentry/resolve.go @@ -36,6 +36,7 @@ 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 @@ -63,9 +64,30 @@ func ComputeResolvedServiceConfig( if !proxyConf.MeshGateway.IsZero() { wildcardUpstreamDefaults["mesh_gateway"] = proxyConf.MeshGateway } - if protocol, ok := thisReply.ProxyConfig["protocol"]; ok { - wildcardUpstreamDefaults["protocol"] = protocol + + // We explicitly DO NOT merge the protocol from proxy-defaults into the wildcard upstream here. + // TProxy will try to use the data from the `wildcardUpstreamDefaults` as a source of truth, which is + // normally correct to inherit from proxy-defaults. However, it is NOT correct for protocol. + // + // This edge-case is different for `protocol` from other fields, since the protocol can be + // set on both the local `ServiceDefaults.UpstreamOverrides` and upstream `ServiceDefaults.Protocol`. + // This means that when proxy-defaults is set, it would always be treated as an explicit override, + // and take precedence over the protocol that is set on the discovery chain (which comes from the + // service's preference in its service-defaults), which is wrong. + // + // When the upstream is not explicitly defined, we should only get the protocol from one of these locations: + // 1. For tproxy non-peering services, it can be fetched via the discovery chain. + // The chain compiler merges the proxy-defaults protocol with the upstream's preferred service-defaults protocol. + // 2. For tproxy non-peering services with default upstream overrides, it will come from the wildcard upstream overrides. + // 3. For tproxy non-peering services with specific upstream overrides, it will come from the specific upstream override defined. + // 4. For tproxy peering services, they do not honor the proxy-defaults, since they reside in a different cluster. + // The data will come from a separate peerMeta field. + // In all of these cases, it is not necessary for the proxy-defaults to exist in the wildcard upstream. + 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 } serviceConf := entries.GetServiceDefaults( @@ -210,6 +232,10 @@ 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) + if proxyConfGlobalProtocol != "" { + resolvedCfg["protocol"] = proxyConfGlobalProtocol + } + if err := mergo.MergeWithOverwrite(&resolvedCfg, wildcardUpstreamDefaults); err != nil { return nil, fmt.Errorf("failed to merge wildcard defaults into upstream: %v", err) } diff --git a/agent/consul/config_endpoint_test.go b/agent/consul/config_endpoint_test.go index ad6dac744cfc..7dc7632fade7 100644 --- a/agent/consul/config_endpoint_test.go +++ b/agent/consul/config_endpoint_test.go @@ -1444,16 +1444,6 @@ func TestConfigEntry_ResolveServiceConfig_Upstreams(t *testing.T) { "protocol": "grpc", }, UpstreamConfigs: structs.OpaqueUpstreamConfigs{ - { - Upstream: structs.PeeredServiceName{ - ServiceName: structs.NewServiceName( - structs.WildcardSpecifier, - acl.DefaultEnterpriseMeta().WithWildcardNamespace()), - }, - Config: map[string]interface{}{ - "protocol": "grpc", - }, - }, { Upstream: cache, Config: map[string]interface{}{ @@ -1510,12 +1500,6 @@ func TestConfigEntry_ResolveServiceConfig_Upstreams(t *testing.T) { "protocol": "grpc", }, UpstreamConfigs: structs.OpaqueUpstreamConfigs{ - { - Upstream: wildcard, - Config: map[string]interface{}{ - "protocol": "grpc", - }, - }, { Upstream: cache, Config: map[string]interface{}{ @@ -2267,17 +2251,6 @@ func TestConfigEntry_ResolveServiceConfig_UpstreamProxyDefaultsProtocol(t *testi require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConfigEntry.ResolveServiceConfig", &args, &out)) expected := structs.OpaqueUpstreamConfigs{ - { - Upstream: structs.PeeredServiceName{ - ServiceName: structs.NewServiceName( - structs.WildcardSpecifier, - acl.DefaultEnterpriseMeta().WithWildcardNamespace(), - ), - }, - Config: map[string]interface{}{ - "protocol": "http", - }, - }, { Upstream: id("bar"), Config: map[string]interface{}{ @@ -2346,16 +2319,6 @@ func TestConfigEntry_ResolveServiceConfig_ProxyDefaultsProtocol_UsedForAllUpstre "protocol": "http", }, UpstreamConfigs: structs.OpaqueUpstreamConfigs{ - { - Upstream: structs.PeeredServiceName{ - ServiceName: structs.NewServiceName( - structs.WildcardSpecifier, - acl.DefaultEnterpriseMeta().WithWildcardNamespace()), - }, - Config: map[string]interface{}{ - "protocol": "http", - }, - }, { Upstream: psn, Config: map[string]interface{}{