-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implementation of the NSS client in Rust #29
Conversation
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.
Excellent draft! I think there are some questions to really ask ourself, in particular around keeping the client connection alive as we saw that processes makes a lot of calls. Otherwise, this is a good start :)
See my comments inline.
nss/src/group/mod.rs
Outdated
Ok(r) => Response::Success(group_entries_to_groups(r.into_inner().entries)), | ||
Err(e) => { | ||
error!("error when listing groups: {}", e); | ||
Response::NotFound |
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.
Are we sure this is the correct error for all error cases?
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.
Oh, I'm definitely sure it isn't 😂 This is a "temporary" thing, as we can only return a static user from the server or an error (which would be a NotFound one). I wanted to go back to this once we had a proper NSS server (after the cache implementation) so I could understand better what I should be parsing from the Error to define the return value.
Looking at aad-auth
, it looks like the most common response is Unavail
but, again, I need the cache implementation to understand what I should be parsing in the error message.
nss/src/main.rs
Outdated
|
||
const SOCKET_PATH: &str = "/run/authd.sock"; | ||
|
||
// This main function is only used for manual testing without the need to link the NSS library and should be removed before the release |
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.
ack then :) Probably add a TODO so that it’s clearly marked as such
ce57770
to
c56896e
Compare
c56896e
to
c25609c
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.
Rust is back to our project :)
I think this PR is the opportunity to hook it back in term of testing with our CI (I don’t think we should merge without basic quality checks). Also, see some of the comments around returned error values and so on.
100d673
to
c34f978
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.
One last thing I will let you change before merging them! Nice work :)
348fdc1
to
fb28651
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.
Awesome work!
This adds the Rust code that will act as our NSS client, along with the implementation of the NSS service definition and messages in the proto files and a static implementation for the NSS server (so we could test the output on the Rust side).
UDENG-749