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

Add port validation checks for HTTP streaming #315

Open
wants to merge 6 commits into
base: v4.x
Choose a base branch
from

Conversation

hallvictoria
Copy link
Contributor

@hallvictoria hallvictoria commented Nov 14, 2024

For HTTP streaming, the port is selected by the platform. However, there was no logic that checks if the selected port is valid and available. In the case where the OS is Windows and VNET integration is enabled, the platform defaults to port 0. Port 0 isn't valid, so any proxy requests fail, and the function execution times out.

This fix adds validation on the selected port value. If the port value is 0, the worker will search for and bind to an available port. It first starts searching at port 55000, and if that port isn't available, it will pick a random port between 55000 and 65535 (max port value) and search again. It continues this process until a port is found.

There are no behavioral changes if the selected port value is not 0. The only scenario this change affects then is for Windows + VNET integration enabled.

@hallvictoria hallvictoria marked this pull request as ready for review November 14, 2024 20:08
@hallvictoria hallvictoria requested a review from a team as a code owner November 14, 2024 20:08
src/http/httpProxy.ts Outdated Show resolved Hide resolved

// Function to find an open port starting from a specified port
function findOpenPort(callback: (port: number) => void): void {
const server = net.createServer();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we creating a new server? Can 't we use server already created?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server we created is already listening in on port 0. We could use that one, but it would be closing and re-listening a lot. I created a new one to keep the logic cleaner and because using net.createServer() should be faster when just checking the ports.

const server = net.createServer();

function tryPort(port: number) {
server.once('error', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a retry limit. If the vnet has blocked all ports, this will lead to stack overflow.

const server = net.createServer();

function tryPort(port: number) {
server.once('error', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will retry finding a new port for all errors. How about we check if (err.code === 'EADDRINUSE') and then find the next port.

const randomPort = getRandomPort();
tryPort(randomPort);
});

Copy link

@gavin-aguiar gavin-aguiar Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add server.removeAllListeners(); . This is to remove all listeners for each recursive call.
https://nodejs.org/api/events.html#nodeeventtargetremovealllistenerstype

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants