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.

Sign up for GitHub

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

Jump to bottom

Fix proxy-defaults incorrectly merging config on upstreams. #16021

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Fix proxy-defaults incorrectly merging config on upstreams. #16021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 11 additions & 18 deletions agent/configentry/resolve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ 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 @@ -50,21 +49,19 @@ func ComputeResolvedServiceConfig(
thisReply.EnvoyExtensions = proxyConf.EnvoyExtensions
thisReply.AccessLogs = proxyConf.AccessLogs

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

// Only MeshGateway and Protocol should affect upstreams.
// MeshGateway is strange. It's marshaled into UpstreamConfigs via the arbitrary map, but it
// uses concrete fields everywhere else. We always take the explicit definition here for
// wildcard upstreams and discard the user setting it via arbitrary map in proxy-defaults.
if err := mergo.MergeWithOverwrite(&wildcardUpstreamDefaults, mapCopy); err != nil {
return nil, fmt.Errorf("failed to merge upstream config map for proxy-defaults: %v", err)
if mgw, ok := thisReply.ProxyConfig["mesh_gateway"]; ok {
wildcardUpstreamDefaults["mesh_gateway"] = mgw
}
if !proxyConf.MeshGateway.IsZero() {
wildcardUpstreamDefaults["mesh_gateway"] = proxyConf.MeshGateway
}
if protocol, ok := thisReply.ProxyConfig["protocol"]; ok {
wildcardUpstreamDefaults["protocol"] = protocol
}
}

serviceConf := entries.GetServiceDefaults(
Expand Down Expand Up @@ -149,9 +146,7 @@ func ComputeResolvedServiceConfig(

// First store all upstreams that were provided in the request
for _, sid := range upstreamIDs {
if _, ok := seenUpstreams[sid]; !ok {
seenUpstreams[sid] = struct{}{}
}
seenUpstreams[sid] = struct{}{}
}

// Then store upstreams inferred from service-defaults and mapify the overrides.
Expand Down Expand Up @@ -204,21 +199,19 @@ 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)
protocol := proxyConfGlobalProtocol
if err := mergo.MergeWithOverwrite(&resolvedCfg, wildcardUpstreamDefaults); err != nil {
return nil, fmt.Errorf("failed to merge wildcard defaults into upstream: %v", err)
}

upstreamSvcDefaults := entries.GetServiceDefaults(
structs.NewServiceID(upstream.ID, &upstream.EnterpriseMeta),
)
if upstreamSvcDefaults != nil {
if upstreamSvcDefaults.Protocol != "" {
protocol = upstreamSvcDefaults.Protocol
resolvedCfg["protocol"] = upstreamSvcDefaults.Protocol
}
}

if protocol != "" {
resolvedCfg["protocol"] = protocol
}

// When dialing an upstream, the goal is to flatten the mesh gateway mode in this order
// (larger number wins):
// 1. Value from the proxy-defaults
Expand Down
11 changes: 9 additions & 2 deletions agent/configentry/resolve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,10 @@ func Test_ComputeResolvedServiceConfig(t *testing.T) {
name: "proxy inherits kitchen sink from proxy-defaults",
args: args{
scReq: &structs.ServiceConfigRequest{
Name: "sid",
Name: "sid",
UpstreamIDs: uids,
},
upstreamIDs: uids,
entries: &ResolvedServiceConfigSet{
ProxyDefaults: map[string]*structs.ProxyConfigEntry{
acl.DefaultEnterpriseMeta().PartitionOrDefault(): {
Expand Down Expand Up @@ -195,7 +197,12 @@ func Test_ComputeResolvedServiceConfig(t *testing.T) {
{
Upstream: wildcard,
Config: map[string]interface{}{
"foo": "bar",
"mesh_gateway": remoteMeshGW,
},
},
{
Upstream: uid,
Config: map[string]interface{}{
"mesh_gateway": remoteMeshGW,
},
},
Expand Down
20 changes: 17 additions & 3 deletions agent/consul/config_endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,8 +1025,9 @@ func TestConfigEntry_ResolveServiceConfig(t *testing.T) {
// Create a dummy proxy/service config in the state store to look up.
state := s1.fsm.State()
require.NoError(t, state.EnsureConfigEntry(1, &structs.ProxyConfigEntry{
Kind: structs.ProxyDefaults,
Name: structs.ProxyConfigGlobal,
Kind: structs.ProxyDefaults,
Name: structs.ProxyConfigGlobal,
MeshGateway: structs.MeshGatewayConfig{Mode: structs.MeshGatewayModeLocal},
Config: map[string]interface{}{
"foo": 1,
},
Expand Down Expand Up @@ -1056,12 +1057,25 @@ func TestConfigEntry_ResolveServiceConfig(t *testing.T) {
"foo": int64(1),
"protocol": "http",
},
MeshGateway: structs.MeshGatewayConfig{
Mode: structs.MeshGatewayModeLocal,
},
UpstreamConfigs: map[string]map[string]interface{}{
"*": {
"foo": int64(1),
"mesh_gateway": map[string]interface{}{
"Mode": "local",
},
},
"bar": {
"protocol": "grpc",
"mesh_gateway": map[string]interface{}{
"Mode": "local",
},
},
"baz": {
"mesh_gateway": map[string]interface{}{
"Mode": "local",
},
},
},
Meta: map[string]string{"foo": "bar"},
Expand Down
19 changes: 0 additions & 19 deletions agent/service_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (

"github.com/stretchr/testify/require"

"github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/hashicorp/consul/testrpc"
Expand Down Expand Up @@ -421,12 +420,6 @@ func TestServiceManager_PersistService_API(t *testing.T) {
"protocol": "http",
},
UpstreamIDConfigs: structs.OpaqueUpstreamConfigs{
{
Upstream: structs.NewServiceID(structs.WildcardSpecifier, acl.DefaultEnterpriseMeta().WithWildcardNamespace()),
Config: map[string]interface{}{
"foo": int64(1),
},
},
{
Upstream: structs.NewServiceID("redis", nil),
Config: map[string]interface{}{
Expand Down Expand Up @@ -475,12 +468,6 @@ func TestServiceManager_PersistService_API(t *testing.T) {
"protocol": "http",
},
UpstreamIDConfigs: structs.OpaqueUpstreamConfigs{
{
Upstream: structs.NewServiceID(structs.WildcardSpecifier, acl.DefaultEnterpriseMeta().WithWildcardNamespace()),
Config: map[string]interface{}{
"foo": int64(1),
},
},
{
Upstream: structs.NewServiceID("redis", nil),
Config: map[string]interface{}{
Expand Down Expand Up @@ -660,12 +647,6 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) {
"protocol": "http",
},
UpstreamIDConfigs: structs.OpaqueUpstreamConfigs{
{
Upstream: structs.NewServiceID(structs.WildcardSpecifier, acl.DefaultEnterpriseMeta().WithWildcardNamespace()),
Config: map[string]interface{}{
"foo": int64(1),
},
},
{
Upstream: structs.NewServiceID("redis", nil),
Config: map[string]interface{}{
Expand Down