From dd56a6800bebc54dabd7883fddc22b25ca2bdb92 Mon Sep 17 00:00:00 2001 From: Michael Zalimeni Date: Thu, 15 Jun 2023 11:46:07 -0400 Subject: [PATCH] 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/structpatcher.go | 25 +++++- .../property-override/structpatcher_test.go | 89 +++++++++++++++++++ 3 files changed, 116 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/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{},