Skip to content

Create gRPC service to be used by the CLI #855

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

adombeck
Copy link
Contributor

The current state of #782 adds the gRPC methods used by the CLI to the NSS service. They don't belong there, so this PR adds a new gRPC service which provides methods intended for use by the CLI.

UDENG-6492

@adombeck adombeck marked this pull request as ready for review March 21, 2025 12:45
@adombeck adombeck requested a review from a team as a code owner March 21, 2025 12:45
@adombeck adombeck requested a review from 3v1n0 March 24, 2025 10:16
@@ -195,3 +195,31 @@ message ShadowEntry {
message ShadowEntries {
repeated ShadowEntry entries = 1;
}

service CLI {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't call it CLI but I rather something like Manager or something similar, as this something that we could ideally expose later in other tools (such as gcc), and we can include here every operation that is about the management of authd (or its users) itself, despite the UI that is used for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, what about this structure then:

service UsersService {
  rpc ListUsers(Empty) returns (Users);
  rpc DisableUser(DisableUserRequest) returns (Empty);
  rpc EnableUser(EnableUserRequest) returns (Empty);
}

message User {
  string name = 1;
  uint32 uid = 2;
  uint32 gid = 3;
  string gecos = 4;
  string homedir = 5;
  string shell = 6;
}

message Users {
  repeated User users = 1;
}

service GroupsService {
  rpc ListGroups(Empty) returns (Groups);
}

message Group {
  string name = 1;
  uint32 gid = 2;
  repeated string members = 3;
}

message Groups {
  repeated Group groups = 1;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's better, we can still keep it more generic, but I feel this is enough, and in case we can add another one if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented something similar to that

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 7 times, most recently from 371a5e8 to 5f50f59 Compare April 1, 2025 12:48
@adombeck adombeck requested review from 3v1n0 and denisonbarbosa April 1, 2025 13:01
@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch from 5f50f59 to 06df31f Compare April 1, 2025 13:41
@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.37%. Comparing base (fbb5642) to head (69dc5ec).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
internal/services/user/user.go 92.53% 5 Missing ⚠️
internal/services/permissions.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
- Coverage   85.37%   85.37%   -0.01%     
==========================================
  Files          80       80              
  Lines        5532     5518      -14     
  Branches      109      109              
==========================================
- Hits         4723     4711      -12     
+ Misses        754      752       -2     
  Partials       55       55              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Some suggestions that I think will make the changes better (I didn't add the same comment on every single crate of the module, but some apply to all three)

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 3 times, most recently from a941098 to 9ddf755 Compare April 2, 2025 11:56
@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch from 9ddf755 to 0fdb6bc Compare April 8, 2025 13:25
@adombeck adombeck marked this pull request as draft April 9, 2025 10:21
@adombeck
Copy link
Contributor Author

adombeck commented Apr 9, 2025

Converting to draft until I fixed the tests

userPasswd := requireNSSUser(t, nssClient, user)
group := requireNSSGroup(t, nssClient, userPasswd.Gid)
if userClient != nil {
userPasswd := requireNSSUser(t, userClient, user)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this function too please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if nssClient != nil {
requireNoNSSUser(t, nssClient, user)
if userClient != nil {
requireNoNSSUser(t, userClient, user)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this function too please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

group := requireNSSGroup(t, nssClient, userPasswd.Gid)
if userClient != nil {
userPasswd := requireNSSUser(t, userClient, user)
group := requireNSSGroup(t, userClient, userPasswd.Gid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

And this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 4 times, most recently from e2cf75f to aaf77bf Compare April 10, 2025 12:28
@adombeck adombeck marked this pull request as ready for review April 10, 2025 13:38
@adombeck adombeck requested a review from denisonbarbosa April 11, 2025 09:29
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.

Looks good to me. Nice work! Once @3v1n0 is also happy, feel free to merge.

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch from 8c7cff5 to 61b1d8d Compare April 11, 2025 15:36
The NSS service duplicated functionality from the user service.
@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch from 61b1d8d to 69dc5ec Compare April 11, 2025 22:08
@shiv-tyagi
Copy link
Contributor

Thanks @adombeck for the awesome work.

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.

6 participants