Support wss:// websocket URLs for api-managed SSH proxying [WIP]

[funkypenguin]

Fixes #5016

Signed-off-by: David Young [email protected]

This PR resolves (in a backwards-compatible way) the issues with ws:// vs wss:// raised in #5016.

I'm creating this as a WIP because I'm seeking guidance about how best to test locally.

Tested (run the relevant ones):

• Code formatting: install pre-commit (auto-check on commit) or bash format.sh
• Any manual or new tests for this PR (please specify below)
• All smoke tests: /smoke-test (CI) or pytest tests/test_smoke.py (local)
• Relevant individual tests: /smoke-test -k test_name (CI) or pytest tests/test_smoke.py::test_name (local)
• Backward compatibility: /quicktest-core (CI) or pytest tests/smoke_tests/test_backward_compat.py (local)

[Michaelvll]

it seems after splitting, the URL passed to the executable will not be affected by the replacement. Should we remove the split, and make the replacement in sky/templates/websocket_proxy.py instead?

[funkypenguin]

I see what you mean, I left the split there, it shouldn't be there! What the best way to set this up to test locally on MacOS? I tried building the Dockerfile, but it just pulls the latest sky cli nightly with pip...

My thinking was to make this backwards-compatible, so that a user who still has a "non-protocoled" hostname in their config can still use sky/templates/websocket_proxy.py as-is (there's a check for :// in the hostname), but that any new configs created after this PR is merged will have either http:// or https:// in the config, which will better inform sky/templates/websocket_proxy.py re which websockets prefix to use.

[Michaelvll]

Good point! For backward compatibility, I think we can deal with it in the websocket_proxy.py, where we can check if the protocol is included in the URL and default to http if not.

[Michaelvll]

For testing it, a simple test might be mock the is_api_server_local to be False all the time?

skypilot/sky/server/common.py

Lines 95 to 97 in bc5394a

	@annotations.lru_cache(scope='global')
	def is_api_server_local():
	return get_server_url() in AVAILABLE_LOCAL_API_SERVER_URLS

[funkypenguin]

What I'm after in terms of advice on testing is more about how to build the CLI locally? I was hoping to do it in a dockerfile to keep dependencies etc clean, but the Dockerfile seems to just pull the latest nightly with pip. Is there a "best practice" way to test this locally?

[Michaelvll]

Thanks for the great effort on this PR @funkypenguin ! This PR #5183 works on the same issue by coincidence. We are thinking of directly having that PR to be merged. Could you help check if that PR solves your issue as well? Your input would be really important :)