From 2e1657cf1249054053f033397ed8e99310040298 Mon Sep 17 00:00:00 2001 From: Moritz Sanft <58110325+msanft@users.noreply.github.com> Date: Fri, 7 Feb 2025 13:14:08 +0100 Subject: [PATCH] packages/kata-runtime: alow arbitrary CDI annotations As discussed via Teams, there is no sense in checking CDI annotations if the agent doesn't care about them anyway. This allows arbitrary CDI annotations in the policy. --- .../0018-agent-remove-CDI-support.patch | 239 ------------------ ...npolicy-support-dynamic-annotations.patch} | 0 ...clear-log-pipes-if-denied-by-policy.patch} | 8 +- .../0020-runtime-remove-CDI-annotations.patch | 40 +++ .../by-name/kata/kata-runtime/package.nix | 16 +- 5 files changed, 52 insertions(+), 251 deletions(-) delete mode 100644 packages/by-name/kata/kata-runtime/0018-agent-remove-CDI-support.patch rename packages/by-name/kata/kata-runtime/{0019-genpolicy-support-dynamic-annotations.patch => 0018-genpolicy-support-dynamic-annotations.patch} (100%) rename packages/by-name/kata/kata-runtime/{0020-agent-clear-log-pipes-if-denied-by-policy.patch => 0019-agent-clear-log-pipes-if-denied-by-policy.patch} (90%) create mode 100644 packages/by-name/kata/kata-runtime/0020-runtime-remove-CDI-annotations.patch diff --git a/packages/by-name/kata/kata-runtime/0018-agent-remove-CDI-support.patch b/packages/by-name/kata/kata-runtime/0018-agent-remove-CDI-support.patch deleted file mode 100644 index 146409ba7..000000000 --- a/packages/by-name/kata/kata-runtime/0018-agent-remove-CDI-support.patch +++ /dev/null @@ -1,239 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Moritz Sanft <58110325+msanft@users.noreply.github.com> -Date: Fri, 3 Jan 2025 13:29:05 +0100 -Subject: [PATCH] agent: remove CDI support - -This reverts commits aa2e1a57bdd4b9e0d6c0810c05229d77278066c6 -and 3995fe71f9b7b4304dc9cdfd842d296a633c15c3 while keeping the Cargo -lockfiles intact to allow for easier rebasing of this patch. ---- - src/agent/src/device/mod.rs | 158 ------------------------------------ - src/agent/src/rpc.rs | 13 +-- - 2 files changed, 1 insertion(+), 170 deletions(-) - -diff --git a/src/agent/src/device/mod.rs b/src/agent/src/device/mod.rs -index 400b6f1386e1b4a1a4cda1e3e3da2f66640165c7..53e77d82c88912488ead9052f44e397345f8b8ad 100644 ---- a/src/agent/src/device/mod.rs -+++ b/src/agent/src/device/mod.rs -@@ -11,9 +11,6 @@ use self::vfio_device_handler::{VfioApDeviceHandler, VfioPciDeviceHandler}; - use crate::pci; - use crate::sandbox::Sandbox; - use anyhow::{anyhow, Context, Result}; --use cdi::annotations::parse_annotations; --use cdi::cache::{new_cache, with_auto_refresh, CdiOption}; --use cdi::spec_dirs::with_spec_dirs; - use kata_types::device::DeviceHandlerManager; - use nix::sys::stat; - use oci::{LinuxDeviceCgroup, Spec}; -@@ -28,8 +25,6 @@ use std::path::PathBuf; - use std::str::FromStr; - use std::sync::Arc; - use tokio::sync::Mutex; --use tokio::time; --use tokio::time::Duration; - use tracing::instrument; - - pub mod block_device_handler; -@@ -243,69 +238,6 @@ pub async fn add_devices( - update_spec_devices(logger, spec, dev_updates) - } - --#[instrument] --pub async fn handle_cdi_devices( -- logger: &Logger, -- spec: &mut Spec, -- spec_dir: &str, -- cdi_timeout: u64, --) -> Result<()> { -- if let Some(container_type) = spec -- .annotations() -- .as_ref() -- .and_then(|a| a.get("io.katacontainers.pkg.oci.container_type")) -- { -- if container_type == "pod_sandbox" { -- return Ok(()); -- } -- } -- -- let (_, devices) = parse_annotations(spec.annotations().as_ref().unwrap())?; -- -- if devices.is_empty() { -- info!(logger, "no CDI annotations, no devices to inject"); -- return Ok(()); -- } -- // Explicitly set the cache options to disable auto-refresh and -- // to use the single spec dir "/var/run/cdi" for tests it can be overridden -- let options: Vec = vec![with_auto_refresh(false), with_spec_dirs(&[spec_dir])]; -- let cache: Arc> = new_cache(options); -- -- for _ in 0..=cdi_timeout { -- let inject_result = { -- // Lock cache within this scope, std::sync::Mutex has no Send -- // and await will not work with time::sleep -- let mut cache = cache.lock().unwrap(); -- match cache.refresh() { -- Ok(_) => {} -- Err(e) => { -- return Err(anyhow!("error refreshing cache: {:?}", e)); -- } -- } -- cache.inject_devices(Some(spec), devices.clone()) -- }; -- -- match inject_result { -- Ok(_) => { -- info!( -- logger, -- "all devices injected successfully, modified CDI container spec: {:?}", &spec -- ); -- return Ok(()); -- } -- Err(e) => { -- info!(logger, "error injecting devices: {:?}", e); -- println!("error injecting devices: {:?}", e); -- } -- } -- time::sleep(Duration::from_millis(1000)).await; -- } -- Err(anyhow!( -- "failed to inject devices after CDI timeout of {} seconds", -- cdi_timeout -- )) --} -- - #[instrument] - async fn validate_device( - logger: &Logger, -@@ -1178,94 +1110,4 @@ mod tests { - assert!(name.is_ok(), "{}", name.unwrap_err()); - assert_eq!(name.unwrap(), devname); - } -- -- #[tokio::test] -- async fn test_handle_cdi_devices() { -- let logger = slog::Logger::root(slog::Discard, o!()); -- let mut spec = Spec::default(); -- -- let mut annotations = HashMap::new(); -- // cdi.k8s.io/vendor1_devices: vendor1.com/device=foo -- annotations.insert( -- "cdi.k8s.io/vfio17".to_string(), -- "kata.com/gpu=0".to_string(), -- ); -- spec.set_annotations(Some(annotations)); -- -- let temp_dir = tempdir().expect("Failed to create temporary directory"); -- let cdi_file = temp_dir.path().join("kata.json"); -- -- let cdi_version = "0.6.0"; -- let kind = "kata.com/gpu"; -- let device_name = "0"; -- let annotation_whatever = "false"; -- let annotation_whenever = "true"; -- let inner_env = "TEST_INNER_ENV=TEST_INNER_ENV_VALUE"; -- let outer_env = "TEST_OUTER_ENV=TEST_OUTER_ENV_VALUE"; -- let inner_device = "/dev/zero"; -- let outer_device = "/dev/null"; -- -- let cdi_content = format!( -- r#"{{ -- "cdiVersion": "{cdi_version}", -- "kind": "{kind}", -- "devices": [ -- {{ -- "name": "{device_name}", -- "annotations": {{ -- "whatever": "{annotation_whatever}", -- "whenever": "{annotation_whenever}" -- }}, -- "containerEdits": {{ -- "env": [ -- "{inner_env}" -- ], -- "deviceNodes": [ -- {{ -- "path": "{inner_device}" -- }} -- ] -- }} -- }} -- ], -- "containerEdits": {{ -- "env": [ -- "{outer_env}" -- ], -- "deviceNodes": [ -- {{ -- "path": "{outer_device}" -- }} -- ] -- }} -- }}"# -- ); -- -- fs::write(&cdi_file, cdi_content).expect("Failed to write CDI file"); -- -- let res = -- handle_cdi_devices(&logger, &mut spec, temp_dir.path().to_str().unwrap(), 0).await; -- println!("modfied spec {:?}", spec); -- assert!(res.is_ok(), "{}", res.err().unwrap()); -- -- let linux = spec.linux().as_ref().unwrap(); -- let devices = linux -- .resources() -- .as_ref() -- .unwrap() -- .devices() -- .as_ref() -- .unwrap(); -- assert_eq!(devices.len(), 2); -- -- let env = spec.process().as_ref().unwrap().env().as_ref().unwrap(); -- -- // find string TEST_OUTER_ENV in env -- let outer_env = env.iter().find(|e| e.starts_with("TEST_OUTER_ENV")); -- assert!(outer_env.is_some(), "TEST_OUTER_ENV not found in env"); -- -- // find TEST_INNER_ENV in env -- let inner_env = env.iter().find(|e| e.starts_with("TEST_INNER_ENV")); -- assert!(inner_env.is_some(), "TEST_INNER_ENV not found in env"); -- } - } -diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs -index 5f2a3eb955ea427478c842ba80ad2a17299b182f..fd824e9ec26728bf8088939aac7a1edb6d886aac 100644 ---- a/src/agent/src/rpc.rs -+++ b/src/agent/src/rpc.rs -@@ -58,7 +58,7 @@ use rustjail::process::ProcessOperations; - use crate::cdh; - use crate::device::block_device_handler::get_virtio_blk_pci_device_name; - use crate::device::network_device_handler::wait_for_net_interface; --use crate::device::{add_devices, handle_cdi_devices, update_env_pci}; -+use crate::device::{add_devices, update_env_pci}; - use crate::features::get_build_features; - use crate::image::KATA_IMAGE_WORK_DIR; - use crate::linux_abi::*; -@@ -130,8 +130,6 @@ const ERR_NO_SANDBOX_PIDNS: &str = "Sandbox does not have sandbox_pidns"; - // not available. - const IPTABLES_RESTORE_WAIT_SEC: u64 = 5; - --const CDI_TIMEOUT_LIMIT: u64 = 100; -- - // Convenience function to obtain the scope logger. - fn sl() -> slog::Logger { - slog_scope::logger() -@@ -227,15 +225,6 @@ impl AgentService { - // cannot predict everything from the caller. - add_devices(&sl(), &req.devices, &mut oci, &self.sandbox).await?; - -- // In guest-kernel mode some devices need extra handling. Taking the -- // GPU as an example the shim will inject CDI annotations that will -- // be used by the kata-agent to do containerEdits according to the -- // CDI spec coming from a registry that is created on the fly by UDEV -- // or other entities for a specifc device. -- // In Kata we only consider the directory "/var/run/cdi", "/etc" may be -- // readonly -- handle_cdi_devices(&sl(), &mut oci, "/var/run/cdi", CDI_TIMEOUT_LIMIT).await?; -- - cdh_handler(&mut oci).await?; - - // Both rootfs and volumes (invoked with --volume for instance) will diff --git a/packages/by-name/kata/kata-runtime/0019-genpolicy-support-dynamic-annotations.patch b/packages/by-name/kata/kata-runtime/0018-genpolicy-support-dynamic-annotations.patch similarity index 100% rename from packages/by-name/kata/kata-runtime/0019-genpolicy-support-dynamic-annotations.patch rename to packages/by-name/kata/kata-runtime/0018-genpolicy-support-dynamic-annotations.patch diff --git a/packages/by-name/kata/kata-runtime/0020-agent-clear-log-pipes-if-denied-by-policy.patch b/packages/by-name/kata/kata-runtime/0019-agent-clear-log-pipes-if-denied-by-policy.patch similarity index 90% rename from packages/by-name/kata/kata-runtime/0020-agent-clear-log-pipes-if-denied-by-policy.patch rename to packages/by-name/kata/kata-runtime/0019-agent-clear-log-pipes-if-denied-by-policy.patch index 0258be81b..a4fe1c7ff 100644 --- a/packages/by-name/kata/kata-runtime/0020-agent-clear-log-pipes-if-denied-by-policy.patch +++ b/packages/by-name/kata/kata-runtime/0019-agent-clear-log-pipes-if-denied-by-policy.patch @@ -20,10 +20,10 @@ Fixes: #10680 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs -index fd824e9ec26728bf8088939aac7a1edb6d886aac..cb5dac7a4a941e11fb9a086ff01633672364902a 100644 +index 5f2a3eb955ea427478c842ba80ad2a17299b182f..06fbcca57e8f5d8e729a379809950ce4f87359e4 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs -@@ -638,11 +638,11 @@ impl AgentService { +@@ -649,11 +649,11 @@ impl AgentService { async fn do_read_stream( &self, @@ -38,7 +38,7 @@ index fd824e9ec26728bf8088939aac7a1edb6d886aac..cb5dac7a4a941e11fb9a086ff0163367 let term_exit_notifier; let reader = { -@@ -889,8 +889,12 @@ impl agent_ttrpc::AgentService for AgentService { +@@ -900,8 +900,12 @@ impl agent_ttrpc::AgentService for AgentService { _ctx: &TtrpcContext, req: protocols::agent::ReadStreamRequest, ) -> ttrpc::Result { @@ -53,7 +53,7 @@ index fd824e9ec26728bf8088939aac7a1edb6d886aac..cb5dac7a4a941e11fb9a086ff0163367 } async fn read_stderr( -@@ -898,8 +902,12 @@ impl agent_ttrpc::AgentService for AgentService { +@@ -909,8 +913,12 @@ impl agent_ttrpc::AgentService for AgentService { _ctx: &TtrpcContext, req: protocols::agent::ReadStreamRequest, ) -> ttrpc::Result { diff --git a/packages/by-name/kata/kata-runtime/0020-runtime-remove-CDI-annotations.patch b/packages/by-name/kata/kata-runtime/0020-runtime-remove-CDI-annotations.patch new file mode 100644 index 000000000..cc52e96ea --- /dev/null +++ b/packages/by-name/kata/kata-runtime/0020-runtime-remove-CDI-annotations.patch @@ -0,0 +1,40 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Moritz Sanft <58110325+msanft@users.noreply.github.com> +Date: Fri, 7 Feb 2025 13:12:28 +0100 +Subject: [PATCH] runtime: remove CDI annotations + +We want to remove CDI annotations before they get to the agent, as they should only influence VM creation. Passing them to the agent is likely to create problems in policy checking, as they are often dynamically injected. +--- + src/runtime/virtcontainers/kata_agent.go | 12 ++++++++++++ + 1 file changed, 12 insertions(+) + +diff --git a/src/runtime/virtcontainers/kata_agent.go b/src/runtime/virtcontainers/kata_agent.go +index 9a794392b927fc8fa231a72ce35bc3fcb2773d85..8e6385e274b16f5ab5be0a90a2229b9cf9f1f83e 100644 +--- a/src/runtime/virtcontainers/kata_agent.go ++++ b/src/runtime/virtcontainers/kata_agent.go +@@ -14,6 +14,7 @@ import ( + "os" + "path" + "path/filepath" ++ "regexp" + "strconv" + "strings" + "sync" +@@ -1080,6 +1081,17 @@ func (k *kataAgent) constrainGRPCSpec(grpcSpec *grpc.Spec, passSeccomp bool, dis + grpcSpec.Linux.Devices = linuxDevices + } + ++ cdiRegexp, err := regexp.Compile(`^cdi\.k8s\.io\/.*$`) ++ if err != nil { ++ k.Logger().WithError(err).Error("compile CDI annotation regexp") ++ } ++ ++ for key := range grpcSpec.Annotations { ++ if cdiRegexp.MatchString(key) { ++ delete(grpcSpec.Annotations, key) ++ } ++ } ++ + return nil + } + diff --git a/packages/by-name/kata/kata-runtime/package.nix b/packages/by-name/kata/kata-runtime/package.nix index 9a5018662..183885c03 100644 --- a/packages/by-name/kata/kata-runtime/package.nix +++ b/packages/by-name/kata/kata-runtime/package.nix @@ -114,25 +114,25 @@ buildGoModule rec { # Upstream issue: https://github.com/kata-containers/kata-containers/issues/10633 ./0017-genpolicy-support-guest-hooks.patch - # Revert CDI support in kata-agent, which breaks legacy mode GPU facilitation which - # we currently use. - # TODO(msanft): Get native CDI working, which will allow us to drop this patch / undo the revert. - # See https://dev.azure.com/Edgeless/Edgeless/_workitems/edit/5061 - ./0018-agent-remove-CDI-support.patch - # This adds support for annotations with dynamic keys *and* values to Genpolicy. # This is required for e.g. GPU containers, which get annotated by an in-cluster # component (i.e. after policy generation based on the Pod spec) with an annotation # like `cdi.k8s.io/vfioXY`, where `XY` corresponds to a dynamic ID. # Upstream issue: https://github.com/kata-containers/kata-containers/issues/10745 - ./0019-genpolicy-support-dynamic-annotations.patch + ./0018-genpolicy-support-dynamic-annotations.patch # This allows denying ReadStream requests without blocking the container on its # stdout/stderr, by redacting the streams instead of blocking them. # Upstream: # * https://github.com/kata-containers/kata-containers/issues/10680 # * https://github.com/kata-containers/kata-containers/pull/10818 - ./0020-agent-clear-log-pipes-if-denied-by-policy.patch + ./0019-agent-clear-log-pipes-if-denied-by-policy.patch + + # This removes CDI annotations from the OCI spec before it is passed to the agent, + # which helps with policy handling of the (oftentimes dynamic) CDI annotations. + # TODO(msanft): Get native CDI working, which will allow us to drop this patch / undo the revert. + # See https://dev.azure.com/Edgeless/Edgeless/_workitems/edit/5061 + ./0020-runtime-remove-CDI-annotations.patch ]; };