Transport.RoundTrip should return a non-nil for failure to obtain a response. #92217

p0lyn0mial · 2020-06-17T10:17:19Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind bug

What this PR does / why we need it: In general RoundTrip should not attempt to interpret the response. In particular, RoundTrip must return err == nil if it obtained a response, regardless of the response's HTTP status code. A non-nil err should be reserved for failure to obtain a response.

This allows higher-level layers to interpret the err and construct the final response to the client.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

p0lyn0mial · 2020-06-17T10:17:34Z

/assign @sttts

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go

p0lyn0mial · 2020-06-23T09:27:13Z

/retest

sttts · 2020-06-23T11:34:27Z

/lgtm
/approve

k8s-ci-robot · 2020-06-23T11:35:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, sttts

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:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [sttts]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2020-06-23T15:48:47Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-06-23T20:00:49Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

liggitt · 2020-06-23T21:31:28Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go

@@ -261,7 +261,7 @@ func TestProxyHandler(t *testing.T) {
 					},
 				},
 			},
-			expectedStatusCode: http.StatusServiceUnavailable,


I would still expect this to surface a 503 error to the caller

/hold

This particular test case fails due to incorrect APIService configuration, that leads to error trying to reach service: x509: certificate is valid for test-service.test-ns.svc, not ..svc. It sounds like a configuration error because at this point we don't know if the server we are trying to reach is available.

from a consistency perspective, if the periodic healthcheck for the APIService encountered this cert error, it would flip the APIService to unavailable, and this code would return a 503:

kubernetes/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

Lines 125 to 128 in e2d8f6c

if !handlingInfo.serviceAvailable {

proxyError(w, req, "service unavailable", http.StatusServiceUnavailable)

return

}

It's unclear to me why the same error encountered by the proxy synchronously would surface a different error code to the requesting user

to preserve the old behaviour we could change the default responder https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go#L240 to return http.StatusServiceUnavailable, are you okay with that?

it seems that it's only used by the proxy - https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go#L178

done.

Now the default responder will return StatusServiceUnavailable, this aligns with the old behavior of the Transport. It looks like it safe to do so because the default responder wasn't wired before (https://github.com/kubernetes/kubernetes/pull/92217/files#diff-16a26bb0d342318c6eb3d03e6c5756e9R239)

that seems better to me, thanks. will let @sttts ack before merge

/hold cancel

…esponse.

p0lyn0mial · 2020-06-26T09:25:36Z

/test pull-kubernetes-integration

p0lyn0mial · 2020-06-26T10:58:56Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

+		// if an optional error interceptor/responder was provided wire it
+		// the custom responder might be used for providing a unified error reporting
+		// or supporting retry mechanisms by not sending non-fatal errors to the clients
+		proxy.ErrorHandler = h.Responder.Error


@liggitt FYI I'm going to use it in #92291 which introduces a retry mechanism for the aggregator. PTAL if you are interested.

sttts · 2020-07-01T09:04:06Z

/lgtm

k8s-ci-robot assigned sttts Jun 17, 2020

k8s-ci-robot requested review from derekwaynecarr and wojtek-t June 17, 2020 10:18

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 17, 2020

p0lyn0mial force-pushed the proxy-transport-pass-err branch 2 times, most recently from b018046 to 212f3cd Compare June 17, 2020 12:57

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 18, 2020

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 18, 2020

sttts reviewed Jun 18, 2020

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go Outdated Show resolved Hide resolved

sttts reviewed Jun 18, 2020

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go Show resolved Hide resolved

p0lyn0mial force-pushed the proxy-transport-pass-err branch from 212f3cd to 32a4ed6 Compare June 23, 2020 10:21

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 23, 2020

p0lyn0mial force-pushed the proxy-transport-pass-err branch from 32a4ed6 to f5978bc Compare June 23, 2020 11:33

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2020

liggitt reviewed Jun 23, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2020

Transport.RoundTrip should return a non-nil for failure to obtain a r…

c0593b6

…esponse.

p0lyn0mial force-pushed the proxy-transport-pass-err branch from f5978bc to c0593b6 Compare June 26, 2020 07:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2020

p0lyn0mial commented Jun 26, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2020

k8s-ci-robot merged commit c53ce48 into kubernetes:master Jul 1, 2020

k8s-ci-robot added this to the v1.19 milestone Jul 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transport.RoundTrip should return a non-nil for failure to obtain a response. #92217

Transport.RoundTrip should return a non-nil for failure to obtain a response. #92217

p0lyn0mial commented Jun 17, 2020

p0lyn0mial commented Jun 17, 2020

p0lyn0mial commented Jun 23, 2020

sttts commented Jun 23, 2020

k8s-ci-robot commented Jun 23, 2020

fejta-bot commented Jun 23, 2020

fejta-bot commented Jun 23, 2020

liggitt Jun 23, 2020

p0lyn0mial Jun 24, 2020

liggitt Jun 24, 2020 •

edited

p0lyn0mial Jun 24, 2020

p0lyn0mial Jun 26, 2020

liggitt Jun 29, 2020

p0lyn0mial commented Jun 26, 2020

p0lyn0mial Jun 26, 2020

sttts commented Jul 1, 2020

	if !handlingInfo.serviceAvailable {
	proxyError(w, req, "service unavailable", http.StatusServiceUnavailable)
	return
	}

Transport.RoundTrip should return a non-nil for failure to obtain a response. #92217

Transport.RoundTrip should return a non-nil for failure to obtain a response. #92217

Conversation

p0lyn0mial commented Jun 17, 2020

p0lyn0mial commented Jun 17, 2020

p0lyn0mial commented Jun 23, 2020

sttts commented Jun 23, 2020

k8s-ci-robot commented Jun 23, 2020

fejta-bot commented Jun 23, 2020

fejta-bot commented Jun 23, 2020

liggitt Jun 23, 2020

Choose a reason for hiding this comment

p0lyn0mial Jun 24, 2020

Choose a reason for hiding this comment

liggitt Jun 24, 2020 • edited

Choose a reason for hiding this comment

p0lyn0mial Jun 24, 2020

Choose a reason for hiding this comment

p0lyn0mial Jun 26, 2020

Choose a reason for hiding this comment

liggitt Jun 29, 2020

Choose a reason for hiding this comment

p0lyn0mial commented Jun 26, 2020

p0lyn0mial Jun 26, 2020

Choose a reason for hiding this comment

sttts commented Jul 1, 2020

liggitt Jun 24, 2020 •

edited