Skip to content

fix: use ephemeral port for postgres, except for TestPubsub_Disconnect #7798

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
Jun 5, 2023
Merged

fix: use ephemeral port for postgres, except for TestPubsub_Disconnect #7798

Show file tree
Hide file tree
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
34 changes: 0 additions & 34 deletions coderd/database/postgres/postgres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@ package postgres
import (
"database/sql"
"fmt"
"net"
"os"
"strconv"
"sync"
"time"

"github.com/cenkalti/backoff/v4"
Expand All @@ -18,10 +16,6 @@ import (
"github.com/coder/coder/cryptorand"
)

// Required to prevent port collision during container creation.
// Super unlikely, but it happened. See: https://github.com/coder/coder/runs/5375197003
var openPortMutex sync.Mutex

// Open creates a new PostgreSQL database instance. With DB_FROM environment variable set, it clones a database
// from the provided template. With the environment variable unset, it creates a new Docker container running postgres.
func Open() (string, func(), error) {
Expand Down Expand Up @@ -68,17 +62,6 @@ func OpenContainerized(port int) (string, func(), error) {
return "", nil, xerrors.Errorf("create tempdir: %w", err)
}

openPortMutex.Lock()
if port == 0 {
// Pick an explicit port on the host to connect to 5432.
// This is necessary so we can configure the port to only use ipv4.
port, err = getFreePort()
if err != nil {
openPortMutex.Unlock()
return "", nil, xerrors.Errorf("get free port: %w", err)
}
}

resource, err := pool.RunWithOptions(&dockertest.RunOptions{
Repository: "postgres",
Tag: "13",
Expand Down Expand Up @@ -114,10 +97,8 @@ func OpenContainerized(port int) (string, func(), error) {
config.RestartPolicy = docker.RestartPolicy{Name: "no"}
})
if err != nil {
openPortMutex.Unlock()
return "", nil, xerrors.Errorf("could not start resource: %w", err)
}
openPortMutex.Unlock()

hostAndPort := resource.GetHostPort("5432/tcp")
dbURL := fmt.Sprintf("postgres://postgres:postgres@%s/postgres?sslmode=disable", hostAndPort)
Expand Down Expand Up @@ -166,18 +147,3 @@ func OpenContainerized(port int) (string, func(), error) {
_ = os.RemoveAll(tempDir)
}, nil
}

// getFreePort asks the kernel for a free open port that is ready to use.
func getFreePort() (port int, err error) {
// Binding to port 0 tells the OS to grab a port for us:
// https://stackoverflow.com/questions/1365265/on-localhost-how-do-i-pick-a-free-port-number
listener, err := net.Listen("tcp", "localhost:0")
if err != nil {
return 0, err
}

defer listener.Close()
// This is always a *net.TCPAddr.
// nolint:forcetypeassert
return listener.Addr().(*net.TCPAddr).Port, nil
}
19 changes: 9 additions & 10 deletions coderd/database/pubsub_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import (
"database/sql"
"fmt"
"math/rand"
"net"
"net/url"
"strconv"
"testing"
"time"
Expand Down Expand Up @@ -117,11 +115,17 @@ func TestPubsub_ordering(t *testing.T) {
}
}

// disconnectTestPort is the hardcoded port for TestPubsub_Disconnect. In this test we need to be able to stop Postgres
// and restart it on the same port. If we use an ephemeral port, there is a chance the OS will reallocate before we
// start back up. The downside is that if the test crashes and leaves the container up, subsequent test runs will fail
// until we manually kill the container.
const disconnectTestPort = 26892
Copy link
Member

@mafredri mafredri Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I hope this, by default, usually falls outside the dynamic port range of :0, so we won't collide with other tests/test packages. Are the rules same on Windows?

At least on my NAS, it seems OK:

❯ cat /proc/sys/net/ipv4/ip_local_port_range
32768	60999

Assuming these values are set differently, there could still be port collisions.

Something like port := testutil.Port(26892), where that function takes the input as preferred, but falls back to seeking the closest free port would help in the case mentioned in the comment. The package can store "used ports", unfortunately I don't think that state will be shared between test packages (could be wrong though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding Windows, we don't run these tests there. Presumably because Docker on Windows is more effort/overhead than it's worth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using testutil.Port() is clever, but thinking about it a bit, I don't think we should be that clever in tests. If the test leaks the container, I'd rather us know about it by failing tests, versus doing something different and rare.


// nolint: paralleltest
func TestPubsub_Disconnect(t *testing.T) {
t.Parallel()
// we always use a Docker container for this test, even in CI, since we need to be able to kill
// postgres and bring it back on the same port.
connectionURL, closePg, err := postgres.OpenContainerized(0)
connectionURL, closePg, err := postgres.OpenContainerized(disconnectTestPort)
require.NoError(t, err)
defer closePg()
db, err := sql.Open("postgres", connectionURL)
Expand Down Expand Up @@ -191,13 +195,8 @@ func TestPubsub_Disconnect(t *testing.T) {

// restart postgres on the same port --- since we only use LISTEN/NOTIFY it doesn't
// matter that the new postgres doesn't have any persisted state from before.
u, err := url.Parse(connectionURL)
require.NoError(t, err)
addr, err := net.ResolveTCPAddr("tcp", u.Host)
require.NoError(t, err)
newURL, closeNewPg, err := postgres.OpenContainerized(addr.Port)
_, closeNewPg, err := postgres.OpenContainerized(disconnectTestPort)
require.NoError(t, err)
require.Equal(t, connectionURL, newURL)
defer closeNewPg()

// now write messages until we DON'T hit an error -- pubsub is back up.
Expand Down