From 1a039393f56292ab77d142a695fa17bfe6be094a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 27 May 2020 13:02:22 -0400 Subject: [PATCH 1/3] config: add HookTranslteKeys This hook replaces lib.TranslateKeys and has a number of advantages: 1. Primarily, aliases for fields are defined on the field itself, making the aliases much easier to maintain, and more obvious to the reader. 2. TranslateKeys translation rules are not aware of structure. It could very easily incorrectly translate a key on one struct that was intended to be a translation rule for a completely different struct, leading to very hard to debug errors. The hook removes the need for the unexpected "translation rule is an empty string to indicate stop traversal" special case. 3. TranslateKeys attempts to duplicate a bunch of tree traversal logic that already exists in mapstructure. Using mapstructure for traversal removes the need to traverse the entire structure multiple times, and makes the behaviour more obvious to the reader. This change is being made to enable a future change of replacing PatchSliceOfMaps. TranslateKeys sits in between PatchSliceOfMaps and mapstructure.Decode, so it must be converted to a hook first, before PatchSliceOfMaps can be replaced by a decode hook. --- lib/decode/decode.go | 93 +++++++++++++++++ lib/decode/decode_test.go | 207 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 300 insertions(+) create mode 100644 lib/decode/decode.go create mode 100644 lib/decode/decode_test.go diff --git a/lib/decode/decode.go b/lib/decode/decode.go new file mode 100644 index 0000000000..f6b81911dc --- /dev/null +++ b/lib/decode/decode.go @@ -0,0 +1,93 @@ +/* +Package decode provides tools for customizing the decoding of configuration, +into structures using mapstructure. +*/ +package decode + +import ( + "reflect" + "strings" +) + +// HookTranslateKeys is a mapstructure decode hook which translates keys in a +// map to their canonical value. +// +// Any struct field with a field tag of `alias` may be loaded from any of the +// values keyed by any of the aliases. A field may have one or more alias. +// Aliases must be lowercase, as keys are compared case-insensitive. +// +// Example alias tag: +// MyField []string `alias:"old_field_name,otherfieldname"` +// +// This hook should ONLY be used to maintain backwards compatibility with +// deprecated keys. For new structures use mapstructure struct tags to set the +// desired serialization key. +// +// IMPORTANT: This function assumes that mapstructure is being used with the +// default struct field tag of `mapstructure`. If mapstructure.DecoderConfig.TagName +// is set to a different value this function will need to be parameterized with +// that value to correctly find the canonical data key. +func HookTranslateKeys(_, to reflect.Type, data interface{}) (interface{}, error) { + // Return immediately if target is not a struct, as only structs can have + // field tags. If the target is a pointer to a struct, mapstructure will call + // the hook again with the struct. + if to.Kind() != reflect.Struct { + return data, nil + } + + // Avoid doing any work if data is not a map + source, ok := data.(map[string]interface{}) + if !ok { + return data, nil + } + + rules := translationsForType(to) + for k, v := range source { + lowerK := strings.ToLower(k) + canonKey, ok := rules[lowerK] + if !ok { + continue + } + delete(source, k) + + // if there is a value for the canonical key then keep it + if _, ok := source[canonKey]; ok { + continue + } + source[canonKey] = v + } + return source, nil +} + +// TODO: could be cached if it is too slow +func translationsForType(to reflect.Type) map[string]string { + translations := map[string]string{} + for i := 0; i < to.NumField(); i++ { + field := to.Field(i) + tag, ok := field.Tag.Lookup("alias") + if !ok { + continue + } + + canonKey := strings.ToLower(canonicalFieldKey(field)) + for _, alias := range strings.Split(tag, ",") { + translations[strings.ToLower(alias)] = canonKey + } + } + return translations +} + +func canonicalFieldKey(field reflect.StructField) string { + tag, ok := field.Tag.Lookup("mapstructure") + if !ok { + return field.Name + } + parts := strings.SplitN(tag, ",", 2) + switch { + case len(parts) < 1: + return field.Name + case parts[0] == "": + return field.Name + } + return parts[0] +} diff --git a/lib/decode/decode_test.go b/lib/decode/decode_test.go new file mode 100644 index 0000000000..e701445471 --- /dev/null +++ b/lib/decode/decode_test.go @@ -0,0 +1,207 @@ +package decode + +import ( + "reflect" + "testing" + + "github.com/mitchellh/mapstructure" + "github.com/stretchr/testify/require" +) + +func TestHookTranslateKeys(t *testing.T) { + var testcases = []struct { + name string + data interface{} + expected interface{} + }{ + { + name: "target of type struct, with struct receiver", + data: map[string]interface{}{ + "S": map[string]interface{}{ + "None": "no translation", + "OldOne": "value1", + "oldtwo": "value2", + }, + }, + expected: Config{ + S: TypeStruct{ + One: "value1", + Two: "value2", + None: "no translation", + }, + }, + }, + { + name: "target of type ptr, with struct receiver", + data: map[string]interface{}{ + "PS": map[string]interface{}{ + "None": "no translation", + "OldOne": "value1", + "oldtwo": "value2", + }, + }, + expected: Config{ + PS: &TypeStruct{ + One: "value1", + Two: "value2", + None: "no translation", + }, + }, + }, + { + name: "target of type ptr, with ptr receiver", + data: map[string]interface{}{ + "PTR": map[string]interface{}{ + "None": "no translation", + "old_THREE": "value3", + "oldfour": "value4", + }, + }, + expected: Config{ + PTR: &TypePtrToStruct{ + Three: "value3", + Four: "value4", + None: "no translation", + }, + }, + }, + { + name: "target of type ptr, with struct receiver", + data: map[string]interface{}{ + "PTRS": map[string]interface{}{ + "None": "no translation", + "old_THREE": "value3", + "old_four": "value4", + }, + }, + expected: Config{ + PTRS: TypePtrToStruct{ + Three: "value3", + Four: "value4", + None: "no translation", + }, + }, + }, + { + name: "target of type map", + data: map[string]interface{}{ + "Blob": map[string]interface{}{ + "one": 1, + "two": 2, + }, + }, + expected: Config{ + Blob: map[string]interface{}{ + "one": 1, + "two": 2, + }, + }, + }, + { + name: "value already exists for canonical key", + data: map[string]interface{}{ + "PS": map[string]interface{}{ + "OldOne": "value1", + "One": "original1", + "oldTWO": "value2", + "two": "original2", + }, + }, + expected: Config{ + PS: &TypeStruct{ + One: "original1", + Two: "original2", + }, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + cfg := Config{} + md := new(mapstructure.Metadata) + decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: HookTranslateKeys, + Metadata: md, + Result: &cfg, + }) + require.NoError(t, err) + + require.NoError(t, decoder.Decode(tc.data)) + require.Equal(t, cfg, tc.expected, "decode metadata: %#v", md) + }) + } +} + +type Config struct { + S TypeStruct + PS *TypeStruct + PTR *TypePtrToStruct + PTRS TypePtrToStruct + Blob map[string]interface{} +} + +type TypeStruct struct { + One string `alias:"oldone"` + Two string `alias:"oldtwo"` + None string +} + +type TypePtrToStruct struct { + Three string `alias:"old_three"` + Four string `alias:"old_four,oldfour"` + None string +} + +func TestHookTranslateKeys_TargetStructHasPointerReceiver(t *testing.T) { + target := &TypePtrToStruct{} + md := new(mapstructure.Metadata) + decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: HookTranslateKeys, + Metadata: md, + Result: target, + }) + require.NoError(t, err) + + data := map[string]interface{}{ + "None": "no translation", + "Old_Three": "value3", + "OldFour": "value4", + } + expected := &TypePtrToStruct{ + None: "no translation", + Three: "value3", + Four: "value4", + } + require.NoError(t, decoder.Decode(data)) + require.Equal(t, expected, target, "decode metadata: %#v", md) +} + +type translateExample struct { + FieldDefaultCanonical string `alias:"first"` + FieldWithMapstructureTag string `alias:"second" mapstructure:"field_with_mapstruct_tag"` + FieldWithMapstructureTagOmit string `mapstructure:"field_with_mapstruct_omit,omitempty" alias:"third"` + FieldWithEmptyTag string `mapstructure:"" alias:"forth"` +} + +func TestTranslationsForType(t *testing.T) { + to := reflect.TypeOf(translateExample{}) + actual := translationsForType(to) + expected := map[string]string{ + "first": "fielddefaultcanonical", + "second": "field_with_mapstruct_tag", + "third": "field_with_mapstruct_omit", + "forth": "fieldwithemptytag", + } + require.Equal(t, expected, actual) +} + +type nested struct { + O map[string]interface{} + Slice []Item + Item Item +} + +type Item struct { + Name string +} From 8ced4300c8d78dafe5fc27d079e0912b2dc6d739 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 27 May 2020 14:28:28 -0400 Subject: [PATCH 2/3] Add alias struct tags for new decode hook --- agent/config/config.go | 10 +++--- agent/discovery_chain_endpoint.go | 6 ++-- agent/structs/config_entry.go | 6 ++-- agent/structs/config_entry_discoverychain.go | 32 ++++++++++---------- agent/structs/config_entry_gateways.go | 6 ++-- agent/structs/connect_proxy_config.go | 6 ++-- agent/xds/config.go | 6 ++-- api/config_entry.go | 10 +++--- api/config_entry_discoverychain.go | 32 ++++++++++---------- api/config_entry_gateways.go | 6 ++-- 10 files changed, 60 insertions(+), 60 deletions(-) diff --git a/agent/config/config.go b/agent/config/config.go index 540a3f4159..f1d39b3dfc 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -430,10 +430,10 @@ type CheckDefinition struct { ID *string `json:"id,omitempty" hcl:"id" mapstructure:"id"` Name *string `json:"name,omitempty" hcl:"name" mapstructure:"name"` Notes *string `json:"notes,omitempty" hcl:"notes" mapstructure:"notes"` - ServiceID *string `json:"service_id,omitempty" hcl:"service_id" mapstructure:"service_id"` + ServiceID *string `json:"service_id,omitempty" hcl:"service_id" mapstructure:"service_id" alias:"serviceid"` Token *string `json:"token,omitempty" hcl:"token" mapstructure:"token"` Status *string `json:"status,omitempty" hcl:"status" mapstructure:"status"` - ScriptArgs []string `json:"args,omitempty" hcl:"args" mapstructure:"args"` + ScriptArgs []string `json:"args,omitempty" hcl:"args" mapstructure:"args" alias:"scriptargs"` HTTP *string `json:"http,omitempty" hcl:"http" mapstructure:"http"` Header map[string][]string `json:"header,omitempty" hcl:"header" mapstructure:"header"` Method *string `json:"method,omitempty" hcl:"method" mapstructure:"method"` @@ -441,18 +441,18 @@ type CheckDefinition struct { OutputMaxSize *int `json:"output_max_size,omitempty" hcl:"output_max_size" mapstructure:"output_max_size"` TCP *string `json:"tcp,omitempty" hcl:"tcp" mapstructure:"tcp"` Interval *string `json:"interval,omitempty" hcl:"interval" mapstructure:"interval"` - DockerContainerID *string `json:"docker_container_id,omitempty" hcl:"docker_container_id" mapstructure:"docker_container_id"` + DockerContainerID *string `json:"docker_container_id,omitempty" hcl:"docker_container_id" mapstructure:"docker_container_id" alias:"dockercontainerid"` Shell *string `json:"shell,omitempty" hcl:"shell" mapstructure:"shell"` GRPC *string `json:"grpc,omitempty" hcl:"grpc" mapstructure:"grpc"` GRPCUseTLS *bool `json:"grpc_use_tls,omitempty" hcl:"grpc_use_tls" mapstructure:"grpc_use_tls"` - TLSSkipVerify *bool `json:"tls_skip_verify,omitempty" hcl:"tls_skip_verify" mapstructure:"tls_skip_verify"` + TLSSkipVerify *bool `json:"tls_skip_verify,omitempty" hcl:"tls_skip_verify" mapstructure:"tls_skip_verify" alias:"tlsskipverify"` AliasNode *string `json:"alias_node,omitempty" hcl:"alias_node" mapstructure:"alias_node"` AliasService *string `json:"alias_service,omitempty" hcl:"alias_service" mapstructure:"alias_service"` Timeout *string `json:"timeout,omitempty" hcl:"timeout" mapstructure:"timeout"` TTL *string `json:"ttl,omitempty" hcl:"ttl" mapstructure:"ttl"` SuccessBeforePassing *int `json:"success_before_passing,omitempty" hcl:"success_before_passing" mapstructure:"success_before_passing"` FailuresBeforeCritical *int `json:"failures_before_critical,omitempty" hcl:"failures_before_critical" mapstructure:"failures_before_critical"` - DeregisterCriticalServiceAfter *string `json:"deregister_critical_service_after,omitempty" hcl:"deregister_critical_service_after" mapstructure:"deregister_critical_service_after"` + DeregisterCriticalServiceAfter *string `json:"deregister_critical_service_after,omitempty" hcl:"deregister_critical_service_after" mapstructure:"deregister_critical_service_after" alias:"deregistercriticalserviceafter"` EnterpriseMeta `hcl:",squash" mapstructure:",squash"` } diff --git a/agent/discovery_chain_endpoint.go b/agent/discovery_chain_endpoint.go index c6dddd64e8..6765fed579 100644 --- a/agent/discovery_chain_endpoint.go +++ b/agent/discovery_chain_endpoint.go @@ -90,9 +90,9 @@ func (s *HTTPServer) DiscoveryChainRead(resp http.ResponseWriter, req *http.Requ // discoveryChainReadRequest is the API variation of structs.DiscoveryChainRequest type discoveryChainReadRequest struct { - OverrideMeshGateway structs.MeshGatewayConfig - OverrideProtocol string - OverrideConnectTimeout time.Duration + OverrideMeshGateway structs.MeshGatewayConfig `alias:"override_mesh_gateway"` + OverrideProtocol string `alias:"override_protocol"` + OverrideConnectTimeout time.Duration `alias:"override_connect_timeout"` } // discoveryChainReadResponse is the API variation of structs.DiscoveryChainResponse diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index 8ec7d43271..f57517dbf2 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -53,10 +53,10 @@ type ServiceConfigEntry struct { Kind string Name string Protocol string - MeshGateway MeshGatewayConfig `json:",omitempty"` + MeshGateway MeshGatewayConfig `json:",omitempty" alias:"mesh_gateway"` Expose ExposeConfig `json:",omitempty"` - ExternalSNI string `json:",omitempty"` + ExternalSNI string `json:",omitempty" alias:"external_sni"` // TODO(banks): enable this once we have upstreams supported too. Enabling // sidecars actually makes no sense and adds complications when you don't @@ -134,7 +134,7 @@ type ProxyConfigEntry struct { Kind string Name string Config map[string]interface{} - MeshGateway MeshGatewayConfig `json:",omitempty"` + MeshGateway MeshGatewayConfig `json:",omitempty" alias:"mesh_gateway"` Expose ExposeConfig `json:",omitempty"` EnterpriseMeta `hcl:",squash" mapstructure:",squash"` diff --git a/agent/structs/config_entry_discoverychain.go b/agent/structs/config_entry_discoverychain.go index 82e233c35e..a0249a71d6 100644 --- a/agent/structs/config_entry_discoverychain.go +++ b/agent/structs/config_entry_discoverychain.go @@ -260,12 +260,12 @@ func (m *ServiceRouteMatch) IsEmpty() bool { // ServiceRouteHTTPMatch is a set of http-specific match criteria. type ServiceRouteHTTPMatch struct { - PathExact string `json:",omitempty"` - PathPrefix string `json:",omitempty"` - PathRegex string `json:",omitempty"` + PathExact string `json:",omitempty" alias:"path_exact"` + PathPrefix string `json:",omitempty" alias:"path_prefix"` + PathRegex string `json:",omitempty" alias:"path_regex"` Header []ServiceRouteHTTPMatchHeader `json:",omitempty"` - QueryParam []ServiceRouteHTTPMatchQueryParam `json:",omitempty"` + QueryParam []ServiceRouteHTTPMatchQueryParam `json:",omitempty" alias:"query_param"` Methods []string `json:",omitempty"` } @@ -308,7 +308,7 @@ type ServiceRouteDestination struct { // // If this field is specified then this route is ineligible for further // splitting. - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` // Namespace is the namespace to resolve the service from instead of the // current namespace. If empty the current namespace is assumed. @@ -320,24 +320,24 @@ type ServiceRouteDestination struct { // PrefixRewrite allows for the proxied request to have its matching path // prefix modified before being sent to the destination. Described more // below in the envoy implementation section. - PrefixRewrite string `json:",omitempty"` + PrefixRewrite string `json:",omitempty" alias:"prefix_rewrite"` // RequestTimeout is the total amount of time permitted for the entire // downstream request (and retries) to be processed. - RequestTimeout time.Duration `json:",omitempty"` + RequestTimeout time.Duration `json:",omitempty" alias:"request_timeout"` // NumRetries is the number of times to retry the request when a retryable // result occurs. This seems fairly proxy agnostic. - NumRetries uint32 `json:",omitempty"` + NumRetries uint32 `json:",omitempty" alias:"num_retries"` // RetryOnConnectFailure allows for connection failure errors to trigger a // retry. This should be expressible in other proxies as it's just a layer // 4 failure bubbling up to layer 7. - RetryOnConnectFailure bool `json:",omitempty"` + RetryOnConnectFailure bool `json:",omitempty" alias:"retry_on_connect_failure"` // RetryOnStatusCodes is a flat list of http response status codes that are // eligible for retry. This again should be feasible in any sane proxy. - RetryOnStatusCodes []uint32 `json:",omitempty"` + RetryOnStatusCodes []uint32 `json:",omitempty" alias:"retry_on_status_codes"` } func (e *ServiceRouteDestination) MarshalJSON() ([]byte, error) { @@ -576,7 +576,7 @@ type ServiceSplit struct { // // If this field is specified then this route is ineligible for further // splitting. - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` // Namespace is the namespace to resolve the service from instead of the // current namespace. If empty the current namespace is assumed (optional). @@ -604,7 +604,7 @@ type ServiceResolverConfigEntry struct { // DefaultSubset is the subset to use when no explicit subset is // requested. If empty the unnamed subset is used. - DefaultSubset string `json:",omitempty"` + DefaultSubset string `json:",omitempty" alias:"default_subset"` // Subsets is a map of subset name to subset definition for all // usable named subsets of this service. The map key is the name @@ -637,7 +637,7 @@ type ServiceResolverConfigEntry struct { // ConnectTimeout is the timeout for establishing new network connections // to this service. - ConnectTimeout time.Duration `json:",omitempty"` + ConnectTimeout time.Duration `json:",omitempty" alias:"connect_timeout"` EnterpriseMeta `hcl:",squash" mapstructure:",squash"` RaftIndex @@ -884,7 +884,7 @@ type ServiceResolverSubset struct { // to true, only instances with checks in the passing state will be // returned. (behaves identically to the similarly named field on prepared // queries). - OnlyPassing bool `json:",omitempty"` + OnlyPassing bool `json:",omitempty" alias:"only_passing"` } type ServiceResolverRedirect struct { @@ -898,7 +898,7 @@ type ServiceResolverRedirect struct { // // If this is specified at least one of Service, Datacenter, or Namespace // should be configured. - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` // Namespace is the namespace to resolve the service from instead of the // current one (optional). @@ -926,7 +926,7 @@ type ServiceResolverFailover struct { // requested service is used (optional). // // This is a DESTINATION during failover. - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` // Namespace is the namespace to resolve the requested service from to form // the failover group of instances. If empty the current namespace is used diff --git a/agent/structs/config_entry_gateways.go b/agent/structs/config_entry_gateways.go index a49a1f0bb8..67031c2112 100644 --- a/agent/structs/config_entry_gateways.go +++ b/agent/structs/config_entry_gateways.go @@ -248,15 +248,15 @@ type LinkedService struct { // CAFile is the optional path to a CA certificate to use for TLS connections // from the gateway to the linked service - CAFile string `json:",omitempty"` + CAFile string `json:",omitempty" alias:"ca_file"` // CertFile is the optional path to a client certificate to use for TLS connections // from the gateway to the linked service - CertFile string `json:",omitempty"` + CertFile string `json:",omitempty" alias:"cert_file"` // KeyFile is the optional path to a private key to use for TLS connections // from the gateway to the linked service - KeyFile string `json:",omitempty"` + KeyFile string `json:",omitempty" alias:"key_file"` // SNI is the optional name to specify during the TLS handshake with a linked service SNI string `json:",omitempty"` diff --git a/agent/structs/connect_proxy_config.go b/agent/structs/connect_proxy_config.go index 780b5fcbb8..bad538781c 100644 --- a/agent/structs/connect_proxy_config.go +++ b/agent/structs/connect_proxy_config.go @@ -114,7 +114,7 @@ type ConnectProxyConfig struct { Upstreams Upstreams `json:",omitempty"` // MeshGateway defines the mesh gateway configuration for this upstream - MeshGateway MeshGatewayConfig `json:",omitempty"` + MeshGateway MeshGatewayConfig `json:",omitempty" alias:"mesh_gateway"` // Expose defines whether checks or paths are exposed through the proxy Expose ExposeConfig `json:",omitempty"` @@ -410,13 +410,13 @@ type ExposeConfig struct { type ExposePath struct { // ListenerPort defines the port of the proxy's listener for exposed paths. - ListenerPort int `json:",omitempty"` + ListenerPort int `json:",omitempty" alias:"listener_port"` // ExposePath is the path to expose through the proxy, ie. "/metrics." Path string `json:",omitempty"` // LocalPathPort is the port that the service is listening on for the given path. - LocalPathPort int `json:",omitempty"` + LocalPathPort int `json:",omitempty" alias:"local_path_port"` // Protocol describes the upstream's service protocol. // Valid values are "http" and "http2", defaults to "http" diff --git a/agent/xds/config.go b/agent/xds/config.go index 756ca51f44..2e2c236c9a 100644 --- a/agent/xds/config.go +++ b/agent/xds/config.go @@ -79,14 +79,14 @@ type GatewayConfig struct { // for those addresses or where an external entity maps that IP to the Envoy // (like AWS EC2 mapping a public IP to the private interface) then this // cannot be used. See the BindAddresses config instead - BindTaggedAddresses bool `mapstructure:"envoy_gateway_bind_tagged_addresses"` + BindTaggedAddresses bool `mapstructure:"envoy_gateway_bind_tagged_addresses" alias:"envoy_mesh_gateway_bind_tagged_addresses"` // BindAddresses additional bind addresses to configure listeners for - BindAddresses map[string]structs.ServiceAddress `mapstructure:"envoy_gateway_bind_addresses"` + BindAddresses map[string]structs.ServiceAddress `mapstructure:"envoy_gateway_bind_addresses" alias:"envoy_mesh_gateway_bind_addresses"` // NoDefaultBind indicates that we should not bind to the default address of the // gateway service - NoDefaultBind bool `mapstructure:"envoy_gateway_no_default_bind"` + NoDefaultBind bool `mapstructure:"envoy_gateway_no_default_bind" alias:"envoy_mesh_gateway_no_default_bind"` // ConnectTimeoutMs is the number of milliseconds to timeout making a new // connection to this upstream. Defaults to 5000 (5 seconds) if not set. diff --git a/api/config_entry.go b/api/config_entry.go index 6279a7c5a2..dc31d6110f 100644 --- a/api/config_entry.go +++ b/api/config_entry.go @@ -71,13 +71,13 @@ type ExposeConfig struct { type ExposePath struct { // ListenerPort defines the port of the proxy's listener for exposed paths. - ListenerPort int `json:",omitempty"` + ListenerPort int `json:",omitempty" alias:"listener_port"` // Path is the path to expose through the proxy, ie. "/metrics." Path string `json:",omitempty"` // LocalPathPort is the port that the service is listening on for the given path. - LocalPathPort int `json:",omitempty"` + LocalPathPort int `json:",omitempty" alias:"local_path_port"` // Protocol describes the upstream's service protocol. // Valid values are "http" and "http2", defaults to "http" @@ -92,9 +92,9 @@ type ServiceConfigEntry struct { Name string Namespace string `json:",omitempty"` Protocol string `json:",omitempty"` - MeshGateway MeshGatewayConfig `json:",omitempty"` + MeshGateway MeshGatewayConfig `json:",omitempty" alias:"mesh_gateway"` Expose ExposeConfig `json:",omitempty"` - ExternalSNI string `json:",omitempty"` + ExternalSNI string `json:",omitempty" alias:"external_sni"` CreateIndex uint64 ModifyIndex uint64 } @@ -120,7 +120,7 @@ type ProxyConfigEntry struct { Name string Namespace string `json:",omitempty"` Config map[string]interface{} `json:",omitempty"` - MeshGateway MeshGatewayConfig `json:",omitempty"` + MeshGateway MeshGatewayConfig `json:",omitempty" alias:"mesh_gateway"` Expose ExposeConfig `json:",omitempty"` CreateIndex uint64 ModifyIndex uint64 diff --git a/api/config_entry_discoverychain.go b/api/config_entry_discoverychain.go index 885b78dc91..f3994f0dd9 100644 --- a/api/config_entry_discoverychain.go +++ b/api/config_entry_discoverychain.go @@ -31,12 +31,12 @@ type ServiceRouteMatch struct { } type ServiceRouteHTTPMatch struct { - PathExact string `json:",omitempty"` - PathPrefix string `json:",omitempty"` - PathRegex string `json:",omitempty"` + PathExact string `json:",omitempty" alias:"path_exact"` + PathPrefix string `json:",omitempty" alias:"path_prefix"` + PathRegex string `json:",omitempty" alias:"path_regex"` Header []ServiceRouteHTTPMatchHeader `json:",omitempty"` - QueryParam []ServiceRouteHTTPMatchQueryParam `json:",omitempty"` + QueryParam []ServiceRouteHTTPMatchQueryParam `json:",omitempty" alias:"query_param"` Methods []string `json:",omitempty"` } @@ -59,13 +59,13 @@ type ServiceRouteHTTPMatchQueryParam struct { type ServiceRouteDestination struct { Service string `json:",omitempty"` - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` Namespace string `json:",omitempty"` - PrefixRewrite string `json:",omitempty"` - RequestTimeout time.Duration `json:",omitempty"` - NumRetries uint32 `json:",omitempty"` - RetryOnConnectFailure bool `json:",omitempty"` - RetryOnStatusCodes []uint32 `json:",omitempty"` + PrefixRewrite string `json:",omitempty" alias:"prefix_rewrite"` + RequestTimeout time.Duration `json:",omitempty" alias:"request_timeout"` + NumRetries uint32 `json:",omitempty" alias:"num_retries"` + RetryOnConnectFailure bool `json:",omitempty" alias:"retry_on_connect_failure"` + RetryOnStatusCodes []uint32 `json:",omitempty" alias:"retry_on_status_codes"` } func (e *ServiceRouteDestination) MarshalJSON() ([]byte, error) { @@ -123,7 +123,7 @@ func (e *ServiceSplitterConfigEntry) GetModifyIndex() uint64 { return e.ModifyIn type ServiceSplit struct { Weight float32 Service string `json:",omitempty"` - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` Namespace string `json:",omitempty"` } @@ -132,11 +132,11 @@ type ServiceResolverConfigEntry struct { Name string Namespace string `json:",omitempty"` - DefaultSubset string `json:",omitempty"` + DefaultSubset string `json:",omitempty" alias:"default_subset"` Subsets map[string]ServiceResolverSubset `json:",omitempty"` Redirect *ServiceResolverRedirect `json:",omitempty"` Failover map[string]ServiceResolverFailover `json:",omitempty"` - ConnectTimeout time.Duration `json:",omitempty"` + ConnectTimeout time.Duration `json:",omitempty" alias:"connect_timeout"` CreateIndex uint64 ModifyIndex uint64 @@ -185,19 +185,19 @@ func (e *ServiceResolverConfigEntry) GetModifyIndex() uint64 { return e.ModifyIn type ServiceResolverSubset struct { Filter string `json:",omitempty"` - OnlyPassing bool `json:",omitempty"` + OnlyPassing bool `json:",omitempty" alias:"only_passing"` } type ServiceResolverRedirect struct { Service string `json:",omitempty"` - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` Namespace string `json:",omitempty"` Datacenter string `json:",omitempty"` } type ServiceResolverFailover struct { Service string `json:",omitempty"` - ServiceSubset string `json:",omitempty"` + ServiceSubset string `json:",omitempty" alias:"service_subset"` Namespace string `json:",omitempty"` Datacenters []string `json:",omitempty"` } diff --git a/api/config_entry_gateways.go b/api/config_entry_gateways.go index d77164b5de..13a5ec7072 100644 --- a/api/config_entry_gateways.go +++ b/api/config_entry_gateways.go @@ -139,15 +139,15 @@ type LinkedService struct { // CAFile is the optional path to a CA certificate to use for TLS connections // from the gateway to the linked service - CAFile string `json:",omitempty"` + CAFile string `json:",omitempty" alias:"ca_file"` // CertFile is the optional path to a client certificate to use for TLS connections // from the gateway to the linked service - CertFile string `json:",omitempty"` + CertFile string `json:",omitempty" alias:"cert_file"` // KeyFile is the optional path to a private key to use for TLS connections // from the gateway to the linked service - KeyFile string `json:",omitempty"` + KeyFile string `json:",omitempty" alias:"key_file"` // SNI is the optional name to specify during the TLS handshake with a linked service SNI string `json:",omitempty"` From 6a2d7d77c074894038db8c6d9a5c80e78c6f2c84 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 27 May 2020 14:42:01 -0400 Subject: [PATCH 3/3] config: use the new HookTranslateKeys instead of lib.TranslateKeys With the exception of CA provider config, which will be migrated at some later time. --- agent/config/config.go | 28 ++------- agent/discovery_chain_endpoint.go | 12 ++-- agent/structs/config_entry.go | 88 +++++++++------------------- agent/xds/config.go | 21 ++++--- command/config/write/config_write.go | 13 ++-- lib/translate.go | 2 + 6 files changed, 58 insertions(+), 106 deletions(-) diff --git a/agent/config/config.go b/agent/config/config.go index f1d39b3dfc..df5bca3c72 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/decode" "github.com/hashicorp/go-multierror" "github.com/hashicorp/hcl" "github.com/mitchellh/mapstructure" @@ -108,32 +109,11 @@ func Parse(data string, format string) (c Config, keys []string, err error) { "config_entries.bootstrap", // completely ignore this tree (fixed elsewhere) }) - // There is a difference of representation of some fields depending on - // where they are used. The HTTP API uses CamelCase whereas the config - // files use snake_case and between the two there is no automatic mapping. - // While the JSON and HCL parsers match keys without case (both `id` and - // `ID` are mapped to an ID field) the same thing does not happen between - // CamelCase and snake_case. Since changing either format would break - // existing setups we have to support both and slowly transition to one of - // the formats. Also, there is at least one case where we use the "wrong" - // key and want to map that to the new key to support deprecation - - // see [GH-3179]. TranslateKeys maps potentially CamelCased values to the - // snake_case that is used in the config file parser. If both the CamelCase - // and snake_case values are set the snake_case value is used and the other - // value is discarded. - lib.TranslateKeys(m, map[string]string{ - "deregistercriticalserviceafter": "deregister_critical_service_after", - "dockercontainerid": "docker_container_id", - "scriptargs": "args", - "serviceid": "service_id", - "tlsskipverify": "tls_skip_verify", - "config_entries.bootstrap": "", - }) - var md mapstructure.Metadata d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ - Metadata: &md, - Result: &c, + DecodeHook: decode.HookTranslateKeys, + Metadata: &md, + Result: &c, }) if err != nil { return Config{}, nil, err diff --git a/agent/discovery_chain_endpoint.go b/agent/discovery_chain_endpoint.go index 6765fed579..01bdf81eb9 100644 --- a/agent/discovery_chain_endpoint.go +++ b/agent/discovery_chain_endpoint.go @@ -9,6 +9,7 @@ import ( cachetype "github.com/hashicorp/consul/agent/cache-types" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/decode" "github.com/mitchellh/mapstructure" ) @@ -105,15 +106,12 @@ func decodeDiscoveryChainReadRequest(raw map[string]interface{}) (*discoveryChai // to do this part first. raw = lib.PatchSliceOfMaps(raw, nil, nil) - lib.TranslateKeys(raw, map[string]string{ - "override_mesh_gateway": "overridemeshgateway", - "override_protocol": "overrideprotocol", - "override_connect_timeout": "overrideconnecttimeout", - }) - var apiReq discoveryChainReadRequest decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: mapstructure.StringToTimeDurationHookFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookTranslateKeys, + mapstructure.StringToTimeDurationHookFunc(), + ), Result: &apiReq, WeaklyTypedInput: true, } diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index f57517dbf2..0cacafef38 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/decode" "github.com/hashicorp/go-msgpack/codec" "github.com/hashicorp/go-multierror" "github.com/mitchellh/hashstructure" @@ -283,7 +284,7 @@ func DecodeConfigEntry(raw map[string]interface{}) (ConfigEntry, error) { return nil, fmt.Errorf("Kind value in payload is not a string") } - skipWhenPatching, translateKeysDict, err := ConfigEntryDecodeRulesForKind(entry.GetKind()) + skipWhenPatching, err := ConfigEntryDecodeRulesForKind(entry.GetKind()) if err != nil { return nil, err } @@ -292,11 +293,12 @@ func DecodeConfigEntry(raw map[string]interface{}) (ConfigEntry, error) { // to do this part first. raw = lib.PatchSliceOfMaps(raw, skipWhenPatching, nil) - lib.TranslateKeys(raw, translateKeysDict) - var md mapstructure.Metadata decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: mapstructure.StringToTimeDurationHookFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookTranslateKeys, + mapstructure.StringToTimeDurationHookFunc(), + ), Metadata: &md, Result: &entry, WeaklyTypedInput: true, @@ -327,80 +329,48 @@ func DecodeConfigEntry(raw map[string]interface{}) (ConfigEntry, error) { // ConfigEntryDecodeRulesForKind returns rules for 'fixing' config entry key // formats by kind. This is shared between the 'structs' and 'api' variations // of config entries. -func ConfigEntryDecodeRulesForKind(kind string) (skipWhenPatching []string, translateKeysDict map[string]string, err error) { +func ConfigEntryDecodeRulesForKind(kind string) (skipWhenPatching []string, err error) { switch kind { case ProxyDefaults: return []string{ - "expose.paths", - "Expose.Paths", - }, map[string]string{ - "local_path_port": "localpathport", - "listener_port": "listenerport", - "mesh_gateway": "meshgateway", - "config": "", - }, nil + "expose.paths", + "Expose.Paths", + }, nil case ServiceDefaults: return []string{ - "expose.paths", - "Expose.Paths", - }, map[string]string{ - "local_path_port": "localpathport", - "listener_port": "listenerport", - "mesh_gateway": "meshgateway", - "external_sni": "externalsni", - }, nil + "expose.paths", + "Expose.Paths", + }, nil case ServiceRouter: return []string{ - "routes", - "Routes", - "routes.match.http.header", - "Routes.Match.HTTP.Header", - "routes.match.http.query_param", - "Routes.Match.HTTP.QueryParam", - }, map[string]string{ - "num_retries": "numretries", - "path_exact": "pathexact", - "path_prefix": "pathprefix", - "path_regex": "pathregex", - "prefix_rewrite": "prefixrewrite", - "query_param": "queryparam", - "request_timeout": "requesttimeout", - "retry_on_connect_failure": "retryonconnectfailure", - "retry_on_status_codes": "retryonstatuscodes", - "service_subset": "servicesubset", - }, nil + "routes", + "Routes", + "routes.match.http.header", + "Routes.Match.HTTP.Header", + "routes.match.http.query_param", + "Routes.Match.HTTP.QueryParam", + }, nil case ServiceSplitter: return []string{ - "splits", - "Splits", - }, map[string]string{ - "service_subset": "servicesubset", - }, nil - case ServiceResolver: - return nil, map[string]string{ - "connect_timeout": "connecttimeout", - "default_subset": "defaultsubset", - "only_passing": "onlypassing", - "service_subset": "servicesubset", + "splits", + "Splits", }, nil + case ServiceResolver: + return nil, nil case IngressGateway: return []string{ "listeners", "Listeners", "listeners.services", "Listeners.Services", - }, nil, nil + }, nil case TerminatingGateway: return []string{ - "services", - "Services", - }, map[string]string{ - "ca_file": "cafile", - "cert_file": "certfile", - "key_file": "keyfile", - }, nil + "services", + "Services", + }, nil default: - return nil, nil, fmt.Errorf("kind %q should be explicitly handled here", kind) + return nil, fmt.Errorf("kind %q should be explicitly handled here", kind) } } diff --git a/agent/xds/config.go b/agent/xds/config.go index 2e2c236c9a..d2a54e443a 100644 --- a/agent/xds/config.go +++ b/agent/xds/config.go @@ -7,7 +7,7 @@ import ( envoycluster "github.com/envoyproxy/go-control-plane/envoy/api/v2/cluster" "github.com/gogo/protobuf/types" "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/decode" "github.com/mitchellh/mapstructure" ) @@ -97,15 +97,18 @@ type GatewayConfig struct { // error occurs during parsing, it is returned along with the default config. This // allows the caller to choose whether and how to report the error func ParseGatewayConfig(m map[string]interface{}) (GatewayConfig, error) { - // Fixup for deprecated mesh gateway names - lib.TranslateKeys(m, map[string]string{ - "envoy_mesh_gateway_bind_tagged_addresses": "envoy_gateway_bind_tagged_addresses", - "envoy_mesh_gateway_bind_addresses": "envoy_gateway_bind_addresses", - "envoy_mesh_gateway_no_default_bind": "envoy_gateway_no_default_bind", - }) - var cfg GatewayConfig - err := mapstructure.WeakDecode(m, &cfg) + d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: decode.HookTranslateKeys, + Result: &cfg, + WeaklyTypedInput: true, + }) + if err != nil { + return cfg, err + } + if err := d.Decode(m); err != nil { + return cfg, err + } if cfg.ConnectTimeoutMs < 1 { cfg.ConnectTimeoutMs = 5000 diff --git a/command/config/write/config_write.go b/command/config/write/config_write.go index 3cc3de3f63..81deeb89e3 100644 --- a/command/config/write/config_write.go +++ b/command/config/write/config_write.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/consul/command/flags" "github.com/hashicorp/consul/command/helpers" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/decode" "github.com/hashicorp/go-multierror" "github.com/mitchellh/cli" "github.com/mitchellh/mapstructure" @@ -132,7 +133,7 @@ func newDecodeConfigEntry(raw map[string]interface{}) (api.ConfigEntry, error) { return nil, fmt.Errorf("Kind value in payload is not a string") } - skipWhenPatching, translateKeysDict, err := structs.ConfigEntryDecodeRulesForKind(entry.GetKind()) + skipWhenPatching, err := structs.ConfigEntryDecodeRulesForKind(entry.GetKind()) if err != nil { return nil, err } @@ -141,14 +142,12 @@ func newDecodeConfigEntry(raw map[string]interface{}) (api.ConfigEntry, error) { // to do this part first. raw = lib.PatchSliceOfMaps(raw, skipWhenPatching, nil) - // CamelCase is the canonical form for these, since this translation - // happens in the `consul config write` command and the JSON form is sent - // off to the server. - lib.TranslateKeys(raw, translateKeysDict) - var md mapstructure.Metadata decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: mapstructure.StringToTimeDurationHookFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookTranslateKeys, + mapstructure.StringToTimeDurationHookFunc(), + ), Metadata: &md, Result: &entry, WeaklyTypedInput: true, diff --git a/lib/translate.go b/lib/translate.go index 6bb934cc6d..815dd73a43 100644 --- a/lib/translate.go +++ b/lib/translate.go @@ -34,6 +34,8 @@ import ( // // item's config field // "widgets.config": "", // }) +// +// Deprecated: Use lib/decode.HookTranslateKeys instead. func TranslateKeys(v map[string]interface{}, dict map[string]string) { // Convert all dict keys for exclusions to lower. so we can match against them // unambiguously with a single lookup.