From b0eb3ec3dd4781c26877996e01e3c70b1601c5b4 Mon Sep 17 00:00:00 2001 From: Michael Zalimeni Date: Thu, 15 Jun 2023 13:46:21 -0400 Subject: [PATCH] Add Patch index to Prop Override validation errors When a patch is found invalid, include its index for easier debugging when multiple patches are provided. --- .../property-override/property_override.go | 4 +- .../property_override_test.go | 53 ++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/agent/envoyextensions/builtin/property-override/property_override.go b/agent/envoyextensions/builtin/property-override/property_override.go index 51d78368523f..cad1b7ae216b 100644 --- a/agent/envoyextensions/builtin/property-override/property_override.go +++ b/agent/envoyextensions/builtin/property-override/property_override.go @@ -255,9 +255,9 @@ func (p *propertyOverride) validate() error { } var resultErr error - for _, patch := range p.Patches { + for i, patch := range p.Patches { if err := patch.validate(p.Debug); err != nil { - resultErr = multierror.Append(resultErr, err) + resultErr = multierror.Append(resultErr, fmt.Errorf("invalid Patches[%d]: %w", i, err)) } } diff --git a/agent/envoyextensions/builtin/property-override/property_override_test.go b/agent/envoyextensions/builtin/property-override/property_override_test.go index 21889d840f4a..f32d882136a9 100644 --- a/agent/envoyextensions/builtin/property-override/property_override_test.go +++ b/agent/envoyextensions/builtin/property-override/property_override_test.go @@ -63,6 +63,7 @@ func TestConstructor(t *testing.T) { expected propertyOverride ok bool errMsg string + errFunc func(*testing.T, error) } validTestCase := func(o Op, d extensioncommon.TrafficDirection, t ResourceType) testCase { @@ -216,6 +217,50 @@ func TestConstructor(t *testing.T) { ok: false, errMsg: fmt.Sprintf("field Value is not supported for %s operation", OpRemove), }, + "multiple patches includes indexed errors": { + arguments: makeArguments(map[string]any{"Patches": []map[string]any{ + makePatch(map[string]any{ + "Op": OpRemove, + "Value": 0, + }), + makePatch(map[string]any{ + "Op": OpAdd, + "Value": nil, + }), + makePatch(map[string]any{ + "Op": OpAdd, + "Path": "/foo", + }), + }}), + ok: false, + errFunc: func(t *testing.T, err error) { + require.ErrorContains(t, err, "invalid Patches[0]: field Value is not supported for remove operation") + require.ErrorContains(t, err, "invalid Patches[1]: non-nil Value is required") + require.ErrorContains(t, err, "invalid Patches[2]: no match for field 'foo'") + }, + }, + "multiple patches single error contains correct index": { + arguments: makeArguments(map[string]any{"Patches": []map[string]any{ + makePatch(map[string]any{ + "Op": OpAdd, + "Value": "foo", + }), + makePatch(map[string]any{ + "Op": OpRemove, + "Value": 1, + }), + makePatch(map[string]any{ + "Op": OpAdd, + "Value": "bar", + }), + }}), + ok: false, + errFunc: func(t *testing.T, err error) { + require.ErrorContains(t, err, "invalid Patches[1]: field Value is not supported for remove operation") + require.NotContains(t, err.Error(), "invalid Patches[0]") + require.NotContains(t, err.Error(), "invalid Patches[2]") + }, + }, "empty service name": { arguments: makeArguments(map[string]any{"Patches": []map[string]any{ makePatch(map[string]any{ @@ -333,7 +378,13 @@ func TestConstructor(t *testing.T) { require.NoError(t, err) require.Equal(t, &extensioncommon.BasicEnvoyExtender{Extension: &tc.expected}, e) } else { - require.ErrorContains(t, err, tc.errMsg) + require.Error(t, err) + if tc.errMsg != "" { + require.ErrorContains(t, err, tc.errMsg) + } + if tc.errFunc != nil { + tc.errFunc(t, err) + } } }) }