-
Notifications
You must be signed in to change notification settings - Fork 9
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(users/services): Local groups cleanup #150
Conversation
a37ab23
to
ee034cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, apart from the lack of getent mocking which actually explains why there is only one positive use case and less granularity, the rest of the actual internal/users/
implementation looks good.
However, I didn’t review internal/services/
: thinking about it, I changed a little bit my vision and I think we can do better in term of handling and when triggering all of this so that this routine is exceptional:
- we should still try, when removing an user from a consistent database (because of expired cache), to clean the local groups this user is in. This is the normal procedure and we know exactly which user is currently under purge, and remove it.
- I really see that as a fallback when the database is seen as corrupted and we shouldn’t attempt anything until getting a "database is corrupted" signal. That way, cleaning the user from system group will become again just a part of the corrupted database cleanup and will not be called more than this. It’s then decoupled from the manager.
Mind reworking this PR to follow this?
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
+ Coverage 88.83% 88.98% +0.15%
==========================================
Files 35 36 +1
Lines 2650 2733 +83
==========================================
+ Hits 2354 2432 +78
- Misses 228 231 +3
- Partials 68 70 +2 ☔ View full report in Codecov by Sentry. |
Is that really ready for review? I still see the manager/ directory calling the users one when we agreed to remove all this for this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, really take the time to look at what is needed or not, and then only, request for review. Look at the diff on github if the local diff is too hard to read, but take the time to do ensure it to give confidence on the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is your review! This time, no undeed leftor, but still some work to do and open questions! Feel free to tell me if anything is not understandable.
8d0240c
to
10fee0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, only one real comment with 2 parts remaining! If you look at the coverage (you do, right?), there should be the Go-C function that is lacking coverage, right? I mentioned yesterday that we should unit test it (even if it’s going to be basic, still better than nothing).
Otherwise, it’s funny to see how this cgo part simplify the code in the end (less parsing, easier to test…)
internal/users/testdata/TestCleanLocalGroups/golden/error_on_any_unignored_delete_gpasswd_error
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not an unit test :p So not the last round, but next one should be!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to me once the test pass and is commits are rebased!
We already had a function to update the groups, but none to clean the groups from inactive users or to just remove the user from all the groups it belongs to. Now we have one to clean a specific user and one to clean all inactive users from the groups, based on the list of users returned by the getpwent call.
36d4856
to
3c81c49
Compare
Due to limitations with our
cache
, we can't know exactly which users were deleted when the database was corrupted and it got cleared up and rebuilt, so we need to have a routine that periodically cleans the local groups from any unexistent users.This is a complex task that could potentially spawn a lot of subprocesses (in the form of
gpasswd --delete
calls) to clean the local groups. However, the dominant factor of complexity in the code is the number of groups (and the number of users that belong to those groups) rather than the number of existent users. Since we query directly/etc/groups
to look for the groups, we are only dealing with local groups here and, as such, we should be fine with performance.UDENG-1864