-
Notifications
You must be signed in to change notification settings - Fork 68
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
Create Connection
abstraction for client communication
#207
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.
Thanks!! Looks really good 💯 a couple of comments below, feel free to discuss
src/front/listener.rs
Outdated
#[derive(Copy, Clone, Debug)] | ||
pub enum ConnectionMetadata { | ||
NoMetadata, | ||
} | ||
|
||
/// Represents a connection to a single client. Contains a stream, used for communication with the | ||
/// client, and some metadata associated with the connection that might be useful elsewhere (i.e. | ||
/// authentication, etc). | ||
pub struct Connection { | ||
pub stream: Box<dyn ReadWrite + Send>, | ||
pub metadata: ConnectionMetadata, | ||
} |
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.
I think it's more idiomatic instead of having an enum with a variant EnumType::NoSomething
, to actually use Option<EnumType>
. You can probably just create an "empty" UnixSocket
variant that we flesh out when we actually need that metadata.
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.
I completely agree. Will change this. :)
src/front/listener.rs
Outdated
impl fmt::Debug for Connection { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
// NOTE: we omit the field `stream` because it doesn't implement `Debug`. | ||
f.debug_struct("Connection") | ||
.field("metadata", &self.metadata) | ||
.finish() | ||
} | ||
} |
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 perfectly correct, but you can do it more easily with #[derivative(...)]
which allows you to ignore some fields from Debug
- you can look at BackEndHandler
as an example.
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.
TIL! Will change.
src/front/domain_socket.rs
Outdated
// TODO: when possible, we want to replace this with the (uid, gid, pid) | ||
// triple for peer credentials. | ||
metadata: ConnectionMetadata::NoMetadata, |
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.
It's not a conversation for here, just a random note, but I'd say that any IPC can/should have associated metadata - e.g. if we use TCP sockets, we'll have the peer IP address and port which could be used in the authentication.
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 🙂
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.
💯
@@ -49,9 +55,10 @@ mod test { | |||
|
|||
let app_name = "app_name".to_string(); | |||
let req_auth = RequestAuth::new(app_name.clone().into_bytes()); | |||
let conn_metadata = ConnectionMetadata::NoMetadata; |
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.
You'll need to remove these and just pass None
😄
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.
Doh -- fixed. :)
7acba33
to
102139c
Compare
This commit is in response to issue parallaxsecond#199. Here, we introduce a `Connection` struct which currently contains two things: * `stream` -- an object representing the communication stream between client and service. * `metadata` -- an optional enum instance that captures metadata about the connection. This abstraction allows us to carry more information forwards toward the frontend/authenticator/... . Specifically, this abstraction was created with UNIX domain sockets in mind (but the usefulness is not limited here). UNIX domain sockets allow incoming connections to be queried for peer metadata, which is a triple (uid, gid, pid) of the peer process that is connecting. Under certain configurations, this can be used for authentication. This commit places us in a position of being able to use said metadata for authentication if needed. Signed-off-by: Joe Ellis <[email protected]>
This commit is in response to issue #199. Here, we introduce a
Connection
struct which currently contains two things:stream
-- an object representing the communication stream betweenclient and service.
metadata
-- an enum instance that captures metadata about theconnection.
This abstraction allows us to carry more information forwards toward the
frontend/authenticator/... .
Specifically, this abstraction was created with UNIX domain sockets in
mind (but the usefulness is not limited here). UNIX domain sockets allow
incoming connections to be queried for peer metadata, which is a triple
(uid, gid, pid) of the peer process that is connecting. Under certain
configurations, this can be used for authentication.
This commit places us in a position of being able to use said metadata
for authentication if needed.
Signed-off-by: Joe Ellis [email protected]