Skip to content

refactor errors and xds for BackendTrafficPolicy and SecurityPolicy#2926

Closed
shawnh2 wants to merge 10 commits intoenvoyproxy:mainfrom
shawnh2:xpolicy-error
Closed

refactor errors and xds for BackendTrafficPolicy and SecurityPolicy#2926
shawnh2 wants to merge 10 commits intoenvoyproxy:mainfrom
shawnh2:xpolicy-error

Conversation

@shawnh2
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 commented Mar 15, 2024 •
edited
Loading

What type of PR is this?

What this PR does / why we need it:

  • reuse the translated logic, currently there're a lot of dup code in translateXXXPolicyForRoute and translateXXXPolicyForGateway for BTP and SP, this PR extract the common part of these two methods into one shared method
  • group BTP and SP features in xds.go into a struct, so we can perform Any() and Validate(), make the code structure more clean
  • wrap the build error in translate method with its feature's name via perr.WrapWithMessage() for SP and BTP, providing more context about errors
  • for BTP, change errors directly return behavior to errors join

Which issue(s) this PR fixes:

Fixes #2894
Fixes #2954

@shawnh2
refactor errors and translation for security policy
b2fdaf2 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 59.75610% with 165 lines in your changes are missing coverage. Please review.

Project coverage is 64.37%. Comparing base (6a57cd1) to head (d92fc4d).

Files Patch % Lines
internal/ir/zz_generated.deepcopy.go 0.00% 92 Missing and 2 partials ⚠️
internal/ir/xds.go 12.28% 46 Missing and 4 partials ⚠️
internal/gatewayapi/backendtrafficpolicy.go 87.36% 8 Missing and 4 partials ⚠️
internal/gatewayapi/securitypolicy.go 88.57% 3 Missing and 1 partial ⚠️
internal/xds/translator/oidc.go 70.00% 1 Missing and 2 partials ⚠️
internal/gatewayapi/route.go 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2926      +/-   ##
==========================================
- Coverage   64.60%   64.37%   -0.23%     
==========================================
  Files         122      122              
  Lines       21147    21161      +14     
==========================================
- Hits        13662    13623      -39     
- Misses       6637     6707      +70     
+ Partials      848      831      -17

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

shawnh2 added 3 commits March 15, 2024 19:59
@shawnh2
resolve conflicts
5fcb483 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
refactor errors and translation for BTP
97f11eb 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
polish comments
e0e8dd5 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2 shawnh2 changed the title refactor errors and irs for xPolicy refactor errors and irs for BackendTrafficPolicy and SecurityPolicy Mar 15, 2024
@shawnh2 shawnh2 marked this pull request as ready for review March 15, 2024 13:03
@shawnh2 shawnh2 requested a review from a team as a code owner March 15, 2024 13:03
shawnh2
shawnh2 commented Mar 16, 2024
View reviewed changes
internal/gatewayapi/backendtrafficpolicy.go
@@ -926,20 +858,23 @@ func (t *Translator) buildCircuitBreaker(policy *egv1a1.BackendTrafficPolicy) (*

func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTTPRoute) (*ir.Timeout, error) {
Copy link
Copy Markdown
Contributor Author

@shawnh2 shawnh2 Mar 16, 2024

Choose a reason for hiding this comment

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

this part is reverted from https://github.com/envoyproxy/gateway/pull/2846/files#diff-ce86bc9474dbc964bb6180e6a110b708dd85081193dca54a2fa7cda70d742247L930, since BTP has supported dealing with multi-errors now

zhaohuabing
zhaohuabing reviewed Mar 18, 2024
View reviewed changes
shawnh2 added 4 commits March 18, 2024 16:38
@shawnh2
refactor BTP and SP xds
7257449 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
correct testdata and fix nil bug
c7db0fc 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
merge from main
cc2fe22 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
resolve conflicts
1ecdd55 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2 shawnh2 requested a review from arkodg March 18, 2024 09:50
@zirain
Copy link
Copy Markdown
Member

zirain commented Mar 19, 2024

/retest

shawnh2 added 2 commits March 19, 2024 15:40
@shawnh2
merge from main
420e243 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
resolve conflicts
d92fc4d 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2 shawnh2 changed the title refactor errors and irs for BackendTrafficPolicy and SecurityPolicy refactor errors and xds for BackendTrafficPolicy and SecurityPolicy Mar 19, 2024
@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Mar 19, 2024

/retest

zhaohuabing
zhaohuabing reviewed Mar 20, 2024
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.

This PR contains many files, It would be easier to review if you could list the changes in the description.

@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Mar 20, 2024

This PR contains many files, It would be easier to review if you could list the changes in the description.

Sure thing

@shawnh2 shawnh2 requested a review from zhaohuabing March 20, 2024 04:36
@zhaohuabing
Copy link
Copy Markdown
Member

zhaohuabing commented Mar 21, 2024 •
edited
Loading

@shawnh2

Thank you for tidying up the code!

This PR is in a good direction, however, I found it a bit difficult for me to review when going through it line by line. Would you mind splitting this PR into three as the following?

  1. reuse the translated logic, currently there're a lot of dup code in translateXXXPolicyForRoute and translateXXXPolicyForGateway for BTP and SP, this PR extract the common part of these two methods into one shared method

  2. group BTP and SP features in xds.go into a struct, so we can perform Any() and Validate(), make the code structure more clean

  3. wrap the build error in translate method with its feature's name via perr.WrapWithMessage() for SP and BTP, providing more context about errors
    for BTP, change errors directly return behavior to errors join

@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Mar 21, 2024

@shawnh2

Thank you for tidying up the code!

This PR is in a good direction, however, I found it a bit difficult for me to review when going through it line by line. Would you mind splitting this PR into three as the following?

  1. reuse the translated logic, currently there're a lot of dup code in translateXXXPolicyForRoute and translateXXXPolicyForGateway for BTP and SP, this PR extract the common part of these two methods into one shared method

  2. group BTP and SP features in xds.go into a struct, so we can perform Any() and Validate(), make the code structure more clean

  3. wrap the build error in translate method with its feature's name via perr.WrapWithMessage() for SP and BTP, providing more context about errors

for BTP, change errors directly return behavior to errors join

Of course, thanks for reviewing this, I will split it up to serval PRs

@shawnh2 shawnh2 closed this Mar 26, 2024
@shawnh2 shawnh2 deleted the xpolicy-error branch March 26, 2024 04:10
@shawnh2 shawnh2 mentioned this pull request Mar 26, 2024
Merged
@shawnh2 shawnh2 mentioned this pull request Apr 14, 2024
Merged
@shawnh2 shawnh2 mentioned this pull request Apr 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arkodg arkodg Awaiting requested review from arkodg

@zhaohuabing zhaohuabing Awaiting requested review from zhaohuabing

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor ir HTTPRoute: group related features into structures Unify the way to process errors for xPolicy

3 participants

@shawnh2 @zirain @zhaohuabing