Store UGID in group cache to handle group renames at provider's end #647

shiv-tyagi · 2024-11-20T13:32:29Z

Closes #620
UDENG-5160

As suggested by @adombeck, to identify if a group is renamed at provider's end, we should store UGID along with the GID and group name in the cache and find if there is already a group record with same UGID in cache. In case we find one, we delete the record being pointed by old name in relevant buckets and populate with record containing new name.

shiv-tyagi · 2024-11-20T13:34:46Z

@adombeck Here is my initial work. Could you please check if I am going in right direction?

adombeck · 2024-11-26T10:46:23Z

Hi @shiv-tyagi, sorry for the late reply, I've been sick for a week.

Here is my initial work. Could you please check if I am going in right direction?

Yes, I took a quick look and it looks like it's going in the right direction! I'll do a more thorough review once the tests pass and you consider the PR ready for review. Let me know if you need any assistance.

shiv-tyagi · 2024-11-26T16:54:12Z

No worries. Health comes first.
I saw the test failures and found that since we have added the UGID field, we need to update the test data files almost everywhere. There are a lot of them.
Should I proceed updating the test data files? That would add a lot of diffs to the PR and might make it difficult to review. What do you suggest?

adombeck · 2024-11-26T17:25:44Z

I saw the test failures and found that since we have added the UGID field, we need to update the test data files almost everywhere. There are a lot of them.

Ah, I assume it's the golden files which need to be updated. You can update them all at once by running TESTS_UPDATE_GOLDEN=1 go test ./....

shiv-tyagi · 2024-12-02T04:13:01Z

TESTS_UPDATE_GOLDEN=1 go test ./....

I ran this and a lot of golden files did get updated, but the *.db files in the test data like this one still couldn't get updated. Do I need to update them manually or am I missing something?

If I am not wrong, golden files are the ones against which the test output is matched, but we also need to update the initial sample db files on which those tests are performed, right?

adombeck · 2024-12-02T10:22:22Z

I ran this and a lot of golden files did get updated, but the *.db files in the test data like this one still couldn't get updated. Do I need to update them manually or am I missing something?

Those should also be updated automatically. I ran TESTS_UPDATE_GOLDEN=1 go test ./internal/services/pam/... locally on your branch and it updated a lot of files, including the cache.db files.

shiv-tyagi · 2024-12-03T04:15:50Z

I get this error when I do run the command

Those should also be updated automatically. I ran TESTS_UPDATE_GOLDEN=1 go test ./internal/services/pam/... locally on your branch and it updated a lot of files, including the cache.db files.

internal/users/localgroups/localgroups.go:21:16: undefined: getPasswdUsernames
FAIL    github.com/ubuntu/authd/internal/services/pam [build failed]
FAIL

I get this error when I run the command. Anything I am missing?

shiv-tyagi · 2024-12-03T18:02:57Z

Those should also be updated automatically. I ran TESTS_UPDATE_GOLDEN=1 go test ./internal/services/pam/... locally on your branch and it updated a lot of files, including the cache.db files

Sorry for bugging you so much. I was finally able to run tests and get the cache.db files updated as well. But I still couldn't get these *.db.yaml files updated. Did yours got updated when you ran it locally?

3v1n0 · 2024-12-03T18:56:00Z

Just use TESTS_UPDATE_GOLDEN=1 go test -C ./internal ./... it should work

adombeck · 2024-12-03T20:32:32Z

internal/users/localgroups/localgroups.go:21:16: undefined: getPasswdUsernames
FAIL github.com/ubuntu/authd/internal/services/pam [build failed]
FAIL

I get this error when I run the command. Anything I am missing?

Do you have CGO_ENABLED set to 0? If so, unset it please:

CGO_ENABLED= TESTS_UPDATE_GOLDEN=1 go test ./internal/services/pam/...

codecov-commenter · 2024-12-04T03:40:51Z

Codecov Report

Attention: Patch coverage is 76.47059% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.10%. Comparing base (36511cd) to head (74e0b9a).
Report is 132 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/users/cache/delete.go	42.85%	2 Missing and 2 partials ⚠️
internal/users/cache/update.go	78.57%	2 Missing and 1 partial ⚠️
internal/users/cache/getgroups.go	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #647      +/-   ##
==========================================
- Coverage   83.43%   83.10%   -0.34%     
==========================================
  Files          83       88       +5     
  Lines        8689     8966     +277     
  Branches       74       74              
==========================================
+ Hits         7250     7451     +201     
- Misses       1111     1169      +58     
- Partials      328      346      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shiv-tyagi · 2024-12-04T05:04:39Z

Thanks @3v1n0 and @adombeck, I was able to run the tests and get the golden files updated. There's this one QA and Sanity check failing. I checked the logs but couldn't find any error message. Any idea how to fix that?

adombeck · 2024-12-04T11:35:44Z

Thanks @3v1n0 and @adombeck, I was able to run the tests and get the golden files updated. There's this one QA and Sanity check failing. I checked the logs but couldn't find any error message. Any idea how to fix that?

That job unfortunately doesn't print its results on the job page, but on the summary page: https://github.com/ubuntu/authd/actions/runs/12152788489/attempts/1?pr=647

adombeck · 2024-12-04T11:41:06Z

To catch linter errors earlier, you could install a git pre-push hook which runs golangci-lint locally:

$ cat .git/hooks/pre-push
#!/bin/sh

set -eu
set -x

golangci-lint run

shiv-tyagi · 2024-12-04T16:00:14Z

Thanks again @adombeck for patiently answering my questions. I am opening the PR for review.

Last few questions - I promise :),

Is there a chat server (discord or slack etc.) where the developers hang out and I can quickly ask my doubts?
Are there any regular online meetings which I can join to learn and get involved more in the project?

adombeck

This looks very good! Just a few comments.

internal/users/manager.go

internal/users/cache/update.go

adombeck · 2024-12-05T15:43:51Z

Thanks again @adombeck for patiently answering my questions. I am opening the PR for review.

No problem :)

Last few questions - I promise :),

Is there a chat server (discord or slack etc.) where the developers hang out and I can quickly ask my doubts?

Are there any regular online meetings which I can join to learn and get involved more in the project?

Currently, we don't have a public chat server or public meetings. I'll mention to the team that you're interested in that, but I can't promise anything 🫤

shiv-tyagi · 2024-12-15T17:23:46Z

@adombeck Sorry it is taking longer because I am getting less time after work. I only get weekends to contribute.
I look forward to get this PR merged soon. I have asked some questions in the comments above.

adombeck

This looks good to me now, thanks a lot @shiv-tyagi!

adombeck · 2024-12-16T17:16:24Z

@denisonbarbosa @3v1n0 does one of you want to take a look before I merge?

shiv-tyagi · 2024-12-17T07:03:23Z

Thanks @adombeck.

adombeck · 2024-12-19T17:59:06Z

@denisonbarbosa @3v1n0 does one of you want to take a look before I merge?

@denisonbarbosa @3v1n0 ping

3v1n0 · 2024-12-19T18:03:30Z

internal/users/cache/update.go

 		if err != nil && !errors.Is(err, NoDataFoundError{}) {
 			return err
 		}
+		groupExists := !errors.Is(err, NoDataFoundError{})


A nit, but I think it becomes clearer with:

groupExists := !errors.Is(err, NoDataFoundError{}) if err != nil && groupExists { return err }

3v1n0

LGTM, just a nit to make things a bit more readable.

…up test

shiv-tyagi force-pushed the group-name-consistency branch from 483afc8 to fee1ce2 Compare November 20, 2024 13:34

shiv-tyagi force-pushed the group-name-consistency branch 4 times, most recently from fb4c819 to 5346307 Compare December 3, 2024 17:59

shiv-tyagi force-pushed the group-name-consistency branch from 5346307 to 046924b Compare December 4, 2024 03:30

shiv-tyagi force-pushed the group-name-consistency branch from 046924b to d48b27b Compare December 4, 2024 04:02

shiv-tyagi force-pushed the group-name-consistency branch from d48b27b to ee15013 Compare December 4, 2024 15:30

shiv-tyagi marked this pull request as ready for review December 4, 2024 16:00

shiv-tyagi requested a review from a team as a code owner December 4, 2024 16:00

shiv-tyagi force-pushed the group-name-consistency branch 2 times, most recently from 4ec2473 to e1ed40b Compare December 5, 2024 15:10

adombeck reviewed Dec 5, 2024

View reviewed changes

internal/users/manager.go Show resolved Hide resolved

internal/users/cache/update.go Outdated Show resolved Hide resolved

internal/users/cache/update.go Outdated Show resolved Hide resolved

internal/users/cache/update.go Outdated Show resolved Hide resolved

shiv-tyagi force-pushed the group-name-consistency branch from e1ed40b to 9d54ba1 Compare December 15, 2024 16:52

shiv-tyagi force-pushed the group-name-consistency branch from 9d54ba1 to 16c24ad Compare December 15, 2024 17:18

adombeck force-pushed the group-name-consistency branch from 12ab536 to f6d898b Compare December 16, 2024 15:56

adombeck approved these changes Dec 16, 2024

View reviewed changes

3v1n0 reviewed Dec 19, 2024

View reviewed changes

3v1n0 approved these changes Dec 19, 2024

View reviewed changes

shiv-tyagi and others added 11 commits December 19, 2024 23:41

Store UGID as well in groupDB cache

72c5f8c

Use UGID to find existing group in database with same GID

c51296f

Use UGID to determine local groups while updating user

0be84bd

update test cases for group renaming scenarios

b51c543

add golden file for TestUpdateUserEntry/Update_user_by_renaming_a_gro…

65056a4

…up test

update sample db files

5555836

update golden files

c7ff19b

fix: Only compare with existing group if it actually exists

0ad0961

refactor: Reformat groupsCases in TestUpdateUser

c7c9c62

fix: Add UGID to testdata databases

f201800

Update golden files

74e0b9a

adombeck force-pushed the group-name-consistency branch from f6d898b to 74e0b9a Compare December 19, 2024 22:43

adombeck merged commit edd2399 into ubuntu:main Dec 19, 2024
7 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store UGID in group cache to handle group renames at provider's end #647

Store UGID in group cache to handle group renames at provider's end #647

shiv-tyagi commented Nov 20, 2024 •

edited by adombeck

Loading

shiv-tyagi commented Nov 20, 2024

adombeck commented Nov 26, 2024

shiv-tyagi commented Nov 26, 2024

adombeck commented Nov 26, 2024

shiv-tyagi commented Dec 2, 2024 •

edited

Loading

adombeck commented Dec 2, 2024

shiv-tyagi commented Dec 3, 2024 •

edited

Loading

shiv-tyagi commented Dec 3, 2024

3v1n0 commented Dec 3, 2024

adombeck commented Dec 3, 2024

codecov-commenter commented Dec 4, 2024 •

edited

Loading

shiv-tyagi commented Dec 4, 2024

adombeck commented Dec 4, 2024

adombeck commented Dec 4, 2024

shiv-tyagi commented Dec 4, 2024 •

edited

Loading

adombeck left a comment

adombeck commented Dec 5, 2024

shiv-tyagi commented Dec 15, 2024

adombeck left a comment

adombeck commented Dec 16, 2024

shiv-tyagi commented Dec 17, 2024

adombeck commented Dec 19, 2024

3v1n0 Dec 19, 2024

3v1n0 left a comment

Store UGID in group cache to handle group renames at provider's end #647

Store UGID in group cache to handle group renames at provider's end #647

Conversation

shiv-tyagi commented Nov 20, 2024 • edited by adombeck Loading

shiv-tyagi commented Nov 20, 2024

adombeck commented Nov 26, 2024

shiv-tyagi commented Nov 26, 2024

adombeck commented Nov 26, 2024

shiv-tyagi commented Dec 2, 2024 • edited Loading

adombeck commented Dec 2, 2024

shiv-tyagi commented Dec 3, 2024 • edited Loading

shiv-tyagi commented Dec 3, 2024

3v1n0 commented Dec 3, 2024

adombeck commented Dec 3, 2024

codecov-commenter commented Dec 4, 2024 • edited Loading

Codecov Report

shiv-tyagi commented Dec 4, 2024

adombeck commented Dec 4, 2024

adombeck commented Dec 4, 2024

shiv-tyagi commented Dec 4, 2024 • edited Loading

adombeck left a comment

Choose a reason for hiding this comment

adombeck commented Dec 5, 2024

shiv-tyagi commented Dec 15, 2024

adombeck left a comment

Choose a reason for hiding this comment

adombeck commented Dec 16, 2024

shiv-tyagi commented Dec 17, 2024

adombeck commented Dec 19, 2024

3v1n0 Dec 19, 2024

Choose a reason for hiding this comment

3v1n0 left a comment

Choose a reason for hiding this comment

shiv-tyagi commented Nov 20, 2024 •

edited by adombeck

Loading

shiv-tyagi commented Dec 2, 2024 •

edited

Loading

shiv-tyagi commented Dec 3, 2024 •

edited

Loading

codecov-commenter commented Dec 4, 2024 •

edited

Loading

shiv-tyagi commented Dec 4, 2024 •

edited

Loading