refactor: return 500 when BackendTLSPolicy translation fails #4363

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

@@ -310,7 +286,6 @@ xdsIR:
              - ECDSA-SECP256R1-SHA256
              sni: example.com
              useSystemTrustStore: true
-            weight: 1


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

@@ -26,28 +26,30 @@ import (

 func (t *Translator) validateBackendRef(backendRefContext BackendRefContext, parentRef *RouteParentContext, route RouteContext,


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

@@ -121,8 +121,6 @@ xdsIR:
      routes:
      - destination:
          name: tlsroute/default/tlsroute-1/rule/-1


should the destination exist in this case?

nope, updated.

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

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

-        message: Route is accepted
-        reason: Accepted
-        status: "True"
+        message: 'backend reference validation failed: backend is not supported for


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

@@ -121,8 +121,6 @@ xdsIR:
      routes:
      - destination:
          name: tlsroute/default/tlsroute-1/rule/-1
-          settings:


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

-        message: Route is accepted
-        reason: Accepted
-        status: "True"
+        message: 'Backend service validation failed: Service default/service-unknown


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

-        status: "True"
+        message: 'Error validating backend port: port number not specified for backend
+          reference.'
+        reason: UnsupportedValue


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

@@ -112,7 +112,7 @@ func (t *Translator) processHTTPRouteParentRefs(httpRoute *HTTPRouteContext, res
 		// any conditions that come out of it have to go on each RouteParentStatus,
 		// not on the Route as a whole.
 		routeRoutes, err := t.processHTTPRouteRules(httpRoute, parentRef, resources)
-		if err != nil {
+		if err != nil && !parentRef.HasCondition(httpRoute, gwapiv1.RouteConditionAccepted, metav1.ConditionFalse) {


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

@@ -470,7 +479,7 @@ func (t *Translator) processGRPCRouteParentRefs(grpcRoute *GRPCRouteContext, res
 		// any conditions that come out of it have to go on each RouteParentStatus,
 		// not on the Route as a whole.
 		routeRoutes, err := t.processGRPCRouteRules(grpcRoute, parentRef, resources)
-		if err != nil {
+		if err != nil && !parentRef.HasCondition(grpcRoute, gwapiv1.RouteConditionAccepted, metav1.ConditionFalse) {


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

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

internal/gatewayapi/route.go

+			ds, err := t.processDestination(backendRef, parentRef, udpRoute, resources)
+			// skip adding the route and provide the reason via route status.
+
+			if err != nil && !parentRef.HasCondition(udpRoute, gwapiv1.RouteConditionAccepted, metav1.ConditionFalse) {


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

+					gwapiv1.RouteConditionAccepted,
+					metav1.ConditionFalse,
+					"Failed to process the settings associated with the TCP route.",
+					err.Error(),


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 <[email protected]>

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

/retest

Signed-off-by: Alexander Volchok <[email protected]>

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 zhaohuabing and guydc 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

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