[bugfix] fix transport race conditions in yurthub #683
Conversation
|
@rambohe-ch: GitHub didn't allow me to assign the following users: your_reviewer. Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openyurt-bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rambohe-ch 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 |
| upgradeAwareHandler.UseRequestLocation = true | ||
| bearerUpgradeAwareHandler := proxy.NewUpgradeAwareHandler(remoteServer, bearerTransport, false, true, &responder{}) |
There was a problem hiding this comment.
The transport passed to NewUpgradeAwareHandler is assigned to UpgradeAwareHandler.Transport. It's used to handle non-upgrade request. So, it doesn't matter here. I think we should assign currentTransport and bearerTransport to UpgradeAwareHandler.UpgradeTransport.
There was a problem hiding this comment.
@Congrool From the code: https://github.com/kubernetes/apimachinery/blob/v0.18.8/pkg/util/proxy/upgradeaware.go#L398-L400
when UpgradeTransport is nil, UpgradeAwareHandler will use Transport to handle upgrade request.
There was a problem hiding this comment.
I think we can group upgradeAwareHandler and bearerUpgradeAwareHandler into one.
upgradeAwareHandler and bearerUpgradeAwareHandler here are used to distinguish bearer and non-bearer requests and use different transport for them. The RemoteProxy is exactly what we need:
func (rp *RemoteProxy) RoundTrip(req *http.Request) (*http.Response, error) {
// when edge client(like kube-proxy, flannel, etc) use service account(default InClusterConfig) to access yurthub,
// Authorization header will be set in request. and when edge client(like kubelet) use x509 certificate to access
// yurthub, Authorization header in request will be empty.
if isBearerRequest(req) {
return rp.bearerTransport.RoundTrip(req)
}
return rp.currentTransport.RoundTrip(req)
}
I think we can remove bearerUpgradeAwareHandler and use proxyBackend as the transport param in NewUpgradeAwareHandler.
The codes should look like:
proxyBackend := &RemoteProxy{
checker: healthChecker,
reverseProxy: httputil.NewSingleHostReverseProxy(remoteServer),
cacheMgr: cacheMgr,
remoteServer: remoteServer,
filterChain: filterChain,
currentTransport: currentTransport,
bearerTransport: bearerTransport,
stopCh: stopCh,
}
upgradeAwareHandler := proxy.NewUpgradeAwareHandler(remoteServer, proxyBackend, false, true, &responder{})
upgradeAwareHandler.UseRequestLocation = true
proxyBackend.reverseProxy.Transport = proxyBackend
proxyBackend.reverseProxy.ModifyResponse = proxyBackend.modifyResponse
proxyBackend.reverseProxy.FlushInterval = -1
proxyBackend.reverseProxy.ErrorHandler = proxyBackend.errorHandler
proxyBackend.upgradeHandler = upgradeAwareHandler
There was a problem hiding this comment.
@DrmagicE A good idea, i will modify it.
There was a problem hiding this comment.
@DrmagicE I have checked the codes of NewUpgradeAwareHandler, and only *http.Transport as http. RoundTripper is supported, so use proxyBackend as http. RoundTripper can not work.
maybe we need to leave the current changed unchanged.
the code link is here: https://github.com/kubernetes/apimachinery/blob/master/pkg/util/net/http.go#L215-L239
There was a problem hiding this comment.
@rambohe-ch Oh, what a pity. I got it.
| } else { | ||
| rp.upgradeHandler.UpgradeTransport = proxy.NewUpgradeRequestRoundTripper(rp.currentTransport, proxy.MirrorRequest) | ||
| rp.upgradeHandler.ServeHTTP(rw, req) |
rambohe-ch
force-pushed
the
fix-race-condition
branch
from
6220d7b to
d00ff78
Compare
rambohe-ch
changed the title
openyurt-bot
removed
the
do-not-merge/work-in-progress
| @@ -106,21 +110,13 @@ func (rp *RemoteProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { | |||
| if httpstream.IsUpgradeRequest(req) { | |||
| klog.V(5).Infof("get upgrade request %s", req.URL) | |||
| if isBearerRequest(req) { | |||
There was a problem hiding this comment.
As mentioned in https://github.com/openyurtio/openyurt/pull/683/files#r769771572, if we group upgradeAwareHandler and bearerUpgradeAwareHandler into one, the codes here can be simplified as follow:
if httpstream.IsUpgradeRequest(req) {
klog.V(5).Infof("get upgrade request %s", req.URL)
rp.upgradeHandler.ServeHTTP(rw, req)
return
}
rp.reverseProxy.ServeHTTP(rw, req)
|
/lgtm |
What type of PR is this?
/kind bug
What this PR does / why we need it:
reconstruct transport setting in
RemoteProxyof yurthub component for preventing race conditions.bearerUpgradeHandlerand existingupgradeHandlerforConnection: Upgraderequest.RoundTrip()func forRemoteProxyto group transport handler.Which issue(s) this PR fixes:
Fixes #674
Special notes for your reviewer:
/assign @DrmagicE
Does this PR introduce a user-facing change?
other Note