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

acmeserver: support additional database types beside bbolt #6097

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mohammed90
Copy link
Member

This change allows the database (storage backend) for the ACME server to be PostgreSQL or MySQL as well. This allows scaling of the ACME server across multiple nodes and avoids issue of losing ACME account data when switching nodes.

@mohammed90 mohammed90 added in progress 🏃‍♂️ Being actively worked on needs docs ✍️ Requires documentation changes needs tests 💯 Requires automated tests labels Feb 11, 2024
@mohammed90 mohammed90 force-pushed the acme-database branch 2 times, most recently from 9f20097 to 2452c23 Compare February 11, 2024 12:47
@hslatman
Copy link
Contributor

Hey @mohammed90, this is nice 🙂

We're working on a new version of our nosql package. The goal is to be compatible with the existing implementation (at least data model wise, and hopefully API too), but it might be good to await its release, just in case there are breaking changes.

Curious why badger is excluded explicitly? In the CA it's our current default, and likely what most people use today.

@francislavoie
Copy link
Member

francislavoie commented Feb 11, 2024

We started using the nobadger build tag since #6031. It was causing our builds for IBM AIX to fail. See #5970. It also significantly reduced our binary size, since we didn't use it anyway. Back in 2020, we switched to bbolt: #3868

@mohammed90
Copy link
Member Author

mohammed90 commented Feb 11, 2024

Curious why badger is excluded explicitly? In the CA it's our current default, and likely what most people use today.

As Francis said, it's been painful. It doesn't appear to be maintained (see dgraph-io/badger#2035) and causes the issues Francis has referenced. See how switching to bbolt resolved numerous issues.

We're working on a new version of our nosql package. The goal is to be compatible with the existing implementation (at least data model wise, and hopefully API too), but it might be good to await its release, just in case there are breaking changes.

The PR can wait 🙂 This enhancement occurred to me as I was writing the post Zero-Trust Architecture with Caddy.

As you're developing the new version, is it possible to decouple the drivers (of PostgreSQL or MySQL) from the implementation to avoid pulling the unused driver into downstream users? One trick I know of is to use URLs for DSN where the scheme is the driver name to use with sql.Open. The rest of the URL can be used almost as-is. The downstream users can underscore-import their favorite driver without the extra baggage of the drivers of the other engines. If this approach is too much of a hassle for you or breaks your workflow, it's ok 🙂

@hslatman
Copy link
Contributor

We started using the nobadger build tag since #6031. It was causing our builds for IBM AIX to fail. See #5970. It also significantly reduced our binary size, since we didn't use it anyway. Back in 2020, we switched to bbolt: #3868

As Francis said, it's been painful. It doesn't appear to be maintained (see dgraph-io/badger#2035) and causes the issues Francis has referenced. See how switching to bbolt resolved numerous issues.

Makes sense

As you're developing the new version, is it possible to decouple the drivers (of PostgreSQL or MySQL) from the implementation to avoid pulling the unused driver into downstream users? One trick I know of is to use URLs for DSN where the scheme is the driver name to use with sql.Open. The rest of the URL can be used almost as-is. The downstream users can underscore-import their favorite driver without the extra baggage of the drivers of the other engines. If this approach is too much of a hassle for you or breaks your workflow, it's ok 🙂

We already support URIs as the DSN, if I'm understanding you correctly. One change in the package will indeed be to have blank imports for the specific drivers, so that it's not needed to specify build tags in the dependent projects, and can rely on just the import in those dependent projects. You would still need the import to happen in the dependent project, or else parsing the DB type from the DSN will result in an unsupported DB type error.

@mohammed90
Copy link
Member Author

mohammed90 commented Feb 12, 2024

We already support URIs as the DSN, if I'm understanding you correctly. One change in the package will indeed be to have blank imports for the specific drivers, so that it's not needed to specify build tags in the dependent projects, and can rely on just the import in those dependent projects.

I meant to exclude the explicit import/dependency on the driver. See how the driver/postgresql/driver.go file has explicit dependency on "github.com/jackc/pgx/v5", which means even clients who are using github.com/smallstep/nosql with bbolt will have transit dependency on the driver (albeit removed by the linker during compilation).

Anyways, looking at the code, it seems to implement an optimization through pgxpool, so it'd be hard to abstract away that dependency. Don't worry about it.

@hslatman
Copy link
Contributor

With my previous comment I meant that the new version will require the use of blank imports to actually import the driver. That seems to attain what you're looking for.

For Caddy it would mean that mysql and postgresql would have to be explicitly imported (next to bbolt) for them to be supported through the DSN configuration. On the Caddy side it's possible to keep the same build behavior by adding some additional Go packages with(out) the build tags specified, or to just include just those you want to be supported (the current build behavior, basically).

It's possible to only import the database specific logic based on the DSN at runtime using something akin to a Go plugin (although I would suggest WASM instead), using a shim that's always compiled, which calls into the DB plugin loaded at runtime. But at the moment that's not in scope, and sounds like kind of a different project 😅

@francislavoie
Copy link
Member

If they were excluded from the build, with Caddy anyway it would be super trivial to add with xcaddy build --with github.com/smallstep/whatever-driver I think, right?

@hslatman
Copy link
Contributor

If they were excluded from the build, with Caddy anyway it would be super trivial to add with xcaddy build --with github.com/smallstep/whatever-driver I think, right?

Hadn't thought about that, but yes, I think that would work too. Maybe needs some changes/thought, but I like it that way.

If it works, it could be an option to only support bbolt in the standard modules, and have users add the others using --with. It could be a bit surprising/unclear to users, because you're not importing an actual Caddy module (i.e. not available in the generated docs, no implemented interfaces, etc.), so maybe that needs some attention.

@mholt
Copy link
Member

mholt commented Sep 26, 2024

Sorry I'm just catching up; is this ready to come out of draft state, or are there other complications/nuances that remain?

@mohammed90
Copy link
Member Author

Sorry I'm just catching up; is this ready to come out of draft state, or are there other complications/nuances that remain?

Herman recommends waiting until v1 of nosql lib is out the door. The nosql PR for v1 brings back Badger and couples all the storage engines into the module, so I'm not very enthusiastic about its direction, but watching closely. This PR can move forward, but I'm not sure how to ensure the user's databases are safely upgraded once v1 of nosql comes out with changes, if any.

@hslatman, any pointers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress 🏃‍♂️ Being actively worked on needs docs ✍️ Requires documentation changes needs tests 💯 Requires automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants