balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.81k stars 1.95k forks source link

sails-postgres doesn't work with unix socket connections #6888

Open alxndrsn opened 4 years ago

alxndrsn commented 4 years ago

DB adapter & version: sails-postgres@

Unless I'm missing something obvious, sails-postgres won't work with PostgreSQL unix socket connections.

This seems to be a direct result of the conversion from separate datastore config values to a connectionString performed in sails-postgresql/helpers/register-data-store.js.

I will look into this further and hopefully provide a patch shortly.

sailsbot commented 4 years ago

@alxndrsn Thanks for posting! We'll take a look as soon as possible.

In the mean time, there are a few ways you can help speed things along:

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

alxndrsn commented 4 years ago

For the brave and desperate:

const fs = require('fs');

function patchAndReplace(path, replacements) {
  let text = fs.readFileSync(path, { encoding:'utf-8' });
  replacements.forEach(r => {
    text = text.replace(r.find, r.replace);
  });
  fs.writeFileSync(path, text, 'utf-8');
}

patchAndReplace('node_modules/sails-postgresql/helpers/register-data-store.js', [
  {
    find:    "if (!_.has(inputs.config, 'url')) {",
    replace: "if (false && !_.has(inputs.config, 'url')) {",
  },
]);

patchAndReplace('node_modules/machinepack-postgresql/machines/create-manager.js', [
  {
    find:    'required: true',
    replace: '//required: true',
  },
  {
    find:    "// Remember: connection string takes priority over `meta` in the event of a conflict.",
    replace: "if(inputs.connectionString !== undefined)",
  },
]);

I'll try to wrap this in a couple of PRs soonish.

Noitidart commented 4 years ago

@alxndrsn thanks always for posting the bug, and then posting a brute force solution until permanent solution lands.

Noitidart commented 4 years ago

by any chance @alxndrsn would you have work around for this - https://github.com/balderdashy/sails/issues/6876 - its using bigserial in sails-postgres

johnabrams7 commented 4 years ago

@alxndrsn Thanks for taking to time to explore this and providing a workaround for the moment (along with future plans for a PR). I'll bring this up with the team to include a fix in our latest sails-sql adapter being developed to feature a combination of PostgreSQL, MySQL, and MSSQL support henceforth. MySQL & MSSQL are currently ready to use with this adapter - PostgreSQL is the primary focus of its development at the moment.

alxndrsn commented 4 years ago

@johnabrams7 thanks for the update.

sails-sql is currently marked DO NOT USE IN PRODUCTION in big letters: https://www.npmjs.com/package/sails-sql

Until this reaches production, it would be great to get this ticket fixed in sails-postgresql; please let me know if that is a possibility, and if so how I can help. I'm happy to provide relevant PRs.

johnabrams7 commented 4 years ago

@alxndrsn I'll ask @mikermcneil if sails-sql's postgresql support is looking ready for production by January next year along with considering this among that release. Regardless, a PR is certainly welcome after the production release.

alxndrsn commented 4 years ago

@johnabrams7 thanks for the response. Does the following refer to sails-sql:

a PR is certainly welcome after the production release.

?

If so, please refer to my previous comment:

Until this reaches production, it would be great to get this ticket fixed in sails-postgresql; please let me know if that is a possibility, and if so how I can help. I'm happy to provide relevant PRs.

johnabrams7 commented 4 years ago

@alxndrsn, I understand the confusion and sorry for the delay. Yes, I was referring to the sails-sql adapter for a PR. We're ultimately still progressing sails-sql for the future of postgres developments so ideally these innovations will integrate best in that one. As soon as the major release of sails-sql becomes certain, I will notify you asap of its postgres status for unix socket connections.

alxndrsn commented 4 years ago

Thanks for the update. In the meantime, I'll be maintaining https://www.npmjs.com/package/sails-postgresql-redacted.

DaAwesomeP commented 4 years ago

@alxndrsn I apologize for posting here...I was going to open an issue but they are disabled in your repo. Would you mind providing a configuration example for using a UNIX socket with your alternative adapter?

alxndrsn commented 4 years ago

@DaAwesomeP thanks for asking :slightly_smiling_face:

I've enabled Issues on the repo: https://github.com/alxndrsn/sails-postgresql/issues, and added usage with sockets examples at https://github.com/alxndrsn/sails-postgresql#usage-with-unix-sockets

mortbauer commented 3 years ago

one year later still open?

Muhammadinaam commented 2 years ago

one year later still open?

two years now :)

chrismerkle commented 1 year ago

Hi @alxndrsn thank you for the sample code.

I have tried to connect with GCP using Unix connections and went through about 20 different tutorials at this point. I have tried your npm package sails-postgres-redacted without any success.

Simply put, I cannot find a way to utilize Sails.js with Google SQL.

Does anyone know if Sails.js plans to support this in the near future?

alxndrsn commented 1 year ago

@chrismerkle I don't use SailsJS any more, so I don't know the current state of things.

I suspect the key change was https://github.com/alxndrsn/sails-postgresql/commit/bfac929f346737101749e8a12634f32210487418, which should allow use of a socketed connection created as per https://node-postgres.com/features/connecting#unix-domain-sockets.

Noitidart commented 1 year ago

@alxndrsn out of curiosity, what are you using now? I heard many were moving to Next.js and Vercel.

alxndrsn commented 1 year ago

@Noitidart express + pg would be my recommendation :upside_down_face:

NachtRitter commented 1 year ago

@Noitidart I've been working for 3+ years with https://tsed.io It's great framework with a big number of built-in features and it's very easily customizable for needs of your project.

Noitidart commented 1 year ago

Thanks guys!

chrismerkle commented 1 year ago

@Noitidart we are moving to Next.js and Prisma ORM.

chrismerkle commented 1 year ago

@alxndrsn thank you SO much for the reply. I appreciate you!

chrismerkle commented 1 year ago

@alxndrsn it looks like the PG connection string just will not take the Unix socket, no matter how I try. Like many others, this is an issue when deploying to App Engine (GCP) and trying to use SQL (GCP). Even with all security and configuration set correctly, this style of connection within Sails.js simply will not connect. Happy to pay someone to help me figure this out.

alxndrsn commented 1 year ago

@chrismerkle IIRC the problem previously was that sails would try to break down the connectionString into its constituent parts, and then supply those separately to pg. At the time I filed this issue, I was using Google App Engine and Google Cloud SQL, and Sails's approach did not work for the unix sockets Cloud SQL provided to App Engine then. I got it working with various patches applied, and used the sails-postgresql-redacted package for a couple of years.

Have you tried connecting from App Engine using pg directly? What errors were you seeing there?

chrismerkle commented 1 year ago

Thank you for the update @alxndrsn!

I finally got Sails.js to work today with GCP SQL. If you follow the tutorials from Google you have either Unix Sockets or TCP options to try -- and sadly neither work with out of the box sails-postgres.

My workaround was to connect via a VPC connector with a dedicated IP address. This way I can connect to Cloud SQL using a regular Postgres connection string, directly to the DB.

https://cloud.google.com/appengine/docs/legacy/standard/python/outbound-ip-addresses

Then I whitelisted the new dedicated IP in Cloud SQL security settings, and forced SSL to require valid SSL certificates.

It may not be best practice for now, but it works -- and that's one less ticket in the "To Do" queue :)