Implement JSON Patch in Xds Translator

[arkodg]

Relates to #24

[arkodg]

trying to fight one last issue wrt unmarshalling

address:
  socket_address:
    address: 0.0.0.0
    port_value: 10080
default_filter_chain:
  filters:
  - name: envoy.filters.network.http_connection_manager
    typed_config:
      '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
      common_http_protocol_options:
        headers_with_underscores_action: REJECT_REQUEST
      http_filters:
      - |
        name: "envoy.filters.http.ratelimit"
        typed_config:
          "@type": "type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit"
          domain: "eg-ratelimit"
          failure_mode_deny: true
          timeout: 1s
          rate_limit_service:
            grpc_service:
              envoy_grpc:
                cluster_name: rate-limit-cluster
            transport_api_version: V3'
      - name: envoy.filters.http.router
        typed_config:
          '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
      http2_protocol_options:
        initial_connection_window_size: 1048576
        initial_stream_window_size: 65536
        max_concurrent_streams: 100
      merge_slashes: true
      normalize_path: true
      path_with_escaped_slashes_action: UNESCAPE_AND_REDIRECT
      rds:
        config_source:
          ads: {}
          resource_api_version: V3
        route_config_name: first-listener
      stat_prefix: http
      upgrade_configs:
      - upgrade_type: websocket
      use_remote_address: true
name: first-listener
per_connection_buffer_limit_bytes: 32768

looks like the | header is getting literally translated instead of being interpreted as a block of YAML string

[zirain]

why there's a |?

[arkodg]

managed to solve the issue by setting the value of Value to interface{} but now controller-gen DeepCopy() is failing, might need to handwrite it myself

[codecov]

Codecov Report

Merging #1606 (a85c6af) into main (3974be6) will decrease coverage by 0.94%.
The diff coverage is 25.86%.

@@            Coverage Diff             @@
##             main    #1606      +/-   ##
==========================================
- Coverage   61.38%   60.44%   -0.94%     
==========================================
  Files          81       82       +1     
  Lines       12157    12404     +247     
==========================================
+ Hits         7462     7498      +36     
- Misses       4237     4416     +179     
- Partials      458      490      +32     

Impacted Files	Coverage Δ
internal/ir/xds.go	71.66% <ø> (ø)
internal/ir/zz_generated.deepcopy.go	10.74% <0.00%> (-0.45%)	⬇️
internal/xds/translator/jsonpatch.go	24.10% <24.10%> (ø)
internal/xds/translator/translator.go	77.92% <57.14%> (-2.85%)	⬇️

... and 2 files with indirect coverage changes

[zirain]

use full name here?

[zirain]

or rename to Type, and change to Enum?

[arkodg]

this naming is tied to JSONPatch spec, since we marshal this and directly pass it to https://github.com/evanphx/json-patch#create-and-apply-a-json-patch

[zirain]

Operation.Op that's odd.

[qicz]

+1

[arkodg]

its part of the spec :)
https://datatracker.ietf.org/doc/html/rfc6902/#section-4

[zhaohuabing]

I know it looks a bit ackward to have an "op" member inside the "Operation" object, but as Arko pointed out, it aligns with the RFC standard.

Below description is extracted from the RFC:

Operation objects MUST have exactly one "op" member, whose value indicates the operation to perform. Its value MUST be one of "add", "remove", "replace", "move", "copy", or "test"; other values are errors.

The following examples are from the same RFC. The Operation structure is the same as the JSONPatchOperation in this PR.

   { "op": "add", "path": "/a/b/c", "value": "foo" }
   { "path": "/a/b/c", "op": "add", "value": "foo" }
   { "value": "foo", "path": "/a/b/c", "op": "add" }

I think we can settle with the current implementation so we won't block user's custom extensions replying on the EnvoyPatchPolicy. It's internal to EG anyway, we can always go back and change it once we figure out a better way to handle it.

[arkodg]

/retest

[zhaohuabing]

LGTM.

As mentioned in my previous comment, the issue with the Operation structure is non-blocking and can potentially be addressed in the future.