Add support for executing commands within a container #63

qmfrederik · 2017-12-02T22:36:00Z

This PR adds support for the equivalent of kubectl exec and allows you to run commands within a container.

I've added a exec sample application which is a very basic attempt to replicate kubectl exec using the C# Kubernetes client.

From a code point of view:

Adds a IKubernetes.WebSocketNamespacedPodExecWithHttpMessagesAsync method which is very similar to IKubernetes.ConnectGetNamespacedPodExecWithHttpMessagesAsync but returns a ClientWebSocket, allowing callers to interact with the process running on the container
Adds a ExecClient convenience class which wraps around ClientWebSocket and provides access to the stdin, stdout and stderr streams.

There are important gotchas related to ClientWebSocket, the WebSocket client which ships as part of .NET Core:

ClientWebSocket is not fully implemented in .NET Core 2.0. Most importantly, the ClientWebSocket.Options are ignored - this is where you set the authorization header or specify the client credentials. Long story short: you need the latest preview bits of .NET Core for this to work. See dotnet/corefx#5120
There was a bug in ClientWebSocket which prevented the client & server on agreeing on the WebSockets subprotocol. This was fixed recently in .NET Core - see Fix subprotocol check in WebSocketHandle.Managed dotnet/corefx#25645
The ClientWebSocket implementation on Linux is fairly complete; this is not the case on Windows. The Linux implementation is a managed implementation which works on Windows, too. See dotnet/corefx#5120 for how you can use the managed implementation on Windows.

I think it's still safe to merge this code as:

This PR doesn't introduce any dependency on a pre-production NuGet packages
Users using the latest corefx bits (me!) would be able to execute commands, something they can't do now.

brendandburns · 2017-12-04T05:03:46Z

examples/exec/NuGet.config

@@ -0,0 +1,5 @@
+<configuration>


Do we want to add this file?

If you want the 'F5' experience to work with this example, then yes.

ClientWebSocket only works properly with corefx 2.1 which has dotnet/corefx#25645; so very recent nightly builds. If you don't target netcoreapp2.1 the code will compile but you'll hit a bug in .NET Core so the sample won't work.

Once there's a public preview of .NET Core 2.1 this can be removed.

Well, turns out Travis doesn't have .NET Core 2.1 (which is reasonable 😄 ), this caused the build to break so I reverted this change for now.

brendandburns · 2017-12-04T05:04:39Z

examples/exec/exec.csproj

+    <ProjectReference Include="..\..\src\KubernetesClient.csproj" />
+  </ItemGroup>
+
+  <ItemGroup>


What does this do? Do we want this in here?

What if that file doesn't exist?

We don't need it, so I removed the reference.

brendandburns · 2017-12-04T05:06:39Z

src/ExecClient.cs

+            var commandLength = Encoding.UTF8.GetByteCount(command);
+            var buffer = ArrayPool<byte>.Shared.Rent(commandLength + 4);
+
+            try


nit: may as well move this try down to around the send, right?

The try/finally protects buffer which comes from the shared array pool. The code that initializes the first 4 bytes of the buffer won't fail, but Encoding.UTF8.GetBytes may throw an exception. So if we move the try down to SendAsync, we may leak a buffer if GetBytes fails.
(It's very unlikely it will since we already call Encoding.UTF8.GetByteCount)

I can move it down; but because the try is there make sure we return buffer once we're done with it, I prefer to keep it close to buffer.

I don't have a strong preference, though, so can always move it down if you prefer it that way.

brendandburns · 2017-12-04T05:07:30Z

src/ExecClient.cs

+
+            while (!cancellationToken.IsCancellationRequested && this.socket.CloseStatus == null)
+            {
+                // We always get data in this format:


Ultimately we're going to want to pull this logic out so that we can use it for Attach and PortForward, but we can do that in a different PR...

Yeah, I didn't have the time to look at the protocol for Attach yet (not sure whether it uses stream multiplexing or not).

I had a cursory look at PortForward and I believe it's simpler - there is no multiplexing so you can just use ReceiveAsync/SendAsync to talk with the remote socket.
You may want to add code which wraps the ClientWebSocket in Stream-like class, though.

That said, one thing at a time 😄

brendandburns · 2017-12-04T05:07:59Z

src/ExecClient.cs

+
+                switch (streamType)
+                {
+                    case 1:


Can you make 1 and 2 constants?

brendandburns · 2017-12-04T05:12:42Z

This looks great! Thanks for doing it (and the thorough investigation...)

Some minor nits, and you need to sign the CLA, then I think this is good to merge!

Thanks again!

qmfrederik · 2017-12-04T09:53:40Z

@brendanburns You're welcome, I updated the code/responded to your comments, the CLA status should also be OK now.

Let me know if you need anything else!

tg123 · 2017-12-04T10:15:30Z

Cloud you please also attach some testcases and also have a look at my suggestion about channel length

qmfrederik · 2017-12-04T10:26:38Z

@tg123 I'll try to add some unit tests.

I'm sorry - I may have missed your comment about channel length. Can you let me know where you left that comment (or repeat it here)?

tg123 · 2017-12-03T05:55:41Z

examples/exec/Program.cs

+            var pod = args[0];
+            var container = args.Length > 1 ? args[1] : null;
+
+            var k8sClientConfig = KubernetesClientConfiguration.BuildConfigFromConfigFile(kubeconfigPath: "kube-config.yml");


cloud you please use ctor() which use $HOME/.kube ?

Thx, I fixed this.

tg123 · 2017-12-03T19:47:26Z

src/ExecClient.cs

+
+namespace k8s
+{
+    public class ExecClient : IDisposable


attach/ exec / port-forward are share the same streaming protocol, it is better to have shared websocket handler like java client does

tg123 · 2017-12-03T20:23:43Z

src/ExecClient.cs

+            try
+            {
+                // The first 4 bytes represent the stream index. For stdin this is 0
+                buffer[0] = 0;


are you sure channel is 4 bytes? not 1 byte?

channel define here
https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#websockets-and-spdy

java client impl:
https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/util/WebSocketStreamHandler.java#L175

python client impl:
https://github.com/kubernetes-client/python-base/blob/master/stream/ws_client.py#L114

tg123 · 2017-12-04T10:13:00Z

src/ExecClient.cs

@@ -9,6 +9,10 @@ namespace k8s
 {
    public class ExecClient : IDisposable
    {
+        private const int StdInStreamIndex = 0;


const from py client
https://github.com/kubernetes-client/python-base/blob/master/stream/ws_client.py#L24

Thx, I added the other channel indices as well.

tg123 · 2017-12-04T10:46:29Z

@qmfrederik really sorry I did not click submit review posted yesterday but reviews were shown on my page. you can now see it.

qmfrederik · 2017-12-04T11:15:10Z

@tg123 Thanks for the feedback! I updated the PR.

As for unit tests, I added unit tests for Kubernetes.WebSocketNamespacedPodExecWithHttpMessagesAsync.
In my unit test I wanted to make sure the constructed URL, the message headers and client certificates are OK.

The easiest way I found to do this was by introducing a WebSocketBuilder class.
You pass your WebSocket configuration to this class and this class gives you a ClientWebSocket in return.
I've modified the Kubernetes class so that you can pass your own WebSocketBuilder. I pass a mock one in the unit tests to intercept this.

Let me see what I can do about the ExecClient (or whatever its future shape may be).

qmfrederik · 2017-12-04T11:33:05Z

@tg123 @brendandburns Thanks, I updated the PR.

Based on your feedback, would it be better to rename ExecClient to ConsoleClient or TerminalClient? It's a generic class which adds support for multiplexing stdout/stderr so I guess it can be reused accross Attach/Exec.

Alternatively, we can split ExecClient off into a separate PR if that needs more design work.

tg123

To my idea clear, the relationship between execlient and websocket might be like

channel related changes in execlient should move to websocket
and execclient register/consumes the channel stream from websocket

Maybe we discuss and do this in next PR

tg123 · 2017-12-04T12:14:02Z

.gitignore

@@ -2,6 +2,7 @@
 .vs
 obj/
 bin/
+kube-config.yml


can be removed

tg123 · 2017-12-04T12:15:05Z

src/ExecClient.cs

+            try
+            {
+                // The first byte represent the stream index. For stdin this is 0
+                buffer[0] = 0;


cloud be const

tg123 · 2017-12-04T12:16:59Z

examples/exec/Program.cs

+            var k8SClientConfig = KubernetesClientConfiguration.BuildConfigFromConfigFile();
+            var client = new Kubernetes(k8SClientConfig);
+
+            var webSocket = await client.WebSocketNamespacedPodExecWithHttpMessagesAsync(pod, container: container, command: "/bin/bash").ConfigureAwait(false);


Maybe the API cloud return ExecClient directly, make websocket with channel for intern use only

tg123 · 2017-12-04T12:18:40Z

src/ExecClient.cs

+            try
+            {
+                // The first byte represent the stream index. For stdin this is 0
+                buffer[0] = 0;


I think it would be better to hide channel in the websocket class which can be reuse in attach. but I think it is OK to impl them in next PR.

tg123 · 2017-12-04T12:24:47Z

tests/ExecClientTests.cs

+    public class ExecClientTests
+    {
+        [Fact]
+        public void ReadWriteTest()


cloud you please align with naming style only verb
like #51

tg123 · 2017-12-04T12:41:39Z

src/ExecClient.cs

+                Encoding.UTF8.GetBytes(command, 0, command.Length, buffer, 1);
+
+                ArraySegment<byte> segment = new ArraySegment<byte>(buffer, 0, commandLength + 1);
+                await this.socket.SendAsync(segment, WebSocketMessageType.Binary, true, cancellationToken).ConfigureAwait(false);


send/recy websocket in C# seems did not thread safe.

tg123 · 2017-12-04T12:45:17Z

src/ExecClient.cs

+        }
+
+        public event EventHandler ConnectionClosed;
+        public event EventHandler<string> StandardOutputReceived;


I prefer java client style

ExecClient with 3 streams Stdin, Stdout, Stderr

event string does not seem to be friendly if you want to readline or something else

qmfrederik · 2017-12-04T13:13:34Z

@tg123 Thanks!

I split off the exec-related functionality for now; we can do this in a separate PR.

I'd really prefer the Kubernetes C# client to expose a "normal" WebSocket class. It gives any user of the C# Kubernetes client the freedom to communicate over the WebSocket class the way they want.

It would be fine for the C# client to add default 'wrappers' around WebSocket which allow you to interact with the data in a specific way (e.g. by demuxing the streams); but as a user I'd really like to have access to the raw WebSocket object, too.
I've been bitten a couple of times in the past by APIs which tried to do "the right thing", but whatever they were doing wasn't the right thing for me, and I was left with no option but to compile from source. That'd be unfortunate.

Let me know if the WebSocket-related changes look good to me.

tg123

would be LGTM if file name changed

And next

Add an Ext class to wrap the StreamConnect to ChannelSupporttedStream

and then another Ext class which
wrap ChannelSupporttedStream to
ExecClient / AttachClient / PortForwardClient

would the be fine?

tg123 · 2017-12-04T15:00:37Z

src/IKubernetes.Exec.cs

+        /// <return>
+        /// A <see cref="ClientWebSocket"/> which can be used to communicate with the process running in the pod.
+        /// </return>
+        Task<WebSocket> WebSocketNamespacedPodExecWithHttpMessagesAsync(string name, string @namespace = "default", string command = "/bin/bash", string container = null, bool stderr = true, bool stdin = true, bool stdout = true, bool tty = true, Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken));


FileName to Ixxx.Websocket.cs or Ixxx.Stream.cs

Cloud we name this StreamConnectAsync(path, ...) or something else, which just return plain websocket
since this PR has nothing to do with exec

qmfrederik · 2017-12-04T15:55:14Z

@tg123 I tried to update the PR in line with your feedback:

I renamed the files to .WebSocket.cs to make it clear the files contain the WebSocket-related functionality
Code which I think can be shared across attach, port forward,... is split off to a StreamConnectAsync
I kept WebSocketNamespacedPodExecWithHttpMessagesAsync as this could will return a WebSocket which connects to the exec endpoint for a specific pod.

tg123 · 2017-12-04T16:57:32Z

src/Kubernetes.WebSocket.cs

+            _queryParameters.Add(string.Format("stdout={0}", stdout ? 1 : 0));
+            _queryParameters.Add(string.Format("tty={0}", tty ? 1 : 0));
+
+            uriBuilder.Query = string.Join("&", _queryParameters);


take look at QueryHelpers.AddQueryString I already imported this.
it might help work encode strings

Like this:

string queryString = string.Empty; queryString = QueryHelpers.AddQueryString(queryString, "command", command); if (container != null) { queryString = QueryHelpers.AddQueryString(queryString, "container", container); } queryString = QueryHelpers.AddQueryString(queryString, "stderr", stderr ? "1" : "0"); queryString = QueryHelpers.AddQueryString(queryString, "stdin", stdin ? "1" : "0"); queryString = QueryHelpers.AddQueryString(queryString, "stdout", stdout ? "1" : "0"); queryString = QueryHelpers.AddQueryString(queryString, "tty", tty ? "1" : "0"); uriBuilder.Query = queryString;

?

var queryString = QueryHelpers.AddQueryString(string.Empty, new Dictionary<string, string> { { "command", "aaaa"}, { "tty", "1"} });

Done, thanks!

tg123 · 2017-12-04T17:01:09Z

LGTM

looking forward friendly api in #64

tg123 · 2017-12-04T18:29:14Z

by the way
cloud you please squash you commits?

qmfrederik · 2017-12-04T19:50:09Z

@tg123 Sure, rebased & squashed.

qmfrederik · 2017-12-06T10:41:43Z

@tg123, @brendandburns Are we good to merge this one? Anything else you need?

tg123 · 2017-12-06T18:33:54Z

please wait for a bit

because

http://events.linuxfoundation.org/events/kubecon-and-cloudnativecon-north-america/program/featured-speakers#brendan-burns

qmfrederik · 2017-12-06T18:50:17Z

Ah, makes sense. Thanks for letting me know!

brendandburns

A few more small-ish comments...

brendandburns · 2017-12-07T04:35:17Z

src/IKubernetes.WebSocket.cs

+        /// <param name='cancellationToken'>
+        /// The cancellation token.
+        /// </param>
+        Task<WebSocket> WebSocketNamespacedPodPortForwardAsync(string name, string @namespace, IEnumerable<int> ports, Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken));


Why is this in the WebSocket class? I feel like this should be in the port forward class...

It's in the IKubernetes interface, this file just contains manual extensions to IKubernetes for API calls which returns WebSockets.
I can move it to something like IKubernetes.PortForward, if that's what you'd prefer - so one file per method?

why not also add attach

brendandburns · 2017-12-07T04:37:58Z

src/Kubernetes.WebSocket.cs

+            {
+                webSocket = await webSocketBuilder.BuildAndConnectAsync(uri, CancellationToken.None).ConfigureAwait(false);
+            }
+            catch (Exception ex)


Please don't catch bare Exception only catch expected Exceptions.

The catch catch statement is there so we can trace errors; the exception itself is rethrown.

.NET doesn't have the concept of checked exceptions and the MSDN documentation doesn't list which exceptions ClientWebSocket.ConnectAsync can throw.

brendandburns · 2017-12-07T04:39:15Z

src/WebSocketBuilder.cs

+    /// By default, this uses the .NET <see cref="ClientWebSocket"/> class, but you can inherit from this class and change it to
+    /// use any class which inherits from <see cref="WebSocket"/>, should you want to use a third party framework or mock the requests.
+    /// </remarks>
+    public class WebSocketBuilder


This class doesn't seem to add much imho. Why not just inline the various calls in the class in the code above...

The calls are not inlined because the unit tests use a mock WebSocketBuilder which traces all the parameters which have been passed and returns a dummy WebSocket.

I used a Builder-pattern becuase the code also uses the UriBuilder class.

I can change it a function/property like GetWebSocket(Dictionary<string,string> headers, Collection<X509Certificate2> clientCertificates, Uri uri), though, if that's what you'd prefer. As long as the unit tests can intercept the call it's fine with ime.

Injectable for testing is fair...

tg123 · 2017-12-07T17:34:12Z

src/KubernetesClient.csproj

@@ -23,5 +23,6 @@
    <PackageReference Include="Newtonsoft.Json" Version="10.0.2" />
    <PackageReference Include="System.ValueTuple" Version="4.4.0" />
    <PackageReference Include="YamlDotNet.NetCore" Version="1.0.0" />
+    <PackageReference Include="System.Net.WebSockets.Client" Version="4.3.2"/>


seems netstandard can use websocket without this dependeny

I think you can on netstandard2.0 but it doesn't work on netstandard1.4. Did you run dotnet restore when you tested this?

…ent#63) * Add support for executing commands within a container * Add WebSocketNamespacedPodPortForwardAsync * Add Attach functionality * Simplify code

brendandburns reviewed Dec 4, 2017

View reviewed changes

tg123 mentioned this pull request Dec 4, 2017

ConnectGetNamespacedPodExec fails with 'Upgrade Required' #62

Closed

tg123 reviewed Dec 4, 2017

View reviewed changes

qmfrederik mentioned this pull request Dec 4, 2017

API surface for exec, attach & port forward clients #64

Closed

tg123 approved these changes Dec 4, 2017

View reviewed changes

Add support for executing commands within a container

3668770

Add WebSocketNamespacedPodPortForwardAsync

930abe1

brendandburns requested changes Dec 7, 2017

View reviewed changes

tg123 reviewed Dec 7, 2017

View reviewed changes

qmfrederik added 2 commits December 7, 2017 20:03

Add Attach functionality

ccd098f

Simplify code

52eb059

brendandburns merged commit 2a54a8c into kubernetes-client:master Dec 11, 2017

Add support for executing commands within a container #63

Add support for executing commands within a container #63

Conversation

qmfrederik commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Dec 4, 2017

qmfrederik commented Dec 4, 2017 • edited Loading

tg123 commented Dec 4, 2017

qmfrederik commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tg123 commented Dec 4, 2017

qmfrederik commented Dec 4, 2017

qmfrederik commented Dec 4, 2017

tg123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmfrederik commented Dec 4, 2017

tg123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmfrederik commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tg123 commented Dec 4, 2017

tg123 commented Dec 4, 2017

qmfrederik commented Dec 4, 2017

qmfrederik commented Dec 6, 2017

tg123 commented Dec 6, 2017

qmfrederik commented Dec 6, 2017

brendandburns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmfrederik commented Dec 4, 2017 •

edited

Loading