JSONPath not correctly translated to JSONPatch paths

[liorokman]

Description: Consider the following fragment of the Envoy Proxy bootstrap yaml:

layered_runtime:
  layers:
  - name: global_config
    static_layer:
      re2.max_program_size.error_level: 4294967295
      re2.max_program_size.warn_level: 1000
  - name: some_other_layer
     static_layer:
        key.entry: value

I would like to add the key envoy.some.key.or.other.made.up into the static_layer of the layer called global_config.

A JSONPatch that works looks like this:

- op: add
  path: /layered_runtime/layers/0/static_layer/envoy.some.key.or.other.made.up
  value: a value

But this patch assumes that the layer I want to add the key to is always the first.

I tried to use a JSONPath instead, with the following example:

- op: add
  jsonPath: $.layered_runtime.layers[?@.name=='global_config'].static_layer['envoy.some.key.or.other.made.up']
  value: a value

I have to use the bracket syntax (['envoy.some.key.or.other.made.up']) because the dots must not be treated as one of the JSONPath separators. This is valid JSONPath according to the RFC:

JSONPath expressions move further down the hierarchy using dot notation ($.store.book[0].title) or the bracket notation ($['store']['book'][0]['title']); both replace XPath's / within query expressions, where dot notation serves as a lightweight but limited syntax while bracket notation is a heavyweight but more general syntax.

This results in the following key added to the correct hash:

layers:
   - name: global_config
     static_layer:
      '[''envoy.some.key.or.other.made.up'']': a value
       envoy.restart_features.use_eds_cache_for_ads: false
       envoy.something.completely.made.up: arbitrary string
       re2.max_program_size.error_level: 4294967295

I get the same result for this JSONPath string as well:

- op: add
  jsonPath: $['layered_runtime']['layers'][?@.name=='global_config']['static_layer']['envoy.some.key.or.other.made.up']
  value: a value

The translation from JSONPath to a Path used by the JSONPatch should not have the brackets as part of the result.

$.layered_runtime.layers[?@.name=='global_config'].static_layer['envoy.some.key.or.other.made.up'] should translate to /layered_runtime/layers/0/static_layer/envoy.some.key.or.other.made.up and not to /layered_runtime/layers/0/static_layer/'[''envoy.some.key.or.other.made.up'']'

[arkodg]

cc @denniskniep

[denniskniep]

@liorokman can you try this:

- op: add
  jsonPath: $.layered_runtime.layers[?@.name=='global_config'].static_layer
  path: envoy.some.key.or.other.made.up
  value: a value

Edit: Reason is, that JsonPath could only select what is currently in the document. If you want to introduce a new property you have to append it via path property.

[liorokman]

@denniskniep I tried this, and this works.

Don't you think that it's a little counter-intuitive relative to how regular JSONPatch add operations are specified? When JSONPath is not being used, the path used also points to a place that doesn't exist yet in the document.

For example:

- op: add
  path: /layered_runtime/layers/0/static_layer/envoy.some.key.or.other.made.up
  value: a value

In this case, the envoy.some.key.or.other.made.up value doesn't exist, yet the JSONPatch RFC says that this is how it should be specified in order to add a new key.

At the very least, the documentation should be clear that the value of the path attribute is appended to the calculated values of the jsonPath attribute. I couldn't find this information in the current documentation.

[eshepelyuk]

Hello @liorokman

Are you using patch via EnvoyPatchPolicy CRD ?

[liorokman]

@eshepelyuk I'm actually using it for modifying the bootstrap configuration in EnvoyProxy

[eshepelyuk]

@eshepelyuk I'm actually using it for modifying the bootstrap configuration in EnvoyProxy

Thank for confirming, i realized that jsonPath support for EnvoyPathcPolicy is not released yet and there is no docs regarding it

[liorokman]

@eshepelyuk I'm actually using it for modifying the bootstrap configuration in EnvoyProxy

Thank for confirming, i realized that jsonPath support for EnvoyPathcPolicy is not released yet and there is no docs regarding it

It's not released yet, but you can see the documentation here

[liorokman]

@denniskniep

- op: add
  jsonPath: $.layered_runtime.layers[?@.name=='global_config'].static_layer
  path: envoy.some.key.or.other.made.up
  value: a value

Edit: Reason is, that JsonPath could only select what is currently in the document. If you want to introduce a new property you have to append it via path property.

If it's an error to specify a JSONPath that doesn't exist in the document, then that error should be returned.

If it's not an error, then the generated Path that is returned should correctly contain the decoded path.

The current behavior is that:

1. No error is returned
2. The generated path contains an invalid value '[''envoy.some.key.or.other.made.up'']'

The result is invalid configuration without any error message.

[denniskniep]

@liorokman I completely agree that this should be better documented. Furthermore I also agree to return the error and that we need to fix the jsonPointer generation, if special characters are used.

Thanks for highlighting these points, I will address them soon