Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manual backport of #17894. #18008

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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