[Websocket] Support exec #65

ityuhui · 2021-06-28T11:56:33Z

websocket and pod exec support for the c client

ityuhui · 2021-06-28T12:01:43Z

.github/workflows/build.yml

README.md

brendandburns · 2021-06-28T15:37:38Z

README.md

@@ -16,6 +16,15 @@ CLIENT_REPO_ROOT=${PWD}/c
 # Install pre-requisites
 sudo apt-get install libssl-dev libcurl4-openssl-dev uncrustify libyaml-dev

+# Build pre-requisite: libwebsockets


Do we want to build from source? Or just apt-get install libwebsockets-dev?

We need build from source for libwebsockets now.

Because the code in this PR uses the latest stable version v4.2, but the version of libwebsockets-dev in Ubuntu 20.04 LTS (docker image ubuntu-latest) is v3.2.1, which is not compatible.

Users using Ubuntu 20.10 or above can install libwebsockets-dev v4.x by

apt-get install libwebsockets-dev

We can update it after Ubuntu 22.04 LTS releases next year.

brendandburns · 2021-06-28T15:39:14Z

kubernetes/websocket/kube_exec.c

+    printf("%s: Received %ld bytes:\n%s", __func__, *p_data_received_len, (char *) (*p_data_received));
+}
+
+static void escape_character_in_url(char *escaped, int escaped_buffer_size, const char chr)


use curl_easy_escape(...) instead?

https://curl.se/libcurl/c/curl_easy_escape.html

escape_character_in_url is different with curl_easy_escape, the former receives only one character e.g. + and then returns an escaped string, e.g. %2B at one time. But curl_easy_escape receives a string and returns the full escaped string for the whole url string.

At the case of kube exec, we need handle the characters one by one because the space (" ") should not be escaped, it needs to be converted to "&command=", e.g.
"ls /" will be converted to
"ls&command=/"

curl_easy_escape cannot do this.

brendandburns · 2021-06-28T15:41:42Z

kubernetes/websocket/kube_exec.c

+    }
+}
+
+static int assemble_command_in_url(char *command_string_in_url, int url_command_string_buffer_size, const char *original_command_string)


If url_command_string_buffer_size is > ESCAPED_STRING_BUFFER_SIZE this will overflow the escaped_string buffer. We should test for this and return -1.

The function escape_character_in_url is different with curl_easy_escape in libcurl.

ESCAPED_STRING_BUFFER_SIZE is not the size of the whole escaped string of url, it is the size of string that only one character escaped, e.g. + escapes to %2B (length is 3) , actually ESCAPED_STRING_BUFFER_SIZE=4 is enough.

I have test command_string_length in function to prevent the command_string_in_url from overflowing

if (command_string_length >= url_command_string_buffer_size) {

kubernetes/websocket/kube_exec.c

kubernetes/websocket/wsclient.c

brendandburns

Few comments.

ityuhui · 2021-06-29T02:48:28Z

Thank you @brendandburns
I will address these comments one by one.

clearday4 · 2021-06-30T08:03:50Z

kubernetes/websocket/wsclient.c

+    free(wsc);
+}
+
+static struct lws_context *g_lws_context;


Should g_lws_context be a global variable?
Do we have to maintain ws context if I run a command with kube_exec?

It's the usage in the samples in libweboscket repo.

Do you concern whether it can work in multi-thread env ?

yes. running in the multithread env, the static variable is not safe.

I don't investigate this but some searching results show multi-thread is not supported easily by libwebsockets.

For the first commit of the feature, I will not consider this case. Welcome do some research for this!

For now, the case that only one thread runs "kube_exec", and other threads use apiClient_t (libcurl) is OK.

clearday4 · 2021-06-30T10:38:32Z

kubernetes/websocket/wsclient.c

+    i.context = g_lws_context;
+    i.port = wsc->server_port;
+    i.address = wsc->server_address;
+    i.path = wsc->path;


is it wss:// connection? or ws:// connection?

It depends on the value of i.ssl_connection

i.ssl_connection = wsc->ssl_config ? LCCSCF_USE_SSL : 0;

ityuhui · 2021-06-30T14:17:19Z

Hi @brendandburns

Review comments are addressed and replied.

Could you please take a look again ?

brendandburns · 2021-07-01T15:59:44Z

Thanks for addressing the comments.

I had one clarification.

Also, I think that the escaping of the chars code is a little complicated, I would suggest that we pre-allocate the entire escaped string and then run through it linearly rather than using strncat, also I think it needs unit tests, but we can address that in a follow up PR.

If we clean up the unnecessary allocation by changing the ownership of the path variable, then I'm ok with merging.

ityuhui · 2021-07-04T09:17:09Z

Thanks @brendandburns .

I will address the comments after coming back from vacation.

brendandburns · 2021-07-05T15:37:40Z

Have a good vacation!

peters95 · 2021-07-06T03:43:49Z

kubernetes/websocket/wsclient.c

+    info.options = LWS_SERVER_OPTION_DO_SSL_GLOBAL_INIT;
+    info.port = CONTEXT_PORT_NO_LISTEN; /* we do not run any server */
+    info.protocols = protocols;
+    info.fd_limit_per_thread = 1 + 1 + 1;


What's the purpose of this expression vs setting the limit to just 3?

This code comes from the sample of libwebsokets and it has below explanation for users :

/* * since we know this lws context is only ever going to be used with * one client wsis / fds / sockets at a time, let lws know it doesn't * have to use the default allocations for fd tables up to ulimit -n. * It will just allocate for 1 internal and 1 (+ 1 http2 nwsi) that we * will use. */ info.fd_limit_per_thread = 1 + 1 + 1;

clearday4 · 2021-07-07T11:39:22Z

kubernetes/websocket/wsclient.c

+#define TTY_STDIN_NUMBER 0
+#define TTY_STDOUT_NUMBER 1
+#define WS_PROTOCOL_DELIM "://"
+#define WS_BASE_PATH_DELIM ":"  /* ip:port */


It can't clarify the port in IPV6 address
for example, if address is [fd04::1]:443
the port is not distinguished.

It's a problem in ipv6 env.
But the delim ":" is right even in the format [fd04::1]:443.
The fix should use strrchr in the function of get_server_address_and_port.
I will do it.

clearday4 · 2021-07-07T11:40:22Z

kubernetes/websocket/wsclient.c

+    const char *port_string = p + strlen(WS_BASE_PATH_DELIM);
+
+    int ws_port = atoi(port_string);
+    if (0 == ws_port) {


I think it's better to use strtol intead of using atoi
atoi is unsafe function.

Why is atoi unsafe ?

afaik, atoi("some-non-numeric-string") is undefined in the C spec, so the strto... family are preferred.

https://wiki.sei.cmu.edu/confluence/display/c/ERR34-C.+Detect+errors+when+converting+a+string+to+a+number

I don't think it matters too much in this case, but it's a good habit to get into.

clearday4 · 2021-07-12T06:03:33Z

examples/exec_pod/main.c

+    char *base_path = NULL;
+    sslConfig_t *ssl_config = NULL;
+    list_t *api_keys = NULL;
+    int rc = load_kube_config(&base_path, &ssl_config, &api_keys, NULL);    /* NULL means loading configuration from $HOME/.kube/config */


Did you test with load_incluster_config?

This PR cannot support running in cluster now.
We may support it in future. ^-^

I'm struggling with not establishing ssl connection while using load_incluster_config.
Do you know what kind of missing implementation will it to fail?

"ssl_config" returning from load_incluster_config only has CA file but does not have the client certificate and private key (which are not provided in cluster) so it cannot pass authentication by Kubernetes API server.

"api_keys" holding the authentication credential should be passed to the websocket client. And the websocket client needs updating to use this credential.

ityuhui · 2021-07-14T11:05:13Z

Hi @brendandburns

I have made below changes:

Removed the ownership of path in wsclient_t
Add the parameter apiKeys to the function

wsclient_t *wsclient_create(const char *base_path, sslConfig_t * ssl_config, list_t * apiKeys, int ws_log_mask)

This parameter is not used now, but for in-cluster authentication in future.

Could you please check whether this PR can merge now ? Thanks.

brendandburns · 2021-07-15T14:54:30Z

@ityuhui this code looks ok to me, but I'd like to make sure that @clearday4 is ok with it also before we merge.

Thanks!

clearday4 · 2021-07-16T08:43:29Z

@ityuhui @brendanburns
it is good in first stage.
In the next step, It's better to consider the multithread and implement an additional LWS_CALLBACK_CLIENT_APPEND_HANDSHAKE_HEADER callback for incluster_config.
Thanks a lot!

clearday4 · 2021-07-16T09:23:33Z

kubernetes/websocket/wsclient.c

+        lwsl_user("%s: wsclient connection established\n", __func__);
+        lws_callback_on_writable(wsi);
+        break;
+


for the incluster_config
I add the this case for adding token to header

case LWS_CALLBACK_CLIENT_APPEND_HANDSHAKE_HEADER: { const char *tpath = "/var/run/secrets/kubernetes.io/serviceaccount/token"; char *token = read_complete_file(tpath); // using internal function char *token_header = strcat_copy("Bearer ", token); // using internal function const char *acc = "*/*"; unsigned char **p = (unsigned char **)in, *end = (*p) + len; if (lws_add_http_header_by_token(wsi, WSI_TOKEN_HTTP_ACCEPT, (unsigned char *)acc, (int)strlen(acc), p, end)) { free(token_header); return -1; } if (lws_add_http_header_by_token(wsi, WSI_TOKEN_HTTP_AUTHORIZATION, (unsigned char *)token_header, (int)strlen(token_header), p, end)) { free(token_header); return -1; } free(token_header); break; }

Thanks. It's helpful.

brendandburns · 2021-07-16T14:17:08Z

/lgtm
/approve

We can merge this and take improvements as the next step.

k8s-ci-robot · 2021-07-16T14:17:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, ityuhui

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:

~~OWNERS~~ [brendandburns,ityuhui]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ityuhui · 2021-07-17T03:58:54Z

Thank you for your review @brendandburns @clearday4

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 28, 2021

k8s-ci-robot requested a review from brendandburns June 28, 2021 11:56

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 28, 2021

brendandburns reviewed Jun 28, 2021

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

brendandburns reviewed Jun 28, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

brendandburns reviewed Jun 28, 2021

View reviewed changes

kubernetes/websocket/kube_exec.c Outdated Show resolved Hide resolved

brendandburns reviewed Jun 28, 2021

View reviewed changes

kubernetes/websocket/kube_exec.c Outdated Show resolved Hide resolved

brendandburns reviewed Jun 28, 2021

View reviewed changes

kubernetes/websocket/wsclient.c Outdated Show resolved Hide resolved

brendandburns reviewed Jun 28, 2021

View reviewed changes

ityuhui force-pushed the yh-websocket-exec branch from 1ca9149 to d08f667 Compare June 30, 2021 12:30

clearday4 reviewed Jun 30, 2021

View reviewed changes

ityuhui force-pushed the yh-websocket-exec branch from d08f667 to 43d23b1 Compare June 30, 2021 13:23

peters95 reviewed Jul 6, 2021

View reviewed changes

clearday4 reviewed Jul 7, 2021

View reviewed changes

clearday4 reviewed Jul 12, 2021

View reviewed changes

ityuhui force-pushed the yh-websocket-exec branch 5 times, most recently from 7b289c6 to be31e23 Compare July 13, 2021 13:19

ityuhui force-pushed the yh-websocket-exec branch 10 times, most recently from 8217811 to b3b8562 Compare July 14, 2021 10:30

[Websocket] Support exec

5d5e323

ityuhui force-pushed the yh-websocket-exec branch from b3b8562 to 5d5e323 Compare July 14, 2021 10:46

clearday4 reviewed Jul 16, 2021

View reviewed changes

k8s-ci-robot assigned brendandburns Jul 16, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2021

k8s-ci-robot merged commit d83ca5a into kubernetes-client:master Jul 16, 2021

ityuhui deleted the yh-websocket-exec branch July 17, 2021 03:58

ityuhui mentioned this pull request Aug 20, 2021

Pod exec in cluster #77

Closed

[Websocket] Support exec #65

[Websocket] Support exec #65

Conversation

ityuhui commented Jun 28, 2021

ityuhui commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ityuhui Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns left a comment

Choose a reason for hiding this comment

ityuhui commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ityuhui Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ityuhui commented Jun 30, 2021

brendandburns commented Jul 1, 2021

ityuhui commented Jul 4, 2021

brendandburns commented Jul 5, 2021

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

ityuhui commented Jul 14, 2021

brendandburns commented Jul 15, 2021

clearday4 commented Jul 16, 2021

clearday4 Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Jul 16, 2021

k8s-ci-robot commented Jul 16, 2021

ityuhui commented Jul 17, 2021

ityuhui Jun 30, 2021 •

edited

Loading

ityuhui Jul 1, 2021 •

edited

Loading

clearday4 Jul 16, 2021 •

edited

Loading