Skip to content

feat: support JSONPatches for proxy bootstrap modifications#4116

Merged
guydc merged 9 commits intoenvoyproxy:mainfrom
liorokman:jsonpatch-in-proxy-bootstrap
Sep 5, 2024
Merged

feat: support JSONPatches for proxy bootstrap modifications#4116
guydc merged 9 commits intoenvoyproxy:mainfrom
liorokman:jsonpatch-in-proxy-bootstrap

Conversation

@liorokman
Copy link
Copy Markdown
Contributor

@liorokman liorokman commented Aug 27, 2024

Which issue(s) this PR fixes:
Fixes #3691

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.00000% with 94 lines in your changes missing coverage. Please review.

Project coverage is 67.93%. Comparing base (37659dc) to head (541eaf5).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
internal/xds/translator/jsonpatch.go 26.08% 39 Missing and 12 partials ⚠️
internal/utils/jsonpatch/patch.go 61.70% 13 Missing and 5 partials ⚠️
internal/xds/bootstrap/validate.go 70.00% 6 Missing and 6 partials ⚠️
internal/xds/bootstrap/util.go 68.57% 6 Missing and 5 partials ⚠️
internal/provider/kubernetes/controller.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4116      +/-   ##
==========================================
- Coverage   67.95%   67.93%   -0.03%     
==========================================
  Files         187      189       +2     
  Lines       23017    23099      +82     
==========================================
+ Hits        15642    15693      +51     
- Misses       6262     6283      +21     
- Partials     1113     1123      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexwo
Copy link
Copy Markdown
Contributor

alexwo commented Aug 27, 2024

/retest

@liorokman liorokman marked this pull request as ready for review August 29, 2024 15:34
@liorokman liorokman requested a review from a team as a code owner August 29, 2024 15:34
@liorokman
Copy link
Copy Markdown
Contributor Author

liorokman commented Sep 3, 2024

Note that the patch coverage check is artificially low because the PR moved some code from the xds/translator package to a common util package ( util/jsonpatch) and code that didn't change, other than some indentation, is now flagged as "untested" as part of this PR.

guydc
guydc reviewed Sep 4, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@guydc guydc 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.

overall LGTM, if we can just add a few more tests to the new validation flow and IR it would be great.

I think that the decoupling the API package from XDS libs is a positive thing.

cc @zhaohuabing

internal/provider/kubernetes/controller.go Outdated
Copy link
Copy Markdown
Contributor

@guydc guydc Sep 4, 2024

Choose a reason for hiding this comment

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

Can we cover this validation with a positive/negative test?

Copy link
Copy Markdown
Contributor Author

@liorokman liorokman Sep 5, 2024

Choose a reason for hiding this comment

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

That's already included here: https://github.com/envoyproxy/gateway/pull/4116/files/c790f2e378c00d169e271049d17c050fb488dd86#diff-6783c0822a78972a488213dd33bef82297eaac567e74fd5ae639020e18448e3c

internal/ir/xds.go Outdated
Copy link
Copy Markdown
Contributor

@guydc guydc Sep 4, 2024

Choose a reason for hiding this comment

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

can we add a single IR translation test with all invalid cases?

Copy link
Copy Markdown
Contributor Author

@liorokman liorokman Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've added unit-tests and made the validate method more aligned with RFC 6902

Copy link
Copy Markdown
Contributor Author

@liorokman liorokman Sep 5, 2024

Choose a reason for hiding this comment

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

I also tried to add CEL validation to the JSONPatchOperation structure in the API folder.

Turns out that because the Value field there is apiextensionsv1.JSON, it's not possible to reference the value field in CEL tests. Any CEL expression that tries doesn't compile correctly. So no CEL tests on the JSONPatchOperation are possible if they reference the value attribute.

@guydc guydc requested review from a team September 4, 2024 21:21
@liorokman
Modify the API to support expressing JSONPatches
e61fdc1 
Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
Support JSONPatch in the bootstrap proxy flow
740f64e 
Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
Accept JSONPatches in a YAML format for unit-tests to be consistent with
44f6bbd 
other unit tests.

Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
Reduce the runtime imports, move imports needed for tests into the test
a31783d 
code.

Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
Moved validation code that requires access to internal/xds out of the
b3f3ccf 
api/v1alpha1/validation package.

Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
Cleanup and make the linter happy
676ea0b 
Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
Added another test for the JSONPatch code
2d51ab9 
Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
Make the linter happy
785c642 
Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman
More unit-tests.
541eaf5 
Signed-off-by: Lior Okman <lior.okman@sap.com>
@liorokman liorokman force-pushed the jsonpatch-in-proxy-bootstrap branch from c790f2e to 541eaf5 Compare September 5, 2024 08:24
@liorokman
Copy link
Copy Markdown
Contributor Author

liorokman commented Sep 5, 2024

/retest

zirain
zirain reviewed Sep 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

overall LGTM

api/v1alpha1/envoyproxy_types.go
)

// ProxyBootstrap defines Envoy Bootstrap configuration.
// +union
Copy link
Copy Markdown
Member

@zirain zirain Sep 5, 2024

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Copy Markdown
Contributor Author

@liorokman liorokman Sep 5, 2024

Choose a reason for hiding this comment

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

Not sure. It appears in other places (e.g. RateLimitSpec, ActiveHealthCheckPayload, ActiveHealthCheck, and more), so I added it here and kept the same semantics as in the other places.

guydc
guydc approved these changes Sep 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@guydc guydc merged commit 9bb1601 into envoyproxy:main Sep 5, 2024
@liorokman liorokman mentioned this pull request Sep 5, 2024
Open
@liorokman liorokman deleted the jsonpatch-in-proxy-bootstrap branch September 5, 2024 13:36
@arkodg arkodg mentioned this pull request Nov 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zirain zirain zirain approved these changes

@guydc guydc guydc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

EnvoyProxy Bootstrap configuration can't merge a list

4 participants

@liorokman @alexwo @zirain @guydc