From 62efaaab21f48200b2785b9d39b369190ccb2c9e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 29 Apr 2021 15:54:27 -0400 Subject: [PATCH 1/3] config-entry: remove Kind and Name field from Mesh config entry No config entry needs a Kind field. It is only used to determine the Go type to target. As we introduce new config entries (like this one) we can remove the kind field and have the GetKind method return the single supported value. In this case (similar to proxy-defaults) the Name field is also unnecessary. We always use the same value. So we can omit the name field entirely. --- agent/config/runtime_test.go | 8 -------- agent/consul/fsm/snapshot_oss_test.go | 2 -- agent/proxycfg/state_test.go | 2 -- agent/structs/config_entry.go | 2 +- agent/structs/config_entry_mesh.go | 14 +------------- agent/structs/config_entry_oss.go | 3 +++ agent/structs/config_entry_test.go | 4 ---- agent/xds/listeners_test.go | 2 -- api/config_entry.go | 2 +- api/config_entry_cluster.go | 5 ++--- api/config_entry_test.go | 3 --- command/config/write/config_write.go | 13 ++++++++++--- command/config/write/config_write_test.go | 6 ------ 13 files changed, 18 insertions(+), 48 deletions(-) diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index dffcc4be96..85db6d3be9 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -4157,7 +4157,6 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { "bootstrap": [ { "kind": "mesh", - "name": "mesh", "meta" : { "foo": "bar", "gir": "zim" @@ -4174,7 +4173,6 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { config_entries { bootstrap { kind = "mesh" - name = "mesh" meta { "foo" = "bar" "gir" = "zim" @@ -4190,8 +4188,6 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.DataDir = dataDir rt.ConfigEntryBootstrap = []structs.ConfigEntry{ &structs.MeshConfigEntry{ - Kind: structs.MeshConfig, - Name: structs.MeshConfigMesh, Meta: map[string]string{ "foo": "bar", "gir": "zim", @@ -4212,7 +4208,6 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { "bootstrap": [ { "Kind": "mesh", - "Name": "mesh", "Meta" : { "foo": "bar", "gir": "zim" @@ -4229,7 +4224,6 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { config_entries { bootstrap { Kind = "mesh" - Name = "mesh" Meta { "foo" = "bar" "gir" = "zim" @@ -4245,8 +4239,6 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.DataDir = dataDir rt.ConfigEntryBootstrap = []structs.ConfigEntry{ &structs.MeshConfigEntry{ - Kind: structs.MeshConfig, - Name: structs.MeshConfigMesh, Meta: map[string]string{ "foo": "bar", "gir": "zim", diff --git a/agent/consul/fsm/snapshot_oss_test.go b/agent/consul/fsm/snapshot_oss_test.go index 99de8ebc9a..cb05399870 100644 --- a/agent/consul/fsm/snapshot_oss_test.go +++ b/agent/consul/fsm/snapshot_oss_test.go @@ -428,8 +428,6 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { // mesh config entry meshConfig := &structs.MeshConfigEntry{ - Kind: structs.MeshConfig, - Name: structs.MeshConfigMesh, TransparentProxy: structs.TransparentProxyMeshConfig{ CatalogDestinationsOnly: true, }, diff --git a/agent/proxycfg/state_test.go b/agent/proxycfg/state_test.go index 122a6e5453..e86d1b21e8 100644 --- a/agent/proxycfg/state_test.go +++ b/agent/proxycfg/state_test.go @@ -1674,8 +1674,6 @@ func TestState_WatchesAndUpdates(t *testing.T) { CorrelationID: meshConfigEntryID, Result: &structs.ConfigEntryResponse{ Entry: &structs.MeshConfigEntry{ - Kind: structs.MeshConfig, - Name: structs.MeshConfigMesh, TransparentProxy: structs.TransparentProxyMeshConfig{}, }, }, diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index afa5f34fc1..e05e131efa 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -529,7 +529,7 @@ func MakeConfigEntry(kind, name string) (ConfigEntry, error) { case ServiceIntentions: return &ServiceIntentionsConfigEntry{Name: name}, nil case MeshConfig: - return &MeshConfigEntry{Name: name}, nil + return &MeshConfigEntry{}, nil default: return nil, fmt.Errorf("invalid config entry kind: %s", kind) } diff --git a/agent/structs/config_entry_mesh.go b/agent/structs/config_entry_mesh.go index 8e7688a06c..74d70765a4 100644 --- a/agent/structs/config_entry_mesh.go +++ b/agent/structs/config_entry_mesh.go @@ -7,9 +7,6 @@ import ( ) type MeshConfigEntry struct { - Kind string - Name string - // TransparentProxy contains cluster-wide options pertaining to TPROXY mode // when enabled. TransparentProxy TransparentProxyMeshConfig `alias:"transparent_proxy"` @@ -36,7 +33,7 @@ func (e *MeshConfigEntry) GetName() string { return "" } - return e.Name + return MeshConfigMesh } func (e *MeshConfigEntry) GetMeta() map[string]string { @@ -51,11 +48,7 @@ func (e *MeshConfigEntry) Normalize() error { return fmt.Errorf("config entry is nil") } - e.Kind = MeshConfig - e.Name = MeshConfigMesh - e.EnterpriseMeta.Normalize() - return nil } @@ -63,11 +56,6 @@ func (e *MeshConfigEntry) Validate() error { if e == nil { return fmt.Errorf("config entry is nil") } - - if e.Name != MeshConfigMesh { - return fmt.Errorf("invalid name (%q), only %q is supported", e.Name, MeshConfigMesh) - } - if err := validateConfigEntryMeta(e.Meta); err != nil { return err } diff --git a/agent/structs/config_entry_oss.go b/agent/structs/config_entry_oss.go index a3ca07c1b8..9773643dde 100644 --- a/agent/structs/config_entry_oss.go +++ b/agent/structs/config_entry_oss.go @@ -19,6 +19,9 @@ func validateUnusedKeys(unused []string) error { for _, k := range unused { switch { case k == "CreateIndex" || k == "ModifyIndex": + case k == "kind" || k == "Kind": + // The kind field is used to determine the target, but doesn't need + // to exist on the target. case strings.HasSuffix(strings.ToLower(k), "namespace"): err = multierror.Append(err, fmt.Errorf("invalid config key %q, namespaces are a consul enterprise feature", k)) default: diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index 38c10deb0b..cace510bb4 100644 --- a/agent/structs/config_entry_test.go +++ b/agent/structs/config_entry_test.go @@ -1310,7 +1310,6 @@ func TestDecodeConfigEntry(t *testing.T) { name: "mesh", snake: ` kind = "mesh" - name = "mesh" meta { "foo" = "bar" "gir" = "zim" @@ -1321,7 +1320,6 @@ func TestDecodeConfigEntry(t *testing.T) { `, camel: ` Kind = "mesh" - Name = "mesh" Meta { "foo" = "bar" "gir" = "zim" @@ -1331,8 +1329,6 @@ func TestDecodeConfigEntry(t *testing.T) { } `, expect: &MeshConfigEntry{ - Kind: MeshConfig, - Name: MeshConfigMesh, Meta: map[string]string{ "foo": "bar", "gir": "zim", diff --git a/agent/xds/listeners_test.go b/agent/xds/listeners_test.go index 2f9102f42a..bab7057798 100644 --- a/agent/xds/listeners_test.go +++ b/agent/xds/listeners_test.go @@ -519,8 +519,6 @@ func TestListenersFromSnapshot(t *testing.T) { snap.ConnectProxy.MeshConfigSet = true snap.ConnectProxy.MeshConfig = &structs.MeshConfigEntry{ - Kind: structs.MeshConfig, - Name: structs.MeshConfigMesh, TransparentProxy: structs.TransparentProxyMeshConfig{ CatalogDestinationsOnly: true, }, diff --git a/api/config_entry.go b/api/config_entry.go index 6ae23be898..5047d07a5b 100644 --- a/api/config_entry.go +++ b/api/config_entry.go @@ -295,7 +295,7 @@ func makeConfigEntry(kind, name string) (ConfigEntry, error) { case ServiceIntentions: return &ServiceIntentionsConfigEntry{Kind: kind, Name: name}, nil case MeshConfig: - return &MeshConfigEntry{Kind: kind, Name: name}, nil + return &MeshConfigEntry{}, nil default: return nil, fmt.Errorf("invalid config entry kind: %s", kind) } diff --git a/api/config_entry_cluster.go b/api/config_entry_cluster.go index 057059f8aa..915fab5d3d 100644 --- a/api/config_entry_cluster.go +++ b/api/config_entry_cluster.go @@ -1,7 +1,6 @@ package api type MeshConfigEntry struct { - Kind string Name string Namespace string `json:",omitempty"` TransparentProxy TransparentProxyMeshConfig `alias:"transparent_proxy"` @@ -15,11 +14,11 @@ type TransparentProxyMeshConfig struct { } func (e *MeshConfigEntry) GetKind() string { - return e.Kind + return MeshConfig } func (e *MeshConfigEntry) GetName() string { - return e.Name + return MeshConfigMesh } func (e *MeshConfigEntry) GetNamespace() string { diff --git a/api/config_entry_test.go b/api/config_entry_test.go index 4bf5e23681..74f92afe88 100644 --- a/api/config_entry_test.go +++ b/api/config_entry_test.go @@ -1141,7 +1141,6 @@ func TestDecodeConfigEntry(t *testing.T) { body: ` { "Kind": "mesh", - "Name": "mesh", "Meta" : { "foo": "bar", "gir": "zim" @@ -1152,8 +1151,6 @@ func TestDecodeConfigEntry(t *testing.T) { } `, expect: &MeshConfigEntry{ - Kind: "mesh", - Name: "mesh", Meta: map[string]string{ "foo": "bar", "gir": "zim", diff --git a/command/config/write/config_write.go b/command/config/write/config_write.go index e150b40af9..c7b3a72fd4 100644 --- a/command/config/write/config_write.go +++ b/command/config/write/config_write.go @@ -6,13 +6,14 @@ import ( "io" "time" + "github.com/hashicorp/go-multierror" + "github.com/mitchellh/cli" + "github.com/mitchellh/mapstructure" + "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/command/flags" "github.com/hashicorp/consul/command/helpers" "github.com/hashicorp/consul/lib/decode" - "github.com/hashicorp/go-multierror" - "github.com/mitchellh/cli" - "github.com/mitchellh/mapstructure" ) func New(ui cli.Ui) *cmd { @@ -155,6 +156,12 @@ func newDecodeConfigEntry(raw map[string]interface{}) (api.ConfigEntry, error) { } for _, k := range md.Unused { + switch k { + case "kind", "Kind": + // The kind field is used to determine the target, but doesn't need + // to exist on the target. + continue + } err = multierror.Append(err, fmt.Errorf("invalid config key %q", k)) } if err != nil { diff --git a/command/config/write/config_write_test.go b/command/config/write/config_write_test.go index ee122f2b38..8b3fe120b9 100644 --- a/command/config/write/config_write_test.go +++ b/command/config/write/config_write_test.go @@ -2627,7 +2627,6 @@ func TestParseConfigEntry(t *testing.T) { name: "mesh", snake: ` kind = "mesh" - name = "mesh" meta { "foo" = "bar" "gir" = "zim" @@ -2638,7 +2637,6 @@ func TestParseConfigEntry(t *testing.T) { `, camel: ` Kind = "mesh" - Name = "mesh" Meta { "foo" = "bar" "gir" = "zim" @@ -2650,7 +2648,6 @@ func TestParseConfigEntry(t *testing.T) { snakeJSON: ` { "kind": "mesh", - "name": "mesh", "meta" : { "foo": "bar", "gir": "zim" @@ -2663,7 +2660,6 @@ func TestParseConfigEntry(t *testing.T) { camelJSON: ` { "Kind": "mesh", - "Name": "mesh", "Meta" : { "foo": "bar", "gir": "zim" @@ -2674,8 +2670,6 @@ func TestParseConfigEntry(t *testing.T) { } `, expect: &api.MeshConfigEntry{ - Kind: api.MeshConfig, - Name: api.MeshConfigMesh, Meta: map[string]string{ "foo": "bar", "gir": "zim", From a07a58a8737ffa3d87453fe73fb7d96196aaf1b6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 29 Apr 2021 17:44:32 -0400 Subject: [PATCH 2/3] config-entry: use custom MarshalJSON for mesh type So that the Kind field is added to the JSON object. --- agent/config_endpoint_test.go | 48 ++++++++++++++++++++++- agent/structs/config_entry_mesh.go | 17 ++++++++ api/config_entry_cluster.go | 17 +++++++- command/config/write/config_write_test.go | 31 +++++++++++++++ 4 files changed, 110 insertions(+), 3 deletions(-) diff --git a/agent/config_endpoint_test.go b/agent/config_endpoint_test.go index ec18747e0d..13ff251d85 100644 --- a/agent/config_endpoint_test.go +++ b/agent/config_endpoint_test.go @@ -7,10 +7,11 @@ import ( "net/http/httptest" "testing" - "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/consul/testrpc" "github.com/pkg/errors" "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/testrpc" ) func TestConfig_Get(t *testing.T) { @@ -48,6 +49,18 @@ func TestConfig_Get(t *testing.T) { }, }, }, + { + Datacenter: "dc1", + Entry: &structs.MeshConfigEntry{ + TransparentProxy: structs.TransparentProxyMeshConfig{ + CatalogDestinationsOnly: true, + }, + Meta: map[string]string{ + "key1": "value1", + "key2": "value2", + }, + }, + }, } for _, req := range reqs { out := false @@ -95,6 +108,37 @@ func TestConfig_Get(t *testing.T) { _, err := a.srv.Config(resp, req) require.Error(t, errors.New("Must provide either a kind or both kind and name"), err) }) + t.Run("get the single mesh config", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/config/mesh/mesh", nil) + resp := httptest.NewRecorder() + obj, err := a.srv.Config(resp, req) + require.NoError(t, err) + + ce, ok := obj.(*structs.MeshConfigEntry) + require.True(t, ok, "wrong type %T", obj) + // Set indexes to expected values for assertions + ce.CreateIndex = 12 + ce.ModifyIndex = 13 + + out, err := a.srv.marshalJSON(req, obj) + require.NoError(t, err) + + expected := ` +{ + "Kind": "mesh", + "TransparentProxy": { + "CatalogDestinationsOnly": true + }, + "Meta":{ + "key1": "value1", + "key2": "value2" + }, + "CreateIndex": 12, + "ModifyIndex": 13 +} +` + require.JSONEq(t, expected, string(out)) + }) } func TestConfig_Delete(t *testing.T) { diff --git a/agent/structs/config_entry_mesh.go b/agent/structs/config_entry_mesh.go index 74d70765a4..33792b381b 100644 --- a/agent/structs/config_entry_mesh.go +++ b/agent/structs/config_entry_mesh.go @@ -1,6 +1,7 @@ package structs import ( + "encoding/json" "fmt" "github.com/hashicorp/consul/acl" @@ -88,3 +89,19 @@ func (e *MeshConfigEntry) GetEnterpriseMeta() *EnterpriseMeta { return &e.EnterpriseMeta } + +// MarshalJSON adds the Kind field so that the JSON can be decoded back into the +// correct type. +// This method is implemented on the structs type (as apposed to the api type) +// because that is what the API currently uses to return a response. +func (e *MeshConfigEntry) MarshalJSON() ([]byte, error) { + type Alias MeshConfigEntry + source := &struct { + Kind string + *Alias + }{ + Kind: MeshConfig, + Alias: (*Alias)(e), + } + return json.Marshal(source) +} diff --git a/api/config_entry_cluster.go b/api/config_entry_cluster.go index 915fab5d3d..48fcc44fcf 100644 --- a/api/config_entry_cluster.go +++ b/api/config_entry_cluster.go @@ -1,7 +1,8 @@ package api +import "encoding/json" + type MeshConfigEntry struct { - Name string Namespace string `json:",omitempty"` TransparentProxy TransparentProxyMeshConfig `alias:"transparent_proxy"` Meta map[string]string `json:",omitempty"` @@ -36,3 +37,17 @@ func (e *MeshConfigEntry) GetCreateIndex() uint64 { func (e *MeshConfigEntry) GetModifyIndex() uint64 { return e.ModifyIndex } + +// MarshalJSON adds the Kind field so that the JSON can be decoded back into the +// correct type. +func (e *MeshConfigEntry) MarshalJSON() ([]byte, error) { + type Alias MeshConfigEntry + source := &struct { + Kind string + *Alias + }{ + Kind: MeshConfig, + Alias: (*Alias)(e), + } + return json.Marshal(source) +} diff --git a/command/config/write/config_write_test.go b/command/config/write/config_write_test.go index 8b3fe120b9..20f20af43e 100644 --- a/command/config/write/config_write_test.go +++ b/command/config/write/config_write_test.go @@ -1,6 +1,7 @@ package write import ( + "bytes" "io" "strings" "testing" @@ -113,6 +114,36 @@ func TestConfigWrite(t *testing.T) { require.NotEqual(t, 0, code) require.NotEmpty(t, ui.ErrorWriter.String()) }) + + t.Run("mesh config entry", func(t *testing.T) { + stdin := new(bytes.Buffer) + stdin.WriteString(` +kind = "mesh" +meta { + "foo" = "bar" + "gir" = "zim" +} +transparent_proxy { + catalog_destinations_only = true +} +`) + + ui := cli.NewMockUi() + c := New(ui) + c.testStdin = stdin + + code := c.Run([]string{"-http-addr=" + a.HTTPAddr(), "-"}) + require.Empty(t, ui.ErrorWriter.String()) + require.Contains(t, ui.OutputWriter.String(), + `Config entry written: mesh/mesh`) + require.Equal(t, 0, code) + + entry, _, err := client.ConfigEntries().Get(api.MeshConfig, api.MeshConfigMesh, nil) + require.NoError(t, err) + proxy, ok := entry.(*api.MeshConfigEntry) + require.True(t, ok) + require.Equal(t, map[string]string{"foo": "bar", "gir": "zim"}, proxy.Meta) + }) } // TestParseConfigEntry is the 'api' mirror image of From c8c85523e182537eafbc073146409885f471c869 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 4 May 2021 17:14:21 -0400 Subject: [PATCH 3/3] config-entries: add a test for the API client Also fixes a bug with listing kind=mesh config entries. ValidateConfigEntryKind was only being used by the List endpoint, and was yet another place where we have to enumerate all the kinds. This commit removes ValidateConfigEntryKind and uses MakeConfigEntry instead. This change removes the need to maintain two separate functions at the cost of creating an instance of the config entry which will be thrown away immediately. --- agent/consul/config_endpoint.go | 6 ++-- agent/structs/config_entry.go | 15 --------- api/config_entry_test.go | 58 +++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 17 deletions(-) diff --git a/agent/consul/config_endpoint.go b/agent/consul/config_endpoint.go index 9704b30b11..51a919675d 100644 --- a/agent/consul/config_endpoint.go +++ b/agent/consul/config_endpoint.go @@ -162,8 +162,10 @@ func (c *ConfigEntry) List(args *structs.ConfigEntryQuery, reply *structs.Indexe return err } - if args.Kind != "" && !structs.ValidateConfigEntryKind(args.Kind) { - return fmt.Errorf("invalid config entry kind: %s", args.Kind) + if args.Kind != "" { + if _, err := structs.MakeConfigEntry(args.Kind, ""); err != nil { + return fmt.Errorf("invalid config entry kind: %s", args.Kind) + } } return c.srv.blockingQuery( diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index e05e131efa..3f7145ee2b 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -535,21 +535,6 @@ func MakeConfigEntry(kind, name string) (ConfigEntry, error) { } } -func ValidateConfigEntryKind(kind string) bool { - switch kind { - case ServiceDefaults, ProxyDefaults: - return true - case ServiceRouter, ServiceSplitter, ServiceResolver: - return true - case IngressGateway, TerminatingGateway: - return true - case ServiceIntentions: - return true - default: - return false - } -} - // ConfigEntryQuery is used when requesting info about a config entry. type ConfigEntryQuery struct { Kind string diff --git a/api/config_entry_test.go b/api/config_entry_test.go index 74f92afe88..85ee5c4364 100644 --- a/api/config_entry_test.go +++ b/api/config_entry_test.go @@ -196,6 +196,64 @@ func TestAPI_ConfigEntries(t *testing.T) { _, _, err = config_entries.Get(ServiceDefaults, "foo", nil) require.Error(t, err) }) + + t.Run("Mesh", func(t *testing.T) { + mesh := &MeshConfigEntry{ + TransparentProxy: TransparentProxyMeshConfig{CatalogDestinationsOnly: true}, + Meta: map[string]string{ + "foo": "bar", + "gir": "zim", + }, + } + ce := c.ConfigEntries() + + runStep(t, "set and get", func(t *testing.T) { + _, wm, err := ce.Set(mesh, nil) + require.NoError(t, err) + require.NotNil(t, wm) + require.NotEqual(t, 0, wm.RequestTime) + + entry, qm, err := ce.Get(MeshConfig, MeshConfigMesh, nil) + require.NoError(t, err) + require.NotNil(t, qm) + require.NotEqual(t, 0, qm.RequestTime) + + result, ok := entry.(*MeshConfigEntry) + require.True(t, ok) + + // ignore indexes + result.CreateIndex = 0 + result.ModifyIndex = 0 + require.Equal(t, mesh, result) + }) + + runStep(t, "list", func(t *testing.T) { + entries, qm, err := ce.List(MeshConfig, nil) + require.NoError(t, err) + require.NotNil(t, qm) + require.NotEqual(t, 0, qm.RequestTime) + require.Len(t, entries, 1) + }) + + runStep(t, "delete", func(t *testing.T) { + wm, err := ce.Delete(MeshConfig, MeshConfigMesh, nil) + require.NoError(t, err) + require.NotNil(t, wm) + require.NotEqual(t, 0, wm.RequestTime) + + // verify deletion + _, _, err = ce.Get(MeshConfig, MeshConfigMesh, nil) + require.Error(t, err) + }) + }) + +} + +func runStep(t *testing.T, name string, fn func(t *testing.T)) { + t.Helper() + if !t.Run(name, fn) { + t.FailNow() + } } func TestDecodeConfigEntry(t *testing.T) {