Aggregator uses the regular transport when handling upgrade requests #104985

caesarxuchao · 2021-09-14T00:02:54Z

/sig api-machinery
/kind bug
/assign
/release-note none

Currently aggregator creates a SPDY transport to the remote if the request contains upgrade headers. This seems wrong to me:

what if the request is trying to upgrade not to SPDY but to websocket?
more fundamentally, aggregator as a proxy should only care about the transport level connectivity. Aggregator should create a transport connection to the aggregated apiserver and then stitch it together with the connection between the client and the aggregator, just like how it treats a non-upgrade request.

This PR let's the aggregator use the original normal transport for upgrade requests. As a side-effect, this fixes #71808, which points out that that the SPDY transport doesn't honor the custom dialer in the original transport. The consequence of #71808 is that upgraded requests to aggregated APIs do not work if SSH tunnels or Konnectivity is used. A real life issue we met is that virtctl console(similar to kubectl exec) doesn't work when Konnectivity is used. We verified that this PR fixed the virtctl console use case.

Side note: how upgrade requests for aggregated apis worked with the SPDY transport when there was NO ssh tunnels or konnectivity involved

It worked because SPDY transport was only used here:

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/proxy/dial.go

Lines 38 to 41 in 35ae8c9

    
           func dialURL(ctx context.Context, url *url.URL, transport http.RoundTripper) (net.Conn, error) { 
        
           	dialAddr := netutil.CanonicalAddr(url) 
        
           	dialer, err := utilnet.DialerFor(transport)

utilnet.DialerFor will return an empty dialer and error, because it doesn't recognize a SPDY transport. dialURL then uses the default golang net.Dialer. So the SPDY transport never actually gets used.

(Supersedes #104763)

Fixes #71808

fedebongio · 2021-09-14T20:18:30Z

/triage accepted

fedebongio · 2021-09-14T20:18:39Z

/cc @deads2k

caesarxuchao · 2021-09-17T21:16:49Z

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

@@ -178,6 +174,8 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	}

 	handler := proxy.NewUpgradeAwareHandler(location, proxyRoundTripper, true, upgrade, &responder{w: w})
+	handler.InterceptRedirects = utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StreamingProxyRedirects)
+	handler.RequireSameHostRedirects = utilfeature.DefaultFeatureGate.Enabled(genericfeatures.ValidateProxyRedirects)


These two flags only affect upgraded requests. I added them here because the removed maybeWrapForConnectionUpgrades function used to set them for the SPDY transport.

They should have no effect as they are deprecated in 1.22 (kubernetes/enhancements#1558). I keep them just for consistency.

caesarxuchao · 2021-09-17T21:20:02Z

/assign @deads2k @sttts

/cc @cheftako

jkh52 · 2021-09-17T23:36:34Z

/cc @jkh52

…grades

deads2k · 2021-09-23T16:03:47Z

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

-	followRedirects := utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StreamingProxyRedirects)
-	requireSameHostRedirects := utilfeature.DefaultFeatureGate.Enabled(genericfeatures.ValidateProxyRedirects)
-	upgradeRoundTripper := spdy.NewRoundTripper(tlsConfig, followRedirects, requireSameHostRedirects)
-	wrappedRT, err := restclient.HTTPWrappersForConfig(restConfig, upgradeRoundTripper)


doesn't this call set up the front proxy headers for the connections we re-establish?

Is the front proxy header the same as the auth proxy headers that are set here? Since we now use the same proxyRoundTripper to handle both upgrade/non-upgrade requests, those auth proxy headers are applied to upgrade requests as well.

Thanks. yes, that's the one I was looking for.

aojea · 2021-09-24T11:14:04Z

more fundamentally, aggregator as a proxy should only care about the transport level connectivity. Aggregator should create a transport connection to the aggregated apiserver and then stitch it together with the connection between the client and the aggregator, just like how it treats a non-upgrade request.

I think that this is the key, no?
for learning purposes, why and or when aggregator -> aggregated should use SPDY?

caesarxuchao · 2021-09-27T18:21:38Z

@aojea I don't think aggregator->aggregated needs SPDY at any time. Even if the client request contains upgrade headers, aggregator's job is i) establishing a transport layer connection to the aggregated API server, ii) using http hijacking to get control of the transport layer connection from the client, and iii) then stitching these two connections together to form a tunnel.

Aggregator itself does not need to understand SPDY. Only the client and the aggregated API server need to speak SPDY.

deads2k · 2021-09-27T18:40:25Z

/lgtm
/approve

k8s-ci-robot · 2021-09-27T18:43:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, deads2k

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/kube-aggregator/OWNERS~~ [deads2k]

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

aojea · 2021-09-27T21:05:42Z

it's missing the release note @caesarxuchao :)

caesarxuchao · 2021-09-27T21:09:15Z

Thanks, @aojea
/release-note-none

liggitt · 2021-09-29T21:06:38Z

shouldn't there be a test exercising the issue this is fixing?

caesarxuchao · 2021-09-29T23:04:23Z

Yeah, there should be a test :)

I had thought of two ways to test this but both seemed to be a lot of work.

Approach 1, we expand the sampleapiserver to handle websocket/spdy, then we exercise the upgrades in the existing aggregator e2e tests.

Approach 2, in the e2e test, we deploy the kubevirt aggregated server and verifies if the virtctl console works. This is basically reproducing the issue in #97763.

I think approach 1 makes more sense. Definitely please let me know if you see a cheaper 3rd option.

liggitt · 2021-09-30T03:28:08Z

we've done upgrade proxy testing before in unit tests (e.g. in staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go)... my first thought would be to try to capture the heart of the bug in a unit test that exercises both the custom dialer (should fail on master) and direct dialer (should pass on master) cases, then make sure this PR fixes the custom dialer case and doesn't break the direct dialer case

aojea · 2021-09-30T06:56:02Z

we've done upgrade proxy testing before in unit tests (e.g. in staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go)... my first thought would be to try to capture the heart of the bug in a unit test that exercises both the custom dialer (should fail on master) and direct dialer (should pass on master) cases, then make sure this PR fixes the custom dialer case and doesn't break the direct dialer case

@liggitt @caesarxuchao let me see if I can do it, that sounds like a good opportunity to me to get familiar with this code

caesarxuchao · 2021-10-01T23:18:21Z

@aojea thanks for offering help. Free free to ping me on Slack (handler is also "caesarxuchao") when you have a PR.

liggitt · 2021-10-08T14:02:41Z

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

-	}
+	proxyRoundTripper := handlingInfo.proxyRoundTripper
+	upgrade := httpstream.IsUpgradeRequest(req)
+
 	proxyRoundTripper = transport.NewAuthProxyRoundTripper(user.GetName(), user.GetGroups(), user.GetExtra(), proxyRoundTripper)

 	// if we are upgrading, then the upgrade path tries to use this request with the TLS config we provide, but it does


is this comment / block still accurate with this change? commenting out the transport.SetAuthProxyHeaders line doesn't fail any unit tests before or after this PR, so it's unclear whether it is needed / effective

liggitt · 2021-10-11T14:02:36Z

If this PR is picked to 1.22, the tests added in #105475 and #105582 should be included as well

…-#104985-#105475-#105582-upstream-release-1.21 Automated cherry pick of #104985: Aggregator uses the regular transport even if the request #105475: apiserver aggregator upgrade unit test #105582: Verifying the auth headers are set for upgraded aggregated

…-#104985-#105475-#105582-upstream-release-1.22 Automated cherry pick of #104985: Aggregator uses the regular transport even if the request #105475: apiserver aggregator upgrade unit test #105582: Verifying the auth headers are set for upgraded aggregated

k8s-ci-robot assigned caesarxuchao Sep 14, 2021

k8s-ci-robot requested review from deads2k and sttts September 14, 2021 00:06

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 14, 2021

caesarxuchao changed the title ~~[WIP] Aggregator uses the regular transport even if the request requires upgrades~~ \Aggregator uses the regular transport even if the request requires upgrades Sep 17, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2021

caesarxuchao force-pushed the aggregator-no-spdy branch from 4843ab7 to 2f40c5b Compare September 17, 2021 21:12

caesarxuchao changed the title ~~\Aggregator uses the regular transport even if the request requires upgrades~~ Aggregator uses the regular transport even if the request requires upgrades Sep 17, 2021

caesarxuchao commented Sep 17, 2021

View reviewed changes

k8s-ci-robot assigned deads2k and sttts Sep 17, 2021

k8s-ci-robot requested a review from cheftako September 17, 2021 21:20

caesarxuchao changed the title ~~Aggregator uses the regular transport even if the request requires upgrades~~ Aggregator uses the regular transport when handling upgrade requests Sep 17, 2021

k8s-ci-robot requested a review from jkh52 September 17, 2021 23:36

Aggregator uses the regular transport even if the request requires up…

224bcf6

…grades

caesarxuchao force-pushed the aggregator-no-spdy branch from 2f40c5b to 224bcf6 Compare September 17, 2021 23:50

k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Sep 17, 2021

k8s-ci-robot assigned aojea and p0lyn0mial Sep 22, 2021

deads2k reviewed Sep 23, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2021

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 27, 2021

k8s-ci-robot merged commit c647c56 into kubernetes:master Sep 27, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 27, 2021

liggitt mentioned this pull request Sep 29, 2021

ConnectionUpgrades in kube-aggregator API does not use the given roundtripper #97763

Closed

aojea mentioned this pull request Oct 4, 2021

apiserver aggregator upgrade unit test #105475

Merged

liggitt reviewed Oct 8, 2021

View reviewed changes

caesarxuchao mentioned this pull request Oct 9, 2021

Verifying the auth headers are set for upgraded aggregated API requests #105582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregator uses the regular transport when handling upgrade requests #104985

Aggregator uses the regular transport when handling upgrade requests #104985

caesarxuchao commented Sep 14, 2021 •

edited

fedebongio commented Sep 14, 2021

fedebongio commented Sep 14, 2021

caesarxuchao Sep 17, 2021

caesarxuchao commented Sep 17, 2021

jkh52 commented Sep 17, 2021

deads2k Sep 23, 2021

caesarxuchao Sep 23, 2021

deads2k Sep 27, 2021

aojea commented Sep 24, 2021

caesarxuchao commented Sep 27, 2021

deads2k commented Sep 27, 2021

k8s-ci-robot commented Sep 27, 2021

aojea commented Sep 27, 2021

caesarxuchao commented Sep 27, 2021

liggitt commented Sep 29, 2021

caesarxuchao commented Sep 29, 2021

liggitt commented Sep 30, 2021

aojea commented Sep 30, 2021

caesarxuchao commented Oct 1, 2021

liggitt Oct 8, 2021

liggitt commented Oct 11, 2021

	func dialURL(ctx context.Context, url *url.URL, transport http.RoundTripper) (net.Conn, error) {
	dialAddr := netutil.CanonicalAddr(url)

	dialer, err := utilnet.DialerFor(transport)

Aggregator uses the regular transport when handling upgrade requests #104985

Aggregator uses the regular transport when handling upgrade requests #104985

Conversation

caesarxuchao commented Sep 14, 2021 • edited

Side note: how upgrade requests for aggregated apis worked with the SPDY transport when there was NO ssh tunnels or konnectivity involved

fedebongio commented Sep 14, 2021

fedebongio commented Sep 14, 2021

caesarxuchao Sep 17, 2021

Choose a reason for hiding this comment

caesarxuchao commented Sep 17, 2021

jkh52 commented Sep 17, 2021

deads2k Sep 23, 2021

Choose a reason for hiding this comment

caesarxuchao Sep 23, 2021

Choose a reason for hiding this comment

deads2k Sep 27, 2021

Choose a reason for hiding this comment

aojea commented Sep 24, 2021

caesarxuchao commented Sep 27, 2021

deads2k commented Sep 27, 2021

k8s-ci-robot commented Sep 27, 2021

aojea commented Sep 27, 2021

caesarxuchao commented Sep 27, 2021

liggitt commented Sep 29, 2021

caesarxuchao commented Sep 29, 2021

liggitt commented Sep 30, 2021

aojea commented Sep 30, 2021

caesarxuchao commented Oct 1, 2021

liggitt Oct 8, 2021

Choose a reason for hiding this comment

liggitt commented Oct 11, 2021

caesarxuchao commented Sep 14, 2021 •

edited