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

Implement exec over the SPDY protocol #410

Closed
wants to merge 17 commits into from

Conversation

admilazz
Copy link
Contributor

This change adds improved exec support. The existing exec support uses web sockets, which has a flawed protocol leading to some commands running forever. This change helps address issue #397 .

I tried to replace the implementation of the existing exec support to provide a seamless upgrade. Unfortunately, many internal implementation details of the web socket approach (e.g. WebSocketNamespacedPodExecAsync, MuxedStream, StreamDemuxer, etc), were publicly exposed, and it would be a breaking change to remove them. I did replace the implementation of NamespacedPodExecAsync, however, which was generic enough.

This PR makes the following public API changes:

  • The fluent API gains new members:
    • ExecCommandAsync - Execs the current resource, which does not necessarily have to be a pod
    • OpenSPDYAsync - Makes the request and upgrades it to a SPDY connection
    • OpenWebSocketAsync - Makes the request and upgrades it to a web socket connection

NOTE: This PR builds on and includes changes from two other PRs (#405 and #406). I'm not sure how best to represent that in a PR, but the files actually added or changed in this specific PR are:

  • ByteBuffer.cs (to edit a comment)
  • Kubernetes.ConfigInit.cs (to remove the automatic retry support added by the Microsoft.Rest.ClientRuntime)
  • Kubernetes.Exec.cs (to replace the NamespacedPodExecAsync implementation)
  • KubernetesRequest.cs (to add the three methods named above)
  • PipeStream.cs
  • SPDYExec.cs

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 12, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: admilazz
To complete the pull request process, please assign krabhishek8260
You can assign the PR to them by writing /assign @krabhishek8260 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 12, 2020
@admilazz admilazz changed the title Spdy exec Implement exec over the SPDY protocol Apr 12, 2020
@admilazz
Copy link
Contributor Author

/assign @krabhishek8260

@brendandburns
Copy link
Contributor

I'll review this after the dependent PRs are merged.

@andykernahan
Copy link
Contributor

@admilazz Are you planning to add tests for the new types and functionality? Similarly, the new SPDY.net dependency was only very recently created (by you under @AdamMil?) and is not tested.

@admilazz
Copy link
Contributor Author

@admilazz Are you planning to add tests for the new types and functionality? Similarly, the new SPDY.net dependency was only very recently created (by you under @AdamMil?) and is not tested.

Yes, but as for the priority of it I will see what happens to the other PRs. This is way down the stack.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2020
@admilazz
Copy link
Contributor Author

admilazz commented Apr 17, 2020

I guess I'll put this in a separate project.

@admilazz admilazz closed this Apr 17, 2020
@k8s-ci-robot
Copy link
Contributor

@admilazz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020
@admilazz
Copy link
Contributor Author

the new SPDY.net dependency was only very recently created (by you under @AdamMil?) and is not tested.

P.S. I wouldn't say it's untested, though. :-) I did run 20,000 execs of various kinds in a row with zero failures... But it's true I haven't written unit tests yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants