First pass at port API needed for port UI #85117

alexr00 · 2019-11-19T12:08:27Z

Part of microsoft/vscode-remote-release#1777 and #81388

roblourens · 2019-11-19T17:57:58Z

Hey @Chuxel, we were talking about this and I wanted to see what you think - is there any reason to prefer ssh-forwarded ports over ports forwarded by vscode?

The SSH extension can declare that it has already forwarded some ports (like ones declared in the SSH config file) but I don't think we need a way for the SSH extension to actually implement the port forward.

src/vs/vscode.proposed.d.ts

roblourens · 2019-11-19T18:02:01Z

src/vs/vscode.proposed.d.ts

+		 * Allows ports that have been forwarded in a workspace to be restored next time that workspace is opened.
+		 * Alternative to this: have API for getting all the ports then an event when they change so that the extension can save and restore ports(as it can do today).
+		 */
+		restorePorts: boolean;


Need to distinguish between the vscode server port which shouldn't be restored, and others. The alternative suggestion might be better so the extension can differentiate.

From what I understand, the vscode server port must be forwarded by the extension. This API deliberately doesn't provide a way to tell the core about that port, so it won't know about it to restore it.

What other ports shouldn't be restored?

IMHO restorePorts: boolean should be a user setting, not something a provider gets to decide.

We were talking about this with Sri today, that if we switch to using this feature, how would you support the ports that already been shared in a session but because user closed the VSO(hence vscode), the ports have been killed. so I assume this restorePorts would solve this issue, am I right? but then where do you save the forwarded port to forward them for next session? would that be part of candidates?

@fara-nak Instead of having restorePorts in the API, I'd like to make it a setting. So when the user opens a workspace again the ports are automatically re-forwarded.

Chuxel · 2019-11-19T21:35:53Z

Hey @Chuxel, we were talking about this and I wanted to see what you think - is there any reason to prefer ssh-forwarded ports over ports forwarded by vscode?

The SSH extension can declare that it has already forwarded some ports (like ones declared in the SSH config file) but I don't think we need a way for the SSH extension to actually implement the port forward.

@roblourens I think using a common mechanism should work.

Playing this out a bit - The main kinds of things that come to mind are around proxies, authentication, etc, but at this point we should be already connected to the other side and we've already authenticated.

The SSH protocol itself allows you to do things like reverse forwarding and allowing connections from things other than localhost, but any dynamic code we have in place today doesn't support that anyway. That seems better to add advanced config to the actual SSH config file.

The one subtlety that bit us on Containers is that "publish" is different than an actual forward. Our forwarding mechanism works even if a host only accepts connections from localhost while publishing via Docker will not (hosts have to use 0.0.0.0 or '*'). I don't think that applies to SSH given we forward localhost as I recall.

UX wise, it probably makes sense to distinguish between the ones forwarded via SSH and a common mechanism like we'll need to for containers just in case we need to explain it.

mjbvz

Direction looks good.

I think we do need an embedder specific API to support this properly for VSO. We can hold off on trying to implement it API until we have a good proposal here though since VSO is in no rush to adopt this.

Also keep in mind that the embedder API for port forwarding will have a few different requirements (such the forwarded endpoint being a https resource instead of just localhost). Not sure if these are something we need to support in our extension API as well or not

src/vs/vscode.proposed.d.ts

alexr00

@Chuxel and @roblourens If the SSH extension wanted to provide it's own forwarding, it could do so with forwardPort in RemoteAuthorityResolver, but it doesn't have too. If that implementation is provided, then all port forwarding would happen through there. If not provided, all would happen through the common mechanism in the core. The ports in the SSH config file would be sent to the core through toForward and forwarded by the core(or the provided implementation in forwardPort.

It makes sense to have the option to show this in the UI. To distinguish how the port is forwarded, I just added a forwardMechanism to Port.

@mjbvz For the embedder API, I don't think the port number vs. https address will mater here since we can change how this is handled in the tunnel service (or before it reaches the tunnel service). For SSH and containers, a port number is enough. I'll need to confirm for VS Online (not embedder version).

alexdima · 2019-11-20T09:47:01Z

src/vs/vscode.proposed.d.ts

+		 * An array of ports to be forwarded once connected. For example,
+		 * if the extension wants to have a configuration file with some default ports, they could be passed through for forwarding here.
+		 */
+		toForward?: { remote: number, local: number, name: string, closeable?: boolean }[];


What is the difference between toForward and candidates ? Why is here a type inlined instead of using Port. Perhaps Port should have the closeable property too.

candidates are ports that the user might want to forward. We will show them in the port UI to make it easy/obvious that these might be useful ports. toForward are ports that the extension knows should definitely be forwarded. toForward ports will be forwarded.

I don't think that closeable or name is appropriate in the other places. A candidate port doesn't need a name, because the user hasn't taken action on it yet. Same for a published port. Similarly, candidate and published ports aren't closeable. I'm also not sure that when an extension implements forwardPort they should be allowed to decide if that port that they're forwarding is closeable.

But then toForward should be removed and the extension can call in a loop asExternalUri.

asExternalUri doesn't allow the extension to give the port a name or choose a specific port.

But then should we just add separate port forwarding API, i.e. acknowledge that asExternalUri is not sufficient and add something more powerful?

I think the use of asExternalUri is different from toForward:

Any extension can call asExternalUri to get a usable uri. Generally, I expect that extension calling asExternalUri only care that they have a usable uri, not the specifics of how they get that usable uri. We don't actually want them to care about specifics. They should "just work"

Only a resolver can use toForward since it is used as part of the remote session setup. The session setup knows that it is remote, so it's the resolver's job to care about specifics such as port numbers.

@alexandrudima does that seem reasonable? I'm open to expanding the API, but I think we actually want to keep it narrow here.

Given the functionality desire that a resolver can forward ports, there are two ways to go about it:

Add PortInformation.toForward as is proposed here. This is an expansion of the current API. This means that only resolvers can forward ports, but then they cannot stop the forwarding because toForward is provided by them once and then resolvers cannot call Dispose on something to stop them. So if a config file changes, it is not possible for a resolver to react to it and stop forwarding the current ports and begin forwarding the new ports.

Add vscode.workspace.forwardPort(...): Port. This means we do not add PortInformation.toForward. This is also an expansion of the current API. But this means any extension, including resolvers, can forward ports. Extensions end up forwarding ports today only as a side-effect of calling asExternalUri, so extensions already have the "access" to do port forwarding. Since Port extends Disposable, an extension could stop a certain port forwarding by calling Dispose. So if a config file changes, a resolver could potentially react to it and stop forwarding the current ports and begin forwarding the new ports.

I don't agree with the argument that PortInformation.toForward is somehow leading to a smaller or less expanded API compared to adding vscode.workspace.forwardPort(...): Port. IMHO they are both API expansions, with different characteristics.

Finally, I think candidates: Port[] is typed in a funny way in this proposal. A resolver would need to return Port objects as candidates, but what does it mean to call dispose() on such candidates?

just FYI, today we let the user to choose port number and the name they desire, and they can dispose the port anytime they want.

@fara-nak thanks for taking a look! Choosing a name and port number will be exposed through the UI.

mjbvz · 2019-11-21T01:04:20Z

@alexr00 Makes sense. I agree we can change our internal TunnelService to be more flexible when we need to support the embedder case as well

src/vs/vscode.proposed.d.ts

alexr00 · 2019-11-25T11:51:52Z

Some notes about the changes just pushed:

Keeping candidates because we don't want to automatically forward ports just because the extension knows they're useful.
Keeping published because it isn't the same as forwarded and we want to differentiate. Full discussion here: Remote Explorer shows container published ports as "forwarded", which is misleading vscode-remote-release#1809
Changing from toForward to a workspace.forwardPort so that extensions that have forwarded ports in settings can respond to settings changes if they want. Also just gives more flexibility.
Separate interface for Tunnel and Port so that candidates and published make more sense.
Added localAddress to Port and Tunnel.
Removed restorePorts with the expectation that it becomes a setting instead.

src/vs/vscode.proposed.d.ts

alexr00 · 2019-11-29T14:12:55Z

Updated again to incorporate further feedback. I think the current state will cover all known use cases, and no longer has the containers specific "published". Instead, I've changed it to detected so that if SSH, for example, wants to find list all open SSH tunnels and show them it can us the same detected API for it.

roblourens · 2019-11-29T19:01:31Z

src/vs/vscode.proposed.d.ts

+		 * The localAddress should be the complete local address for connecting to the port. Tunnels provided through
+		 * detected are read-only from the forwarded ports UI.
+		 */
+		detected?: { remotePort: number, localAddress: string }[];


complete local address for connecting to the port like a URL or just localhost:1234?

just localhost:1234, I'll add an example to the comment.

Part of microsoft/vscode-remote-release#1777 and #81388

alexr00 · 2019-12-10T14:45:19Z

Thank you for all the feedback! Proposal is merged in to vscode.proposed.d.ts and will continue to evolve as it's used.

alexr00 self-assigned this Nov 19, 2019

roblourens reviewed Nov 19, 2019

View reviewed changes

mjbvz approved these changes Nov 19, 2019

View reviewed changes

src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved

alexr00 commented Nov 20, 2019

View reviewed changes

alexdima reviewed Nov 20, 2019

View reviewed changes

roblourens reviewed Nov 22, 2019

View reviewed changes

src/vs/vscode.proposed.d.ts Show resolved Hide resolved

roblourens reviewed Nov 26, 2019

View reviewed changes

src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved

roblourens approved these changes Nov 29, 2019

View reviewed changes

alexr00 added 13 commits December 10, 2019 14:22

First pass at port API needed for port UI

5b3d436

Part of microsoft/vscode-remote-release#1777 and #81388

Add toForward and restorePorts

fc45e5a

some more tweaks

9a6fb78

Feedback

97918fc

Feedback

3e992ce

Return type of forwardPort

36d102e

More comments

be18ebe

Remove candidates

b3f6e2d

Feedback from API discussion.

005319b

change address to host

87ee853

Switch from resolver event to makeTunnel

6f5deb1

Feedback from standup

9ff228c

Touch-ups

309c8df

alexr00 force-pushed the alexr00/PortForwardingAPI branch from c544068 to 309c8df Compare December 10, 2019 14:42

Fix compile errors

bb47afe

alexr00 merged commit 9566da2 into master Dec 10, 2019

alexr00 deleted the alexr00/PortForwardingAPI branch December 10, 2019 14:44

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at port API needed for port UI #85117

First pass at port API needed for port UI #85117

alexr00 commented Nov 19, 2019

roblourens commented Nov 19, 2019

roblourens Nov 19, 2019

alexr00 Nov 20, 2019

alexdima Nov 20, 2019

fara-nak Nov 23, 2019 •

edited

Loading

alexr00 Nov 25, 2019

Chuxel commented Nov 19, 2019 •

edited

Loading

mjbvz left a comment

alexr00 left a comment •

edited

Loading

alexdima Nov 20, 2019

alexr00 Nov 20, 2019

alexr00 Nov 20, 2019

alexdima Nov 20, 2019

alexr00 Nov 20, 2019

alexdima Nov 21, 2019

alexr00 Nov 21, 2019 •

edited

Loading

alexdima Nov 22, 2019

fara-nak Nov 23, 2019

alexr00 Nov 25, 2019

mjbvz commented Nov 21, 2019

alexr00 commented Nov 25, 2019 •

edited

Loading

alexr00 commented Nov 29, 2019

roblourens Nov 29, 2019

alexr00 Dec 2, 2019

alexr00 commented Dec 10, 2019

First pass at port API needed for port UI #85117

First pass at port API needed for port UI #85117

Conversation

alexr00 commented Nov 19, 2019

roblourens commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fara-nak Nov 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chuxel commented Nov 19, 2019 • edited Loading

mjbvz left a comment

Choose a reason for hiding this comment

alexr00 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexr00 Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjbvz commented Nov 21, 2019

alexr00 commented Nov 25, 2019 • edited Loading

alexr00 commented Nov 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexr00 commented Dec 10, 2019

fara-nak Nov 23, 2019 •

edited

Loading

Chuxel commented Nov 19, 2019 •

edited

Loading

alexr00 left a comment •

edited

Loading

alexr00 Nov 21, 2019 •

edited

Loading

alexr00 commented Nov 25, 2019 •

edited

Loading