-
Notifications
You must be signed in to change notification settings - Fork 462
[ID-Mapped Mount] Impl Channel & Message #3409
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
Changes from 5 commits
57d1594
a7aadfa
0a2a0b1
07afe2a
8be375d
0dde427
8cabf49
5290a28
c1c6d88
900e26d
4c60749
6518958
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,24 @@ | ||
| use std::collections::HashMap; | ||
| use std::os::unix::prelude::{AsRawFd, RawFd}; | ||
| use std::os::unix::prelude::{AsRawFd, FromRawFd, OwnedFd, RawFd}; | ||
|
|
||
| use nix::unistd::Pid; | ||
|
|
||
| use crate::channel::{Receiver, Sender, channel}; | ||
| use crate::network::cidr::CidrAddress; | ||
| use crate::process::message::Message; | ||
| use crate::process::message::{Message, MountMsg}; | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum ChannelError { | ||
| #[error("received unexpected message: {received:?}, expected: {expected:?}")] | ||
| #[error( | ||
| "received unexpected message: {received:?}{expected_suffix}", | ||
| expected_suffix = .expected | ||
| .as_ref() | ||
| .map(|msg| format!(", expected: {msg:?}")) | ||
| .unwrap_or_default() | ||
| )] | ||
| UnexpectedMessage { | ||
| expected: Message, | ||
| received: Message, | ||
| expected: Option<Box<Message>>, | ||
| received: Box<Message>, | ||
| }, | ||
| #[error("failed to receive. {msg:?}. {source:?}")] | ||
| ReceiveError { | ||
|
|
@@ -28,6 +34,10 @@ pub enum ChannelError { | |
| ExecError(String), | ||
| #[error("intermediate process error {0}")] | ||
| OtherError(String), | ||
| #[error("missing fd from mount request")] | ||
| MissingMountFds, | ||
| #[error("mount request failed: {0}")] | ||
| MountFdError(String), | ||
| } | ||
|
|
||
| // Channel Design | ||
|
|
@@ -67,6 +77,12 @@ impl MainSender { | |
| Ok(()) | ||
| } | ||
|
|
||
| pub fn request_mount_fd(&mut self, msg: MountMsg) -> Result<(), ChannelError> { | ||
| self.sender.send(Message::MountFdPlease(msg))?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn network_setup_ready(&mut self) -> Result<(), ChannelError> { | ||
| tracing::debug!("notify network setup ready"); | ||
| self.sender.send(Message::SetupNetworkDeviceReady)?; | ||
|
|
@@ -131,8 +147,8 @@ impl MainReceiver { | |
| Message::ExecFailed(err) => Err(ChannelError::ExecError(err)), | ||
| Message::OtherError(err) => Err(ChannelError::OtherError(err)), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::IntermediateReady(0), | ||
| received: msg, | ||
| expected: Some(Box::new(Message::IntermediateReady(0))), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -148,12 +164,39 @@ impl MainReceiver { | |
| match msg { | ||
| Message::WriteMapping => Ok(()), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::WriteMapping, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::WriteMapping)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
||
| pub fn wait_for_mount_fd_request(&mut self) -> Result<MountMsg, ChannelError> { | ||
| let msg = self | ||
| .receiver | ||
| .recv() | ||
| .map_err(|err| ChannelError::ReceiveError { | ||
| msg: "waiting for mount fd request".to_string(), | ||
| source: err, | ||
| })?; | ||
|
|
||
| match msg { | ||
| Message::MountFdPlease(req) => Ok(req), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: None, | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
||
| pub fn recv_message_with_fds(&mut self) -> Result<(Message, Option<[RawFd; 1]>), ChannelError> { | ||
| self.receiver | ||
| .recv_with_fds::<[RawFd; 1]>() | ||
| .map_err(|err| ChannelError::ReceiveError { | ||
| msg: "waiting for message".to_string(), | ||
| source: err, | ||
| }) | ||
| } | ||
|
|
||
| pub fn wait_for_seccomp_request(&mut self) -> Result<i32, ChannelError> { | ||
| let (msg, fds) = self.receiver.recv_with_fds::<[RawFd; 1]>().map_err(|err| { | ||
| ChannelError::ReceiveError { | ||
|
|
@@ -177,8 +220,8 @@ impl MainReceiver { | |
| Ok(fd) | ||
| } | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::SeccompNotify, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::SeccompNotify)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -194,8 +237,8 @@ impl MainReceiver { | |
| match msg { | ||
| Message::SetupNetworkDeviceReady => Ok(()), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::SetupNetworkDeviceReady, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::SetupNetworkDeviceReady)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -217,8 +260,8 @@ impl MainReceiver { | |
| "error in executing process : {err}" | ||
| ))), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::InitReady, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::InitReady)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -234,8 +277,8 @@ impl MainReceiver { | |
| match msg { | ||
| Message::HookRequest => Ok(()), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::HookRequest, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::HookRequest)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -292,8 +335,8 @@ impl IntermediateReceiver { | |
| match msg { | ||
| Message::MappingWritten => Ok(()), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::MappingWritten, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::MappingWritten)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -340,6 +383,17 @@ impl InitSender { | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn send_mount_fd_reply(&mut self, fd: RawFd) -> Result<(), ChannelError> { | ||
| self.sender.send_fds(Message::MountFdReply, &[fd])?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn send_mount_fd_error(&mut self, err: String) -> Result<(), ChannelError> { | ||
| self.sender.send(Message::MountFdError(err))?; | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| pub struct InitReceiver { | ||
|
|
@@ -359,8 +413,8 @@ impl InitReceiver { | |
| match msg { | ||
| Message::SeccompNotifyDone => Ok(()), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::SeccompNotifyDone, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::SeccompNotifyDone)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -378,8 +432,8 @@ impl InitReceiver { | |
| match msg { | ||
| Message::MoveNetworkDevice(addr) => Ok(addr), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::WriteMapping, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::WriteMapping)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -395,8 +449,8 @@ impl InitReceiver { | |
| match msg { | ||
| Message::HookDone => Ok(()), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Message::HookDone, | ||
| received: msg, | ||
| expected: Some(Box::new(Message::HookDone)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -406,10 +460,37 @@ impl InitReceiver { | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn wait_for_mount_fd_reply(&mut self) -> Result<OwnedFd, ChannelError> { | ||
| let (msg, fds) = self.receiver.recv_with_fds::<[RawFd; 1]>().map_err(|err| { | ||
| ChannelError::ReceiveError { | ||
| msg: "waiting for mount fd reply".to_string(), | ||
| source: err, | ||
| } | ||
| })?; | ||
|
|
||
| match msg { | ||
| Message::MountFdReply => { | ||
| let fd = match fds { | ||
| Some([fd]) => fd, | ||
| _ => return Err(ChannelError::MissingMountFds), | ||
| }; | ||
| Ok(unsafe { OwnedFd::from_raw_fd(fd) }) | ||
| } | ||
| Message::MountFdError(err) => Err(ChannelError::MountFdError(err)), | ||
| msg => Err(ChannelError::UnexpectedMessage { | ||
| expected: Some(Box::new(Message::MountFdReply)), | ||
| received: Box::new(msg), | ||
| }), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use std::io::{Read, Seek, SeekFrom, Write}; | ||
| use std::os::fd::AsRawFd; | ||
|
|
||
| use anyhow::{Context, Result}; | ||
| use nix::sys::wait; | ||
| use nix::unistd; | ||
|
|
@@ -490,6 +571,81 @@ mod tests { | |
| Ok(()) | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a test for |
||
| #[test] | ||
| #[serial] | ||
| fn test_channel_mount_fd_error() -> Result<()> { | ||
| let (sender, receiver) = &mut init_channel()?; | ||
| sender.send_mount_fd_error("boom".to_string())?; | ||
| let err = receiver.wait_for_mount_fd_reply().unwrap_err(); | ||
| assert!(matches!(err, ChannelError::MountFdError(msg) if msg == "boom")); | ||
| sender.close()?; | ||
| receiver.close()?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| #[serial] | ||
| fn test_channel_mount_fd_reply_success() -> Result<()> { | ||
| let (sender, receiver) = &mut init_channel()?; | ||
| let mut file = tempfile::tempfile()?; | ||
| file.write_all(b"ok")?; | ||
|
|
||
| sender.send_mount_fd_reply(file.as_raw_fd())?; | ||
| let fd = receiver.wait_for_mount_fd_reply()?; | ||
| let mut received = std::fs::File::from(fd); | ||
| received.seek(SeekFrom::Start(0))?; | ||
| let mut buf = String::new(); | ||
| received.read_to_string(&mut buf)?; | ||
| assert_eq!(buf, "ok"); | ||
|
|
||
| sender.close()?; | ||
| receiver.close()?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| #[serial] | ||
| fn test_channel_mount_fd_reply_missing_fds() -> Result<()> { | ||
| let (mut sender, receiver) = channel::<Message>()?; | ||
| let mut receiver = InitReceiver { receiver }; | ||
|
|
||
| sender.send(Message::MountFdReply)?; | ||
| let err = receiver.wait_for_mount_fd_reply().unwrap_err(); | ||
| assert!(matches!(err, ChannelError::MissingMountFds)); | ||
|
|
||
| sender.close()?; | ||
| receiver.close()?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| #[serial] | ||
| fn test_channel_mount_fd_request() -> Result<()> { | ||
| let (sender, receiver) = &mut main_channel()?; | ||
| let request = MountMsg { | ||
| source: "/proc/self/ns/user".to_string(), | ||
| idmap: Some(crate::process::message::MountIdMap { | ||
| uid_mappings: vec![], | ||
| gid_mappings: vec![], | ||
| recursive: true, | ||
| }), | ||
| }; | ||
|
|
||
| sender.request_mount_fd(request.clone())?; | ||
| let received = receiver.wait_for_mount_fd_request()?; | ||
|
|
||
| assert_eq!(received.source, request.source); | ||
| let received_idmap = received.idmap.context("missing idmap in mount request")?; | ||
| let request_idmap = request.idmap.context("missing idmap in mount request")?; | ||
| assert_eq!(received_idmap.recursive, request_idmap.recursive); | ||
| assert_eq!(received_idmap.uid_mappings, request_idmap.uid_mappings); | ||
| assert_eq!(received_idmap.gid_mappings, request_idmap.gid_mappings); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good idea to add PartialEq and Eq. assert_eq!(received, request); |
||
|
|
||
| sender.close()?; | ||
| receiver.close()?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| #[serial] | ||
| fn test_channel_init_ready() -> Result<()> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||
| use core::fmt; | ||||||
| use std::collections::HashMap; | ||||||
|
|
||||||
| use oci_spec::runtime::LinuxIdMapping; | ||||||
| use serde::{Deserialize, Serialize}; | ||||||
|
|
||||||
| use crate::network::cidr::CidrAddress; | ||||||
|
|
@@ -16,8 +17,11 @@ pub enum Message { | |||||
| SeccompNotifyDone, | ||||||
| SetupNetworkDeviceReady, | ||||||
| MoveNetworkDevice(HashMap<String, Vec<CidrAddress>>), | ||||||
| MountFdPlease(MountMsg), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about this?
Suggested change
|
||||||
| MountFdReply, | ||||||
| ExecFailed(String), | ||||||
| OtherError(String), | ||||||
| MountFdError(String), | ||||||
| HookRequest, | ||||||
| HookDone, | ||||||
| } | ||||||
|
|
@@ -35,8 +39,24 @@ impl fmt::Display for Message { | |||||
| Message::SeccompNotifyDone => write!(f, "SeccompNotifyDone"), | ||||||
| Message::HookRequest => write!(f, "HookRequest"), | ||||||
| Message::HookDone => write!(f, "HookDone"), | ||||||
| Message::MountFdPlease(_) => write!(f, "MountFdPlease"), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason why you aren't showing the contents of |
||||||
| Message::MountFdReply => write!(f, "MountFdReply"), | ||||||
| Message::MountFdError(err) => write!(f, "MountFdError({})", err), | ||||||
| Message::ExecFailed(s) => write!(f, "ExecFailed({})", s), | ||||||
| Message::OtherError(s) => write!(f, "OtherError({})", s), | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Serialize, Deserialize, Clone)] | ||||||
| pub struct MountMsg { | ||||||
| pub source: String, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a path, right? If so, it'd be better to use Path. |
||||||
| pub idmap: Option<MountIdMap>, | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Serialize, Deserialize, Clone)] | ||||||
| pub struct MountIdMap { | ||||||
| pub uid_mappings: Vec<LinuxIdMapping>, | ||||||
| pub gid_mappings: Vec<LinuxIdMapping>, | ||||||
| pub recursive: bool, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use an Enum here since we might need to add more types in the future. |
||||||
| } | ||||||
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.
How about this approach? I want to avoid Option as it leaves the meaning of None unclear.
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.
@nayuta723
That’s certainly true.