Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 14, 2024
Merged

test: prevent apptest from choosing ports in use #12580

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions coderd/workspaceapps/apptest/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

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?

Copy link
Member Author

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 👍

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor

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 👍

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,
// Hopefully this IP and port doesn't exist.
Url: "http://127.1.0.1:65535",
Subdomain: true,
Url: fakeAppURL,
Subdomain: true,
},
{
Slug: proxyTestAppNameOwner,
Expand Down