refactor: return 500 when BackendTLSPolicy translation fails by alexwo · Pull Request #4363 · envoyproxy/gateway

alexwo · 2024-09-29T12:20:09Z

What type of PR is this?

What this PR does / why we need it:

500 for HTTP
connection reset for TCP/UDP

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

codecov · 2024-09-29T12:27:35Z

Codecov Report

Attention: Patch coverage is 81.62162% with 34 lines in your changes missing coverage. Please review.

Project coverage is 66.28%. Comparing base (b9f9a9f) to head (c2c8b4b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/gatewayapi/route.go	75.00%	16 Missing and 5 partials ⚠️
internal/gatewayapi/filters.go	63.63%	7 Missing and 1 partial ⚠️
internal/gatewayapi/ext_service.go	40.00%	2 Missing and 1 partial ⚠️
internal/gatewayapi/validate.go	95.55%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4363      +/-   ##
==========================================
- Coverage   66.29%   66.28%   -0.01%     
==========================================
  Files         209      209              
  Lines       31912    31956      +44     
==========================================
+ Hits        21157    21183      +26     
- Misses       9507     9523      +16     
- Partials     1248     1250       +2

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

alexwo · 2024-09-29T18:08:00Z

/retest

zhaohuabing

@alexwo Thanks for picking up this! This PR looks great overall! I just have a few nits.

internal/gatewayapi/route.go

zhaohuabing · 2024-09-30T00:44:43Z

internal/gatewayapi/testdata/envoyproxy-tls-settings-invalid-ns.out.yaml

Why this is changed?

updated, a route will not get translated, if it is a TCP/UDP route & contains invalid configurations

@alexwo - let's maybe update the PR description and title so that the release notes will reflect the two behaviors:

500 for HTTP

connection reset for TCP/UDP

In terms of UX, users experience a reset in this case right? Just want to make sure that the connections on the client side don't hang and wait for some timeout, so that it's clear to users that the request is explicitly rejected.

If the route associated with the TLS connection is removed, I would expect ongoing connections that are relying on this route will be dropped. This will most likely result in a TCP reset (RST) or a termination of the TLS session, effectively severing the connection. Maybe we can try to confirm it.

internal/gatewayapi/testdata/envoyproxy-tls-settings-invalid.out.yaml

guydc

LGTM. Few questions on behavior and scope.

guydc · 2024-09-30T21:39:46Z

internal/gatewayapi/validate.go

Are the changes made here extending the scope of the PR to reject additional cases like invalid backend config, or is this already handled with a 500 response?

Yes, these changes capture cases where the backend configuration is invalid.

guydc · 2024-09-30T21:43:34Z

internal/gatewayapi/testdata/tlsroute-with-backend.out.yaml

should the destination exist in this case?

nope, updated.

guydc · 2024-09-30T21:43:52Z

internal/gatewayapi/testdata/tcproute-with-backend.out.yaml

alexwo · 2024-10-01T10:25:42Z

/retes

alexwo · 2024-10-01T10:25:46Z

/retest

alexwo · 2024-10-01T10:50:16Z

/retest

alexwo · 2024-10-02T16:58:37Z

/retest

alexwo · 2024-10-08T14:29:51Z

/retest

arkodg · 2024-10-09T01:00:44Z

internal/gatewayapi/testdata/tlsroute-with-backend.out.yaml

curious what happened here, and why this got deleted ?

In this scenario, the route generally shouldn’t be operational. I’ll look into why the settings are being removed to better understand the problem. For now, the route is left as-is because fully invalidating it causes the conformance test to fail. Perhaps we can ignore the TLS route translation errors in the affected flow and create a separate task to address it.

arkodg · 2024-10-09T01:04:27Z

internal/gatewayapi/testdata/httproute-with-mirror-filter-service-not-found.out.yaml

this 2 similar conditions here

Backend service validation failed: Service default/service-unknown

&&

Service default/service-unknown not found

Would it be better to provide a static message explaining why the status is not accepted, such as “invalid configuration detected”? This approach would avoid repeating the message in the accepted status reason. The types, such as “Accepted” and “ResolvedRefs,” are already distinct.

I think its fine for this PR

arkodg · 2024-10-09T01:05:17Z

internal/gatewayapi/testdata/httproute-with-mirror-filter-service-no-port.out.yaml

similar comment as https://github.com/envoyproxy/gateway/pull/4363/files#r1792663080

internal/gatewayapi/route.go

internal/gatewayapi/backendtlspolicy.go

arkodg · 2024-10-09T01:34:56Z

internal/gatewayapi/route.go

curious why we need to change the status generation logic

To clarify why the traffic cannot be routed, previously, the route was accepted, and the traffic was considered routable before this change.

trying to better understand why do we need an additional check here, isnt if err != nil enough ?

At this stage, it is already possible that a route is not applicable due to other reasons. This check attempts to retain the original reason, as it may be more specific. However, if that is the case, no traffic would be routed regardless to the settings validations errors.

can you share an example of what the status looks like with and w/o this change ?

Actually, this does not seem to happen, I did not notice any different translations when this extra condition is not in place. I have removed it.

arkodg · 2024-10-09T01:35:17Z

internal/gatewayapi/route.go

same as https://github.com/envoyproxy/gateway/pull/4363/files#r1792677269

arkodg · 2024-10-09T01:51:13Z

internal/gatewayapi/route.go

this isnt added for HTTPRoute and GRPCRoute but is added for UDP and TCP ?

For HTTP and TCP routes, we currently accept the route but return a 500 response. However, for UDP and TCP routes, the suggestion is to reject and not translate it at all incase of errors, and indicate this via the route status as traffic won't be routable at all.

arkodg · 2024-10-09T01:51:20Z

internal/gatewayapi/route.go

same as above

internal/gatewayapi/backendtlspolicy.go

arkodg · 2024-10-17T21:17:25Z

@alexwo I dont see the conformance tests passing. I'm worried about the size of this change that not only applies to BTP but every error while translating route rules and backendRefs. For example if one out of the many backendRefs is invalid, only a portion of the requests should receive a 503
https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.HTTPBackendRef

alexwo · 2024-10-18T08:16:31Z

@alexwo I dont see the conformance tests passing. I'm worried about the size of this change that not only applies to BTP but every error while translating route rules and backendRefs. For example if one out of the many backendRefs is invalid, only a portion of the requests should receive a 503 https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.HTTPBackendRef

@arkodg indeed, the impact of the change is large and it will render out many of the partial working, not fully configured routes, I can fully relate to it.

We should be covered as long as many of the necessary tests should be in place,what can detect potential issues.

The conformance tests are passing, but there was a linting issue previously that prevented them from running.

Now, if a part of the backend reference configuration is invalid — for example, if one of the associated configurations cannot be processed — the key question is: should routing to this backend reference still occur in such cases?

alexwo · 2024-10-18T13:51:02Z

/retest

arkodg · 2024-10-18T22:02:49Z

Now, if a part of the backend reference configuration is invalid — for example, if one of the associated configurations cannot be processed — the key question is: should routing to this backend reference still occur in such cases?

According to the spec, the answer is a portion of the requests (based on weights) should continue to be sent to the valid backendRef

alexwo · 2024-10-19T09:18:45Z

Now, if a part of the backend reference configuration is invalid — for example, if one of the associated configurations cannot be processed — the key question is: should routing to this backend reference still occur in such cases?

According to the spec, the answer is a portion of the requests (based on weights) should continue to be sent to the valid backendRef

We’re processing all backend routes and addressing only falty configured ones. This should ensures alignment with the spec, as valid backend routes will continue to function, while only those with invalid configurations will be skipped / not translated.
( we process all routes for each backend and go over all routes, even if some one the backend routes did have errors . )

alexwo · 2024-10-20T10:25:59Z

/retest

github-actions · 2024-11-19T12:03:38Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

arkodg

LGTM thanks !

guydc

LGTM!
@alexwo - can you update the PR?

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo · 2024-12-07T21:13:24Z

/retest

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo · 2024-12-07T21:47:17Z

/retest

alexwo · 2024-12-07T21:57:21Z

/retest

alexwo · 2024-12-08T05:46:25Z

/retest

alexwo requested a review from a team as a code owner September 29, 2024 12:20

alexwo changed the title ~~return 500 when backtls policy config fails~~ Return 500 when BackendTLSPolicy translation fails #4087 Sep 29, 2024

zirain changed the title ~~Return 500 when BackendTLSPolicy translation fails #4087~~ bugfix: return 500 when BackendTLSPolicy translation fails Sep 29, 2024

alexwo changed the title ~~bugfix: return 500 when BackendTLSPolicy translation fails~~ refactor: return 500 when BackendTLSPolicy translation fails Sep 29, 2024

zhaohuabing reviewed Sep 30, 2024

View reviewed changes

guydc previously approved these changes Sep 30, 2024

View reviewed changes

alexwo dismissed guydc’s stale review via 4a4750f October 1, 2024 03:34

alexwo force-pushed the tlspolicy500 branch from 7b0f7ab to 24eaa95 Compare October 1, 2024 12:59

arkodg requested review from guydc and zhaohuabing October 8, 2024 05:20

guydc added the release-note Indicates a required release note label Oct 8, 2024

guydc previously approved these changes Oct 8, 2024

View reviewed changes

arkodg reviewed Oct 9, 2024

View reviewed changes

internal/gatewayapi/route.go Outdated Show resolved Hide resolved

arkodg reviewed Oct 9, 2024

View reviewed changes

internal/gatewayapi/backendtlspolicy.go Outdated Show resolved Hide resolved

arkodg reviewed Oct 9, 2024

View reviewed changes

internal/gatewayapi/route.go Outdated

Copy link
Copy Markdown

Contributor

arkodg Oct 9, 2024

Choose a reason for hiding this comment

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

same as https://github.com/envoyproxy/gateway/pull/4363/files#r1792677269

arkodg reviewed Oct 9, 2024

View reviewed changes

internal/gatewayapi/route.go Outdated

Copy link
Copy Markdown

Contributor

arkodg Oct 9, 2024

Choose a reason for hiding this comment

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

same as above

alexwo dismissed guydc’s stale review via 7248531 October 9, 2024 02:42

zhaohuabing reviewed Oct 15, 2024

View reviewed changes

internal/gatewayapi/backendtlspolicy.go Outdated Show resolved Hide resolved

zhaohuabing reviewed Oct 15, 2024

View reviewed changes

internal/gatewayapi/backendtlspolicy.go Outdated Show resolved Hide resolved

zhaohuabing reviewed Oct 15, 2024

View reviewed changes

internal/gatewayapi/backendtlspolicy.go Outdated Show resolved Hide resolved

alexwo force-pushed the tlspolicy500 branch from 85c8198 to 48b61b7 Compare October 15, 2024 13:54

github-actions bot added the stale label Nov 19, 2024

arkodg previously approved these changes Dec 4, 2024

View reviewed changes

guydc previously approved these changes Dec 4, 2024

View reviewed changes

github-actions bot removed the stale label Dec 5, 2024

updating

f71e441

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo dismissed stale reviews from guydc and arkodg via f71e441 December 7, 2024 20:28

alexwo force-pushed the tlspolicy500 branch from e5b0dcd to f71e441 Compare December 7, 2024 20:28

updating

1263d48

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

updating

c2c8b4b

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

zirain approved these changes Dec 8, 2024

View reviewed changes

zirain requested review from arkodg and guydc December 8, 2024 11:47

arkodg merged commit 05ee5f4 into envoyproxy:main Dec 8, 2024

This was referenced Nov 11, 2025

1 Invalid BackendRef causes 500 for all requests #7483

Closed

fix: do not return 500 for all requests when part of BackendRefs are invalid #7488

Merged

Conversation

alexwo commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexwo commented Sep 29, 2024

Uh oh!

zhaohuabing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwo Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwo Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guydc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwo commented Oct 1, 2024

Uh oh!

alexwo commented Oct 1, 2024

Uh oh!

alexwo commented Oct 1, 2024

Uh oh!

alexwo commented Oct 2, 2024

Uh oh!

alexwo commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwo Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwo Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwo commented Sep 29, 2024 •

edited

Loading

codecov bot commented Sep 29, 2024 •

edited

Loading

alexwo Sep 30, 2024 •

edited

Loading

alexwo Oct 1, 2024 •

edited

Loading

alexwo Oct 9, 2024 •

edited

Loading

alexwo Oct 16, 2024 •

edited

Loading

alexwo commented Oct 18, 2024 •

edited

Loading

alexwo commented Oct 19, 2024 •

edited

Loading