fix: map proxy errors to correct HTTP status codes) #765
+87
−63
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/invalid-commit-message
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ipochi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
ipochi
force-pushed
the
imran/fix-error-response-http-connect
branch
from
619e98f to
939a08c
Compare
Previously, the apiserver-network-proxy returned a blanket 503 (Service Unavailable) status for all backend connection failures, which was incorrect for a proxy server. Additionally, HTTP CONNECT mode would send 200 OK immediately after hijacking, preventing proper error reporting but also violating HTTP CONNECT protocol This commit: - Adds mapDialErrorToHTTPStatus() to map TCP/network errors to appropriate HTTP status codes: * 502 Bad Gateway: connection refused, DNS failures, network unreachable * 503 Service Unavailable: resource exhaustion (too many open files) * 504 Gateway Timeout: I/O timeouts, deadline exceeded - Delays sending "200 Connection Established" until after successful backend connection in HTTP CONNECT mode - Ensures proper HTTP/1.1 protocol version in error responses - Preserves original error messages in response body for debugging Signed-off-by: Imran Pochi <[email protected]>
ipochi
force-pushed
the
imran/fix-error-response-http-connect
branch
from
939a08c to
4d6bd6b
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
Signed-off-by: Imran Pochi <[email protected]>
k8s-ci-robot
added
size/L
|
Looks like way back in commit #1 we considered returning structured error data: To avoid parsing strings in the server, maybe now is the time to add that to the client (and keep But in general, I don't feel that I'm the best reviewer for non-trivial http-connect mode changes because I am mostly emeritus, and never deeply ramped on on the http-connect side, only the grpc side. Maybe @cheftako or another? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the apiserver-network-proxy returned a blanket 503 (Service Unavailable) status for all backend connection failures, which was incorrect for a proxy server. Additionally, HTTP CONNECT mode would send 200 OK immediately after hijacking, preventing proper error reporting but also violating HTTP CONNECT protocol
This PR:
Fixes: #764