Skip to content

Commit

Permalink
Fix incorrect protocol for transparent proxy upstreams.
Browse files Browse the repository at this point in the history
This PR fixes a bug that was introduced in:
#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.
  • Loading branch information
hashi-derek committed Jun 27, 2023
1 parent 6bc2222 commit 29b3597
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 39 deletions.
30 changes: 28 additions & 2 deletions agent/configentry/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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)
}
Expand Down
37 changes: 0 additions & 37 deletions agent/consul/config_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}{
Expand Down Expand Up @@ -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{}{
Expand Down Expand Up @@ -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{}{
Expand Down Expand Up @@ -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{}{
Expand Down

0 comments on commit 29b3597

Please sign in to comment.