Skip to content
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: don't add FLY_REGION prefix when using recent flyctl version #187

Merged
merged 2 commits into from
May 16, 2023
Merged

fix: don't add FLY_REGION prefix when using recent flyctl version #187

merged 2 commits into from
May 16, 2023

Conversation

benwaffle
Copy link
Contributor

@benwaffle benwaffle commented May 8, 2023

New versions of flyctl, when running fly postgres attach, use flycast URLs to connect to the DB. This provides the same geo-aware load balancing that the stack already achieves today by prefixing the hostname with the region. Flycast doesn't support region prefixes. This has been causing failures for new apps.

For example:

Old: postgres://top2.nearest.of.my-postgres.internal
New: postgres://my-postgres.flycast

The old format supports a region prefix, but the new one does not.

Closes #88
Closes #101
Closes #182

New versions of `flyctl`, when running `fly postgres attach`, use
[flycast](https://fly.io/docs/reference/private-networking/#flycast-private-load-balancing)
URLs to connect to the DB. This provides the same geo-aware load
balancing that the stack already achieves today by prefixing the
hostname with the region. Flycast doesn't support region prefixes. This
has been causing failures for new apps.

For example:

Old: `postgres://top2.nearest.of.my-postgres.internal`
New: `postgres://my-postgres.flycast`

The old format supports a region prefix, but the new one does not.

Fixes #182
@benwaffle
Copy link
Contributor Author

cc @rubys @DAlperin

@benwaffle
Copy link
Contributor Author

@MichaelDeBoey can you take a look at this PR when you get some time?

@MichaelDeBoey MichaelDeBoey changed the title Fix DB connections with recent flyctl fix: don't add FLY_REGION prefix when using recent flyctl version May 11, 2023
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to fix this @benwaffle! 🙏

I do have 1 concern though: I would either remove the message in the README and just assume people are using the latest version of flyctl (just like we do with all other changes in our stacks) or I would conditionally add/remove the FLY_REGION prefix to databaseUrl.host

Not sure which one I would prefer though 🤔

Pinging the team to hear what they think about this @brophdawg11 @jacob-ebey @markdalgleish @mcansh

@dangra
Copy link

dangra commented May 12, 2023

What about adding the FLY_REGION prefix only if the host ends with .internal? Or not adding it if it ends with .flycast ?

@rubys
Copy link

rubys commented May 16, 2023

I'd appreciate it if this were expedited, developers are seeing problems with apps v2 and we are making a push to move people off of apps v1.

@markdalgleish markdalgleish merged commit c8f2dec into remix-run:main May 16, 2023
@markdalgleish
Copy link
Member

@rubys The updated code looked good to me. Merged! :)

@benwaffle benwaffle deleted the fly-db-url-fix branch May 16, 2023 02:24
This was referenced May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fly.io -> Error: Can't reach database server Error connect database
5 participants