Allowing matching a filterchain when applying EnvoyPatchPolicy

[zhaohuabing]

EnvoyPatchPolicy utilities JSON Patch for applying patches to XDS resources, but JSON Patch lacks the ability to select a specific member in an array based on conditions other than its index.

For example: assuming there're two filter chains in a listener, there's no way that we could target the second filter chain based on its serverName www.bar.com when applying a patch.

We won't be able to know the index of the targeted filter chain without inspecting the generated Envoy configuration, which is impossible when we want to use a controller to generate the patch. We could possibly create that patch manually, but it's cumbersome and unreliable, and the index might change with updates to the EG releases.

      - address:
          socketAddress:
            address: 0.0.0.0
            portValue: 10443
        filterChains:
        - filterChainMatch:
            serverNames:
            - www.foo.com
          filters:
          - name: envoy.filters.network.http_connection_manager
            typedConfig:
              ......          
        - filterChainMatch:
            serverNames:
            - www.bar.com
          filters:
          - name: envoy.filters.network.http_connection_manager
            typedConfig:
              ......

[arkodg]

thanks for surfacing this limitation where today with the JSONPatch approach, for lists, the patch producer cannot always proactively create a patch and be sure it will eventually patch, instead must reactively patch to be sure of a successful outcome.
there are 3 ways to tackle this

• dont do anything, and document this limitation
• if JSONPath gets standardized and is accepted in JSONPatch as well as implemented in libraries such as https://github.com/evanphx/json-patch, https://datatracker.ietf.org/wg/jsonpath/about/ , we can achieve this
• we can define our own match criteria, but requests for extending these matches is hard to maintain and support

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

[zhaohuabing]

Close this as we don't need it anymore. We can reopen if someone else asks for this feature.

[denniskniep]

Hi @zhaohuabing,

I would like to restart the discussion on that topic.

I stumbled upon several limitations, which are not yet addressed or merged etc.
I think EnvoyPatchPolicy is a great feature to interfere directly with the envoy config.
Theoretically I would have been able to get around the limitations I stumbled upon. But in reality the EnvoyPatchPolicy is not able to select a specific member in an array based on conditions other than its index.

As I said its a great feature, but array element selection is essential.

Can we move away from JSON Patch or provide an additional option?

And maybe use something like that:
https://github.com/itchyny/gojq?tab=readme-ov-file#usage-as-a-library

This would allow to manipulate the json doc like this:

json:

{
    "a": "123",
    "b": "456",
    "c": [
        {
            "id": "50",
            "value": "test"
        },
        {
            "id": "31",
            "value": "xyz"
        }
    ]
}

jq expression:

(.c[] | select(.id == "31") | .value) = {x: 1, y:2}

result:

{
    "a": "123",
    "b": "456",
    "c": [
        {
            "id": "50",
            "value": "test"
        },
        {
            "id": "31",
            "value": {
                "x": 1,
                "y": 2
            }
        }
    ]
}

full command to reproduce:

echo '{"a":"123","b":"456", "c": [{"id":"50", "value":"test"}, {"id":"31", "value":"xyz"}]}' | gojq -r '(.c[] | select(.id == "31") | .value) = {x: 1, y:2}'

BTW: Happy to create a PR for that, if we can agree on something like that

[zhaohuabing]

@denniskniep Thanks for proposing this solution. It looks promising.

@arkodg How do you think about supporting jq style matching within the EnvoyPatchPolicy?

[denniskniep]

Essentially what we discussed on eg community meeting:

• There are objections because of potential missing backwards combability of jq expressions (cause its not coupled with a standard)

To bring up an additionally point: that lib is released under MIT License, is that fine for eg project?

cc: @zhaohuabing, @arkodg, @guydc

Anything further that needs to be mentioned?

[denniskniep]

From my point of view it would be simply another type: jq how you can execute an EnvoyPatch in addition to type: JsonPatch.
We can document, that jq is following no standard and that it could break something with a new release, if the syntax change.

[arkodg]

thanks for bringing it up in the community meeting today @denniskniep
the core problem this GH issue is trying to address - is being able to search/find the right object within a field that is a list and modify it
would ideally prefer if we used a JSON/YAML standard here

[denniskniep]

Hi,
lets discuss a different approach utilizing JsonPath:

1. Add an additional property jsonPath to EnvoyPatchPolicy.spec.jsonPatches.operation. (Either path or jsonPath must be set)
2. Use an existing JsonPath lib and resolve JsonPath Expression to exact locations in json document
3. Convert the locations to JsonPointer syntax
4. Execute for each JsonPointer the normal JsonPatch operation

I did a small PoC here: https://github.com/denniskniep/json-patch-by-path-demo
Disclaimer: JsonPathLocation to JsonPointer conversion is not really seriously implemented in that PoC...

The core logic is:

func Patch(req PatchRequest) (string, error) {
	jObj, err := oj.ParseString(req.JsonDoc)
	if err != nil {
		return "", errors.Wrap(err, "Error during parsing json")
	}

	jPath, err := jp.ParseString(req.JsonPath)
	if err != nil {
		return "", errors.Wrap(err, "Error during parsing jpath")
	}

	locations := jPath.Locate(jObj, 0)

	jsonDoc := req.JsonDoc
	for _, l := range locations {

		resolvedJsonPath := l.String()
		jsonPointer := convertToJsonPointer(resolvedJsonPath)

		patch, err := createJsonPatch(req.Operation, jsonPointer, req.Value)
		if err != nil {
			return "", err
		}

		modified, err := patch.Apply([]byte(jsonDoc))
		if err != nil {
			return "", errors.Wrap(err, "Error during patch apply")
		}

		jsonDoc = string(modified)
	}

	return jsonDoc, nil
}

In this PoC I used the following JsonPath lib: https://github.com/ohler55/ojg

What do you think @zhaohuabing @arkodg ? Is that conceptionally something we can aim for?

[arkodg]

hey @denniskniep thanks for digging deeper, I'd be a +1 for this approach wdyt @envoyproxy/gateway-maintainers

[zhaohuabing]

@denniskniep brilliant! +1 for this.

[arkodg]

thanks for bringing this up in the community meeting today @denniskniep, there were no objections for this approach yet, please feel free to continue with this.
A good suggestion made by @guydc was to look into which library kuberenetes was using for JsonPath, and comparing that with the one suggested above

[denniskniep]

Seems to be that kubectl is using its own implementation from k8s client-go lib

Client-Go Lib parser is called from the get command here

I dont think that it makes sense to include the entire client-go lib only for that json path. Furthermore it has no implementation to resolve the path to location strings. That would make the jsonPath to Pointer conversion even harder.

Therefore my vote for https://github.com/ohler55/ojg
(BTW: This lib has a pretty good coverage of the rfc9535 see here)

Once we agreed on the lib, I can start the implementation

[arkodg]

https://cburgmer.github.io/json-path-comparison/ is a great view, please go ahead with https://github.com/ohler55/ojg

[denniskniep]

@arkodg can we add this again to milestone v1.1.0-rc1 ?

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

[meektechie]

@zhaohuabing is it possible to share the EnvoyPatchPolicy CR file which you are using to allow your domain. I am stuck up with an issue on envoy-gateway i hope it might help me

[zhaohuabing]

@meektechie Not sure that I got what you meant :-) What EnvoyPatchPolicy are you looking for?

[meektechie]

Thanks for the response @zhaohuabing . I am trying to configure SNI for listener and found this can be achieved through EnvoyPatchPolicy. But unable to get the sample for it. With that approach i hope we might setup ssl certificate for the explicit subdomains. So requesting to share that.

Also, is it possible to refer certificate from the secret instead of filename/path

  common_tls_context:
    tls_certificates:
    - certificate_chain: { filename: "example_com_cert.pem" }
      private_key: { filename: "example_com_key.pem" }