migrate token pooling to postgres #8922

chris48s · 2023-02-18T19:25:17Z

This PR implements the suggestion from issue #8921 allowing us to migrate the token pool from redis to fly.io postgres.

github-actions · 2023-02-18T19:26:27Z

	Warnings
⚠️	This PR modified the server but none of its tests. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against dbf5c67

socket-security · 2023-02-18T19:26:51Z

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]

Powered by socket.dev

core/token-pooling/sql-token-persistence.integration.js

chris48s · 2023-02-18T20:29:46Z

package.json

@@ -54,8 +54,10 @@
    "lodash.groupby": "^4.6.0",
    "lodash.times": "^4.3.2",
    "node-env-flag": "^0.1.0",
+    "node-pg-migrate": "^6.2.2",


Even though we don't expect to change our DB schema super often, I think its worth having a migration package to make it easier to evolve this over time. Ideally we should also look at running npm run migrate up on deploy. For the first deploy I think it can live with doing it manually.

Does this need to be a prod dep or can it still be utilized as a dev dep?

In order to create the DB (in the first instance) or modify it (later) we need to be able to run npm run migrate up in the production environment so it has to be a prod dependency.

migrations/1676731511125_initialize.cjs

core/token-pooling/sql-token-persistence.js

package-lock.json

chris48s · 2023-02-18T20:32:42Z

core/token-pooling/sql-token-persistence.integration.js

+  beforeEach('Create temporary table', async function () {
+    await pool.query(
+      `CREATE TEMPORARY TABLE ${tableName} (LIKE github_user_tokens INCLUDING ALL);`
+    )
+  })
+  afterEach('Drop temporary table', async function () {
+    await pool.query(`DROP TABLE IF EXISTS pg_temp.${tableName};`)
+  })


This copies our table to a temp schema for the purposes of running the tests then clears it up again after. This means we can run the tests against a local DB with real data in it and not break our local data. We don't need a separate test DB. It also means the tests will keep step with schema changes from the migrations. We do need to migrate up before running the tests though.

This means we can run the tests against a local DB with real data in it and not break our local data. We don't need a separate test DB. It also means the tests will keep step with schema changes from the migrations. We do need to migrate up before running the tests though

No objections to this in the local/inner dev loop (nor CI) context, and agree the benefits outweigh the tradeoffs. Would be nervous abut this in the context of the real DB though (whether connected to deliberately or accidentally)

In principle it should be safe but I agree - I wouldn't want to run the tests while connected to the prod DB. Tbh that would be fairly unusual thing to do for any application. I think the fact that the dev dependencies (e.g: mocha, etc) aren't installed in the prod container is enough of a guard rail to stop this from happening.

Personally I'd tend to avoid connecting my dev environment up to a prod database for any application.

package.json

chris48s · 2023-02-18T20:34:26Z

server.js

+if (config.private.redis_url != null) {
+  console.warning(
+    'RedisTokenPersistence is deprecated for token pooling and will be removed in a future release. Migrate to SqlTokenPersistence'
+  )
+}
+


I decided that it is useful to keep both backends around for some length of time because it allows us to switch between them just by changing an env var - no code changes needed. Doing this means we can:

deploy this code with REDIS_URL set (so we're still using the redis backend)

migrate our data to postgres with scripts/migrate-token-pool.js

set POSTGRES_URL to switch to the postgres backend

if it all goes wrong, we can easily just unset POSTGRES_URL to go back to redis

unset REDIS_URL once we're happy everything is fine

delete the redis code at a later date

This both makes the migration super low-risk/easy to back out of and means we can provide a migration path for self-hosting users with basically no effort.

Might be worth extending the warning message to include a link to #8921 (or some pertinent issue/discussion)

Yes. I'll call this out at the top of the changelog for the next server release and write it with a bit more of a focus on self-hosting users than the discussion in #8921 (which is quite shields.io-specific). I'm going to set myself a reminder to ping me so I remember to do this when I do the changelog.

scripts/migrate-token-pool.js

core/token-pooling/sql-token-persistence.integration.js

calebcartwright · 2023-02-19T17:10:42Z

migrations/1676731511125_initialize.cjs

@@ -0,0 +1,14 @@
+/* eslint-disable camelcase */
+
+exports.shorthands = undefined


What's this?

https://github.com/salsita/node-pg-migrate/blob/9331f6fda98795e3f3733461f9b71345168b99d8/docs/migrations.md?plain=1#L3-L19

calebcartwright · 2023-02-19T17:14:40Z

LGTM, happy to 👍 once merge conflicts are sorted

chris48s · 2023-02-19T19:10:29Z

@SnoozeThis wait until Feb 28 2023

TODO: update console.warn() with a link to changelog when you write changelog for next server release

SnoozeThis · 2023-02-19T19:10:44Z

(https://snoozeth.is/0ky67I-_i6g) I will wait until Tue, 28 Feb 2023 00:00:00 UTC and then add a comment.

chris48s · 2023-02-19T19:29:57Z

Should be good to go, I think.

I can look at setting up a postgres instance on fly and getting the actual data migration done in the week.

SnoozeThis · 2023-02-28T00:00:02Z

Resolved.

chris48s added the operations Hosting, monitoring, and reliability for the production badge servers label Feb 18, 2023

chris48s mentioned this pull request Feb 18, 2023

Proposal: migrate GitHub token pool to fly.io postgres #8921

Closed

add ability to store token pool in Postgres DB

169a6aa

chris48s force-pushed the 8921-postgres branch from 20e284d to 169a6aa Compare February 18, 2023 20:25

chris48s commented Feb 18, 2023

View reviewed changes

core/token-pooling/sql-token-persistence.integration.js Show resolved Hide resolved

chris48s commented Feb 18, 2023

View reviewed changes

chris48s marked this pull request as ready for review February 18, 2023 20:36

chris48s changed the title ~~WIP migrate token pooling to postgres~~ migrate token pooling to postgres Feb 18, 2023

chris48s requested a review from calebcartwright February 18, 2023 20:42

calebcartwright reviewed Feb 19, 2023

View reviewed changes

chris48s and others added 2 commits February 19, 2023 19:12

Merge branch 'master' into 8921-postgres

2c60f2f

update transitive ansi-regex dependencies

66267b4

calebcartwright approved these changes Feb 22, 2023

View reviewed changes

Merge branch 'master' into 8921-postgres

dbf5c67

chris48s merged commit 091ccfd into badges:master Feb 23, 2023

chris48s mentioned this pull request Feb 23, 2023

update docs following token pool changes #8933

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate token pooling to postgres #8922

migrate token pooling to postgres #8922

chris48s commented Feb 18, 2023 •

edited

Loading

github-actions bot commented Feb 18, 2023 •

edited

Loading

socket-security bot commented Feb 18, 2023 •

edited

Loading

chris48s Feb 18, 2023

calebcartwright Feb 19, 2023

chris48s Feb 19, 2023

chris48s Feb 18, 2023

calebcartwright Feb 19, 2023

chris48s Feb 19, 2023

chris48s Feb 18, 2023

calebcartwright Feb 19, 2023 •

edited

Loading

chris48s Feb 19, 2023 •

edited

Loading

calebcartwright Feb 19, 2023

chris48s Feb 19, 2023

calebcartwright commented Feb 19, 2023

chris48s commented Feb 19, 2023 •

edited

Loading

SnoozeThis commented Feb 19, 2023

chris48s commented Feb 19, 2023

SnoozeThis commented Feb 28, 2023

		@@ -0,0 +1,14 @@
		/* eslint-disable camelcase */

		exports.shorthands = undefined

migrate token pooling to postgres #8922

migrate token pooling to postgres #8922

Conversation

chris48s commented Feb 18, 2023 • edited Loading

github-actions bot commented Feb 18, 2023 • edited Loading

socket-security bot commented Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

chris48s Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Feb 19, 2023

chris48s commented Feb 19, 2023 • edited Loading

SnoozeThis commented Feb 19, 2023

chris48s commented Feb 19, 2023

SnoozeThis commented Feb 28, 2023

chris48s commented Feb 18, 2023 •

edited

Loading

github-actions bot commented Feb 18, 2023 •

edited

Loading

socket-security bot commented Feb 18, 2023 •

edited

Loading

calebcartwright Feb 19, 2023 •

edited

Loading

chris48s Feb 19, 2023 •

edited

Loading

chris48s commented Feb 19, 2023 •

edited

Loading