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

Make port forward local port default to the remote port #505

Closed
AustinScola opened this issue Oct 10, 2024 · 5 comments · Fixed by #506
Closed

Make port forward local port default to the remote port #505

AustinScola opened this issue Oct 10, 2024 · 5 comments · Fixed by #506
Labels
bug Something isn't working

Comments

@AustinScola
Copy link

Which project are you reporting a bug for?

kr8s

What happened?

Synchronous port forwards do not appear to be working. It looks like it is b/c the the start method tries to use run run_forever in a thread. but run_forever is async:

    def start(self):
        """Start a background thread with the port forward running."""
        self._bg_thread = threading.Thread(target=self.run_forever, daemon=True)
        self._bg_thread.start()

Anything else?

No response

@AustinScola AustinScola added the bug Something isn't working label Oct 10, 2024
@AustinScola
Copy link
Author

Hmm, nvm seems to be working I was just missing the local port (which I thought would default to the remote port).

@jacobtomlinson
Copy link
Member

Maybe we should rename this issue to "Make port forward local port default to the remote port".

Could you share a code example of exactly the snippet you are using so we can reproduce the same result and write a test for the fix?

@AustinScola
Copy link
Author

AustinScola commented Oct 10, 2024

Sure, the snippet was roughly this:

svc: str = ...
remote_port: int = ...
svc = Service.get(svc)
port_forward = svc.portforward(remote_port=remote_port)
port_forward.start()

I think k8s defaults to having the local port be the same as the remote port but I could be wrong?

@jacobtomlinson jacobtomlinson changed the title Sync port forwards broken Make port forward local port default to the remote port Oct 10, 2024
@jacobtomlinson
Copy link
Member

jacobtomlinson commented Oct 11, 2024

So I've done a quick check locally with kubectl and observed the following behaviour.

# Start a Pod that is exposing a port
kubectl run nginx --image nginx --port 80

# Port forward and only specify the remote port
kubectl port-forward po/nginx 80
# This binds remote port 80 to port 80 locally

# Port forward and specify an empty local port
kubectl port-forward po/nginx :80
# This binds to a random port locally

# Port forward and specify a local port
kubectl port-forward po/nginx 8080:80
# This binds to port 8080 locally

So the current behaviour in kr8s is the same as kubectl port-forward po/nginx :80. It's not clear to me whether we want to switch that to the kubectl port-forward po/nginx 80 behaviour.

@jacobtomlinson
Copy link
Member

After thinking on this a bit I feel if the local_port argument is completely omitted it should match the kubectl behaviour of using the remote_port. Then if it is explicitly set to None it should choose a random high port.

I've opened #506 to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants