Implement port forwarding. by iciclespider · Pull Request #210 · kubernetes-client/python-base

iciclespider · 2020-08-24T03:43:02Z

Looking for feed back if this looks on track as a way of implementing port forwarding.

See the example and unittests added in this PR kubernetes-client/python#1237 for usage.

k8s-ci-robot · 2020-08-24T03:43:05Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Details

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. I understand the commands that are listed here.

k8s-ci-robot · 2020-08-24T03:43:10Z

Welcome @iciclespider!

It looks like this is your first PR to kubernetes-client/python-base 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/python-base has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

codecov-commenter · 2020-08-24T03:49:01Z

Codecov Report

Merging #210 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   92.37%   92.37%           
=======================================
  Files          13       13           
  Lines        1613     1613           
=======================================
  Hits         1490     1490           
  Misses        123      123

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 471a678...7bf04b3. Read the comment docs.

iciclespider · 2020-08-24T04:43:30Z

/check-cla

iciclespider · 2020-08-24T04:43:54Z

/check-cla

iciclespider · 2020-08-24T04:49:16Z

/assign @mbohlool

yliaog · 2020-08-24T20:55:06Z

/assign

yliaog · 2020-08-26T17:04:14Z

thanks for the PR. it looks good overall.

could you please split this PR into two PRs, the first PR is for refactoring the existing code to make it reusable for port forwarding, the second PR is for port forwarding.

this makes it easier to review, and also easier to rollback or fix the portforward PR in case it has any issue.

iciclespider · 2020-08-27T04:26:25Z

@yliaog PR to refactor existing implementation in preparation for implementing port forwarding: #211

iciclespider · 2020-08-29T00:03:42Z

@yliaog I rebased this PR against the merged master with the refactored code. I also updated the PR in kubernetes-client/python#1237 with a pod_portforward.py example.

palnabarun · 2020-08-31T14:44:46Z

+    ports = []
+    for key, value in query_params:
+        if key == 'ports':
+            for port in value.split(','):
+                try:
+                    port = int(port)
+                    if not (0 < port < 65536):
+                        raise ValueError
+                    ports.append(port)
+                except ValueError:
+                    raise ApiValueError("Invalid port number `" + str(port) + "`")
+    if not ports:
+        raise ApiValueError("Missing required parameter `ports`")


I think this logic can be abstracted out to another function with a simpler implementation like below.

def _valid_ports(ports): _ports = [] for p in ports: try: p = int(p) except ValueError: raise ApiValueError("Invalid port number `{}`. Port number must be an integer".format(p)) if (p < 0) or (p > 65536): raise ApiValueError("Invalid port number `{}`. Port number must be between 0 and 65536".format(p)) _ports.append(p) return _ports ports = query_params.get("ports", "") if not ports: raise ApiValueError("Missing required parameter `ports`") ports = ports.split(",") ports = _valid_ports(ports) # There is no check required here, since any unwanted port value would be checked in _valid_ports itself. # We can be assured that we get a non-empty port list here.

This line will not work:

ports = query_params.get("ports", "")

query_params is not a dictionary, it a list of two element tuples.

palnabarun · 2020-08-31T14:56:06Z

/assign

yliaog · 2020-08-31T23:01:34Z

+            raise ValueError("Invalid port number")
+        return self.ports[port_number].socket
+
+    def error(self, port_number):


please change it to error_channel or something more descriptive. 'error' looks like error/exception, which is confusing.

I just pushed the change to error_channel.

However, a little clarification. This is not really a "channel" that can send and receive data. From the client perspective, it is read only and cannot be written to. And the only time it is written to by kubernetes is when there is an error/exception case on the server and that port is unusable. It is written as the parting message.

See: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cri/streaming/portforward/websocket.go#L184-L197

func (h *websocketStreamHandler) portForward(p *websocketStreamPair) { defer p.dataStream.Close() defer p.errorStream.Close() klog.V(5).Infof("(conn=%p) invoking forwarder.PortForward for port %d", h.conn, p.port) err := h.forwarder.PortForward(h.pod, h.uid, p.port, p.dataStream) klog.V(5).Infof("(conn=%p) done invoking forwarder.PortForward for port %d", h.conn, p.port) if err != nil { msg := fmt.Errorf("error forwarding port %d to pod %s, uid %v: %v", p.port, h.pod, h.uid, err) runtime.HandleError(msg) fmt.Fprint(p.errorStream, msg.Error()) } }

yliaog · 2020-09-01T00:08:39Z

+    def _proxy(self):
+        channel_ports = []
+        channel_initialized = []
+        python_ports = {}


what about call it local_port?

* Rename `_Port.error` to be `_Port.error_channel`. * Correct comment about where setsockopt is being called. * Add comments clarifying why the double call to the same methods to setup channel information. * Allow for ports specified with both local and remote port numbers.

…t and a remote port.

…erminate.

iciclespider · 2020-09-06T19:29:50Z

@yliaog I submitted another commit for review.

…ing.

iciclespider

@yliaog I pushed a commit that addresses the comments.

yliaog · 2020-09-08T17:05:14Z

thanks for the PR, really appreciate it.

/lgtm
/approve

k8s-ci-robot · 2020-09-08T17:05:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iciclespider, yliaog

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [yliaog]

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 requested review from mbohlool and roycaihw August 24, 2020 03:43

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 24, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 24, 2020

iciclespider mentioned this pull request Aug 24, 2020

Unittests for portforwarding ability added in python-base. kubernetes-client/python#1237

Merged

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 24, 2020

k8s-ci-robot assigned mbohlool Aug 24, 2020

k8s-ci-robot assigned yliaog Aug 24, 2020

iciclespider mentioned this pull request Aug 27, 2020

Refactor stream package to enable common method helpers for other streaming api classes. #211

Merged

Implement port forwarding.

74d0e29

iciclespider force-pushed the port-forward branch from 1dd2756 to 74d0e29 Compare August 28, 2020 23:55

palnabarun suggested changes Aug 31, 2020

View reviewed changes

k8s-ci-robot assigned palnabarun Aug 31, 2020

yliaog reviewed Sep 1, 2020

View reviewed changes

iciclespider added 2 commits August 31, 2020 15:53

Rework the parsing of the requested ports to support both a local por…

72e3725

…t and a remote port.

yliaog reviewed Sep 3, 2020

View reviewed changes

Rework how the PortForward._proxy thread determines when and how to t…

7bf04b3

…erminate.

yliaog reviewed Sep 7, 2020

View reviewed changes

Comment thread stream/ws_client.py Outdated

Comment thread stream/ws_client.py Outdated

Comment thread stream/ws_client.py Outdated

Rework loop which collects the local python sockets for read and writ…

ce3a1a2

…ing.

iciclespider commented Sep 7, 2020

View reviewed changes

Comment thread stream/ws_client.py Outdated

Comment thread stream/ws_client.py Outdated

Better handling of error channel reponse, and comment typo.

2e86b71

iciclespider force-pushed the port-forward branch from d22b840 to 27d3369 Compare September 8, 2020 07:01

Support both python 2.7 and 3.x.

5d39d0d

iciclespider force-pushed the port-forward branch from 27d3369 to 5d39d0d Compare September 8, 2020 07:04

yliaog reviewed Sep 8, 2020

View reviewed changes

Comment thread stream/ws_client.py

Comment thread stream/ws_client.py

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2020

k8s-ci-robot merged commit 3dc7fe0 into kubernetes-client:master Sep 8, 2020

This was referenced Mar 15, 2021

Bump kubernetes from 8.0.1 to 12.0.1 in /tests sethbergman/kubernetes-ingress#23

Closed

Bump kubernetes from 8.0.1 to 12.0.1 in /perf-tests sethbergman/kubernetes-ingress#24

Closed

Conversation

iciclespider commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 24, 2020

Uh oh!

k8s-ci-robot commented Aug 24, 2020

Uh oh!

codecov-commenter commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

iciclespider commented Aug 24, 2020

Uh oh!

iciclespider commented Aug 24, 2020

Uh oh!

iciclespider commented Aug 24, 2020

Uh oh!

yliaog commented Aug 24, 2020

Uh oh!

yliaog commented Aug 26, 2020

Uh oh!

iciclespider commented Aug 27, 2020

Uh oh!

iciclespider commented Aug 29, 2020

Uh oh!

palnabarun Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

iciclespider Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palnabarun commented Aug 31, 2020

Uh oh!

Uh oh!

yliaog Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

iciclespider Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yliaog Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iciclespider commented Sep 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iciclespider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yliaog commented Sep 8, 2020

Uh oh!

k8s-ci-robot commented Sep 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

iciclespider commented Aug 24, 2020 •

edited

Loading

codecov-commenter commented Aug 24, 2020 •

edited

Loading

iciclespider Aug 31, 2020 •

edited

Loading

iciclespider Sep 1, 2020 •

edited

Loading