Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor errors and xds for BackendTrafficPolicy and SecurityPolicy #2926

Closed
wants to merge 10 commits into from

Conversation

shawnh2
Copy link
Contributor

@shawnh2 shawnh2 commented Mar 15, 2024

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

Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Copy link

codecov bot commented Mar 15, 2024

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.

Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
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
@@ -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
Contributor Author

Choose a reason for hiding this comment

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

internal/ir/xds.go Outdated Show resolved Hide resolved
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2 shawnh2 requested a review from arkodg March 18, 2024 09:50
@zirain
Copy link
Contributor

zirain commented Mar 19, 2024

/retest

Signed-off-by: shawnh2 <shawnhxh@outlook.com>
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
Contributor Author

shawnh2 commented Mar 19, 2024

/retest

Copy link
Member

@zhaohuabing zhaohuabing left a comment

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
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

@zhaohuabing
Copy link
Member

zhaohuabing 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

@shawnh2
Copy link
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
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