Backport of Property Override validation improvements into release/1.16.x

.changelog/17759.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+extensions: Improve validation and error feedback for `property-override` builtin Envoy extension
+```

agent/envoyextensions/builtin/property-override/property_override.go

@@ -191,6 +191,10 @@ func (f *ResourceFilter) validate() error {
 		return err
 	}
 
+	if len(f.Services) > 0 && f.TrafficDirection != extensioncommon.TrafficDirectionOutbound {
+		return fmt.Errorf("patch contains non-empty ResourceFilter.Services but ResourceFilter.TrafficDirection is not %q",
+			extensioncommon.TrafficDirectionOutbound)
+	}
 	for i := range f.Services {
 		sn := f.Services[i]
 		sn.normalize()

agent/envoyextensions/builtin/property-override/property_override_test.go

@@ -229,6 +229,20 @@ func TestConstructor(t *testing.T) {
 			ok:     false,
 			errMsg: "service name is required",
 		},
+		"non-empty services with invalid traffic direction": {
+			arguments: makeArguments(map[string]any{"Patches": []map[string]any{
+				makePatch(map[string]any{
+					"ResourceFilter": makeResourceFilter(map[string]any{
+						"TrafficDirection": extensioncommon.TrafficDirectionInbound,
+						"Services": []map[string]any{
+							{"Name:": "foo"},
+						},
+					}),
+				}),
+			}}),
+			ok:     false,
+			errMsg: "patch contains non-empty ResourceFilter.Services but ResourceFilter.TrafficDirection is not \"outbound\"",
+		},
 		// See decode.HookWeakDecodeFromSlice for more details. In practice, we can end up
 		// with a "Patches" field decoded to the single "Patch" value contained in the
 		// serialized slice (raised from the containing slice). Using WeakDecode solves

agent/envoyextensions/builtin/property-override/structpatcher.go

@@ -75,14 +75,29 @@ func findTargetMessageAndField(m protoreflect.Message, parsedPath []string, patc
 		}
 
 		// Check whether we have a non-terminal (parent) field in the path for which we
-		// don't support child lookup.
+		// don't support child operations.
 		switch {
 		case fieldDesc.IsList():
 			return nil, nil, fmt.Errorf("path contains member of repeated field '%s'; repeated field member access is not supported",
 				fieldName)
 		case fieldDesc.IsMap():
 			return nil, nil, fmt.Errorf("path contains member of map field '%s'; map field member access is not supported",
 				fieldName)
+		case fieldDesc.Message() != nil && fieldDesc.Message().FullName() == "google.protobuf.Any":
+			// Return a more helpful error for Any fields early.
+			//
+			// Doing this here prevents confusing two-step errors, e.g. "no match for field @type"
+			// on Any, when in fact we don't support variant proto message fields like Any in general.
+			// Because Any is a Message, we'd fail on invalid child fields or unsupported bytes target
+			// fields first.
+			//
+			// In the future, we could support Any by using the type field to initialize a struct for
+			// the nested message value.
+			return nil, nil, fmt.Errorf("variant-type message fields (google.protobuf.Any) are not supported")
+		case !(fieldDesc.Kind() == protoreflect.MessageKind):
+			// Non-Any fields that could be used to serialize protos as bytes will get a clear error message
+			// in this scenario. This also catches accidental use of non-complex fields as parent fields.
+			return nil, nil, fmt.Errorf("path contains member of non-message field '%s' (type '%s'); this type does not support child fields", fieldName, fieldDesc.Kind())
 		}
 
 		fieldM := m.Get(fieldDesc).Message()
@@ -137,6 +152,10 @@ func applyAdd(parentM protoreflect.Message, fieldDesc protoreflect.FieldDescript
 	// similar to a list (repeated field). This map handling is specific to _our_ patch semantics for
 	// updating multiple message fields at once.
 	if isMapValue && !fieldDesc.IsMap() {
+		if fieldDesc.Kind() != protoreflect.MessageKind {
+			return fmt.Errorf("non-message field type '%s' cannot be set via a map", fieldDesc.Kind())
+		}
+
 		// Get a fresh copy of the target field's message, then set the children indicated by the patch.
 		fieldM := parentM.Get(fieldDesc).Message().New()
 		for k, v := range mapValue {
@@ -151,6 +170,7 @@ func applyAdd(parentM protoreflect.Message, fieldDesc protoreflect.FieldDescript
 			fieldM.Set(targetFieldDesc, val)
 		}
 		parentM.Set(fieldDesc, protoreflect.ValueOf(fieldM))
+
 	} else {
 		// Just set the field directly, as our patch value is not a map.
 		val, err := toProtoValue(parentM, fieldDesc, patch.Value)
@@ -280,6 +300,9 @@ func toProtoValue(parentM protoreflect.Message, fieldDesc protoreflect.FieldDesc
 		case float64:
 			return toProtoNumericValue(fieldDesc, val)
 		}
+	case protoreflect.BytesKind,
+		protoreflect.GroupKind:
+		return unsupportedTargetTypeErr(fieldDesc)
 	}
 
 	// Fall back to protoreflect.ValueOf, which may panic if an unexpected type is passed.

agent/envoyextensions/builtin/property-override/structpatcher_test.go

@@ -2,6 +2,7 @@ package propertyoverride
 
 import (
 	"fmt"
+	"google.golang.org/protobuf/types/known/anypb"
 	"testing"
 
 	envoy_cluster_v3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
@@ -592,6 +593,31 @@ func TestPatchStruct(t *testing.T) {
 			},
 			ok: true,
 		},
+		"remove single field: Any": {
+			args: args{
+				k: &envoy_cluster_v3.Cluster{
+					ClusterDiscoveryType: &envoy_cluster_v3.Cluster_ClusterType{
+						ClusterType: &envoy_cluster_v3.Cluster_CustomClusterType{
+							TypedConfig: &anypb.Any{
+								TypeUrl: "foo",
+							},
+						},
+					},
+				},
+				patches: []Patch{
+					makeRemovePatch(
+						"/cluster_type/typed_config",
+					),
+				},
+			},
+			// Invalid actual config, but used as an example of removing Any field directly
+			expected: &envoy_cluster_v3.Cluster{
+				ClusterDiscoveryType: &envoy_cluster_v3.Cluster_ClusterType{
+					ClusterType: &envoy_cluster_v3.Cluster_CustomClusterType{},
+				},
+			},
+			ok: true,
+		},
 		"remove single field deeply nested": {
 			args: args{
 				k: &envoy_cluster_v3.Cluster{
@@ -858,6 +884,69 @@ func TestPatchStruct(t *testing.T) {
 			ok:     false,
 			errMsg: "unsupported target field type 'map'",
 		},
+		"add unsupported target: non-message field via map": {
+			args: args{
+				k: &envoy_cluster_v3.Cluster{},
+				patches: []Patch{
+					makeAddPatch(
+						"/name",
+						map[string]any{
+							"cluster_refresh_rate":       "5s",
+							"cluster_refresh_timeout":    "3s",
+							"redirect_refresh_interval":  "5s",
+							"redirect_refresh_threshold": 5,
+						},
+					),
+				},
+			},
+			ok:     false,
+			errMsg: "non-message field type 'string' cannot be set via a map",
+		},
+		"add unsupported target: non-message parent field via single value": {
+			args: args{
+				k: &envoy_cluster_v3.Cluster{},
+				patches: []Patch{
+					makeAddPatch(
+						"/name/foo",
+						"bar",
+					),
+				},
+			},
+			ok:     false,
+			errMsg: "path contains member of non-message field 'name' (type 'string'); this type does not support child fields",
+		},
+		"add unsupported target: non-message parent field via map": {
+			args: args{
+				k: &envoy_cluster_v3.Cluster{},
+				patches: []Patch{
+					makeAddPatch(
+						"/name/foo",
+						map[string]any{
+							"cluster_refresh_rate":       "5s",
+							"cluster_refresh_timeout":    "3s",
+							"redirect_refresh_interval":  "5s",
+							"redirect_refresh_threshold": 5,
+						},
+					),
+				},
+			},
+			ok:     false,
+			errMsg: "path contains member of non-message field 'name' (type 'string'); this type does not support child fields",
+		},
+		"add unsupported target: Any field": {
+			args: args{
+				k: &envoy_cluster_v3.Cluster{},
+				patches: []Patch{
+					makeAddPatch(
+						// Purposefully use a wrong-but-reasonable field name to ensure special error is returned
+						"/cluster_type/typed_config/@type",
+						"foo",
+					),
+				},
+			},
+			ok:     false,
+			errMsg: "variant-type message fields (google.protobuf.Any) are not supported",
+		},
 		"add unsupported target: repeated message": {
 			args: args{
 				k: &envoy_cluster_v3.Cluster{},