Skip to content

Implement JSON Patch in Xds Translator#1606

Merged
zirain merged 11 commits intoenvoyproxy:mainfrom
arkodg:xds-jsonpatch
Jul 5, 2023
Merged

Implement JSON Patch in Xds Translator#1606
zirain merged 11 commits intoenvoyproxy:mainfrom
arkodg:xds-jsonpatch

Conversation

@arkodg
Copy link
Copy Markdown
Contributor

@arkodg arkodg commented Jun 29, 2023

Relates to #24

Implement JSON Patch in Xds Translator
b9baf83 
Relates to envoyproxy#24

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from a team as a code owner June 29, 2023 23:02
@arkodg arkodg marked this pull request as draft June 29, 2023 23:05
Arko Dasgupta added 2 commits June 29, 2023 16:44
use temp variable to unmarshal into
019bf45 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
lint
74ea031 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jun 30, 2023

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
Copy link
Copy Markdown
Member

zirain commented Jun 30, 2023

why there's a |?

fix test
631dace 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jun 30, 2023

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
Copy link
Copy Markdown

codecov bot commented Jun 30, 2023 •
edited
Loading

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
zirain reviewed Jun 30, 2023
View reviewed changes
Arko Dasgupta added 4 commits June 29, 2023 20:45
use apiextensionsv1.JSON
5fca2d9 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
routeConfig test
b323f06 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
add entire resource and more test cases
5b91f56 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
lint
ce795b7 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg marked this pull request as ready for review June 30, 2023 19:17
@arkodg arkodg requested review from a team, LanceEa, chauhanshubham, kflynn and zirain and removed request for a team June 30, 2023 19:17
zirain
zirain reviewed Jun 30, 2023
View reviewed changes
internal/ir/xds.go
// +k8s:deepcopy-gen=true
type JSONPatchOperation struct {
// Op is the type of operation to perform
Op string `json:"op"`
Copy link
Copy Markdown
Member

@zirain zirain Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use full name here?

Copy link
Copy Markdown
Member

@zirain zirain Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or rename to Type, and change to Enum?

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Jul 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@zirain zirain Jul 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operation.Op that's odd.

Copy link
Copy Markdown
Member

@qicz qicz Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

zirain
zirain reviewed Jun 30, 2023
View reviewed changes
move marshaller out of for loop
2918ea1 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from zirain July 1, 2023 00:39
zirain
zirain reviewed Jul 2, 2023
View reviewed changes
qicz
qicz reviewed Jul 3, 2023
View reviewed changes
address comments
b933529 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested review from qicz and zirain July 3, 2023 17:05
@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jul 4, 2023

/retest

zhaohuabing
zhaohuabing approved these changes Jul 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

@zirain zirain merged commit e171678 into envoyproxy:main Jul 5, 2023
@arkodg arkodg deleted the xds-jsonpatch branch July 5, 2023 15:28
@arkodg arkodg mentioned this pull request Jul 5, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhaohuabing zhaohuabing zhaohuabing approved these changes

@zirain zirain zirain approved these changes

@kflynn kflynn Awaiting requested review from kflynn kflynn was automatically assigned from envoyproxy/gateway-reviewers

@LanceEa LanceEa Awaiting requested review from LanceEa LanceEa was automatically assigned from envoyproxy/gateway-reviewers

@chauhanshubham chauhanshubham Awaiting requested review from chauhanshubham chauhanshubham was automatically assigned from envoyproxy/gateway-reviewers

@qicz qicz Awaiting requested review from qicz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@arkodg @zirain @zhaohuabing @qicz