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

Use SQLite instead of bbolt #779

Merged
merged 24 commits into from
Feb 17, 2025
Merged

Use SQLite instead of bbolt #779

merged 24 commits into from
Feb 17, 2025

Conversation

adombeck
Copy link
Contributor

@adombeck adombeck commented Feb 6, 2025

Our database requirements are better met by a relational database than a key value store, so we now use SQLite instead of bbolt.

UDENG-4890

@adombeck adombeck force-pushed the UDENG-4890-use-sqlite branch from 763b8ed to 99bfaab Compare February 6, 2025 18:10
@adombeck adombeck changed the title Use SQLite Use SQLite instead of bbolt Feb 7, 2025
@adombeck adombeck force-pushed the UDENG-4890-use-sqlite branch from 99bfaab to 79ce337 Compare February 10, 2025 14:04
@3v1n0 3v1n0 linked an issue Feb 10, 2025 that may be closed by this pull request
2 tasks
@adombeck adombeck marked this pull request as ready for review February 10, 2025 17:21
@adombeck adombeck requested a review from a team as a code owner February 10, 2025 17:21
@adombeck adombeck force-pushed the UDENG-4890-use-sqlite branch 3 times, most recently from 143ef36 to 1c7b268 Compare February 11, 2025 12:06
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

oof, this was a huge one. Overall, amazing work! I have some comments and nitpicks (of course 🙃)...

@adombeck adombeck force-pushed the UDENG-4890-use-sqlite branch 4 times, most recently from 136001f to 4a8bb53 Compare February 13, 2025 15:27
Comment on lines 21 to 25
// Z_ForTests_DumpNormalizedYAML gets the content of the database, normalizes it
// (so that it can be compared with a golden file) and returns it as a YAML string.
//
// nolint:revive,nolintlint // We want to use underscores in the function name here.
func Z_ForTests_DumpNormalizedYAML(t *testing.T, c *Manager) string {
Copy link
Member

Choose a reason for hiding this comment

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

Just to be safe: I remember you mentioning that these symbols are left out of the release code, but what happens to the dependencies? Are they still included in the package or they are also left out? Here you are importing the testing package inside the db one and that's something we avoid at all costs... If it's not included in the release, I guess we are fine, but if it is, we should probably go back to return errors and let the test function deal with them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not in the binary:

go build -o authd ./cmd/authd && go tool nm authd | grep Z_ForTests

Copy link
Member

@denisonbarbosa denisonbarbosa Feb 14, 2025

Choose a reason for hiding this comment

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

I'm aware that the function isn't part of it, but does it pull the testing package into the binary even though the function itself is trimmed out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I understand what you mean now. Yes, there are indeed some symbols from the testing package:

go build -o authd ./cmd/authd && go tool nm authd | grep testing
 1234e28 d os.testingForceReadDirLstat
 11d6160 d testing..inittask
  d48f60 r testing..stmp_19
  d48f70 r testing..stmp_20
  d472fc r testing..stmp_60
  97a240 t testing.init
 1210ad8 d testing.supportedTypes

I'll change the functions to return errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, that's probably something for a follow-up task, because the testing package is already used in that file on main:

Copy link
Member

Choose a reason for hiding this comment

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

The only line I see testing being used there is here:

func Z_ForTests_CreateDBFromYAML(t *testing.T, src, destDir string) {

So the problem is the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so we need to remove it from that function as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I still think that could have happened in a follow-up task, but I pushed a commit which removes the dependency on the testing package.

@adombeck adombeck force-pushed the UDENG-4890-use-sqlite branch 2 times, most recently from bea2eab to 981dfe2 Compare February 14, 2025 14:13
@adombeck adombeck force-pushed the UDENG-4890-use-sqlite branch from 6a6377d to f9f5d85 Compare February 16, 2025 13:08
Changes done by running:

   git rm "internal/*/testdata/golden/*"
   TESTS_UPDATE_GOLDEN=1 go test ./internal/...
   git add -A
We don't use our database as a cache anymore. It's time to reflect this
in the package name.
We can't use the "AAAAATIME" placeholders anymore when using SQLite,
because the schema only allows actual timestamps.
We don't store the current UID in golden files anymore since
3f81332.
These files were forgotten in 5555836.

Also simplify the testdata for the "pam_unix_non_existent" test case,
because we don't need multiple users and groups for that test case.
Our database requirements are better met by a relational database than a key value store, so we now use SQLite instead of bbolt.
Some cases were not tested.
And inline the `userRow` struct into `UserDB`, because it now doesn't
have any additional fields anymore.
Explain what a local group is
* Make groupRow exported by renaming it to to GroupRow. This allows us
  to use it whenever we don't need the members of the group.
* Add GroupWithMembersByID and GroupWithMembersByName, to allow the
  users manager to fetch a group with its members in a single
  transaction.
* Rename GroupDB to GroupWithMembers, which is more descriptive.
* Rename UserDB to UserRow. More descriptive and consistent with
  GroupRow.
To make it a bit more clear from the variable name why we use the GID of
this group for the user.
... but don't fail if the database has other permissions than 0600,
because it doesn't contain any secrets, so it's fine if it's readable by
other users than the owner. Check that it's not *writable* by other
users though.
@adombeck adombeck force-pushed the UDENG-4890-use-sqlite branch from f9f5d85 to df8b137 Compare February 17, 2025 11:43
@adombeck adombeck merged commit 4143f53 into main Feb 17, 2025
15 of 16 checks passed
@adombeck adombeck deleted the UDENG-4890-use-sqlite branch February 17, 2025 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue: DB cache tests are randomly failing based depending on current time
5 participants