github/kata-containers
1
0
Fork 1
mirror of https://github.com/kata-containers/kata-containers.git synced 2025-06-27 15:57:09 +00:00
Code Issues Projects Releases Wiki Activity

Agent: OCI hooks return malformed json

Browse Source 
This PR fixes wrong serialization of OCI state object.
OCI hooks end up with a JSON string with double quotes in `state` field.

This happens because of confusion `Debug` and `Display` traits. Debug trait
returns a string representation with double quotes.

Ideally we should not use Debug as a part of serialization process, so a bit
more safer fix would be to move container states to `oci` crate and simply
disallow wrong values in that field.

`ContainerState` in go spec: https://github.com/opencontainers/runtime-spec/blob/master/specs-go/state.go#L4

Fixes: #1404

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
This commit is contained in:
Maksym Pavlenko 2021-02-11 18:55:24 -08:00
parent 96196e102e
commit df14d386a5
2 changed files with 45 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
src/agent/oci/src/lib.rs
View File

@ -784,7 +784,17 @@ pub struct LinuxIntelRdt {
pub l3_cache_schema: String, pub l3_cache_schema: String,
} }
#[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] #[derive(Debug, Serialize, Deserialize, Copy, Clone, PartialEq)]
#[serde(rename_all = "lowercase")]
pub enum ContainerState {
CREATING,
CREATED,
RUNNING,
STOPPED,
PAUSED,
}
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
pub struct State { pub struct State {
#[serde( #[serde(
default, default,
@ -794,8 +804,7 @@ pub struct State {
pub version: String, pub version: String,
#[serde(default, skip_serializing_if = "String::is_empty")] #[serde(default, skip_serializing_if = "String::is_empty")]
pub id: String, pub id: String,
#[serde(default, skip_serializing_if = "String::is_empty")] pub status: ContainerState,
pub status: String,
#[serde(default)] #[serde(default)]
pub pid: i32, pub pid: i32,
#[serde(default, skip_serializing_if = "String::is_empty")] #[serde(default, skip_serializing_if = "String::is_empty")]
@ -806,6 +815,8 @@ pub struct State {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*;
#[test] #[test]
fn test_deserialize_state() { fn test_deserialize_state() {
let data = r#"{ let data = r#"{
@ -818,10 +829,10 @@ mod tests {
"myKey": "myValue" "myKey": "myValue"
} }
}"#; }"#;
let expected = crate::State { let expected = State {
version: "0.2.0".to_string(), version: "0.2.0".to_string(),
id: "oci-container1".to_string(), id: "oci-container1".to_string(),
status: "running".to_string(), status: ContainerState::RUNNING,
pid: 4422, pid: 4422,
bundle: "/containers/redis".to_string(), bundle: "/containers/redis".to_string(),
annotations: [("myKey".to_string(), "myValue".to_string())] annotations: [("myKey".to_string(), "myValue".to_string())]

86
src/agent/rustjail/src/container.rs
View File

@ -5,11 +5,10 @@
use anyhow::{anyhow, Context, Result}; use anyhow::{anyhow, Context, Result};
use libc::pid_t; use libc::pid_t;
use oci::{ContainerState, LinuxDevice, LinuxIDMapping};
use oci::{Hook, Linux, LinuxNamespace, LinuxResources, Spec}; use oci::{Hook, Linux, LinuxNamespace, LinuxResources, Spec};
use oci::{LinuxDevice, LinuxIDMapping};
use std::clone::Clone; use std::clone::Clone;
use std::ffi::{CStr, CString}; use std::ffi::{CStr, CString};
use std::fmt;
use std::fmt::Display; use std::fmt::Display;
use std::fs; use std::fs;
use std::os::unix::io::RawFd; use std::os::unix::io::RawFd;
@ -73,37 +72,29 @@ const FIFO_FD: &str = "FIFO_FD";
const HOME_ENV_KEY: &str = "HOME"; const HOME_ENV_KEY: &str = "HOME";
const PIDNS_FD: &str = "PIDNS_FD"; const PIDNS_FD: &str = "PIDNS_FD";
#[derive(PartialEq, Clone, Copy)]
pub enum Status {
CREATED,
RUNNING,
STOPPED,
PAUSED,
}
#[derive(Debug)] #[derive(Debug)]
pub struct ContainerStatus { pub struct ContainerStatus {
pre_status: Status, pre_status: ContainerState,
cur_status: Status, cur_status: ContainerState,
} }
impl ContainerStatus { impl ContainerStatus {
fn new() -> Self { fn new() -> Self {
ContainerStatus { ContainerStatus {
pre_status: Status::CREATED, pre_status: ContainerState::CREATED,
cur_status: Status::CREATED, cur_status: ContainerState::CREATED,
} }
} }
fn status(&self) -> Status { fn status(&self) -> ContainerState {
self.cur_status self.cur_status
} }
fn pre_status(&self) -> Status { fn pre_status(&self) -> ContainerState {
self.pre_status self.pre_status
} }
fn transition(&mut self, to: Status) { fn transition(&mut self, to: ContainerState) {
self.pre_status = self.status(); self.pre_status = self.status();
self.cur_status = to; self.cur_status = to;
} }
@ -112,17 +103,6 @@ impl ContainerStatus {
pub type Config = CreateOpts; pub type Config = CreateOpts;
type NamespaceType = String; type NamespaceType = String;
impl fmt::Debug for Status {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Status::CREATED => write!(f, "{:?}", "created"),
Status::RUNNING => write!(f, "{:?}", "running"),
Status::STOPPED => write!(f, "{:?}", "stopped"),
Status::PAUSED => write!(f, "{:?}", "paused"),
}
}
}
lazy_static! { lazy_static! {
static ref NAMESPACES: HashMap<&'static str, CloneFlags> = { static ref NAMESPACES: HashMap<&'static str, CloneFlags> = {
let mut m = HashMap::new(); let mut m = HashMap::new();
@ -222,7 +202,7 @@ pub struct BaseState {
#[async_trait] #[async_trait]
pub trait BaseContainer { pub trait BaseContainer {
fn id(&self) -> String; fn id(&self) -> String;
fn status(&self) -> Status; fn status(&self) -> ContainerState;
fn state(&self) -> Result<State>; fn state(&self) -> Result<State>;
fn oci_state(&self) -> Result<OCIState>; fn oci_state(&self) -> Result<OCIState>;
fn config(&self) -> Result<&Config>; fn config(&self) -> Result<&Config>;
@ -286,7 +266,7 @@ pub trait Container: BaseContainer {
impl Container for LinuxContainer { impl Container for LinuxContainer {
fn pause(&mut self) -> Result<()> { fn pause(&mut self) -> Result<()> {
let status = self.status(); let status = self.status();
if status != Status::RUNNING && status != Status::CREATED { if status != ContainerState::RUNNING && status != ContainerState::CREATED {
return Err(anyhow!( return Err(anyhow!(
"failed to pause container: current status is: {:?}", "failed to pause container: current status is: {:?}",
status status
@ -299,7 +279,7 @@ impl Container for LinuxContainer {
.unwrap() .unwrap()
.freeze(FreezerState::Frozen)?; .freeze(FreezerState::Frozen)?;
self.status.transition(Status::PAUSED); self.status.transition(ContainerState::PAUSED);
return Ok(()); return Ok(());
} }
Err(anyhow!("failed to get container's cgroup manager")) Err(anyhow!("failed to get container's cgroup manager"))
@ -307,7 +287,7 @@ impl Container for LinuxContainer {
fn resume(&mut self) -> Result<()> { fn resume(&mut self) -> Result<()> {
let status = self.status(); let status = self.status();
if status != Status::PAUSED { if status != ContainerState::PAUSED {
return Err(anyhow!("container status is: {:?}, not paused", status)); return Err(anyhow!("container status is: {:?}, not paused", status));
} }
@ -317,7 +297,7 @@ impl Container for LinuxContainer {
.unwrap() .unwrap()
.freeze(FreezerState::Thawed)?; .freeze(FreezerState::Thawed)?;
self.status.transition(Status::RUNNING); self.status.transition(ContainerState::RUNNING);
return Ok(()); return Ok(());
} }
Err(anyhow!("failed to get container's cgroup manager")) Err(anyhow!("failed to get container's cgroup manager"))
@ -736,7 +716,7 @@ impl BaseContainer for LinuxContainer {
self.id.clone() self.id.clone()
} }
fn status(&self) -> Status { fn status(&self) -> ContainerState {
self.status.status() self.status.status()
} }
@ -751,7 +731,7 @@ impl BaseContainer for LinuxContainer {
}; };
let status = self.status(); let status = self.status();
let pid = if status != Status::STOPPED { let pid = if status != ContainerState::STOPPED {
self.init_process_pid self.init_process_pid
} else { } else {
0 0
@ -771,7 +751,7 @@ impl BaseContainer for LinuxContainer {
Ok(OCIState { Ok(OCIState {
version: oci.version.clone(), version: oci.version.clone(),
id: self.id(), id: self.id(),
status: format!("{:?}", status), status,
pid, pid,
bundle, bundle,
annotations: oci.annotations.clone(), annotations: oci.annotations.clone(),
@ -1014,7 +994,7 @@ impl BaseContainer for LinuxContainer {
if init { if init {
self.exec()?; self.exec()?;
self.status.transition(Status::RUNNING); self.status.transition(ContainerState::RUNNING);
} }
Ok(()) Ok(())
@ -1036,7 +1016,7 @@ impl BaseContainer for LinuxContainer {
} }
} }
self.status.transition(Status::STOPPED); self.status.transition(ContainerState::STOPPED);
mount::umount2( mount::umount2(
spec.root.as_ref().unwrap().path.as_str(), spec.root.as_ref().unwrap().path.as_str(),
MntFlags::MNT_DETACH, MntFlags::MNT_DETACH,
@ -1072,7 +1052,7 @@ impl BaseContainer for LinuxContainer {
.unwrap() .unwrap()
.as_secs(); .as_secs();
self.status.transition(Status::RUNNING); self.status.transition(ContainerState::RUNNING);
unistd::close(fd)?; unistd::close(fd)?;
Ok(()) Ok(())
@ -1676,7 +1656,7 @@ mod tests {
&OCIState { &OCIState {
version: "1.2.3".to_string(), version: "1.2.3".to_string(),
id: "321".to_string(), id: "321".to_string(),
status: "".to_string(), status: ContainerState::RUNNING,
pid: 2, pid: 2,
bundle: "".to_string(), bundle: "".to_string(),
annotations: Default::default(), annotations: Default::default(),
@ -1689,11 +1669,11 @@ mod tests {
#[test] #[test]
fn test_status_transtition() { fn test_status_transtition() {
let mut status = ContainerStatus::new(); let mut status = ContainerStatus::new();
let status_table: [Status; 4] = [ let status_table: [ContainerState; 4] = [
Status::CREATED, ContainerState::CREATED,
Status::RUNNING, ContainerState::RUNNING,
Status::PAUSED, ContainerState::PAUSED,
Status::STOPPED, ContainerState::STOPPED,
]; ];
for s in status_table.iter() { for s in status_table.iter() {
@ -1727,14 +1707,6 @@ mod tests {
set_stdio_permissions(old_uid).unwrap(); set_stdio_permissions(old_uid).unwrap();
} }
#[test]
fn test_status_fmt() {
assert_eq!("\"created\"", format!("{:?}", Status::CREATED));
assert_eq!("\"running\"", format!("{:?}", Status::RUNNING));
assert_eq!("\"paused\"", format!("{:?}", Status::PAUSED));
assert_eq!("\"stopped\"", format!("{:?}", Status::STOPPED));
}
#[test] #[test]
fn test_namespaces() { fn test_namespaces() {
lazy_static::initialize(&NAMESPACES); lazy_static::initialize(&NAMESPACES);
@ -1838,7 +1810,7 @@ mod tests {
fn test_linuxcontainer_pause_bad_status() { fn test_linuxcontainer_pause_bad_status() {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| { let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
// Change state to pause, c.pause() should fail // Change state to pause, c.pause() should fail
c.status.transition(Status::PAUSED); c.status.transition(ContainerState::PAUSED);
c.pause().map_err(|e| anyhow!(e)) c.pause().map_err(|e| anyhow!(e))
}); });
@ -1870,7 +1842,7 @@ mod tests {
fn test_linuxcontainer_resume_bad_status() { fn test_linuxcontainer_resume_bad_status() {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| { let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
// Change state to created, c.resume() should fail // Change state to created, c.resume() should fail
c.status.transition(Status::CREATED); c.status.transition(ContainerState::CREATED);
c.resume().map_err(|e| anyhow!(e)) c.resume().map_err(|e| anyhow!(e))
}); });
@ -1881,7 +1853,7 @@ mod tests {
#[test] #[test]
fn test_linuxcontainer_resume_cgroupmgr_is_none() { fn test_linuxcontainer_resume_cgroupmgr_is_none() {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| { let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
c.status.transition(Status::PAUSED); c.status.transition(ContainerState::PAUSED);
c.cgroup_manager = None; c.cgroup_manager = None;
c.resume().map_err(|e| anyhow!(e)) c.resume().map_err(|e| anyhow!(e))
}); });
@ -1894,7 +1866,7 @@ mod tests {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| { let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
c.cgroup_manager = FsManager::new("").ok(); c.cgroup_manager = FsManager::new("").ok();
// Change status to paused, this way we can resume it // Change status to paused, this way we can resume it
c.status.transition(Status::PAUSED); c.status.transition(ContainerState::PAUSED);
c.resume().map_err(|e| anyhow!(e)) c.resume().map_err(|e| anyhow!(e))
}); });