Add ssh_host_key router #8295
Add ssh_host_key router #8295
Conversation
iQQBot
force-pushed
the
pd/ssh-hostkey
branch
from
47f97dc to
31eee3d
Compare
Codecov Report
@@ Coverage Diff @@
## main #8295 +/- ##
==========================================
- Coverage 31.23% 29.02% -2.21%
==========================================
Files 39 39
Lines 5910 3335 -2575
==========================================
- Hits 1846 968 -878
+ Misses 3923 2313 -1610
+ Partials 141 54 -87
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
iQQBot
force-pushed
the
pd/ssh-hostkey
branch
2 times, most recently
from
d9a962e to
1befbf3
Compare
|
Hi, Do you have any associated issue for this PR to get more context on the changes? |
|
@princerachit just created one #8296 |
|
@princerachit see internal thread |
iQQBot
force-pushed
the
pd/ssh-hostkey
branch
from
1befbf3 to
b79520d
Compare
| r.Use(logHandler) | ||
|
|
||
| // Note: the order of routes defines their priority. | ||
| // Routes registered first have priority over those that come afterwards. | ||
| routes := newIDERoutes(config, ip) | ||
|
|
||
| // if host key is not empty, we use /__ws_proxy/ssh_host_key to provider public host key | ||
| if len(hostKeyList) > 0 { | ||
| routes.HandleSSHHostKeyRoute(r.Path("/__ws_proxy/ssh_host_key"), hostKeyList) |
There was a problem hiding this comment.
Do we want to introduce more component specific routes? Considering this is essentially API, what other cases for ws-proxy provided values do we see?
There was a problem hiding this comment.
Want to get a wsProxyRouter like iderouter?
There was a problem hiding this comment.
I observed that other routers have specific domain rules, and we just want to put it under ideDomain
There was a problem hiding this comment.
@csweichel Do you mean to match path for __ws_proxy and have subroute for it?
There was a problem hiding this comment.
I meant not tying it to ws-proxy, e.g. calling it __ssh or something like that. Just so that if we ever move this functionality elsewhere, we're not left with a misnomer we can't move away from.
There was a problem hiding this comment.
why does this path use two _ ?
There was a problem hiding this comment.
Let's go with one :) I thought we are using two for supervisor, but it is one there as well.
There was a problem hiding this comment.
@csweichel Could you have a look again please? 🙏
|
I verified that it works with JB plugin hooks. @csweichel @kylos101 good to merge and deploy? |
|
Here is a PR for GW plugin: #8317 to auto validate host keys based on changes on this PR. |
iQQBot
force-pushed
the
pd/ssh-hostkey
branch
from
b79520d to
ae5f486
Compare
| } | ||
| byt, err := json.Marshal(shk) | ||
| if err != nil { | ||
| log.WithError(err).Error("ssh_host_key router setup failed") |
There was a problem hiding this comment.
this would produce a subtle failure mode: we'd get this error message once, but SSH access would be broken.
Not a reason not to merge, but maybe a follow up. Instead of just logging, we could bubble up the error and exit.
roboquat
added
deployed: workspace
Description
This PR add a router for ssh_host_key, router URL is
workspaceURL/_ssh/host_keysit will show all hostkeysRelated Issue(s)
Relate #8296
How to test
curl workspaceURL/_ssh/host_keysto test, it will contain all available public hostkey and typeRelease Notes
Documentation