Backport of Property Override validation improvements into release/1.16.x (#17778)

* backport of commit 97c779b5a2308a05fde93247209fa6e9cd3fc310

* backport of commit dd56a6800bebc54dabd7883fddc22b25ca2bdb92

---------

Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com>
This commit is contained in:
hc-github-team-consul-core 2023-06-15 14:20:16 -04:00 committed by GitHub
parent 68e191dd50
commit 8e201f8964
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 134 additions and 1 deletions

3
.changelog/17759.txt Normal file
View File

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

View File

@ -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()

View File

@ -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

View File

@ -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.

View File

@ -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{},