Skip to content

chore: Set accepted condition as true when failed to resolve backends#6081

Merged
zhaohuabing merged 7 commits intoenvoyproxy:mainfrom
zhaohuabing:fix-5850
Jun 5, 2025
Merged

chore: Set accepted condition as true when failed to resolve backends#6081
zhaohuabing merged 7 commits intoenvoyproxy:mainfrom
zhaohuabing:fix-5850

Conversation

@zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented May 15, 2025

According to Gateway API specs, Backend resolve failure should not affect the Accepted condition.

This also makes the Status handling for UDPRoutes consistent with how it's done for HTTPRoutes and TCPRoutes.

Fixes #5850

@zhaohuabing zhaohuabing requested a review from a team as a code owner May 15, 2025 06:41
@codecov
Copy link

codecov bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.50%. Comparing base (6775832) to head (e702401).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6081      +/-   ##
==========================================
+ Coverage   70.46%   70.50%   +0.03%     
==========================================
  Files         220      220              
  Lines       36625    36615      -10     
==========================================
+ Hits        25809    25815       +6     
+ Misses       9286     9274      -12     
+ Partials     1530     1526       -4     

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

@zhaohuabing zhaohuabing force-pushed the fix-5850 branch 2 times, most recently from 71bc86c to ea7130e Compare May 15, 2025 06:53
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
route:
destination:
name: udproute/default/udproute-1/rule/-1
name: udproute/default/udproute-1
Copy link
Member Author

@zhaohuabing zhaohuabing May 15, 2025

Choose a reason for hiding this comment

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

For HTTPRoute, we generate a route with a 500 direct response for routes with failed backends. However, there's no similar mechanism for UDP/TCP/TLS Route.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you plug this IR into xds translator test so we can see what this inconsistent state looks like ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Add a test in the xDS - it'll generate a cluster without endpoints.

@zhaohuabing zhaohuabing requested a review from arkodg June 4, 2025 06:18
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>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Copy link
Contributor

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

@arkodg arkodg requested review from a team June 4, 2025 16:10
message: UDP Port 8080 not found on Service default/service-1
reason: Failed to process the settings associated with the UDP route.
status: "False"
message: Route is accepted
Copy link
Member

Choose a reason for hiding this comment

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

TBH, the prev one is better for debuging.

Copy link
Member Author

Choose a reason for hiding this comment

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

We still get the UDP Port 8080 not found on Service default/service-1 message in the ResolvedRefs condition.

@zhaohuabing zhaohuabing merged commit 9576038 into envoyproxy:main Jun 5, 2025
40 of 45 checks passed
@zhaohuabing zhaohuabing deleted the fix-5850 branch June 5, 2025 02:59
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.

Inconsistent between how UDPRoute and HTTPRoute handles Accepted condition

3 participants