Skip to content

chore: improve xRoute status errors#5803

Merged
zhaohuabing merged 3 commits into
envoyproxy:mainfrom
zhaohuabing:route-status
Apr 30, 2025
Merged

chore: improve xRoute status errors#5803
zhaohuabing merged 3 commits into
envoyproxy:mainfrom
zhaohuabing:route-status

Conversation

@zhaohuabing
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing commented Apr 24, 2025

#5747 improves the route status for HTTPRoute. This PR does the same for other(GRPC/TLS/TCP/UDP) Routes.

  • Translation errors of all the RouteRule are now surfaced to the XRoute status. This makes it easier for users to find out which parts of their config are invalid.
  • Consolidate all the Route translation error handling in a single location rather than updating status throughout the code. This simplifies the XRoute translation logic and allows us to aggregate all errors into a single status message for better clarity.
  • Fix some inconsistent behavior between HTTPRoute and other Route types.

@zhaohuabing zhaohuabing requested a review from a team as a code owner April 24, 2025 04:08
@zhaohuabing zhaohuabing marked this pull request as draft April 24, 2025 04:08
@zhaohuabing zhaohuabing changed the title improve route status chore improve xRoute status Apr 24, 2025
@zhaohuabing zhaohuabing changed the title chore improve xRoute status chore: improve xRoute status errors Apr 24, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 84.93151% with 22 lines in your changes missing coverage. Please review.

Project coverage is 65.25%. Comparing base (7c88e66) to head (9953a63).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/gatewayapi/route.go 83.45% 19 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5803      +/-   ##
==========================================
- Coverage   65.32%   65.25%   -0.07%     
==========================================
  Files         222      222              
  Lines       35459    35418      -41     
==========================================
- Hits        23164    23113      -51     
- Misses      10859    10867       +8     
- Partials     1436     1438       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member Author

@zhaohuabing zhaohuabing Apr 24, 2025

Choose a reason for hiding this comment

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

There is an inconsistency between the current Accepted status behavior of TCPRoute and HTTPRoute when the envoyproxy tls setting is invalid. To align with the HTTPRoute, the Accepted status for TCPRoute should be set to True here.

https://github.com/envoyproxy/gateway/blob/81e41c13520362f7b47b9c5e59af1b5bca279f14/internal/gatewayapi/testdata/envoyproxy-tls-settings-invalid-ns.out.yaml#L124-L136

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The condition for multiple rules should be Accepted instead of ResolvedRefs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The condition for multiple rules should be Accepted instead of ResolvedRefs.

Comment thread internal/gatewayapi/route.go Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The Appcepted status should be True when failed to Resolve the BackendRef for a MirrorFiliter.

Comment thread internal/gatewayapi/route.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about the case when backend has no endpoints, do we end up here ?

Copy link
Copy Markdown
Member Author

@zhaohuabing zhaohuabing Apr 30, 2025

Choose a reason for hiding this comment

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

Yes, the pervious behavior for backend without endpoints is 500 direct response, and no error status for Accepted and Resolved conditions. This PR doesn't change it.

@arkodg arkodg added this to the v1.4.0-rc.1 milestone Apr 30, 2025
@zhaohuabing zhaohuabing requested a review from arkodg April 30, 2025 00:57
@arkodg arkodg requested review from a team April 30, 2025 01:22
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing merged commit b3f584b into envoyproxy:main Apr 30, 2025
24 of 25 checks passed
@zhaohuabing zhaohuabing deleted the route-status branch April 30, 2025 03:00
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.

3 participants