From 922844b8e001ed6118dcb93d21e99341154d77c8 Mon Sep 17 00:00:00 2001 From: Derek Menteer <105233703+hashi-derek@users.noreply.github.com> Date: Mon, 5 Feb 2024 16:00:19 -0600 Subject: [PATCH] Fix issue with persisting proxy-defaults (#20481) Fix issue with persisting proxy-defaults This resolves an issue introduced in hashicorp/consul#19829 where the proxy-defaults configuration entry with an HTTP protocol cannot be updated after it has been persisted once and a router exists. This occurs because the protocol field is not properly pre-computed before being passed into validation functions. --- .changelog/20481.txt | 3 ++ agent/config_endpoint_test.go | 52 ++++++++++++++++++++++++++++++ agent/consul/state/config_entry.go | 23 ++++--------- agent/structs/config_entry.go | 20 ++++++++++++ agent/structs/config_entry_test.go | 41 +++++++++++++++++++++++ 5 files changed, 122 insertions(+), 17 deletions(-) create mode 100644 .changelog/20481.txt diff --git a/.changelog/20481.txt b/.changelog/20481.txt new file mode 100644 index 0000000000..5c2772648c --- /dev/null +++ b/.changelog/20481.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect: Fix issue where re-persisting existing proxy-defaults using `http` protocol fails with a protocol-mismatch error. +``` diff --git a/agent/config_endpoint_test.go b/agent/config_endpoint_test.go index ab831307eb..8697b55e5b 100644 --- a/agent/config_endpoint_test.go +++ b/agent/config_endpoint_test.go @@ -517,6 +517,58 @@ func TestConfig_Apply_ProxyDefaultsMeshGateway(t *testing.T) { } } +func TestConfig_Apply_ProxyDefaultsProtocol(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + t.Parallel() + + a := NewTestAgent(t, "") + defer a.Shutdown() + testrpc.WaitForTestAgent(t, a.RPC, "dc1") + + writeConf := func(body string) { + req, _ := http.NewRequest("PUT", "/v1/config", bytes.NewBuffer([]byte(body))) + resp := httptest.NewRecorder() + _, err := a.srv.ConfigApply(resp, req) + require.NoError(t, err) + require.Equal(t, 200, resp.Code, "non-200 Response Code: %s", resp.Body.String()) + } + + // Set the default protocol + writeConf(`{ + "Kind": "proxy-defaults", + "Name": "global", + "Config": { + "Protocol": "http" + } + }`) + + // Create a router that depends on the protocol + writeConf(`{ + "Kind": "service-router", + "Name": "route1" + }`) + + // Ensure we can rewrite the proxy-defaults without a protocol-mismatch error. + // This should be taken care of in the ProxyConfigEntry.Normalize() function. + writeConf(`{ + "Kind": "proxy-defaults", + "Name": "global", + "Config": { + "Protocol": "http", + "some-field": "is_changed" + } + }`) + + // Rewrite the router that depends on the protocol + writeConf(`{ + "Kind": "service-router", + "Name": "route1" + }`) +} + func TestConfig_Apply_CAS(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index 67f8b63277..e0fe9e73ec 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -6,7 +6,6 @@ package state import ( "errors" "fmt" - "github.com/mitchellh/mapstructure" "strings" memdb "github.com/hashicorp/go-memdb" @@ -528,7 +527,12 @@ func insertConfigEntryWithTxn(tx WriteTxn, idx uint64, conf structs.ConfigEntry) return err } case structs.ProxyDefaults: - err := addProtocol(conf.(*structs.ProxyConfigEntry)) + proxyDefaults, ok := conf.(*structs.ProxyConfigEntry) + if !ok { + return fmt.Errorf("unable to cast config entry to proxy-defaults") + } + // Ensure we pre-compute the protocol before persisting always. + err := proxyDefaults.ComputeProtocol() if err != nil { return err } @@ -557,21 +561,6 @@ func insertConfigEntryWithTxn(tx WriteTxn, idx uint64, conf structs.ConfigEntry) return nil } -// proxyConfig is a snippet from agent/xds/config.go:ProxyConfig -type proxyConfig struct { - Protocol string `mapstructure:"protocol"` -} - -func addProtocol(conf *structs.ProxyConfigEntry) error { - var cfg proxyConfig - err := mapstructure.WeakDecode(conf.Config, &cfg) - if err != nil { - return err - } - conf.Protocol = cfg.Protocol - return nil -} - func configEntryHasVirtualIP(c structs.ConfigEntry) bool { if c == nil || c.GetName() == "" { return false diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index b6172a54e2..04393b2761 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -507,6 +507,22 @@ func (e *ProxyConfigEntry) GetMeta() map[string]string { return e.Meta } +func (e *ProxyConfigEntry) ComputeProtocol() error { + // proxyConfig is a snippet from agent/xds/config.go:ProxyConfig + // We calculate this up-front so that the expensive mapstructure decode + // is not needed during discovery chain compile time. + type proxyConfig struct { + Protocol string `mapstructure:"protocol"` + } + var cfg proxyConfig + err := mapstructure.WeakDecode(e.Config, &cfg) + if err != nil { + return err + } + e.Protocol = cfg.Protocol + return nil +} + func (e *ProxyConfigEntry) Normalize() error { if e == nil { return fmt.Errorf("config entry is nil") @@ -524,6 +540,10 @@ func (e *ProxyConfigEntry) Normalize() error { e.EnterpriseMeta.Normalize() + if err := e.ComputeProtocol(); err != nil { + return err + } + h, err := HashConfigEntry(e) if err != nil { return err diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index d52c5fd3da..e57e2c4041 100644 --- a/agent/structs/config_entry_test.go +++ b/agent/structs/config_entry_test.go @@ -3662,6 +3662,47 @@ func TestProxyConfigEntry(t *testing.T) { testConfigEntryNormalizeAndValidate(t, cases) } +func TestProxyConfigEntry_ComputeProtocol(t *testing.T) { + t.Run("ComputeProtocol sets protocol field correctly", func(t *testing.T) { + pd := &ProxyConfigEntry{ + Kind: ProxyDefaults, + Name: "global", + Config: map[string]interface{}{ + "protocol": "http", + }, + } + require.NoError(t, pd.ComputeProtocol()) + require.Equal(t, &ProxyConfigEntry{ + Kind: ProxyDefaults, + Name: "global", + Protocol: "http", + Config: map[string]interface{}{ + "protocol": "http", + }, + }, pd) + }) + t.Run("Normalize sets protocol field correctly", func(t *testing.T) { + pd := &ProxyConfigEntry{ + Kind: ProxyDefaults, + Name: "global", + Config: map[string]interface{}{ + "protocol": "http", + }, + } + require.NoError(t, pd.Normalize()) + pd.Hash = 0 + require.Equal(t, &ProxyConfigEntry{ + Kind: ProxyDefaults, + Name: "global", + Protocol: "http", + Config: map[string]interface{}{ + "protocol": "http", + }, + EnterpriseMeta: *acl.DefaultEnterpriseMeta(), + }, pd) + }) +} + func requireContainsLower(t *testing.T, haystack, needle string) { t.Helper() require.Contains(t, strings.ToLower(haystack), strings.ToLower(needle))