-
Notifications
You must be signed in to change notification settings - Fork 124
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
fix(cli): Use an IPV6-enabled network in CLI's compose file #1299
base: main
Are you sure you want to change the base?
Conversation
4663b68
to
ef6a1c7
Compare
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.
Looks great, and nice work with reducing the config to a single file, much easier to maintain!
I've left a comment with regard to configuring the network, as my opinion is that we should aim to make it work by default as much as possible and offload any additional requirements to the user via a guide on setting up a custom docker compose, e.g. by adding a CLI command electric-sql eject
that copies the docker compose file to the dev's project for them to configure any way they want.
e1f75a5
to
4b54948
Compare
I've tested the updated implementation in the following scenarios by running a starter app on an Ubuntu DO droplet and installing the development version of the CLI as an npm pack:
|
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 work! Looks good and love the new way of configuring the network!
Testing it I seem to be having issues with flags and defaults - the EDIT: it was a local build issue, runs fine when I make sure I'm running the right CLI build!--with-postgres
flag doesn't seem to be working at all no matter how I set it. I saw you raised a seemingly related issue #1404, are you seeing the same thing or have I perhaps messed something up in my local build?
Also, we might want to switch the ELECTRIC_WRITE_TO_PG_MODE
default to direct_writes
rather than logical_replication
since we're shifting to that soon (see #1342 and we've already updated examples and docs to use direct writes), cause when the --with-postgres
flag was not detected the error I got was the LOGICAL_PUBLISHER_HOST
not being set which can be confusing (as opposed to the "check your DATABASE_URL" error in direct_writes
mode) - feel free to ignore this for this PR though
) | ||
} | ||
|
||
function useExternalDockerNetwork(opt: string): string { |
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.
nit: we can use a more descriptive/strict type signature and argument
function useExternalDockerNetwork(networkName?: string) : 'true' | 'false' {
return (networkName && networkName.length > 0) ? 'true' : 'false'
}
or even return a boolean and let the calling function cast it to an appropriate string (?) I'd avoid the toString
call cause its behaviour never feels obvious to me
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.
But returning a boolean here means using toString()
later to cast it. I'm not sure what is not obvious about the behaviour of toString
.
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.
mostly that in JS I'm never sure what the toString
implementation will result in (e.g. 't'
and 'f'
) and if we specifically need 'true'
or 'false'
strings having a flag ? 'true' : 'false'
feels more explicit. Granted for the boolean case it's fairly standard so feel free to ignore me, I just always start from a "don't trust JS to have standard behaviour" standpoint :P
This needs to be addressed separately. |
This PR changes two things:
Background
Beyond requiring support in the OS, the network router and the ISP, there are two more problems with using IPv6 for database connections:
docker compose
The first problem is relevant to those users who utilize
docker run
to start the sync service. The second problem currently makes our CLI unusable with IPv6 because the compose file bundled with it has no provisions for IPv6 networking. This PR address the latter.Testing
I have tested connectivity from the sync service to a Supabase database over IPv6 and to a DigitalOcean Managed PostgreSQL over IPv4. Here's what I've learned:
docker run ... curlimages/curl -6 https://ifconfig.co/ip
fails to connect to the remote host) it is still possible to define an IPv6-enabled network in a compose file and the services started bydocker compose
will be able to connect to remote hosts over IPv6.enabled_ipv6: true
line. But for this to work, the Docker daemon must be configured with an IPv6 default pool. Otherwise, the network definition in the compose file has to include a subnet definition.external: true
to instructdocker compose
to use a user-created Docker network instead of creating a new one.In summary, including a default IPv6-enabled network definition in our packaged compose file should work regardless of how users have their Docker daemon configured.
There is a potential for conflict, though. If there's already a Docker network on the machine that has an overlapping subnet with the one we define in our compose file,
docker compose up
will fail. To minimize the chances of conflicts, I have chosen a subnet with randomly generated groups that has the size of only 4 address. To make it fully conflict-proof, the compose file has three environment variables that can be used to either redefine the subnet or instructdocker compose
to use a different, user-created Docker network by name.Fixes #863.