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

Provide extension API to exclude ports from forwarding #115616

Open
alexr00 opened this issue Feb 2, 2021 · 28 comments
Open

Provide extension API to exclude ports from forwarding #115616

alexr00 opened this issue Feb 2, 2021 · 28 comments
Assignees
Labels
api api-finalization feature-request Request for new features or functionality ghcs-in-progress remote-explorer Remote explorer view
Milestone

Comments

@alexr00
Copy link
Member

alexr00 commented Feb 2, 2021

From @weinand:
Today the tunneling service blindly forwards all communication ports.
This includes ports that are used for debugging (even if our remote debugging feature does not need these ports to be forwarded).
This is confusing for users because they see ports that they are not really interested in.

I propose to add extension API so that individual ports or port ranges can be excluded from forwarding.
Debug extensions could use this API.

@alexr00 alexr00 self-assigned this Feb 2, 2021
@alexr00 alexr00 transferred this issue from microsoft/vscode-remote-release Feb 2, 2021
@alexr00 alexr00 added api api-proposal feature-request Request for new features or functionality remote-explorer Remote explorer view labels Feb 2, 2021
@alexr00 alexr00 added this to the February 2021 milestone Feb 2, 2021
@alexr00
Copy link
Member Author

alexr00 commented Feb 2, 2021

We currently have a setting that lets user configure "ignored" ports:

	"remote.portsAttributes": {
		"35000-38000": {
			"label": "My server",
			"onAutoForward": "ignore",
		}
	}

This setting also allows setting other properties of the port, such as the label.

Proposal:

	export enum PortAutoForwardAction {
		Notify,
		OpenBrowser,
		Silent,
		Ignore
	}

	export interface PortAttributes {
		port: number;
		autoForwardAction: PortAutoForwardAction
	}

	export interface PortAttributesProvider {
		providePortAttributes(port: number[], token: CancellationToken): ProviderResult<PortAttributes[]>;
	}

	export namespace workspace {
		export function registerPortAttributesProvider(portRange: [number, number], provider: PortAttributesProvider): Disposable;
	}

@alexr00
Copy link
Member Author

alexr00 commented Feb 3, 2021

I discussed with @weinand and there are several places unwanted ports can come from:

a) Debugees. These are started by debugger extensions, and the debug internals of VS Code know nothing about them.
b) Debug extensions setting up a server for communication with the debuggee. This is what JS debug does.
c) Debug adapters. The debug internals of VS Code actually do know about this, but this isn't where we're seeing most unwanted ports come from.
d) Language servers. I haven't see reports of this, but it could absolutely happen.
e) Extensions in general. Some of these servers could be user facing, and some may not be. We don't have data on this.

Cases (a) and (b) above are the most common cases that we want to solve for. We discussed excluding all processes that come from the extension host from auto forwarded, but that does not solve case (a).

@connor4312 since JS debug would be the first client of any API I'd love to get your input here. The above PortsAttributesProvider API is very ports centric. We could instead have a ProcessDetailProvider, which returns something that indicates whether a process is intended to be user-facing.

@connor4312
Copy link
Member

The API seems generally sensible to me.

While a consumer could pass it as [0, 2 ** 16], it may be more ergonomic for the portRange to be optional: for debugging I always request port 0 and let the OS choose a free port to use. I imagine that other consumers frequently do this too.

Particularly in this case, providers will often get called with ports they know nothing about. It looks like providePortAttributes can take a bulk list of ports, but it can only return information for none of them (undefined) or all of them. If my debug port is mixed in a list of other ports, I can't say to ignore that one and fall back on the others. So maybe it's better to have it take a single port and return a ProviderResult for the one port.

@weinand
Copy link
Contributor

weinand commented Feb 8, 2021

@connor4312 you said

"I always request port 0 and let the OS choose a free port to use."

When does your DA or extension know what the free port is?
I'm asking because the port forwarding service will call PortAttributesProvider.providePortAttributes as soon as it detects new ports through scanning and it would be a problem if your DA would not know at that time that the given port is a debug port.


and you said:

"providers will often get called with ports they know nothing about" but there is no way "to ignore that one".

The return type of PortAttributesProvider.providePortAttributes could be an array of PortAttributes or undefined, that is ProviderResult<(PortAttributes | undefined)[]>.

@connor4312
Copy link
Member

it would be a problem if your DA would not know at that time that the given port is a debug port.

Yea, that's correct. I would need to wait a few moments so that any port resolutions can happen. Or if this becomes problematic I could restrict it to a specific port range, then manually choose a free port and only have the waiting logic for ports in that range

could be an array of

Yep

@alexr00
Copy link
Member Author

alexr00 commented Feb 9, 2021

That should still be ok then. Even if you have to wait a few moments, you already know that you need to wait a few moments. So if the call comes from VS Code to your extension asking your extension to provide port information, you can wait the time you need to know the port number before returning the port information.

@alexr00
Copy link
Member Author

alexr00 commented Feb 9, 2021

New proposal:

	export interface ProcessInformation {
		isUserFacing: boolean;
	}

	export interface ProcessInformationProvider {
		provideProcessInformation(process: { pid: number, port: number }, token: CancellationToken): ProviderResult<ProcessInformation>;
	}

	export namespace workspace {
		export function registerProcessInformationProvider(selector: { portRange?: [number, number], processRange?: [number, number] }, processInformationProvider: ProcessInformationProvider): Disposable;
	}

@alexr00
Copy link
Member Author

alexr00 commented Feb 9, 2021

@connor4312 the API includes the pid, and I imagine that you'll know the pid almost immediately. Is that correct?

@connor4312
Copy link
Member

In the auto attach / debug terminal case, not any earlier than I will the port: the debugee still needs to connect back to the extension and tell us it exists before we'll know its port. But for less esoteric cases passing the pid is good, I'm sure other consumers will appreciate that.

API looks good now. I'm not sure how useful the process range is -- as far as I know all OSes assign process IDs with an increment counter, so an extension would not know the process ID of whatever they're interested in ahead of time.

@alexr00
Copy link
Member Author

alexr00 commented Feb 9, 2021

@connor4312 since this API is very specific, I'm now trying to find a way to avoid adding API.

You mentioned above using a port range instead of 0. How averse to this are you?

If you use a range, then you could use the configuration API to simply add to remote.portsAttributes to exclude that range. Ex:

"remote.portsAttributes": {
  "60000-63000": {
    "onAutoForward": "ignore"
  }
}

If you set that before starting your debugee, then I will get the setting changed event first and know to ignore your range. What do you think of this?

@connor4312
Copy link
Member

I think using a provider in combination with a ports range is ideal, in order to avoid accidentally excluding user programs. Then the delay in that case would be less problematic: in the high likelihood that the queried port is actually a debugged program, it's not going to be shown to the user anyway.

@alexr00
Copy link
Member Author

alexr00 commented Feb 10, 2021

My (and @jrieken's) concern is that this a lot of API to add to support a very narrow scenario: don't auto forward debugee's debug ports. This API doesn't support any other scenario. Would you be willing to try the settings approach?

@jrieken
Copy link
Member

jrieken commented Feb 12, 2021

Yes, the concern is that port forwarding, by its nature, forwards many unwanted ports. Ports from processes I am not interested in, ports that don't speak http, ports I cannot connect to etc. The screen shot below is what I see when I connect to my raspberry pi which run jupyter-lab. There are 7 ports forwarded (prominently announced via the status bar and notification) and I am only interested in one of them (iff at all)

image

I know that debugging ports contribute to this problem but they are only a small part of this overall problem. So, an API should tackle (or enable tackling) the whole problem not just a selected portion.

@alexr00
Copy link
Member Author

alexr00 commented Feb 12, 2021

Proposal as of the time of this comment:

	export enum PortAutoForwardAction {
		Notify,
		OpenBrowser,
		Silent,
		Ignore
	}

	export interface PortAttributes {
		port: number;
		autoForwardAction: PortAutoForwardAction
	}

	export interface PortAttributesProvider {
		providePortAttributes(port: number, pid: number, token: CancellationToken): ProviderResult<PortAttributes>;
	}

	export namespace workspace {
		export function registerPortAttributesProvider(portRange: [number, number], provider: PortAttributesProvider): Disposable;
	}

@connor4312
Copy link
Member

I can give the config approach a shot. Could scope to say 100 ports somewhere random, and try to preferentially choose those when starting to debug.

@alexr00
Copy link
Member Author

alexr00 commented Mar 8, 2021

@connor4312 I have merged the first pass at this API. My hope is that it is pretty easy to use, so even if it changes a lot as we continue to review it, you won't lose a lot of work by adopting it now.

export enum PortAutoForwardAction {
Notify = 1,
OpenBrowser = 2,
OpenPreview = 3,
Silent = 4,
Ignore = 5
}
export interface PortAttributes {
port: number;
autoForwardAction: PortAutoForwardAction
}
export interface PortAttributesProvider {
providePortAttributes(ports: number[], pid: number | undefined, commandLine: string | undefined, token: CancellationToken): ProviderResult<PortAttributes[]>;
}
export namespace workspace {
/**
* If your extension listens on ports, consider registering a PortAttributesProvider to provide information
* about the ports. For example, a debug extension may know about debug ports in it's debuggee. By providing
* this information with a PortAttributesProvider the extension can tell VS Code that these ports should be
* ignored, since they don't need to be user facing.
*
* @param portSelector If registerPortAttributesProvider is called after you start your process then you may already
* know the range of ports or the pid of your process.
* @param provider The PortAttributesProvider
*/
export function registerPortAttributesProvider(portSelector: { pid?: number, portRange?: [number, number] }, provider: PortAttributesProvider): Disposable;
}

You'll need to implement a PortAttributesProvider. providePortAttributes will be called whenever a port is auto-detected. For any ports that shouldn't be user facing, you should return a PortAttributes with an autoForwardAction: PortAutoForwardAction .Ignore.

If you have time to register your provider after you know the pids you care about, then you can include the pid your portSelector.

Internally, I batch ports that have the same pid, so you should see almost no delay between calls if your ports are all in the same process.

I'm thinking of adding a regex "commandLine" to the portSelector so that you can select on that too. Would that be useful for JS Debug?

@alexr00 alexr00 modified the milestones: May 2023, June 2023 May 26, 2023
@alexr00 alexr00 modified the milestones: June 2023, July 2023 Jun 22, 2023
alexr00 added a commit that referenced this issue Jun 22, 2023
alexr00 added a commit that referenced this issue Jun 22, 2023
@jrieken jrieken modified the milestones: July 2023, August 2023 Jul 17, 2023
@alexr00 alexr00 modified the milestones: August 2023, July 2023 Jul 18, 2023
alexr00 added a commit that referenced this issue Aug 9, 2023
* Update port attributes arguments to be a property bag
Part of #115616

* Gracefully handle API breakage
@alexr00 alexr00 modified the milestones: August 2023, September 2023 Aug 22, 2023
@joaomoreno
Copy link
Member

Let's bring this back into the plan once microsoft/vscode-python#21292 is complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api api-finalization feature-request Request for new features or functionality ghcs-in-progress remote-explorer Remote explorer view
Projects
None yet
Development

No branches or pull requests

9 participants