- 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.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -309,15 +309,31 @@ func createWorkspaceWithApps(t *testing.T, client *codersdk.Client, orgID uuid.U | ||
| }, | ||
| }, workspaceMutators...) | ||
| // Intentionally going to choose a port that will never be chosen. | ||
| // 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 commentThe reason will be displayed to describe this comment to others.Learn more. AFAIK ports <1024 are privileged. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others.Learn more. Nice, thanks for the explanation 👍 | ||
| conn, err := net.DialTimeout("tcp", fakeAppURL, time.Millisecond*100) | ||
| if err == nil { | ||
| // In the absolute rare case someone hits this. Writing code to find a free port | ||
| // seems like a waste of time to program and run. | ||
| _ = conn.Close() | ||
| t.Errorf("an unused port is required for the fake app. "+ | ||
| "The url %q happens to be an active port. If you hit this, then this test"+ | ||
| "will need to be modified to run on your system. Or you can stop serving an"+ | ||
| "app on that port.", fakeAppURL) | ||
| t.FailNow() | ||
| } | ||
| appURL := fmt.Sprintf("%s://127.0.0.1:%d?%s", scheme, port, proxyTestAppQuery) | ||
| protoApps := []*proto.App{ | ||
| { | ||
| Slug: proxyTestAppNameFake, | ||
| DisplayName: proxyTestAppNameFake, | ||
| SharingLevel: proto.AppSharingLevel_OWNER, | ||
| Url: fakeAppURL, | ||
| Subdomain: true, | ||
| }, | ||
| { | ||
| Slug: proxyTestAppNameOwner, | ||