[Websocket] Support exec

kubernetes-client / c

Official C client library for Kubernetes

Apache License 2.0

144 stars 46 forks source link

[Websocket] Support exec #65

Closed ityuhui closed 3 years ago

ityuhui commented 3 years ago

websocket and pod exec support for the c client

ityuhui commented 3 years ago

/cc @brendandburns

ityuhui commented 3 years ago

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

ityuhui commented 3 years ago

Hi @brendandburns

Review comments are addressed and replied.

Could you please take a look again ?

brendandburns commented 3 years ago

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 commented 3 years ago

Thanks @brendandburns .

I will address the comments after coming back from vacation.

brendandburns commented 3 years ago

Have a good vacation!

ityuhui commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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!

brendandburns commented 3 years ago

/lgtm /approve

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

k8s-ci-robot commented 3 years ago

[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](https://github.com/kubernetes-client/c/blob/master/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 commented 3 years ago

Thank you for your review @brendandburns @clearday4