Skip to content

Commit

Permalink
Manual backport of #17894.
Browse files Browse the repository at this point in the history
  • Loading branch information
hashi-derek committed Jul 5, 2023
1 parent 3011885 commit 1bc2b6d
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 48 deletions.
3 changes: 3 additions & 0 deletions .changelog/17894.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
connect: Fix incorrect protocol config merging for transparent proxy implicit upstreams.
```
30 changes: 28 additions & 2 deletions agent/configentry/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,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 All @@ -59,9 +60,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 @@ -227,6 +249,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
46 changes: 0 additions & 46 deletions agent/consul/config_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1316,16 +1316,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 @@ -1377,15 +1367,6 @@ func TestConfigEntry_ResolveServiceConfig_Upstreams(t *testing.T) {
"protocol": "grpc",
},
UpstreamIDConfigs: structs.OpaqueUpstreamConfigsDeprecated{
{
Upstream: structs.NewServiceID(
structs.WildcardSpecifier,
acl.DefaultEnterpriseMeta().WithWildcardNamespace(),
),
Config: map[string]interface{}{
"protocol": "grpc",
},
},
{
Upstream: structs.NewServiceID("cache", nil),
Config: map[string]interface{}{
Expand Down Expand Up @@ -1442,12 +1423,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 @@ -2202,17 +2177,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 @@ -2281,16 +2245,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 1bc2b6d

Please sign in to comment.