Add v5 WebSocket streaming protocol support (stdin close/EOF)#1725
Add v5 WebSocket streaming protocol support (stdin close/EOF)#1725
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
added
size/XS
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
k8s-ci-robot
added
size/L
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, Copilot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
There was a problem hiding this comment.
Pull request overview
Adds Kubernetes remotecommand v5 WebSocket streaming protocol support to allow per-channel close/EOF signaling (notably for stdin), extending the existing demux/mux stream implementation and adding test coverage.
Changes:
- Added v5 WebSocket subprotocol constants (
v5.channel.k8s.io,v5.base64.channel.k8s.io). - Extended
StreamDemuxer/IStreamDemuxerwith v5 channel-close support and inbound close-frame handling. - Updated
MuxedStream.Disposeto automatically send the v5 close frame when enabled; added new unit tests for v5 close behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/KubernetesClient/WebSocketProtocol.cs |
Adds v5 subprotocol constants for selecting the protocol at connect time. |
src/KubernetesClient/IStreamDemuxer.cs |
Extends the demuxer contract with CloseChannel overloads. |
src/KubernetesClient/StreamDemuxer.cs |
Implements sending/receiving v5 close-channel frames and exposes SupportsClose. |
src/KubernetesClient/MuxedStream.cs |
Sends close-channel on disposal of writable streams when v5 close is enabled. |
tests/KubernetesClient.Tests/StreamDemuxerTests.cs |
Adds tests covering send/receive close-channel and dispose behavior for v5 vs v4. |
Comments suppressed due to low confidence (1)
src/KubernetesClient/StreamDemuxer.cs:226
CloseChannelsends a v5 close frame unconditionally. WhenSupportsCloseis false (v4 protocol), this will still transmit the[255, index]message and can confuse/violate the negotiated subprotocol. Consider guarding this method (e.g., throwInvalidOperationExceptionor no-op) whenSupportsCloseis false, and document the behavior in the interface XML docs.
public async Task CloseChannel(byte index, CancellationToken cancellationToken = default)
{
var closeBuffer = new byte[] { ChannelCloseIndex, index };
await webSocket.SendAsync(new ArraySegment<byte>(closeBuffer), WebSocketMessageType.Binary, true, cancellationToken)
.ConfigureAwait(false);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var channelToClose = buffer[1]; | ||
| if (buffers.ContainsKey(channelToClose)) | ||
| { | ||
| buffers[channelToClose].WriteEnd(); | ||
| } |
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing && !disposed && outputIndex.HasValue && muxer != null && muxer.SupportsClose) | ||
| { | ||
| try | ||
| { | ||
| muxer.CloseChannel(outputIndex.Value).GetAwaiter().GetResult(); | ||
| } |
| stream.Dispose(); | ||
|
|
||
| // Wait a short time and confirm no message was sent. | ||
| await Task.Delay(50).ConfigureAwait(true); | ||
| Assert.Empty(sentMessages); |
| /// </returns> | ||
| public async Task CloseChannel(byte index, CancellationToken cancellationToken = default) | ||
| { | ||
| var closeBuffer = new byte[] { ChannelCloseIndex, index }; |
| if (SupportsClose && streamIndex == ChannelCloseIndex) | ||
| { | ||
| if (result.Count >= 2) | ||
| { | ||
| var channelToClose = buffer[1]; | ||
| if (buffers.ContainsKey(channelToClose)) | ||
| { | ||
| buffers[channelToClose].WriteEnd(); | ||
| } | ||
| } | ||
|
|
||
| continue; | ||
| } |
|
@copilot please address comments and fix the ci/cd failure |
…, test cleanup Agent-Logs-Url: https://github.com/kubernetes-client/csharp/sessions/f7ecce7d-42eb-4bcb-a761-de8b9a69956c Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
Addressed all five review comments in commit
The CI failure (MSBuild 17 not supporting .NET SDK 10 on the Windows runner) is a pre-existing infrastructure issue unrelated to these changes. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
The C# client only supported the v4 streaming protocol, which has no mechanism to signal EOF on stdin. The v5 protocol adds a dedicated close message (
[255, channelIndex]) to close individual channels, enabling proper stdin EOF when piping data to a container process.Changes
WebSocketProtocol.cs— AddedV5BinaryWebsocketProtocol("v5.channel.k8s.io") andV5Base64WebsocketProtocol("v5.base64.channel.k8s.io") constants.StreamDemuxer.cs— AddedsupportsCloseconstructor parameter +SupportsCloseproperty;CloseChannel(byte/ChannelIndex)method that sends[255, channelIndex];RunLoophandles incoming v5 close messages underlock(buffers)and tracksclosedChannelsso lateGetStreamcallers get immediate EOF;Writenow setsendOfMessage: trueon the final (or only) chunk so close frames are always standalone WebSocket messages.IStreamDemuxer.cs— AddedCloseChanneloverloads to the interface.MuxedStream.cs— OverridesDispose(bool)to send the v5 close frame whenSupportsCloseis true; also implementsIAsyncDisposablewithDisposeAsync()to allow callers to await the channel-close without blocking.Usage
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.