From f9aa7aebb3c778148068717ae8cf1a914fb7e8d4 Mon Sep 17 00:00:00 2001 From: Michael Zalimeni Date: Thu, 15 Jun 2023 13:51:47 -0400 Subject: [PATCH] Property Override validation improvements (#17759) * Reject inbound Prop Override patch with Services Services filtering is only supported for outbound TrafficDirection patches. * Improve Prop Override unexpected type validation - Guard against additional invalid parent and target types - Add specific error handling for Any fields (unsupported) --- .changelog/17759.txt | 3 + .../property-override/property_override.go | 4 + .../property_override_test.go | 14 +++ .../property-override/structpatcher.go | 25 +++++- .../property-override/structpatcher_test.go | 89 +++++++++++++++++++ 5 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 .changelog/17759.txt diff --git a/.changelog/17759.txt b/.changelog/17759.txt new file mode 100644 index 000000000000..0836608ae1f2 --- /dev/null +++ b/.changelog/17759.txt @@ -0,0 +1,3 @@ +```release-note:improvement +extensions: Improve validation and error feedback for `property-override` builtin Envoy extension +``` diff --git a/agent/envoyextensions/builtin/property-override/property_override.go b/agent/envoyextensions/builtin/property-override/property_override.go index 51d78368523f..d42e3d2d50cb 100644 --- a/agent/envoyextensions/builtin/property-override/property_override.go +++ b/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() diff --git a/agent/envoyextensions/builtin/property-override/property_override_test.go b/agent/envoyextensions/builtin/property-override/property_override_test.go index 21889d840f4a..4a80db8671bb 100644 --- a/agent/envoyextensions/builtin/property-override/property_override_test.go +++ b/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 diff --git a/agent/envoyextensions/builtin/property-override/structpatcher.go b/agent/envoyextensions/builtin/property-override/structpatcher.go index 3a54ca25e40a..91de4cf7f86d 100644 --- a/agent/envoyextensions/builtin/property-override/structpatcher.go +++ b/agent/envoyextensions/builtin/property-override/structpatcher.go @@ -75,7 +75,7 @@ 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", @@ -83,6 +83,21 @@ func findTargetMessageAndField(m protoreflect.Message, parsedPath []string, patc 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. diff --git a/agent/envoyextensions/builtin/property-override/structpatcher_test.go b/agent/envoyextensions/builtin/property-override/structpatcher_test.go index 579f0f71c98a..ac7379f9f186 100644 --- a/agent/envoyextensions/builtin/property-override/structpatcher_test.go +++ b/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{},