From 2dd55510033526c02bd1c96ddf37ec57741c5622 Mon Sep 17 00:00:00 2001 From: Michael Zalimeni Date: Tue, 6 Jun 2023 15:40:37 -0400 Subject: [PATCH] Fix Property Override Services parsing (#17584) Ensure that the embedded api struct is properly parsed when deserializing config containing a set ResourceFilter.Services field. Also enhance existing integration test to guard against bugs and exercise this field. --- .../property-override/property_override.go | 19 ++++++++- .../property_override_test.go | 40 ++++++++++++++++++- .../envoy/case-property-override/capture.sh | 1 + .../case-property-override/service_s1.hcl | 4 ++ .../case-property-override/service_s3.hcl | 8 ++++ .../envoy/case-property-override/setup.sh | 23 ++++++++++- .../envoy/case-property-override/vars.sh | 2 +- .../envoy/case-property-override/verify.bats | 7 ++++ 8 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 test/integration/connect/envoy/case-property-override/service_s3.hcl diff --git a/agent/envoyextensions/builtin/property-override/property_override.go b/agent/envoyextensions/builtin/property-override/property_override.go index 6bdf9e7a39..c018d334a5 100644 --- a/agent/envoyextensions/builtin/property-override/property_override.go +++ b/agent/envoyextensions/builtin/property-override/property_override.go @@ -7,6 +7,7 @@ import ( envoy_endpoint_v3 "github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3" envoy_listener_v3 "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3" envoy_route_v3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" + "github.com/hashicorp/consul/lib/decode" "github.com/hashicorp/go-multierror" "github.com/mitchellh/mapstructure" "google.golang.org/protobuf/proto" @@ -73,7 +74,7 @@ func matchesResourceFilter[K proto.Message](rf ResourceFilter, resourceType Reso } type ServiceName struct { - api.CompoundServiceName + api.CompoundServiceName `mapstructure:",squash"` } // ResourceType is the type of Envoy resource being patched. @@ -275,7 +276,21 @@ func Constructor(ext api.EnvoyExtension) (extensioncommon.EnvoyExtender, error) if name := ext.Name; name != api.BuiltinPropertyOverrideExtension { return nil, fmt.Errorf("expected extension name %q but got %q", api.BuiltinPropertyOverrideExtension, name) } - if err := mapstructure.WeakDecode(ext.Arguments, &p); err != nil { + // This avoids issues with decoding nested slices, which are error-prone + // due to slice<->map coercion by mapstructure. See HookWeakDecodeFromSlice + // and WeaklyTypedInput docs for more details. + d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookWeakDecodeFromSlice, + decode.HookTranslateKeys, + ), + WeaklyTypedInput: true, + Result: &p, + }) + if err != nil { + return nil, fmt.Errorf("error configuring decoder: %v", err) + } + if err := d.Decode(ext.Arguments); err != nil { return nil, fmt.Errorf("error decoding extension arguments: %v", err) } if err := p.validate(); err != nil { diff --git a/agent/envoyextensions/builtin/property-override/property_override_test.go b/agent/envoyextensions/builtin/property-override/property_override_test.go index 7faa3795c2..68a895edb8 100644 --- a/agent/envoyextensions/builtin/property-override/property_override_test.go +++ b/agent/envoyextensions/builtin/property-override/property_override_test.go @@ -220,8 +220,8 @@ func TestConstructor(t *testing.T) { arguments: makeArguments(map[string]any{"Patches": []map[string]any{ makePatch(map[string]any{ "ResourceFilter": makeResourceFilter(map[string]any{ - "Services": []ServiceName{ - {CompoundServiceName: api.CompoundServiceName{}}, + "Services": []map[string]any{ + {}, }, }), }), @@ -240,6 +240,42 @@ func TestConstructor(t *testing.T) { expected: validTestCase(OpAdd, extensioncommon.TrafficDirectionOutbound, ResourceTypeRoute).expected, ok: true, }, + // Ensure that embedded api struct used for Services is parsed correctly. + // See also above comment on decode.HookWeakDecodeFromSlice. + "single value Services decoded as map construction succeeds": { + arguments: makeArguments(map[string]any{"Patches": []map[string]any{ + makePatch(map[string]any{ + "ResourceFilter": makeResourceFilter(map[string]any{ + "Services": []map[string]any{ + {"Name": "foo"}, + }, + }), + }), + }}), + expected: propertyOverride{ + Patches: []Patch{ + { + ResourceFilter: ResourceFilter{ + ResourceType: ResourceTypeRoute, + TrafficDirection: extensioncommon.TrafficDirectionOutbound, + Services: []*ServiceName{ + {CompoundServiceName: api.CompoundServiceName{ + Name: "foo", + Namespace: "default", + Partition: "default", + }}, + }, + }, + Op: OpAdd, + Path: "/name", + Value: "foo", + }, + }, + Debug: true, + ProxyType: api.ServiceKindConnectProxy, + }, + ok: true, + }, "invalid ProxyType": { arguments: makeArguments(map[string]any{ "Patches": []map[string]any{ diff --git a/test/integration/connect/envoy/case-property-override/capture.sh b/test/integration/connect/envoy/case-property-override/capture.sh index 5268305f8f..88f1dd6465 100644 --- a/test/integration/connect/envoy/case-property-override/capture.sh +++ b/test/integration/connect/envoy/case-property-override/capture.sh @@ -5,3 +5,4 @@ snapshot_envoy_admin localhost:19000 s1 primary || true snapshot_envoy_admin localhost:19001 s2 primary || true +snapshot_envoy_admin localhost:19002 s3 primary || true diff --git a/test/integration/connect/envoy/case-property-override/service_s1.hcl b/test/integration/connect/envoy/case-property-override/service_s1.hcl index 6eaca12985..6bacecf916 100644 --- a/test/integration/connect/envoy/case-property-override/service_s1.hcl +++ b/test/integration/connect/envoy/case-property-override/service_s1.hcl @@ -11,6 +11,10 @@ services { { destination_name = "s2" local_bind_port = 5000 + }, + { + destination_name = "s3" + local_bind_port = 5001 } ] } diff --git a/test/integration/connect/envoy/case-property-override/service_s3.hcl b/test/integration/connect/envoy/case-property-override/service_s3.hcl new file mode 100644 index 0000000000..d616906e4c --- /dev/null +++ b/test/integration/connect/envoy/case-property-override/service_s3.hcl @@ -0,0 +1,8 @@ +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: MPL-2.0 + +services { + name = "s3" + port = 8181 + connect { sidecar_service {} } +} diff --git a/test/integration/connect/envoy/case-property-override/setup.sh b/test/integration/connect/envoy/case-property-override/setup.sh index 0bc569a706..1bf2021c0c 100644 --- a/test/integration/connect/envoy/case-property-override/setup.sh +++ b/test/integration/connect/envoy/case-property-override/setup.sh @@ -28,6 +28,12 @@ EnvoyExtensions = [ ] ' +upsert_config_entry primary ' +Kind = "service-defaults" +Name = "s3" +Protocol = "http" +' + upsert_config_entry primary ' Kind = "service-defaults" Name = "s1" @@ -37,7 +43,8 @@ EnvoyExtensions = [ Name = "builtin/property-override" Arguments = { ProxyType = "connect-proxy" - Patches = [{ + Patches = [ + { ResourceFilter = { ResourceType = "cluster" TrafficDirection = "outbound" @@ -45,7 +52,19 @@ EnvoyExtensions = [ Op = "add" Path = "/upstream_connection_options/tcp_keepalive/keepalive_probes" Value = 1234 - }] + }, + { + ResourceFilter = { + ResourceType = "cluster" + TrafficDirection = "outbound" + Services = [{ + Name = "s2" + }] + } + Op = "remove" + Path = "/outlier_detection" + } + ] } } ] diff --git a/test/integration/connect/envoy/case-property-override/vars.sh b/test/integration/connect/envoy/case-property-override/vars.sh index 091a358e13..358866872a 100644 --- a/test/integration/connect/envoy/case-property-override/vars.sh +++ b/test/integration/connect/envoy/case-property-override/vars.sh @@ -3,4 +3,4 @@ # SPDX-License-Identifier: MPL-2.0 -export REQUIRED_SERVICES="s1 s1-sidecar-proxy s2 s2-sidecar-proxy" +export REQUIRED_SERVICES="s1 s1-sidecar-proxy s2 s2-sidecar-proxy s3 s3-sidecar-proxy" diff --git a/test/integration/connect/envoy/case-property-override/verify.bats b/test/integration/connect/envoy/case-property-override/verify.bats index 8b8997f82a..4453409eed 100644 --- a/test/integration/connect/envoy/case-property-override/verify.bats +++ b/test/integration/connect/envoy/case-property-override/verify.bats @@ -19,6 +19,13 @@ load helpers [ "$status" == 0 ] [ "$(echo "$output" | jq -r '.upstream_connection_options.tcp_keepalive.keepalive_probes')" == "1234" ] + [ "$(echo "$output" | jq -r '.outlier_detection')" == "null" ] + + run get_envoy_cluster_config localhost:19000 s3 + [ "$status" == 0 ] + + [ "$(echo "$output" | jq -r '.upstream_connection_options.tcp_keepalive.keepalive_probes')" == "1234" ] + [ "$(echo "$output" | jq -r '.outlier_detection')" == "{}" ] } @test "s2 proxy is configured with the expected envoy patches" {