From 9162da8b042030009a6a5519a280b092e16a0dec Mon Sep 17 00:00:00 2001 From: George Pisaltu Date: Mon, 1 Nov 2021 11:43:16 +0200 Subject: [PATCH 1/3] refactor api_server for endpoint deprecation Signed-off-by: George Pisaltu --- src/api_server/src/lib.rs | 35 ++++++--- src/api_server/src/parsed_request.rs | 82 ++++++++++++++++++--- src/api_server/src/request/instance_info.rs | 5 +- src/api_server/src/request/mmds.rs | 12 +-- src/api_server/src/request/version.rs | 5 +- 5 files changed, 105 insertions(+), 34 deletions(-) diff --git a/src/api_server/src/lib.rs b/src/api_server/src/lib.rs index 5625a62a794..d94751a7df3 100644 --- a/src/api_server/src/lib.rs +++ b/src/api_server/src/lib.rs @@ -14,8 +14,10 @@ use std::path::PathBuf; use std::sync::{mpsc, Arc, Mutex}; use std::{fmt, io}; -use crate::parsed_request::ParsedRequest; -use logger::{debug, error, info, update_metric_with_elapsed_time, ProcessTimeReporter, METRICS}; +use crate::parsed_request::{ParsedRequest, RequestAction}; +use logger::{ + debug, error, info, update_metric_with_elapsed_time, warn, ProcessTimeReporter, METRICS, +}; pub use micro_http::{ Body, HttpServer, Method, Request, RequestError, Response, ServerError, ServerRequest, ServerResponse, StatusCode, Version, @@ -244,16 +246,25 @@ impl ApiServer { request: &Request, request_processing_start_us: u64, ) -> Response { - match ParsedRequest::try_from_request(request) { - Ok(ParsedRequest::Sync(vmm_action)) => { - self.serve_vmm_action_request(vmm_action, request_processing_start_us) - } - Ok(ParsedRequest::GetMMDS) => self.get_mmds(), - Ok(ParsedRequest::PatchMMDS(value)) => self.patch_mmds(value), - Ok(ParsedRequest::PutMMDS(value)) => self.put_mmds(value), - Ok(ParsedRequest::ShutdownInternal) => { - self.shutdown_flag = true; - Response::new(Version::Http11, StatusCode::NoContent) + match ParsedRequest::try_from_request(request).map(|r| r.into_parts()) { + Ok((req_action, mut parsing_info)) => { + let mut response = match req_action { + RequestAction::Sync(vmm_action) => { + self.serve_vmm_action_request(vmm_action, request_processing_start_us) + } + RequestAction::GetMMDS => self.get_mmds(), + RequestAction::PatchMMDS(value) => self.patch_mmds(value), + RequestAction::PutMMDS(value) => self.put_mmds(value), + RequestAction::ShutdownInternal => { + self.shutdown_flag = true; + Response::new(Version::Http11, StatusCode::NoContent) + } + }; + if let Some(message) = parsing_info.take_deprecation_message() { + warn!("{}", message); + response.set_deprecation(); + } + response } Err(e) => { error!("{}", e); diff --git a/src/api_server/src/parsed_request.rs b/src/api_server/src/parsed_request.rs index f0fff729681..ab16b308674 100644 --- a/src/api_server/src/parsed_request.rs +++ b/src/api_server/src/parsed_request.rs @@ -27,7 +27,7 @@ use micro_http::{Body, Method, Request, Response, StatusCode, Version}; use logger::{error, info}; use vmm::rpc_interface::{VmmAction, VmmActionError}; -pub(crate) enum ParsedRequest { +pub(crate) enum RequestAction { GetMMDS, PatchMMDS(Value), PutMMDS(Value), @@ -35,7 +35,46 @@ pub(crate) enum ParsedRequest { ShutdownInternal, // !!! not an API, used by shutdown to thread::join the API thread } +#[derive(Default)] +#[cfg_attr(test, derive(PartialEq))] +pub(crate) struct ParsingInfo { + deprecation_message: Option, +} + +impl ParsingInfo { + pub fn append_deprecation_message(&mut self, message: &str) { + match self.deprecation_message.as_mut() { + None => self.deprecation_message = Some(message.to_owned()), + Some(s) => (*s).push_str(message), + } + } + + pub fn take_deprecation_message(&mut self) -> Option { + self.deprecation_message.take() + } +} + +pub(crate) struct ParsedRequest { + action: RequestAction, + parsing_info: ParsingInfo, +} + impl ParsedRequest { + pub(crate) fn new(action: RequestAction) -> Self { + Self { + action, + parsing_info: Default::default(), + } + } + + pub(crate) fn into_parts(self) -> (RequestAction, ParsingInfo) { + (self.action, self.parsing_info) + } + + pub(crate) fn parsing_info(&mut self) -> &mut ParsingInfo { + &mut self.parsing_info + } + pub(crate) fn try_from_request(request: &Request) -> Result { let request_uri = request.uri().get_abs_path().to_string(); log_received_api_request(describe( @@ -78,7 +117,9 @@ impl ParsedRequest { (Method::Put, "network-interfaces", Some(body)) => { parse_put_net(body, path_tokens.get(1)) } - (Method::Put, "shutdown-internal", None) => Ok(ParsedRequest::ShutdownInternal), + (Method::Put, "shutdown-internal", None) => { + Ok(ParsedRequest::new(RequestAction::ShutdownInternal)) + } (Method::Put, "snapshot", Some(body)) => parse_put_snapshot(body, path_tokens.get(1)), (Method::Put, "vsock", Some(body)) => parse_put_vsock(body), (Method::Put, _, None) => method_to_error(Method::Put), @@ -145,7 +186,7 @@ impl ParsedRequest { /// Helper function to avoid boiler-plate code. pub(crate) fn new_sync(vmm_action: VmmAction) -> ParsedRequest { - ParsedRequest::Sync(Box::new(vmm_action)) + ParsedRequest::new(RequestAction::Sync(Box::new(vmm_action))) } } @@ -281,15 +322,19 @@ pub(crate) mod tests { impl PartialEq for ParsedRequest { fn eq(&self, other: &ParsedRequest) -> bool { - match (self, other) { - (&ParsedRequest::Sync(ref sync_req), &ParsedRequest::Sync(ref other_sync_req)) => { + if self.parsing_info.deprecation_message != other.parsing_info.deprecation_message { + return false; + } + + match (&self.action, &other.action) { + (RequestAction::Sync(ref sync_req), RequestAction::Sync(ref other_sync_req)) => { sync_req == other_sync_req } - (&ParsedRequest::GetMMDS, &ParsedRequest::GetMMDS) => true, - (&ParsedRequest::PutMMDS(ref val), &ParsedRequest::PutMMDS(ref other_val)) => { + (RequestAction::GetMMDS, RequestAction::GetMMDS) => true, + (RequestAction::PutMMDS(ref val), RequestAction::PutMMDS(ref other_val)) => { val == other_val } - (&ParsedRequest::PatchMMDS(ref val), &ParsedRequest::PatchMMDS(ref other_val)) => { + (RequestAction::PatchMMDS(ref val), RequestAction::PatchMMDS(ref other_val)) => { val == other_val } _ => false, @@ -298,8 +343,21 @@ pub(crate) mod tests { } pub(crate) fn vmm_action_from_request(req: ParsedRequest) -> VmmAction { - match req { - ParsedRequest::Sync(vmm_action) => *vmm_action, + match req.action { + RequestAction::Sync(vmm_action) => *vmm_action, + _ => panic!("Invalid request"), + } + } + + pub(crate) fn depr_action_from_req(req: ParsedRequest, msg: Option) -> VmmAction { + let (action_req, mut parsing_info) = req.into_parts(); + match action_req { + RequestAction::Sync(vmm_action) => { + let req_msg = parsing_info.take_deprecation_message(); + assert!(req_msg.is_some()); + assert_eq!(req_msg, msg); + *vmm_action + } _ => panic!("Invalid request"), } } @@ -853,8 +911,8 @@ pub(crate) mod tests { .unwrap(); assert!(connection.try_read().is_ok()); let req = connection.pop_parsed_request().unwrap(); - match ParsedRequest::try_from_request(&req).unwrap() { - ParsedRequest::ShutdownInternal => (), + match ParsedRequest::try_from_request(&req).unwrap().into_parts() { + (RequestAction::ShutdownInternal, _) => (), _ => panic!("wrong parsed request"), }; } diff --git a/src/api_server/src/request/instance_info.rs b/src/api_server/src/request/instance_info.rs index a2e65a2d6dc..978b11ce1a3 100644 --- a/src/api_server/src/request/instance_info.rs +++ b/src/api_server/src/request/instance_info.rs @@ -13,11 +13,12 @@ pub(crate) fn parse_get_instance_info() -> Result { #[cfg(test)] mod tests { use super::*; + use crate::RequestAction; #[test] fn test_parse_get_instance_info_request() { - match parse_get_instance_info() { - Ok(ParsedRequest::Sync(action)) if *action == VmmAction::GetVmInstanceInfo => {} + match parse_get_instance_info().unwrap().into_parts() { + (RequestAction::Sync(action), _) if *action == VmmAction::GetVmInstanceInfo => {} _ => panic!("Test failed."), } } diff --git a/src/api_server/src/request/mmds.rs b/src/api_server/src/request/mmds.rs index 76e6217cbeb..e2691ffe752 100644 --- a/src/api_server/src/request/mmds.rs +++ b/src/api_server/src/request/mmds.rs @@ -1,7 +1,7 @@ // Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::parsed_request::{Error, ParsedRequest}; +use crate::parsed_request::{Error, ParsedRequest, RequestAction}; use crate::request::Body; use logger::{IncMetric, METRICS}; use micro_http::StatusCode; @@ -9,7 +9,7 @@ use vmm::rpc_interface::VmmAction::SetMmdsConfiguration; pub(crate) fn parse_get_mmds() -> Result { METRICS.get_api_requests.mmds_count.inc(); - Ok(ParsedRequest::GetMMDS) + Ok(ParsedRequest::new(RequestAction::GetMMDS)) } pub(crate) fn parse_put_mmds( @@ -18,12 +18,12 @@ pub(crate) fn parse_put_mmds( ) -> Result { METRICS.put_api_requests.mmds_count.inc(); match path_second_token { - None => Ok(ParsedRequest::PutMMDS( + None => Ok(ParsedRequest::new(RequestAction::PutMMDS( serde_json::from_slice(body.raw()).map_err(|e| { METRICS.put_api_requests.mmds_fails.inc(); Error::SerdeJson(e) })?, - )), + ))), Some(&"config") => Ok(ParsedRequest::new_sync(SetMmdsConfiguration( serde_json::from_slice(body.raw()).map_err(|e| { METRICS.put_api_requests.mmds_fails.inc(); @@ -42,12 +42,12 @@ pub(crate) fn parse_put_mmds( pub(crate) fn parse_patch_mmds(body: &Body) -> Result { METRICS.patch_api_requests.mmds_count.inc(); - Ok(ParsedRequest::PatchMMDS( + Ok(ParsedRequest::new(RequestAction::PatchMMDS( serde_json::from_slice(body.raw()).map_err(|e| { METRICS.patch_api_requests.mmds_fails.inc(); Error::SerdeJson(e) })?, - )) + ))) } #[cfg(test)] diff --git a/src/api_server/src/request/version.rs b/src/api_server/src/request/version.rs index 7045c11782c..f28e3cf5ad5 100644 --- a/src/api_server/src/request/version.rs +++ b/src/api_server/src/request/version.rs @@ -13,11 +13,12 @@ pub(crate) fn parse_get_version() -> Result { #[cfg(test)] mod tests { use super::*; + use crate::RequestAction; #[test] fn test_parse_get_version_request() { - match parse_get_version() { - Ok(ParsedRequest::Sync(action)) if *action == VmmAction::GetVmmVersion => {} + match parse_get_version().unwrap().into_parts() { + (RequestAction::Sync(action), _) if *action == VmmAction::GetVmmVersion => {} _ => panic!("Test failed."), } } From ea5bdbbaaa3a93c88a41a17a4656fd50f73b7d97 Mon Sep 17 00:00:00 2001 From: George Pisaltu Date: Mon, 1 Nov 2021 14:48:55 +0200 Subject: [PATCH 2/3] make vsock_id field in PUT /vsock optional In this commit we deprecate the `vsock_id` field in PUTs on /vsock by making it optional. Signed-off-by: George Pisaltu --- CHANGELOG.md | 4 ++ docs/vsock.md | 3 +- src/api_server/src/request/vsock.rs | 49 ++++++++++++++++++++++--- src/api_server/swagger/firecracker.yaml | 2 +- src/devices/src/virtio/vsock/mod.rs | 1 + src/logger/src/metrics.rs | 4 ++ src/vmm/src/builder.rs | 3 +- src/vmm/src/device_manager/persist.rs | 2 +- src/vmm/src/resources.rs | 10 ++--- src/vmm/src/rpc_interface.rs | 10 ++--- src/vmm/src/vmm_config/vsock.rs | 11 ++++-- 11 files changed, 73 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3d0352c215..e8d899bda47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ - Added permanent HTTP endpoint for `GET` on `/version` for getting the Firecracker version. +### Changed + +- Deprecated `vsock_id` body field in `PUT`s on `/vsock`. + ### Fixed - Fixed incorrect propagation of init parameters in kernel commandline. diff --git a/docs/vsock.md b/docs/vsock.md index a0d8bf512e5..f5c478b27dd 100644 --- a/docs/vsock.md +++ b/docs/vsock.md @@ -88,7 +88,7 @@ images/vsock-connections.png?raw=true ## Setting up the virtio-vsock device -The virtio-vsock device will require an ID, a CID, and the path to a backing +The virtio-vsock device will require a CID, and the path to a backing AF_UNIX socket: ```bash @@ -97,7 +97,6 @@ curl --unix-socket /tmp/firecracker.socket -i \ -H 'Accept: application/json' \ -H 'Content-Type: application/json' \ -d '{ - "vsock_id": "1", "guest_cid": 3, "uds_path": "./v.sock" }' diff --git a/src/api_server/src/request/vsock.rs b/src/api_server/src/request/vsock.rs index 70d11f2ae07..bb8fe4ba257 100644 --- a/src/api_server/src/request/vsock.rs +++ b/src/api_server/src/request/vsock.rs @@ -4,32 +4,71 @@ use super::super::VmmAction; use crate::parsed_request::{Error, ParsedRequest}; use crate::request::Body; +use logger::{IncMetric, METRICS}; use vmm::vmm_config::vsock::VsockDeviceConfig; pub(crate) fn parse_put_vsock(body: &Body) -> Result { - Ok(ParsedRequest::new_sync(VmmAction::SetVsockDevice( - serde_json::from_slice::(body.raw()).map_err(Error::SerdeJson)?, - ))) + METRICS.put_api_requests.vsock_count.inc(); + let vsock_cfg = serde_json::from_slice::(body.raw()).map_err(|e| { + METRICS.put_api_requests.vsock_fails.inc(); + Error::SerdeJson(e) + })?; + + // Check for the presence of deprecated `vsock_id` field. + let mut deprecation_message = None; + if vsock_cfg.vsock_id.is_some() { + // vsock_id field in request is deprecated. + METRICS.deprecated_api.deprecated_http_api_calls.inc(); + deprecation_message = Some("PUT /vsock: vsock_id field is deprecated."); + } + + // Construct the `ParsedRequest` object. + let mut parsed_req = ParsedRequest::new_sync(VmmAction::SetVsockDevice(vsock_cfg)); + // If `vsock_id` was present, set the deprecation message in `parsing_info`. + if let Some(msg) = deprecation_message { + parsed_req.parsing_info().append_deprecation_message(msg); + } + + Ok(parsed_req) } #[cfg(test)] mod tests { use super::*; + use crate::parsed_request::tests::depr_action_from_req; #[test] fn test_parse_put_vsock_request() { let body = r#"{ - "vsock_id": "foo", "guest_cid": 42, "uds_path": "vsock.sock" }"#; assert!(parse_put_vsock(&Body::new(body)).is_ok()); let body = r#"{ - "vsock_id": "foo", "guest_cid": 42, "invalid_field": false }"#; assert!(parse_put_vsock(&Body::new(body)).is_err()); } + + #[test] + fn test_depr_vsock_id() { + let body = r#"{ + "vsock_id": "foo", + "guest_cid": 42, + "uds_path": "vsock.sock" + }"#; + depr_action_from_req( + parse_put_vsock(&Body::new(body)).unwrap(), + Some("PUT /vsock: vsock_id field is deprecated.".to_string()), + ); + + let body = r#"{ + "guest_cid": 42, + "uds_path": "vsock.sock" + }"#; + let (_, mut parsing_info) = parse_put_vsock(&Body::new(body)).unwrap().into_parts(); + assert!(!parsing_info.take_deprecation_message().is_some()); + } } diff --git a/src/api_server/swagger/firecracker.yaml b/src/api_server/swagger/firecracker.yaml index 27643f8c3a4..ab4e70f7295 100644 --- a/src/api_server/swagger/firecracker.yaml +++ b/src/api_server/swagger/firecracker.yaml @@ -1147,7 +1147,6 @@ definitions: required: - guest_cid - uds_path - - vsock_id properties: guest_cid: type: integer @@ -1158,3 +1157,4 @@ definitions: description: Path to UNIX domain socket, used to proxy vsock connections. vsock_id: type: string + description: This parameter has been deprecated since v1.0.0. diff --git a/src/devices/src/virtio/vsock/mod.rs b/src/devices/src/virtio/vsock/mod.rs index a810a49eb37..e5d370ca660 100644 --- a/src/devices/src/virtio/vsock/mod.rs +++ b/src/devices/src/virtio/vsock/mod.rs @@ -18,6 +18,7 @@ use std::os::unix::io::AsRawFd; use crate::virtio::persist::Error as VirtioStateError; pub use self::defs::uapi::VIRTIO_ID_VSOCK as TYPE_VSOCK; +pub use self::defs::VSOCK_DEV_ID; pub use self::device::Vsock; pub use self::unix::{Error as VsockUnixBackendError, VsockUnixBackend}; diff --git a/src/logger/src/metrics.rs b/src/logger/src/metrics.rs index 83342e11e82..5624112baf9 100644 --- a/src/logger/src/metrics.rs +++ b/src/logger/src/metrics.rs @@ -397,6 +397,10 @@ pub struct PutRequestsMetrics { pub mmds_count: SharedIncMetric, /// Number of failures in creating a new mmds. pub mmds_fails: SharedIncMetric, + /// Number of PUTs for creating a vsock device. + pub vsock_count: SharedIncMetric, + /// Number of failures in creating a vsock device. + pub vsock_fails: SharedIncMetric, } /// Metrics specific to PATCH API Requests for counting user triggered actions and/or failures. diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index baebb74bcc5..46306f589f2 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -979,6 +979,7 @@ pub mod tests { use crate::vmm_config::vsock::tests::default_config; use crate::vmm_config::vsock::{VsockBuilder, VsockDeviceConfig}; use arch::DeviceType; + use devices::virtio::vsock::VSOCK_DEV_ID; use devices::virtio::{TYPE_BALLOON, TYPE_BLOCK, TYPE_VSOCK}; use linux_loader::cmdline::Cmdline; use utils::tempfile::TempFile; @@ -1127,7 +1128,7 @@ pub mod tests { event_manager: &mut EventManager, vsock_config: VsockDeviceConfig, ) { - let vsock_dev_id = vsock_config.vsock_id.clone(); + let vsock_dev_id = VSOCK_DEV_ID.to_owned(); let vsock = VsockBuilder::create_unixsock_vsock(vsock_config).unwrap(); let vsock = Arc::new(Mutex::new(vsock)); diff --git a/src/vmm/src/device_manager/persist.rs b/src/vmm/src/device_manager/persist.rs index aaa6ee7e5b3..2ae1d4fabd2 100644 --- a/src/vmm/src/device_manager/persist.rs +++ b/src/vmm/src/device_manager/persist.rs @@ -584,7 +584,7 @@ mod tests { // Add a vsock device. let vsock_dev_id = "vsock"; let vsock_config = VsockDeviceConfig { - vsock_id: vsock_dev_id.to_string(), + vsock_id: Some(vsock_dev_id.to_string()), guest_cid: 3, uds_path: tmp_sock_file.as_path().to_str().unwrap().to_string(), }; diff --git a/src/vmm/src/resources.rs b/src/vmm/src/resources.rs index 260ed83db9e..cd0ac95d9d1 100644 --- a/src/vmm/src/resources.rs +++ b/src/vmm/src/resources.rs @@ -360,6 +360,7 @@ mod tests { use crate::vmm_config::vsock::tests::default_config; use crate::vmm_config::RateLimiterConfig; use crate::vstate::vcpu::VcpuConfig; + use devices::virtio::vsock::VSOCK_DEV_ID; use logger::{LevelFilter, LOGGER}; use utils::net::mac::MacAddr; use utils::tempfile::TempFile; @@ -961,14 +962,9 @@ mod tests { tmp_sock_file.remove().unwrap(); let new_vsock_cfg = default_config(&tmp_sock_file); assert!(vm_resources.vsock.get().is_none()); - vm_resources - .set_vsock_device(new_vsock_cfg.clone()) - .unwrap(); + vm_resources.set_vsock_device(new_vsock_cfg).unwrap(); let actual_vsock_cfg = vm_resources.vsock.get().unwrap(); - assert_eq!( - actual_vsock_cfg.lock().unwrap().id(), - &new_vsock_cfg.vsock_id - ); + assert_eq!(actual_vsock_cfg.lock().unwrap().id(), VSOCK_DEV_ID); } #[test] diff --git a/src/vmm/src/rpc_interface.rs b/src/vmm/src/rpc_interface.rs index 4d5bd2efbd5..a9b3cde8598 100644 --- a/src/vmm/src/rpc_interface.rs +++ b/src/vmm/src/rpc_interface.rs @@ -1178,7 +1178,7 @@ mod tests { #[test] fn test_preboot_set_vsock_dev() { let req = VmmAction::SetVsockDevice(VsockDeviceConfig { - vsock_id: String::new(), + vsock_id: Some(String::new()), guest_cid: 0, uds_path: String::new(), }); @@ -1188,7 +1188,7 @@ mod tests { }); let req = VmmAction::SetVsockDevice(VsockDeviceConfig { - vsock_id: String::new(), + vsock_id: Some(String::new()), guest_cid: 0, uds_path: String::new(), }); @@ -1584,7 +1584,7 @@ mod tests { ); check_runtime_request_err( VmmAction::SetVsockDevice(VsockDeviceConfig { - vsock_id: String::new(), + vsock_id: Some(String::new()), guest_cid: 0, uds_path: String::new(), }), @@ -1596,7 +1596,7 @@ mod tests { ); check_runtime_request_err( VmmAction::SetVsockDevice(VsockDeviceConfig { - vsock_id: String::new(), + vsock_id: Some(String::new()), guest_cid: 0, uds_path: String::new(), }), @@ -1676,7 +1676,7 @@ mod tests { verify_load_snap_disallowed_after_boot_resources(req, "SetBalloonDevice"); let req = VmmAction::SetVsockDevice(VsockDeviceConfig { - vsock_id: String::new(), + vsock_id: Some(String::new()), guest_cid: 0, uds_path: String::new(), }); diff --git a/src/vmm/src/vmm_config/vsock.rs b/src/vmm/src/vmm_config/vsock.rs index 45f65058349..2608dff29b3 100644 --- a/src/vmm/src/vmm_config/vsock.rs +++ b/src/vmm/src/vmm_config/vsock.rs @@ -39,8 +39,10 @@ type Result = std::result::Result; #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] #[serde(deny_unknown_fields)] pub struct VsockDeviceConfig { + #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] /// ID of the vsock device. - pub vsock_id: String, + pub vsock_id: Option, /// A 32-bit Context Identifier (CID) used to identify the guest. pub guest_cid: u32, /// Path to local unix socket. @@ -56,7 +58,7 @@ impl From<&VsockAndUnixPath> for VsockDeviceConfig { fn from(vsock: &VsockAndUnixPath) -> Self { let vsock_lock = vsock.vsock.lock().unwrap(); VsockDeviceConfig { - vsock_id: vsock_lock.id().to_string(), + vsock_id: None, guest_cid: u32::try_from(vsock_lock.cid()).unwrap(), uds_path: vsock.uds_path.clone(), } @@ -113,11 +115,12 @@ impl VsockBuilder { #[cfg(test)] pub(crate) mod tests { use super::*; + use devices::virtio::vsock::VSOCK_DEV_ID; use utils::tempfile::TempFile; pub(crate) fn default_config(tmp_sock_file: &TempFile) -> VsockDeviceConfig { VsockDeviceConfig { - vsock_id: "vsock".to_string(), + vsock_id: None, guest_cid: 3, uds_path: tmp_sock_file.as_path().to_str().unwrap().to_string(), } @@ -140,7 +143,7 @@ pub(crate) mod tests { store.insert(vsock_config.clone()).unwrap(); let vsock = store.get().unwrap(); - assert_eq!(vsock.lock().unwrap().id(), &vsock_config.vsock_id); + assert_eq!(vsock.lock().unwrap().id(), VSOCK_DEV_ID); let new_cid = vsock_config.guest_cid + 1; vsock_config.guest_cid = new_cid; From 859885d0422c75ed6e9dc4cdfa7e015f4fa7bc58 Mon Sep 17 00:00:00 2001 From: George Pisaltu Date: Mon, 1 Nov 2021 14:47:57 +0200 Subject: [PATCH 3/3] add deprecation case for vsock_id in integ tests Signed-off-by: George Pisaltu --- tests/framework/resources.py | 7 +- .../integration_tests/functional/test_api.py | 83 +++++++++++++------ 2 files changed, 61 insertions(+), 29 deletions(-) diff --git a/tests/framework/resources.py b/tests/framework/resources.py index 9666093c363..a9092671f7e 100644 --- a/tests/framework/resources.py +++ b/tests/framework/resources.py @@ -752,14 +752,15 @@ def patch(self, **args): @staticmethod def create_json( - vsock_id, guest_cid, - uds_path): + uds_path, + vsock_id=None): """Create the json for the vsock specific API request.""" datax = { - 'vsock_id': vsock_id, 'guest_cid': guest_cid, 'uds_path': uds_path } + if vsock_id: + datax['vsock_id'] = vsock_id return datax diff --git a/tests/integration_tests/functional/test_api.py b/tests/integration_tests/functional/test_api.py index a4fc2625c55..41ca1fe05e3 100644 --- a/tests/integration_tests/functional/test_api.py +++ b/tests/integration_tests/functional/test_api.py @@ -18,6 +18,10 @@ import host_tools.drive as drive_tools import host_tools.network as net_tools +from conftest import _test_images_s3_bucket +from framework.artifacts import ArtifactCollection +from framework.builder import MicrovmBuilder + MEM_LIMIT = 1000000000 @@ -940,52 +944,81 @@ def test_api_version(test_microvm_with_api): assert out.strip()[1:] == preboot_response.json()['firecracker_version'] -def test_api_vsock(test_microvm_with_api): +def test_api_vsock(bin_cloner_path): """ Test vsock related API commands. @type: functional """ - test_microvm = test_microvm_with_api - test_microvm.spawn() - test_microvm.basic_config() + builder = MicrovmBuilder(bin_cloner_path) + artifacts = ArtifactCollection(_test_images_s3_bucket()) - response = test_microvm.vsock.put( - vsock_id='vsock1', + # Test with the current build. + vm_instance = builder.build_vm_nano() + _test_vsock(vm_instance.vm) + + # Fetch 1.0.0 and older firecracker binaries. + # Create a vsock device with each FC binary + # artifact. + firecracker_artifacts = artifacts.firecrackers( + # v1.0.0 deprecated `vsock_id`. + min_version="1.0.0") + + for firecracker in firecracker_artifacts: + firecracker.download() + jailer = firecracker.jailer() + jailer.download() + + vm_instance = builder.build_vm_nano( + fc_binary=firecracker.local_path(), + jailer_binary=jailer.local_path()) + + _test_vsock(vm_instance.vm) + + +def _test_vsock(vm): + # Create a vsock device. + response = vm.vsock.put( guest_cid=15, uds_path='vsock.sock' ) - assert test_microvm.api_session.is_status_no_content(response.status_code) + assert vm.api_session.is_status_no_content(response.status_code) # Updating an existing vsock is currently fine. - response = test_microvm.vsock.put( - vsock_id='vsock1', + response = vm.vsock.put( guest_cid=166, uds_path='vsock.sock' ) - assert test_microvm.api_session.is_status_no_content(response.status_code) + assert vm.api_session.is_status_no_content(response.status_code) - # No other vsock action is allowed after booting the VM. - test_microvm.start() - - # Updating an existing vsock should not be fine at this point. - response = test_microvm.vsock.put( + # Check PUT request. Although vsock_id is deprecated, it must still work. + response = vm.vsock.put( vsock_id='vsock1', - guest_cid=17, + guest_cid=15, uds_path='vsock.sock' ) - assert test_microvm.api_session.is_status_bad_request(response.status_code) + assert vm.api_session.is_status_no_content(response.status_code) + assert response.headers['deprecation'] - # Attaching a new vsock device should not be fine at this point. - response = test_microvm.vsock.put( - vsock_id='vsock3', - guest_cid=18, + # Updating an existing vsock is currently fine even with deprecated + # `vsock_id`. + response = vm.vsock.put( + vsock_id='vsock1', + guest_cid=166, uds_path='vsock.sock' ) - assert test_microvm.api_session.is_status_bad_request(response.status_code) + assert vm.api_session.is_status_no_content(response.status_code) + assert response.headers['deprecation'] - response = test_microvm.vm.patch(state='Paused') - assert test_microvm.api_session.is_status_no_content(response.status_code) + # No other vsock action is allowed after booting the VM. + vm.start() + + # Updating an existing vsock should not be fine at this point. + response = vm.vsock.put( + guest_cid=17, + uds_path='vsock.sock' + ) + assert vm.api_session.is_status_bad_request(response.status_code) def test_api_balloon(test_microvm_with_api): @@ -1127,13 +1160,11 @@ def test_get_full_config(test_microvm_with_api): # Add a vsock device. response = test_microvm.vsock.put( - vsock_id='vsock', guest_cid=15, uds_path='vsock.sock' ) assert test_microvm.api_session.is_status_no_content(response.status_code) expected_cfg['vsock'] = { - 'vsock_id': 'vsock', 'guest_cid': 15, 'uds_path': 'vsock.sock' }