diff --git a/.changelog/17894.txt b/.changelog/17894.txt new file mode 100644 index 0000000000..5749f995f7 --- /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 de6165d5cc..882f1d16b5 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 ad6dac744c..7dc7632fad 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{}{