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

Command line to get passwd, group and shadow entries from local cache. #56

Merged
merged 21 commits into from Sep 5, 2022

Conversation

denisonbarbosa
Copy link
Member

Interface

aad_auth <command> [args]

Command

  • getent <database> [key]
    • database: Name of the database. We currently support 3 databases: passwd, group, shadow. It will query the respective services.
    • key is optional:
      - key can be a name or id;
      - If key is found the record corresponding to the key is returned;
      - If key is not found no record is returned and the exit status is 0;
      - If key is not specified then the entire database is listed;

Exit status

If everything goes right (no input parsing issues), the command always returns 0. We don't reflect the nss return status in the returned code.

Debug

Logs verbosity are controlled in getent by NSS_AAD_DEBUG.

Output

Output is printed on stdout, and has multiple lines. The first line is the nss_status_code and errno values. Following line is the elements printed by the request, separated by ':'

{nssStatus}:{errno}
<return value 1>
<return value 2>

This functions will be useful for the nss getent results and, this way,
the privacy of the struct fields can be preserved.
There might some details missing, but the core is here.
@denisonbarbosa denisonbarbosa requested a review from a team as a code owner August 31, 2022 19:42
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

I think you missed some of the things we discussed yesterday, like tests needed for the next export API in shadow/group/passwd, some comments needed on the error handling in the function, having --help printing the usage, filtering invalid inputs, copy all test cases from our existing integration tests and add the "empty db" test case…). This was for all those reasons we discussed for you to take more time before submitting for review :)

I think the general approach is correct, but here are some suggestions for a more consistent API too and testing story. Let me know what you think!

internal/nss/group/group.go Show resolved Hide resolved
nss/aad_auth/aadauth_test.go Outdated Show resolved Hide resolved
nss/aad_auth/error_c.go Show resolved Hide resolved
nss/aad_auth/getent.go Outdated Show resolved Hide resolved
nss/aad_auth/getent.go Outdated Show resolved Hide resolved
nss/aad_auth/internal_test.go Outdated Show resolved Hide resolved
nss/aad_auth/main.go Show resolved Hide resolved
nss/aad_auth/main.go Show resolved Hide resolved
nss/aad_auth/main.go Outdated Show resolved Hide resolved
nss/aad_auth/main.go Outdated Show resolved Hide resolved
@denisonbarbosa denisonbarbosa marked this pull request as draft September 1, 2022 11:21
Added tests for the new exported function String(), along with their
golden files.
Still thinking about the some of the func names. Open for suggestions. Pls help!
Adding some more testcases and helpers in mock_test to increase our
options when trying to test the formatter (might be overkill, but isn't
hurting anyone).
Added a LOT of new test cases for TestGetEnt along with some
refactoring.
Might still be missing a few test cases, but it's going well so far.
A little unsure about the naming of some of them, specially about the
"try" ones.
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2022

Codecov Report

Merging #56 (236b5cb) into main (79f51d5) will increase coverage by 0.93%.
The diff coverage is 82.93%.

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   75.04%   75.97%   +0.93%     
==========================================
  Files          36       39       +3     
  Lines        2224     2435     +211     
==========================================
+ Hits         1669     1850     +181     
- Misses        492      524      +32     
+ Partials       63       61       -2     
Impacted Files Coverage Δ
nss/aad_auth/main.go 0.00% <0.00%> (ø)
nss/aad_auth/getent.go 99.13% <99.13%> (ø)
internal/nss/group/group.go 100.00% <100.00%> (ø)
internal/nss/passwd/passwd.go 100.00% <100.00%> (ø)
internal/nss/shadow/shadow.go 100.00% <100.00%> (ø)
nss/aad_auth/error_c.go 100.00% <100.00%> (ø)
internal/cache/shadowdb.go 88.52% <0.00%> (+9.83%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

This is still in draft, but you told me on MM that it’s ready now, so rereviewing.

Some small comments, but this is going in the right direction! I like it :)

The more annoying part is the wrong statuses returned on the "empty cache" and "no cache" tests. Do you mind having a look there? (I think you can start printing by what we return from the API).

nss/aad_auth/error_c.go Show resolved Hide resolved
nss/aad_auth/main.go Outdated Show resolved Hide resolved
nss/aad_auth/main.go Outdated Show resolved Hide resolved
nss/aad_auth/main.go Outdated Show resolved Hide resolved
nss/aad_auth/main.go Outdated Show resolved Hide resolved
nss/aad_auth/aadauth_test.go Outdated Show resolved Hide resolved
nss/aad_auth/aadauth_test.go Show resolved Hide resolved
@denisonbarbosa denisonbarbosa marked this pull request as ready for review September 2, 2022 18:24
@denisonbarbosa denisonbarbosa requested review from didrocks and a team September 2, 2022 18:24
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Still some fixes to do.

Btw, the QA linting tests is failing from the start, you should have a look there ;)

nss/aad_auth/main.go Outdated Show resolved Hide resolved
nss/aad_auth/main.go Show resolved Hide resolved
nss/aad_auth/main.go Outdated Show resolved Hide resolved
nss/aad_auth/internal_test.go Outdated Show resolved Hide resolved
nss/aad_auth/error_c.go Outdated Show resolved Hide resolved
nss/aad_auth/getent.go Show resolved Hide resolved
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

All tests passes, no more comments. Excellent work!

Feel free to merge now :)

@denisonbarbosa denisonbarbosa merged commit 2ec50d4 into main Sep 5, 2022
@denisonbarbosa denisonbarbosa deleted the get-entries-command branch September 5, 2022 14:06
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.

None yet

3 participants