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.

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 {

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{

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

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
           }
         ]
       }

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 {} }
+}

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"
+        }
+      ]
     }
   }
 ]

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"

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" {