Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

command exec: Adding support for streamProtocolV4 #297

Closed
jraby opened this issue Jul 18, 2017 · 2 comments
Closed

command exec: Adding support for streamProtocolV4 #297

jraby opened this issue Jul 18, 2017 · 2 comments

Comments

@jraby
Copy link
Contributor

jraby commented Jul 18, 2017

I've had a use case to exec some commands in pods and return the exit code of the remote program transparently to the parent process.

At the moment doing so involves fishing out the exit code by parsing the string returned via the error channel of the websocket when the remote command exit.

While looking at how kubectl handles this, I found that the api does in fact support reporting the exit code directly via the error channel of the websocket since this commit:
kubernetes/kubernetes@e792d41

This requires support for the v4 stream protocol by the client (server defaults to v3).

I'm not sure what would be the best way to add support for this.
In my test, simply adding this header to the upgrade request makes it Just Work, but I'm not sure if this would be an appropriate fix:

diff --git a/kubernetes/client/ws_client.py b/kubernetes/client/ws_client.py
index b0a60e5..edcaa08 100644
--- a/kubernetes/client/ws_client.py
+++ b/kubernetes/client/ws_client.py
@@ -46,6 +46,8 @@ class WSClient:
         if headers and 'authorization' in headers:
             header.append("authorization: %s" % headers['authorization'])
 
+        header.append('Sec-WebSocket-Protocol: v4.channel.k8s.io')
+
         if url.startswith('wss://') and configuration.verify_ssl:
             ssl_opts = {
                 'cert_reqs': ssl.CERT_REQUIRED,

With this patch, instead of getting a plain string down the error channel, we get this:

    "metadata": {},
    "status": "Failure",
    "message": "command terminated with non-zero exit code: Error executing in Docker Container: 4",
    "reason": "NonZeroExitCode",
    "details": {
        "causes": [
            {
                "reason": "ExitCode",
                "message": "4"
            }
        ]
    }
}

originally we'd get this:

command terminated with non-zero exit code: Error executing in Docker Container: 4

What would be a good way forward for this? Should the client default to v4? Or should it provide a handle to set whatever protocol version string we might wish to send?

Thanks.

@dims
Copy link
Collaborator

dims commented Jul 18, 2017

@jraby since that commit you referenced has been in kubernetes since v1.4.0-alpha.3, we should just default to v4. However it's @mbohlool 's call :)

@mbohlool
Copy link
Contributor

mbohlool commented Jul 18, 2017

I don't see any problem with making v4 default. I would make it a configuration though so people can change it. Also a general headers configuration would be useful (that is better to be added upstream to swagger-codegen).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants