Return 500 responses for invalid backends by Alice-Lilith · Pull Request #443 · envoyproxy/gateway

Alice-Lilith · 2022-09-28T01:46:18Z

When a HTTPRoute has invalid backends we need to return 500 responses for gateway API conformance. If there are no valid backends at all then a direct response is issued with status 500, if there is a mix of valid and invalid backends then a weighted cluster is used.

resolves #162

internal/gatewayapi/translator.go

internal/xds/translator/route.go

codecov-commenter · 2022-09-28T02:22:11Z

Codecov Report

Merging #443 (6b328da) into main (3da08fe) will increase coverage by 2.00%.
The diff coverage is 97.84%.

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   60.08%   62.09%   +2.00%     
==========================================
  Files          40       40              
  Lines        4347     4406      +59     
==========================================
+ Hits         2612     2736     +124     
+ Misses       1583     1525      -58     
+ Partials      152      145       -7

Impacted Files	Coverage Δ
internal/ir/xds.go	`85.71% <ø> (ø)`
internal/ir/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
internal/xds/translator/translator.go	`63.29% <33.33%> (-1.65%)`	⬇️
internal/gatewayapi/translator.go	`91.09% <100.00%> (+5.01%)`	⬆️
internal/xds/translator/route.go	`60.84% <100.00%> (+20.72%)`	⬆️
internal/provider/kubernetes/gatewayclass.go	`67.50% <0.00%> (+3.33%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

internal/xds/translator/route.go

internal/gatewayapi/testdata/httproute-with-some-invalid-backend-refs-no-service.out.yaml

arkodg · 2022-09-28T21:35:22Z

can you also add the appropriate conformance test to

gateway/test/conformance/conformance_test.go

Line 41 in 3da08fe

egTests := []suite.ConformanceTest{

arkodg · 2022-09-28T21:40:46Z

can you also add the appropriate conformance test to

gateway/test/conformance/conformance_test.go

Line 41 in 3da08fe

egTests := []suite.ConformanceTest{

#431 and #432 should have the names of the tests

Signed-off-by: AliceProxy <alicewasko@datawire.io>

Alice-Lilith · 2022-09-28T22:36:14Z

can you also add the appropriate conformance test to

gateway/test/conformance/conformance_test.go

Line 41 in 3da08fe

egTests := []suite.ConformanceTest{

#431 and #432 should have the names of the tests

That all I needed to do? Looks like they ran and passed. I checked the logs to confirm they really ran.

 --- PASS: TestGatewayAPIConformance/HTTPRouteInvalidNonExistentBackendRef (8.04s)

 --- PASS: TestGatewayAPIConformance/HTTPRouteInvalidNonExistentBackendRef/HTTPRoute_with_only_a_nonexistent_BackendRef_has_a_ResolvedRefs_Condition_with_status_False_and_Reason_BackendNotFound (0.00s)

--- PASS: TestGatewayAPIConformance/HTTPRouteInvalidNonExistentBackendRef/HTTP_Request_to_invalid_nonexistent_backend_receive_a_500 (0.01s)

--- PASS: TestGatewayAPIConformance/HTTPRouteInvalidBackendRefUnknownKind (7.06s)

--- PASS: TestGatewayAPIConformance/HTTPRouteInvalidBackendRefUnknownKind/HTTPRoute_with_Invalid_Kind_has_a_ResolvedRefs_Condition_with_status_False_and_Reason_InvalidKind (0.00s)

--- PASS: TestGatewayAPIConformance/HTTPRouteInvalidBackendRefUnknownKind/HTTP_Request_to_invalid_backend_with_invalid_Kind_receives_a_500 (0.00s)

arkodg

LGTM, thanks for adding this tricky feature !

Alice-Lilith · 2022-09-28T22:58:36Z

LGTM, thanks for adding this tricky feature !

Thanks for the many reviews after all my changes 😅

Alice-Lilith requested a review from a team as a code owner September 28, 2022 01:46