Skip to content

Return 500 responses for invalid backends #443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 29, 2022
Merged

Return 500 responses for invalid backends #443

merged 5 commits into from
Sep 29, 2022

Conversation

Alice-Lilith
Copy link
Member

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

@Alice-Lilith Alice-Lilith requested a review from a team as a code owner September 28, 2022 01:46
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2022

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.

@arkodg
Copy link
Contributor

arkodg commented Sep 28, 2022

can you also add the appropriate conformance test to

egTests := []suite.ConformanceTest{

@arkodg
Copy link
Contributor

arkodg commented Sep 28, 2022

can you also add the appropriate conformance test to

egTests := []suite.ConformanceTest{

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

AliceProxy added 5 commits September 28, 2022 15:05
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
@Alice-Lilith
Copy link
Member Author

can you also add the appropriate conformance test to

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)

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 for adding this tricky feature !

@Alice-Lilith
Copy link
Member Author

LGTM, thanks for adding this tricky feature !

Thanks for the many reviews after all my changes 😅

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.

return 500's for invalid HTTPRoute backend refs
5 participants