-
Notifications
You must be signed in to change notification settings - Fork 381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(translator): JsonPath in PatchPolicy #3757
feat(translator): JsonPath in PatchPolicy #3757
Conversation
@arkodg can you plz have a quick look if this is the right direction? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3757 +/- ##
========================================
Coverage 67.64% 67.64%
========================================
Files 185 187 +2
Lines 22578 22859 +281
========================================
+ Hits 15272 15464 +192
- Misses 6213 6282 +69
- Partials 1093 1113 +20 ☔ View full report in Codecov by Sentry. |
ff58774
to
e3ff2eb
Compare
e3ff2eb
to
c8a10a5
Compare
@@ -0,0 +1,163 @@ | |||
envoyPatchPolicies: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add examples with filters, which is the real value prop for using jsonpatch over path since its hard to figure out index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add this test. It makes clear, that you do not need to go through all parent nested properties and you can select explicitly the route by name, which is much more reliable then selecting via index:
- type: "type.googleapis.com/envoy.config.route.v3.RouteConfiguration"
name: "first-listener"
operation:
op: "replace"
jsonPath: "..routes[?(@.name=='second-route')].route.upgrade_configs"
value:
- upgrade_type: CONNECT
connect_config:
{}
but there is already this scenario:
jsonPath: "..endpoints[*].load_balancing_weight"
which addresses ALL endpoints, which is not possible right now
"github.com/stretchr/testify/require" | ||
) | ||
|
||
type pointerTest struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to maintain uniformity, can these tests be represented in this
gateway/internal/ir/xds_test.go
Line 533 in eeb62c8
tests := []struct { |
hey @denniskniep sorry for the delay in review, the diff looks good ! |
JsonPath can be utilized to select elements for JSONPatch in EnvoyPatchPolicy Signed-off-by: Dennis Kniep <[email protected]>
c8a10a5
to
72b8113
Compare
cool, thx @arkodg addressed your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks for adding this feature !
hopefully this makes it easier to author EnvoyPatchPolicies for modifying elements within a list
JsonPath can be utilized to select elements for JSONPatch in EnvoyPatchPolicy
Fixes #1686
EnvoyPatchPolicy
API Changes:jsonPath
path
is optionalThese changes are backwards compatible
Change Summary:
jsonPath
is evaluated based on the json and the patch is done for all found items (pointers)We can only "find" existing items in the json with jsonPath. Therefore we need a way to extend the found items (pointers). This enables us to add not existing properties. This is implemented by adding the string of the
path
to the end of the found items (pointers)Example:
jsonPath
:"filter_chains[*].filters[*].typed_config"
path
:"/preserve_external_request_id"
will find following items (pointers):
/filter_chains/0/filters/0/typed_config
/filter_chains/0/filters/1/typed_config
Final jsonPointers which are used withing the JSONPatch:
/filter_chains/0/filters/0/typed_config/preserve_external_request_id
/filter_chains/0/filters/1/typed_config/preserve_external_request_id