bug fix - remove port bytes during port forwarding #233
Conversation
when multiple call to GetStream happens around the same time, on the same inputIndex, a race condition will cause this.buffers.Add() to throw exception.
In StreamDemuxer, if the buffer is created before connection is established, the port bytes are not removed when the bytes are delivering to the client. Repro code looks like (the key is to call remoteStreams.Start AFTER GetStream): var ws = await kubernetesClient.WebSocketNamespacedPodPortForwardAsync (... ) var remoteStreams = new StreamDemuxer(ws); var stream = remoteStreams.GetStream(0, 0); remoteStreams.Start(); This change filters out the port bytes which are the 2nd and 3rd bytes sent in all cases.
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xinyanmsft If they are not already assigned, you can assign the PR to them by writing 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
the
size/XS
xinyanmsft
changed the title
add an enum StreamType to StreamDemuxer so it knows whether / how the data stream should be handled.
k8s-ci-robot
added
size/M
| @@ -199,6 +204,13 @@ protected async Task RunLoop(CancellationToken cancellationToken) | |||
|
|
|||
| var streamIndex = buffer[0]; | |||
| var extraByteCount = 1; | |||
| byteCount += result.Count - extraByteCount; | |||
| if (this.streamType == StreamType.PortForward && byteCount <= 2) | |||
There was a problem hiding this comment.
I'm not sure this will work correctly, what if byteCount becomes greater than 2 immediately, you won't strip the first two bytes.
I think the right thing to do is to have a boolean skippedPort outside the loop and then clip the two bytes at the front if !skippedPort and then set skippedPort to true...
Eitherway, I think this deserves a unit test to make sure it's working.
Thanks!
There was a problem hiding this comment.
here's another pass. there was a mistake in my previous change where I didn't handle StreamDemuxer with multiple streams. Now there is a dictionary that holds the skipped bytes for each stream.
it turns out there is 0 StreamDemuxer tests. add StreamDemuxerTests that covers send / receive for both command and port forwarding. Added WebSocketMock as a helper.
k8s-ci-robot
added
size/XL
|
Awesome, thanks! |
In StreamDemuxer, if the buffer is created before connection is established, the port bytes are not removed when the bytes are delivering to the client. Repro code looks like (the key is to call remoteStreams.Start AFTER GetStream):
var ws = await kubernetesClient.WebSocketNamespacedPodPortForwardAsync (... )
var remoteStreams = new StreamDemuxer(ws);
var stream = remoteStreams.GetStream(0, 0);
remoteStreams.Start();
This change filters out the port bytes which are the 2nd and 3rd bytes sent in all cases.