acaloiaro / neoq

Queue-agnostic background job library for Go, with a pleasant API and powerful features.
MIT License
270 stars 4 forks source link

separate migrations from deployment #83

Closed gregwebs closed 2 months ago

gregwebs commented 1 year ago

Neoq auto-migration is very convenient. However, that means it has elevated database permissions. In a deployment with higher security, neoq should not have permission to alter tables when working a job queue.

Today this can probably be implemented by running neoq locally and then copy and pasting the SQL from the local DB and then applying that SQL in the out of band migration process that has permissions. Although I suspect neoq might still fail in this case if it doesn't have permission to inspect the schema.

acaloiaro commented 1 year ago

I think migrations need a bit more holistic thought at this point. Particularly #77

You raise some important points as well.

The motivation behind the auto-migrate concept is that when a user upgrades to a new version of neoq which requires a different schema, we need the schema to be updated on start, before jobs can run.

As a neoq user, I don't want to have to care about what the neoq schema is, and I don't particularly want that for other neoq users.

But your point is quite valid -- neoq may authenticate with a user that doesn't have permission to migrate the schema. So this needs to be thought through more holistically if we want to allow reduced-permission users to use neoq.

Implicitly, neoq currently doesn't allow reduced-permission users to use neoq. But we need to either make that explicit, or think through the schema migration story more thoroughly.

gregwebs commented 1 year ago

I think the behavior now is great for users that just care about convenience and are not concerned about security. But these same users may eventually migrate to a more security conscientious setup.

An automatic approach to version change would be for neoq to save its (schema) version to the DB as part of its migrations. Then it would be able to reject a new version of the Go code unless the schema version is up to date. It's a bit of a shame to do this since it already has the ability to inspect the schema to figure this out.

What I have done in the past (I used to maintain a library that did auto-migrate) was provide the option to either print the migration sql or to execute it (this still requires permission to inspect the schema of the table, but that is a read-only permission). That also provides the flexibility to get the migration contents and put them into a migration system (without the hack of deploying locally and copying the schema, which may miss out on operational parts of the migration such as creating an index CONCURRENTLY).

In a current deployment setup, I perform a migration check when deploying that notifies me if there are any unapplied migrations (it doesn't block production migration because I may want to deploy code before running a migration). I could also include a neoq migration check if neoq had the option to run and print out pending migrations (this migration check can run with elevated privileges). Actually, if neoq had the option to just migrate and then exit, without working any jobs, it would be possible to run that as a separate (elevated privilege) process that auto-migrates as part of the deployment. If the neoq migrations are compatible with a zero-downtime approach then I could deploy this migration job ahead of the new version of neoq. That would be a manual deployment checklist item in my setup (Kubernetes), but that would be fine.

I think once this library matures to 1.0 there will be little reason to change the schema? I think a schema change could be a major version change, and that would prevent accidental upgrade and be a strong signal to RTFM, which a security conscientious user would do.

acaloiaro commented 1 year ago

This is good feedback and perspective, so thanks for sharing it.

I think the behavior now is great for users that just care about convenience and are not concerned about security. But these same users may eventually migrate to a more security conscientious setup.

Indeed. And I don't see it as an option to simply ignore users with better security practices.

An automatic approach to version change would be for neoq to save its (schema) version to the DB as part of its migrations.

The latest migration gets written to neoq_schema_migrations by way of https://github.com/golang-migrate/migrate

Then it would be able to reject a new version of the Go code unless the schema version is up to date. It's a bit of a shame to do this since it already has the ability to inspect the schema to figure this out.

By this do you mean that your expectation would be that neoq.Start returns error when there are pending migrations? (assuming this behavior is behind a configuration option, like WithAutoMigrate(false))

What I have done in the past (I used to maintain a library that did auto-migrate) was provide the option to either print the migration sql or to execute it (this still requires permission to inspect the schema of the table, but that is a read-only permission). That also provides the flexibility to get the migration contents and put them into a migration system (without the hack of deploying locally and copying the schema, which may miss out on operational parts of the migration such as creating an index CONCURRENTLY).

Neoq's migration code lives in .sql files embedded within the library. To get a dry-run printout of pending migrations would be straight forward to implement. I'm no sure that we need schema inspection permission to do this. The current schema version lives in neoq_schema_migrations, which can be used to enumerate unapplied migrations.

In a current deployment setup, I perform a migration check when deploying that notifies me if there are any unapplied migrations (it doesn't block production migration because I may want to deploy code before running a migration). I could also include a neoq migration check if neoq had the option to run and print out pending migrations (this migration check can run with elevated privileges).

Makes sense.

Actually, if neoq had the option to just migrate and then exit, without working any jobs, it would be possible to run that as a separate (elevated privilege) process that auto-migrates as part of the deployment. If the neoq migrations are compatible with a zero-downtime approach then I could deploy this migration job ahead of the new version of neoq. That would be a manual deployment checklist item in my setup (Kubernetes), but that would be fine.

This can be done on the application side as-is by setting a DATABASE_URL with heightened permissions. I definitely wouldn't want the average user to have to think about this, but it is an option for those who do want to think about it. This could become a Wiki article.

I think once this library matures to 1.0 there will be little reason to change the schema? I think a schema change could be a major version change, and that would prevent accidental upgrade and be a strong signal to RTFM, which a security conscientious user would do.

I think you're right. I'd like the schema to be set in stone by 1.0. But even if that's the case, I want to make sure the post-1.0 migration story is thought out in advance, and this thread will be the start of that.

P.S.

There are reasons other than schema changes to have migrations. E.g. here's a PR that moves new job notification into a trigger: https://github.com/acaloiaro/neoq/pull/79

gregwebs commented 1 year ago

By this do you mean that your expectation would be that neoq.Start returns error when there are pending migrations? (assuming this behavior is behind a configuration option, like WithAutoMigrate(false))

Yes.

Oh, the migration files are right there already. So I guess those can be lifted into a different migration system already.

And I see that I can probably just call Start() if I want neoq to do the migrations. So I will try out what I can do now.

acaloiaro commented 1 year ago

Oh, the migration files are right there already. So I guess those can be lifted into a different migration system already.

Yeah that's right, and it hadn't crossed my mind. I wonder if it would be worthwhile to give users a public API into these embedded migration files.

And I see that I can probably just call Start() if I want neoq to do the migrations.

Migrations will run as a result of neoq.New(ctx, neoq.WithBackend(postgres.Backend)). I made a mistake in my earlier message; I meant New. Migrations are run in postgres.Backend.

acaloiaro commented 11 months ago

Hey @gregwebs -- I'm actually thinking of removing migrations in the future, under the assumption that the schema is now stable. Were you able to move forward from our last discussion?

gregwebs commented 11 months ago

yeah, we are able to workaround such issues right now.

mgdigital commented 8 months ago

Hi @acaloiaro , I'm looking at using neoq to replace the current queueing solution in https://github.com/bitmagnet-io/bitmagnet. Thanks, this library looks closest to what I need out of all the available Go libraries!

A sticking point for me is also these migrations. I'm already using Goose for migrations and would prefer not to have 2 separate migrators - I can easily copy your provided SQL into my own migrations, the problem is I can't stop it trying to run the built-in migrations when instantiating the Postgres backend. I get the convenience of auto-migration but agree an option to disable this would be great, allowing consumers of the library to manage migrations themselves if they choose to do so. As a step further, I feel the migrations could be completely separate from the backend to avoid importing the bloat of golang-migrate and its dependencies where these aren't required.

So, I thought - I'll just make my own Postgres backend to use with neoq, by copying the one here and removing the migrations. Unfortunately that's also difficult because the backend depends on several things in the internal package (mostly status names and some helper functions) - so in the short term I may have to fork to make the tweaks I need I'll have to duplicate the couple of things being imported from internal. This possibly needs a separate issue, but neoq should probably be exposing those things in its public API as they would be needed by anyone implementing a custom backend...

acaloiaro commented 8 months ago

Hi @mgdigital, I'm not at all opposed to refactoring neoq such that migrations are optional. At 1.0, I'd like to even remove migrations completely (except for the initial schema bootstrap).

Are you interested in doing any of the refactoring? I ask, because I don't have a ton of free time right now to work on bigger neoq refactors at the moment.

mgdigital commented 8 months ago

Thanks for getting back to me @acaloiaro . So in the end I went with a custom queue implementation, but it used neoq as a starting point (and I have put a credit as such in the code).

I wanted to integrate more tightly with Gorm ORM that's used throughout the project, strip some stuff out that I didn't need, and add some stuff that I did need (like garbage collection). It's probably sufficiently different from neoq that there's not much potential for incorporating these changes here. Thanks for the inspiration though!

If you're interested, the implementation is in this PR: https://github.com/bitmagnet-io/bitmagnet/pull/131/files

acaloiaro commented 8 months ago

@mgdigital Neat! Thanks for the heads up.

github-actions[bot] commented 2 months ago

This issue is stale. Feel free to re-open it if it requires more attention