- Notifications
You must be signed in to change notification settings - Fork928
test: prevent apptest from choosing ports in use#12580
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Apptest requires a port without a listening server to test failurecases. This port was chosen and had a chance of actually beingprovisioned. To prevent this accident, a port <1k is chosen,since those will never be allocated.
| // Ports <1k will never be selected. 396 is for some old OS over IP. | ||
| // It will never likely be provisioned. Using quick timeout since | ||
| // it's all localhost | ||
| fakeAppURL := "http://127.1.0.1:396" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AFAIK ports <1024 are privileged.
Does the test suite run as root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
They do not. We do not want a listener on this port. So the fact they are privileged is what I am hoping for 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Essentially we want a port that will never be listened on in our tests. If we listen on port 0, then read the port and close the listener, there's a potential that another test will take that port that we hope nothing will listen on.
Using a port < 1024 and on an uncommon app as the URL for a "not running" app should serve our use case here. We're just dialing it to make sure it's not in use and then failing fast if it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's unfortunate, but to do this in a less potentially flakey way we would need to stub out the dialer somehow. In CI and on dev machines though I don't expect this port to ever be in use so I doubt we'll ever hit this problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice, thanks for the explanation 👍
Apptest requires a port without a listening server to test failure cases. This port was chosen and had a chance of actually being provisioned. To prevent this accident, a port <1k is chosen, since those will never be allocated.
Closes 2/3 of#12531 (comment)
One of the flakes (#12531 (comment)) looks different.
I am able to reproduce the exact error if I spin up a server on
65535. I think this fix should prevent this in future.