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

feat: Add ent-based sqlite3 storage #1906

Merged
merged 4 commits into from
May 5, 2021
Merged

Conversation

nabokihms
Copy link
Member

@nabokihms nabokihms commented Dec 30, 2020

Overview

This PR adds an ability to switch to experimental ent-based sqlite3 storage

What this PR does / why we need it

Related #1864

There is the first part of the SQL layer refactoring. What this PR does:

  • Database schema described using an ORM.
  • Add new ORM-based storage for SQLite.
  • Manage to mimic most SQL storages logic.
  • Utilize best practices of other storages.
  • Include conformance tests for the ent-based sqlite3 storage.

Special notes for your reviewer

Following the "one step at a time" ideology, it looks like a good idea to split the refactoring into chunks.
This part has no negative impact and shows the most vital benefits of switching to using ORM.
I will add a full roadmap to the issue mentioned above.

Does this PR introduce a user-facing change?

Users can switch to utilizing the new driver. Unfortunately, there is no doc, neither migration to switch from using sqlite3 driver to sqlite3-ent. It will be implemented in future PRs.

Add an experimental ent-based sqlite3 storage

@sagikazarmark sagikazarmark self-requested a review January 1, 2021 15:10
@sagikazarmark
Copy link
Member

Thanks @nabokihms !

I briefly reviewed the PR, here are a couple thoughts:

  • Can you please separate the generated code into a different commit? It would make reviewing much easier.
  • In the long term, I don't think we want ent to be a new storage type, but one that replaces the existing ones. So here is an idea: either use a build tag for replacing the current implementation with ent or an env var (eg. DEX_ENT_ENABLED or DEX_EXPERIMENTAL or DEX_EXPERIMENTAL_ENT)
  • I can see some packages have been updated. I think those should be part of a separate PR.

@nabokihms
Copy link
Member Author

  1. Yes, it is possible to exclude all generated code since we already have the generate step in the Makefile.
  2. Yeah, I thought about something like an env variable. The thing that stopped me to use it is that Dex had no standardized way to add new env-based settings. However, in this case, it simplifies the switching process, thus I will follow your suggestion.
  3. I will double-check this.

@sagikazarmark
Copy link
Member

Yes, it is possible to exclude all generated code since we already have the generate step in the Makefile.

I don't think we have to exclude generated code, but putting it in separate commits definitely helps.

@nabokihms nabokihms force-pushed the ent-sqlite branch 8 times, most recently from 67ae6f9 to 1d519ad Compare January 2, 2021 08:52
@nabokihms
Copy link
Member Author

There are three commits now:

  • storage code
  • auto-generated code
  • dependencies updating required by ent (which will be moved to a separate PR)

@nabokihms
Copy link
Member Author

@sagikazarmark, thanks for updating dependencies and adding dependabot! The only package that left required an update is the MySQL driver. Should I open PR for it?

@sagikazarmark sagikazarmark added this to the v2.29.0 milestone Feb 10, 2021
@nabokihms
Copy link
Member Author

@sagikazarmark I bumped ent version to 0.8.0, resolved conflicts, added support for recently merged settings for refresh tokens.

Dependencies changes look good now.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

LGTM!

@sagikazarmark
Copy link
Member

@nabokihms Feel free to merge it if you think it's ready.

@nabokihms nabokihms merged commit 81c4dc7 into dexidp:master May 5, 2021
@sagikazarmark
Copy link
Member

🎉

@a8m
Copy link

a8m commented May 7, 2021

Happy to see this PR! 🥳

Please, feel free to tag me on GitHub or ping me in ent Slack channel if you need any support for Ent.

@sagikazarmark
Copy link
Member

Thanks @a8m !

We are still experimenting with ent here, but I followed the project for quite some time now and it's amazing (by far the best Go ORM).

One thing that's going to be a concern is migrations (not very familiar with how ent does that yet) in a production environment.

@a8m
Copy link

a8m commented May 8, 2021

We're working now on a new migration platform for ent that will cover 99% of the cases. I'll be happy to get your feedback or just to hear your thoughts before we're OSS-ing it. Thanks 🙏

@sagikazarmark
Copy link
Member

When it comes to migration, I'm always worried about automated migrations:

  • What if something goes wrong?
  • How do you roll back?
  • Can you even roll back?
  • How does the underlying database (eg. MySQL) cluster handles schema changes?
  • What if the database user doesn't have the permissions to do schema changes (for security reasons)?
  • What happens when multiple instances of the same service spawns (eg. during a deployment) and they all try to run the migrations?

Therefore when working for large systems, I always insisted on having a separate, manual process available for migrations. Not just that, but it was also mandated that these changes must be backward compatible between two subsequent versions. For example: if you wanted to delete a column from the database, first you needed to remove any code using that column (version 1) and in the next release (or rather before deploying it) you were allowed to add a migration step that removed that column.

I think there is no good solution available for these issues, especially in the Kubernetes/Cloud Native world (where Dex is mostly used). That being said, we also use automatic migrations in Dex, because it's just easier. But I'd love to see a solution where we can give operators the means to manually run migrations prior to deploying a new version or at least see some tooling that can help with recovering from a bad state.

@a8m These are my thoughts about migration. Hope it helps a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants