From fc70559783a665f98d57654b73235a9484d78bc6 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Nov 2021 15:08:53 -0500 Subject: [PATCH 1/8] WIP Nexus download endpoint --- Cargo.lock | 20 ++++++ nexus/Cargo.toml | 1 + nexus/src/internal_api/http_entrypoints.rs | 82 ++++++++++++++++++++++ nexus/src/nexus.rs | 39 ++++++++++ sled-agent/src/bin/sled-agent.rs | 2 + sled-agent/src/http_entrypoints.rs | 41 +++++++++-- sled-agent/src/sled_agent.rs | 13 +++- 7 files changed, 189 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 39217b4ef79..1d096f5fef2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1381,6 +1381,16 @@ version = "0.3.16" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2a60c7ce501c71e03a9c9c0d35b861413ae925bd979cc7a4e30d060069aaac8d" +[[package]] +name = "mime_guess" +version = "2.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2684d4c2e97d99848d30b324b00c8fcc7e5c897b7cbb5819b09e7c90e8baf212" +dependencies = [ + "mime", + "unicase", +] + [[package]] name = "mio" version = "0.7.14" @@ -1602,6 +1612,7 @@ dependencies = [ "lazy_static", "libc", "macaddr", + "mime_guess", "newtype_derive", "omicron-common", "omicron-rpaths", @@ -3714,6 +3725,15 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "56dee185309b50d1f11bfedef0fe6d036842e3fb77413abef29f8f8d1c5d4c1c" +[[package]] +name = "unicase" +version = "2.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50f37be617794602aabbeee0be4f259dc1778fabe05e2d67ee8f79326d5cb4f6" +dependencies = [ + "version_check", +] + [[package]] name = "unicode-bidi" version = "0.3.7" diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index 1eb94a8cf93..6a2cfa8de89 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -23,6 +23,7 @@ ipnetwork = "0.18" lazy_static = "1.4.0" libc = "0.2.107" macaddr = { version = "1.0.1", features = [ "serde_std" ]} +mime_guess = "2.0" newtype_derive = "0.1.6" oso = "0.23" parse-display = "0.5.3" diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index 30def44effb..0abbba5d6dc 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -1,3 +1,4 @@ +use crate::nexus::BASE_ARTIFACT_DIR; /** * Handler functions (entrypoints) for HTTP APIs internal to the control plane */ @@ -12,6 +13,8 @@ use dropshot::HttpResponseUpdatedNoContent; use dropshot::Path; use dropshot::RequestContext; use dropshot::TypedBody; +use http::{Response, StatusCode}; +use hyper::Body; use omicron_common::api::internal::nexus::DiskRuntimeState; use omicron_common::api::internal::nexus::InstanceRuntimeState; use omicron_common::api::internal::nexus::ProducerEndpoint; @@ -19,6 +22,7 @@ use oximeter::types::ProducerResults; use oximeter_producer::{collect, ProducerIdPathParams}; use schemars::JsonSchema; use serde::Deserialize; +use std::path::PathBuf; use std::sync::Arc; use uuid::Uuid; @@ -35,6 +39,7 @@ pub fn internal_api() -> NexusApiDescription { api.register(cpapi_producers_post)?; api.register(cpapi_collectors_post)?; api.register(cpapi_metrics_collect)?; + api.register(cpapi_artifact_download)?; Ok(()) } @@ -207,3 +212,80 @@ async fn cpapi_metrics_collect( .instrument_dropshot_handler(&request_context, handler) .await } + +/// Deserialized input path. +#[derive(Deserialize, JsonSchema)] +struct AllPath { + path: Vec, +} + +/// Endpoint used by Sled Agents to download cached artifacts. +#[endpoint { + method = GET, + path = "/artifacts/{path:.*}", + // TODO: Buggy without this + unpublished = true, +}] +async fn cpapi_artifact_download( + request_context: Arc>>, + path: Path, +) -> Result, HttpError> { + let context = request_context.context(); + let nexus = &context.nexus; + let mut entry = PathBuf::from(BASE_ARTIFACT_DIR); + let path = path.into_inner().path; + + let handler = async { + for component in &path { + // Dropshot should not provide "." and ".." components. + assert_ne!(component, "."); + assert_ne!(component, ".."); + entry.push(component); + + // We explicitly prohibit consumers from following symlinks to prevent + // showing data outside of the intended directory. + let m = entry.symlink_metadata().map_err(|_| { + HttpError::for_bad_request( + None, + "Cannot query for symlink info".to_string(), + ) + })?; + if m.file_type().is_symlink() { + return Err(HttpError::for_bad_request( + None, + "Cannot traverse symlinks".to_string(), + )); + } + } + + if entry.is_dir() { + return Err(HttpError::for_bad_request( + None, + "Directory download not supported".to_string(), + )); + } + + let entry = entry.canonicalize().map_err(|e| { + HttpError::for_bad_request( + None, + format!("Cannot canonicalize path: {}", e), + ) + })?; + + let body = nexus.download_artifact(&entry).await?; + + // Derive the MIME type from the file name + let content_type = mime_guess::from_path(&entry) + .first() + .map_or_else(|| "text/plain".to_string(), |m| m.to_string()); + + Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, content_type) + .body(body.into())?) + }; + context + .internal_latencies + .instrument_dropshot_handler(&request_context, handler) + .await +} diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 7a33af71651..447f5b9a573 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -64,6 +64,7 @@ use rand::{rngs::StdRng, RngCore, SeedableRng}; use slog::Logger; use std::convert::TryInto; use std::net::SocketAddr; +use std::path::Path; use std::sync::Arc; use std::time::Duration; use steno::SagaId; @@ -104,6 +105,8 @@ pub trait TestInterfaces { ) -> CreateResult; } +pub static BASE_ARTIFACT_DIR: &str = "/var/tmp/oxide_artifacts"; + /** * Manages an Oxide fleet -- the heart of the control plane */ @@ -2163,6 +2166,42 @@ impl Nexus { pub async fn session_hard_delete(&self, token: String) -> DeleteResult { self.db_datastore.session_hard_delete(token).await } + + /// Downloads a file from the within [`BASE_ARTIFACT_DIR`]. + pub async fn download_artifact>( + &self, + path: P, + ) -> Result, Error> { + let path = path.as_ref(); + if !path.starts_with(BASE_ARTIFACT_DIR) { + return Err(Error::internal_error( + "Cannot access path outside artifact directory", + )); + } + + // TODO: If the artifact doesn't exist, we should download it. + // + // There also exists the question of "when should we *remove* things + // from BASE_ARTIFACT_DIR", which we should also resolve. Demo-quality solution + // could be "destroy it on boot" or something. + + // TODO: These artifacts could be quite large - we should figure out how to + // stream this file back instead of holding it entirely in-memory in a + // Vec. + // + // Options: + // - RFC 7233 - "Range Requests" (is this HTTP/1.1 only?) + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests + // - "Roll our own". See: + // https://stackoverflow.com/questions/20969331/standard-method-for-http-partial-upload-resume-upload + let body = tokio::fs::read(&path).await.map_err(|e| { + Error::internal_error(&format!( + "Cannot read artifact from filesystem: {}", + e + )) + })?; + Ok(body) + } } fn generate_session_token() -> String { diff --git a/sled-agent/src/bin/sled-agent.rs b/sled-agent/src/bin/sled-agent.rs index 77432e0138d..3a1df431251 100644 --- a/sled-agent/src/bin/sled-agent.rs +++ b/sled-agent/src/bin/sled-agent.rs @@ -117,6 +117,8 @@ async fn do_run() -> Result<(), CmdError> { nexus_address: nexus_addr, dropshot: ConfigDropshot { bind_address: sled_agent_addr, + // TODO: Check me? + // request_body_max_bytes: 1 << 20, ..Default::default() }, log: ConfigLogging::StderrTerminal { diff --git a/sled-agent/src/http_entrypoints.rs b/sled-agent/src/http_entrypoints.rs index f6f567991c3..bcb8514fbc3 100644 --- a/sled-agent/src/http_entrypoints.rs +++ b/sled-agent/src/http_entrypoints.rs @@ -1,13 +1,10 @@ //! HTTP entrypoint functions for the sled agent's exposed API use super::params::DiskEnsureBody; -use dropshot::endpoint; -use dropshot::ApiDescription; -use dropshot::HttpError; -use dropshot::HttpResponseOk; -use dropshot::Path; -use dropshot::RequestContext; -use dropshot::TypedBody; +use dropshot::{ + endpoint, ApiDescription, HttpError, HttpResponseOk, Path, RequestContext, + TypedBody, +}; use omicron_common::api::internal::nexus::DiskRuntimeState; use omicron_common::api::internal::nexus::InstanceRuntimeState; use omicron_common::api::internal::sled_agent::InstanceEnsureBody; @@ -25,6 +22,7 @@ pub fn api() -> SledApiDescription { fn register_endpoints(api: &mut SledApiDescription) -> Result<(), String> { api.register(instance_put)?; api.register(disk_put)?; + api.register(update_artifact)?; Ok(()) } @@ -86,3 +84,32 @@ async fn disk_put( .await?, )) } + +#[derive(Clone, Debug, Deserialize, JsonSchema)] +pub enum UpdateArtifactKind { + Zone, +} + +// TODO: De-duplicate this struct with the one in iliana's PR? +#[derive(Clone, Debug, Deserialize, JsonSchema)] +struct UpdateArtifact { + pub name: String, + pub version: i64, + pub kind: UpdateArtifactKind, +} + +#[endpoint { + method = POST, + path = "/update" +}] +async fn update_artifact( + rqctx: Arc>, + artifact: TypedBody, +) -> Result, HttpError> { + let sa = rqctx.context(); + + // TODO: pass to `update_artifact`. + let _artifact = artifact.into_inner(); + + Ok(HttpResponseOk(sa.update_artifact().await?)) +} diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index f1a345d86bb..8056729d160 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -24,6 +24,7 @@ use crate::instance_manager::InstanceManager; /// /// Contains both a connection to the Nexus, as well as managed instances. pub struct SledAgent { + _nexus_client: Arc, instances: InstanceManager, } @@ -37,9 +38,9 @@ impl SledAgent { ) -> Result { info!(&log, "created sled agent"; "id" => ?id); - let instances = InstanceManager::new(log, vlan, nexus_client)?; + let instances = InstanceManager::new(log, vlan, nexus_client.clone())?; - Ok(SledAgent { instances }) + Ok(SledAgent { _nexus_client: nexus_client, instances }) } /// Idempotently ensures that a given Instance is running on the sled. @@ -64,4 +65,12 @@ impl SledAgent { ) -> Result { todo!("Disk attachment not yet implemented"); } + + /// Downloads and applies an artifact. + pub async fn update_artifact(&self) -> Result<(), Error> { + // TODO: Call nexus endpoint, download the thing + // TODO: put it in the right spot + // TODO: "handle zones" - restart the service + todo!(); + } } From 80250e16a8571a33610d7409b2ae52466272a373 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 23 Nov 2021 16:17:16 -0500 Subject: [PATCH 2/8] WIP writing files --- nexus/src/internal_api/http_entrypoints.rs | 16 +++++---- openapi/nexus-internal.json | 18 ++++++++++ sled-agent/src/http_entrypoints.rs | 20 ++--------- sled-agent/src/lib.rs | 1 + sled-agent/src/mocks/mod.rs | 4 +++ sled-agent/src/sled_agent.rs | 16 +++++---- sled-agent/src/updates.rs | 39 ++++++++++++++++++++++ 7 files changed, 84 insertions(+), 30 deletions(-) create mode 100644 sled-agent/src/updates.rs diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index 0abbba5d6dc..f2299e6ed49 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -213,18 +213,15 @@ async fn cpapi_metrics_collect( .await } -/// Deserialized input path. #[derive(Deserialize, JsonSchema)] struct AllPath { - path: Vec, + path: String, } /// Endpoint used by Sled Agents to download cached artifacts. #[endpoint { method = GET, - path = "/artifacts/{path:.*}", - // TODO: Buggy without this - unpublished = true, + path = "/artifacts/{path}", }] async fn cpapi_artifact_download( request_context: Arc>>, @@ -233,7 +230,14 @@ async fn cpapi_artifact_download( let context = request_context.context(); let nexus = &context.nexus; let mut entry = PathBuf::from(BASE_ARTIFACT_DIR); - let path = path.into_inner().path; + + // TODO: Most of the below code is ready to accept a multi-component path, + // such as in: + // https://github.com/oxidecomputer/dropshot/blob/78be3deda556a9339ea09f3a9961fd91389f8757/dropshot/examples/file_server.rs#L86-L89 + // + // However, openapi does *not* like that currently, so we limit the endpoint + // to only accepting single-component paths. + let path = vec![path.into_inner().path]; let handler = async { for component in &path { diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index d4f610c0b89..8289c41c176 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -10,6 +10,24 @@ "version": "0.0.1" }, "paths": { + "/artifacts/{path}": { + "get": { + "description": "Endpoint used by Sled Agents to download cached artifacts.", + "operationId": "cpapi_artifact_download", + "parameters": [ + { + "in": "path", + "name": "path", + "required": true, + "schema": { + "type": "string" + }, + "style": "simple" + } + ], + "responses": {} + } + }, "/disks/{disk_id}": { "put": { "description": "Report updated state for a disk.", diff --git a/sled-agent/src/http_entrypoints.rs b/sled-agent/src/http_entrypoints.rs index bcb8514fbc3..af9404764be 100644 --- a/sled-agent/src/http_entrypoints.rs +++ b/sled-agent/src/http_entrypoints.rs @@ -1,6 +1,7 @@ //! HTTP entrypoint functions for the sled agent's exposed API use super::params::DiskEnsureBody; +use super::updates::UpdateArtifact; use dropshot::{ endpoint, ApiDescription, HttpError, HttpResponseOk, Path, RequestContext, TypedBody, @@ -85,19 +86,6 @@ async fn disk_put( )) } -#[derive(Clone, Debug, Deserialize, JsonSchema)] -pub enum UpdateArtifactKind { - Zone, -} - -// TODO: De-duplicate this struct with the one in iliana's PR? -#[derive(Clone, Debug, Deserialize, JsonSchema)] -struct UpdateArtifact { - pub name: String, - pub version: i64, - pub kind: UpdateArtifactKind, -} - #[endpoint { method = POST, path = "/update" @@ -107,9 +95,7 @@ async fn update_artifact( artifact: TypedBody, ) -> Result, HttpError> { let sa = rqctx.context(); + let artifact = artifact.into_inner(); - // TODO: pass to `update_artifact`. - let _artifact = artifact.into_inner(); - - Ok(HttpResponseOk(sa.update_artifact().await?)) + Ok(HttpResponseOk(sa.update_artifact(&artifact).await?)) } diff --git a/sled-agent/src/lib.rs b/sled-agent/src/lib.rs index da1ae80ec17..32d179160a6 100644 --- a/sled-agent/src/lib.rs +++ b/sled-agent/src/lib.rs @@ -22,6 +22,7 @@ mod instance_manager; mod params; pub mod server; mod sled_agent; +mod updates; #[cfg(test)] mod mocks; diff --git a/sled-agent/src/mocks/mod.rs b/sled-agent/src/mocks/mod.rs index a1a6e27c1f6..6b08a0d1299 100644 --- a/sled-agent/src/mocks/mod.rs +++ b/sled-agent/src/mocks/mod.rs @@ -21,5 +21,9 @@ mock! { id: &Uuid, new_runtime_state: &InstanceRuntimeState, ) -> Result<(), Error>; + pub async fn cpapi_artifact_download( + &self, + name: &str + ) -> Result<(), Error>; } } diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 8056729d160..c9f614f2b9a 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -1,5 +1,6 @@ //! Sled agent implementation +use crate::updates; use crate::params::DiskStateRequested; use omicron_common::api::{ external::Error, internal::nexus::DiskRuntimeState, @@ -24,7 +25,7 @@ use crate::instance_manager::InstanceManager; /// /// Contains both a connection to the Nexus, as well as managed instances. pub struct SledAgent { - _nexus_client: Arc, + nexus_client: Arc, instances: InstanceManager, } @@ -40,7 +41,7 @@ impl SledAgent { let instances = InstanceManager::new(log, vlan, nexus_client.clone())?; - Ok(SledAgent { _nexus_client: nexus_client, instances }) + Ok(SledAgent { nexus_client, instances }) } /// Idempotently ensures that a given Instance is running on the sled. @@ -67,10 +68,11 @@ impl SledAgent { } /// Downloads and applies an artifact. - pub async fn update_artifact(&self) -> Result<(), Error> { - // TODO: Call nexus endpoint, download the thing - // TODO: put it in the right spot - // TODO: "handle zones" - restart the service - todo!(); + pub async fn update_artifact( + &self, + artifact: &updates::UpdateArtifact, + ) -> Result<(), Error> { + updates::apply_update(self.nexus_client.as_ref(), artifact).await; + Ok(()) } } diff --git a/sled-agent/src/updates.rs b/sled-agent/src/updates.rs new file mode 100644 index 00000000000..abd291f7290 --- /dev/null +++ b/sled-agent/src/updates.rs @@ -0,0 +1,39 @@ +//! Management of per-sled updates + +use schemars::JsonSchema; +use serde::Deserialize; +use std::path::PathBuf; + +#[cfg(test)] +use crate::mocks::MockNexusClient as NexusClient; +#[cfg(not(test))] +use omicron_common::NexusClient; + +#[derive(Clone, Debug, Deserialize, JsonSchema)] +pub enum UpdateArtifactKind { + Zone, +} + +// TODO: De-duplicate this struct with the one in iliana's PR? +#[derive(Clone, Debug, Deserialize, JsonSchema)] +pub struct UpdateArtifact { + pub name: String, + pub version: i64, + pub kind: UpdateArtifactKind, +} + +/// Downloads an entire update artifact. +pub async fn apply_update(nexus: &NexusClient, artifact: &UpdateArtifact) -> anyhow::Result<()> { + let file_name = format!("{}-{}", artifact.name, artifact.version); + let response = nexus.cpapi_artifact_download(&file_name).await?; + + let mut path = PathBuf::from("/opt/oxide"); + path.push(file_name); + tokio::fs::write(path, response.content()).await?; + + // TODO: O_TRUNC file + + // TODO: Call nexus endpoint, download the thing + // TODO: put it in the right spot + Ok(()) +} From 67fa74cc525e9ab3b365906e83263c05dad9c61b Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 30 Nov 2021 10:53:32 -0500 Subject: [PATCH 3/8] free function now method --- sled-agent/src/sled_agent.rs | 2 +- sled-agent/src/updates.rs | 44 ++++++++++++++++++++++++------------ 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index bffdaba41c4..4f51af3fcdc 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -76,7 +76,7 @@ impl SledAgent { &self, artifact: &updates::UpdateArtifact, ) -> Result<(), Error> { - updates::apply_update(self.nexus_client.as_ref(), artifact).await?; + artifact.download(self.nexus_client.as_ref()).await?; Ok(()) } } diff --git a/sled-agent/src/updates.rs b/sled-agent/src/updates.rs index 5dfd6201a8e..fe661eee1a1 100644 --- a/sled-agent/src/updates.rs +++ b/sled-agent/src/updates.rs @@ -26,19 +26,33 @@ pub struct UpdateArtifact { pub kind: UpdateArtifactKind, } -/// Downloads an entire update artifact. -// TODO: Fix error types -pub async fn apply_update(nexus: &NexusClient, artifact: &UpdateArtifact) -> anyhow::Result<()> { - let file_name = format!("{}-{}", artifact.name, artifact.version); - let response = nexus.cpapi_artifact_download(&file_name).await?; - - let mut path = PathBuf::from("/opt/oxide"); - path.push(file_name); - - // Write the file in its entirety, replacing it if it exists. - tokio::fs::write(path, response.bytes().await?).await?; - - // TODO: Call nexus endpoint, download the thing - // TODO: put it in the right spot - Ok(()) +impl UpdateArtifact { + fn artifact_directory(&self) -> &str { + match self.kind { + UpdateArtifactKind::Zone => "/var/tmp/zones", + } + } + + /// Downloads an update artifact. + // TODO: Fix error types + pub async fn download(&self, nexus: &NexusClient) -> anyhow::Result<()> { + let file_name = format!("{}-{}", self.name, self.version); + let response = nexus.cpapi_artifact_download(&file_name).await?; + + let mut path = PathBuf::from(self.artifact_directory()); + tokio::fs::create_dir_all(&path).await?; + + // We download the file to a location named "-". + // We then rename it to "" after it has successfully + // downloaded, to signify that it is ready for usage. + let mut tmp_path = path.clone(); + tmp_path.push(file_name); + path.push(&self.name); + + // Write the file in its entirety, replacing it if it exists. + tokio::fs::write(&tmp_path, response.bytes().await?).await?; + tokio::fs::rename(&tmp_path, &path).await?; + + Ok(()) + } } From 79009295ffb1b5933c8dd5e698fd26a5681e4add Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 30 Nov 2021 15:53:16 -0500 Subject: [PATCH 4/8] sorting out errs --- sled-agent/src/sled_agent.rs | 4 +++- sled-agent/src/updates.rs | 14 ++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 4f51af3fcdc..77c99d6845d 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -76,7 +76,9 @@ impl SledAgent { &self, artifact: &updates::UpdateArtifact, ) -> Result<(), Error> { - artifact.download(self.nexus_client.as_ref()).await?; + artifact.download(self.nexus_client.as_ref()) + .await + .map_err(|e| Error::internal_error(&format!("Failed download: {}", e)))?; Ok(()) } } diff --git a/sled-agent/src/updates.rs b/sled-agent/src/updates.rs index fe661eee1a1..247f1d36013 100644 --- a/sled-agent/src/updates.rs +++ b/sled-agent/src/updates.rs @@ -13,6 +13,15 @@ use crate::mocks::MockNexusClient as NexusClient; #[cfg(not(test))] use nexus_client::Client as NexusClient; +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error("I/O Error: {0}")] + Io(#[from] std::io::Error), + + #[error("Failed to contact nexus: {0}")] + Nexus(anyhow::Error), +} + #[derive(Clone, Debug, Deserialize, JsonSchema)] pub enum UpdateArtifactKind { Zone, @@ -35,9 +44,10 @@ impl UpdateArtifact { /// Downloads an update artifact. // TODO: Fix error types - pub async fn download(&self, nexus: &NexusClient) -> anyhow::Result<()> { + pub async fn download(&self, nexus: &NexusClient) -> Result<(), Error> { let file_name = format!("{}-{}", self.name, self.version); - let response = nexus.cpapi_artifact_download(&file_name).await?; + let response = nexus.cpapi_artifact_download(&file_name) + .await?; let mut path = PathBuf::from(self.artifact_directory()); tokio::fs::create_dir_all(&path).await?; From 4523cbd0d3c3076ba9075555563cd99c2c1fdff2 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 1 Dec 2021 14:41:53 -0500 Subject: [PATCH 5/8] Add tests, fix bugs --- Cargo.lock | 1 + common/src/api/external/mod.rs | 2 + nexus/src/internal_api/http_entrypoints.rs | 64 ++++++++---------- nexus/src/nexus.rs | 60 ++++++++++++++-- nexus/tests/test_artifact_download.rs | 77 +++++++++++++++++++++ sled-agent/Cargo.toml | 1 + sled-agent/src/bin/sled-agent.rs | 2 - sled-agent/src/mocks/mod.rs | 2 +- sled-agent/src/updates.rs | 79 +++++++++++++++++++--- 9 files changed, 234 insertions(+), 54 deletions(-) create mode 100644 nexus/tests/test_artifact_download.rs diff --git a/Cargo.lock b/Cargo.lock index 8463f35e178..bfcde9f29b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1709,6 +1709,7 @@ dependencies = [ "dropshot", "expectorate", "futures", + "http", "ipnetwork", "mockall", "nexus-client", diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 03d4a9644d9..e2880d0f973 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -472,6 +472,7 @@ pub enum ResourceType { Dataset, Disk, DiskAttachment, + DownloadArtifact, Instance, Rack, Sled, @@ -497,6 +498,7 @@ impl Display for ResourceType { ResourceType::Dataset => "dataset", ResourceType::Disk => "disk", ResourceType::DiskAttachment => "disk attachment", + ResourceType::DownloadArtifact => "download artifact", ResourceType::Instance => "instance", ResourceType::Rack => "rack", ResourceType::Sled => "sled", diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index d847f32afcd..1954c3328c5 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -312,19 +312,19 @@ async fn cpapi_artifact_download( // to only accepting single-component paths. let path = vec![path.into_inner().path]; - let handler = async { - for component in &path { - // Dropshot should not provide "." and ".." components. - assert_ne!(component, "."); - assert_ne!(component, ".."); - entry.push(component); + for component in &path { + // Dropshot should not provide "." and ".." components. + assert_ne!(component, "."); + assert_ne!(component, ".."); + entry.push(component); + if entry.exists() { // We explicitly prohibit consumers from following symlinks to prevent // showing data outside of the intended directory. - let m = entry.symlink_metadata().map_err(|_| { + let m = entry.symlink_metadata().map_err(|e| { HttpError::for_bad_request( None, - "Cannot query for symlink info".to_string(), + format!("Failed to query file metadata: {}", e), ) })?; if m.file_type().is_symlink() { @@ -334,35 +334,27 @@ async fn cpapi_artifact_download( )); } } + } - if entry.is_dir() { - return Err(HttpError::for_bad_request( - None, - "Directory download not supported".to_string(), - )); - } - - let entry = entry.canonicalize().map_err(|e| { - HttpError::for_bad_request( - None, - format!("Cannot canonicalize path: {}", e), - ) - })?; - - let body = nexus.download_artifact(&entry).await?; + // Note - at this point, "entry" may or may not actually exist. + // We try to avoid creating intermediate artifacts until we know there + // is something "real" to download, as this would let malformed paths + // create defunct intermediate directories. + if entry.is_dir() { + return Err(HttpError::for_bad_request( + None, + "Directory download not supported".to_string(), + )); + } + let body = nexus.download_artifact(&entry).await?; - // Derive the MIME type from the file name - let content_type = mime_guess::from_path(&entry) - .first() - .map_or_else(|| "text/plain".to_string(), |m| m.to_string()); + // Derive the MIME type from the file name + let content_type = mime_guess::from_path(&entry) + .first() + .map_or_else(|| "text/plain".to_string(), |m| m.to_string()); - Ok(Response::builder() - .status(StatusCode::OK) - .header(http::header::CONTENT_TYPE, content_type) - .body(body.into())?) - }; - context - .internal_latencies - .instrument_dropshot_handler(&request_context, handler) - .await + Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, content_type) + .body(body.into())?) } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index d3829d576bf..66eaec11577 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -2253,7 +2253,7 @@ impl Nexus { self.db_datastore.session_hard_delete(token).await } - /// Downloads a file from the within [`BASE_ARTIFACT_DIR`]. + /// Downloads a file from within [`BASE_ARTIFACT_DIR`]. pub async fn download_artifact>( &self, path: P, @@ -2265,11 +2265,59 @@ impl Nexus { )); } - // TODO: If the artifact doesn't exist, we should download it. - // - // There also exists the question of "when should we *remove* things - // from BASE_ARTIFACT_DIR", which we should also resolve. Demo-quality solution - // could be "destroy it on boot" or something. + if !path.exists() { + info!( + self.log, + "Accessing {} - needs to be downloaded", + path.display() + ); + // If the artifact doesn't exist, we should download it. + // + // TODO: There also exists the question of "when should we *remove* + // things from BASE_ARTIFACT_DIR", which we should also resolve. + // Demo-quality solution could be "destroy it on boot" or something? + // (we aren't doing that yet). + + let file_name = path.strip_prefix(BASE_ARTIFACT_DIR).unwrap(); + match file_name.to_str().unwrap() { + // TODO: iliana if you're reading this, + // 1. I'm sorry + // 2. We should probably do something less bad here + // + // At the moment, the only file we "know" how to download is a + // testfile, which is pulled out of thin air. Realistically, we + // should pull this from the DB + query an external server. + // Happy to delete this as soon as we can. + "testfile" => { + // We should only create the intermediate directories + // after validating that this is a real artifact that + // can (and should) be downloaded. + if let Some(parent) = path.parent() { + tokio::fs::create_dir_all(parent).await.map_err(|e| { + Error::internal_error( + &format!("Failed to create intermediate directory: {}", e) + ) + })?; + } + tokio::fs::write(path, "testfile contents").await.map_err( + |e| { + Error::internal_error(&format!( + "Failed to write file: {}", + e + )) + }, + )?; + } + _ => { + return Err(Error::not_found_other( + ResourceType::DownloadArtifact, + file_name.display().to_string(), + )); + } + } + } else { + info!(self.log, "Accessing {} - already exists", path.display()); + } // TODO: These artifacts could be quite large - we should figure out how to // stream this file back instead of holding it entirely in-memory in a diff --git a/nexus/tests/test_artifact_download.rs b/nexus/tests/test_artifact_download.rs new file mode 100644 index 00000000000..d7c697bb442 --- /dev/null +++ b/nexus/tests/test_artifact_download.rs @@ -0,0 +1,77 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use http::method::Method; +use http::StatusCode; + +pub mod common; +use common::test_setup; + +extern crate slog; + +// Tests the "normal" case of downloading an artifact. +// +// This will typically be invoked by the Sled Agent, after instructed +// to access an artifact. +#[tokio::test] +async fn test_download_known_artifact_returns_ok() { + let cptestctx = test_setup("test_download_known_artifact_returns_ok").await; + let client = &cptestctx.internal_client; + + // TODO: Can we replace this with a "real" small file that must be + // downloaded, instead of synthetically created? + let filename = "testfile"; + let artifact_get_url = format!("/artifacts/{}", filename); + + let response = client + .make_request_no_body(Method::GET, &artifact_get_url, StatusCode::OK) + .await + .unwrap(); + + assert_eq!( + hyper::body::to_bytes(response.into_body()).await.unwrap(), + "testfile contents" + ); + cptestctx.teardown().await; +} + +// Tests that missing artifacts return "NOT_FOUND". +#[tokio::test] +async fn test_download_bad_artifact_not_found() { + let cptestctx = test_setup("test_download_bad_artifact_not_found").await; + let client = &cptestctx.internal_client; + + let filename = "not_a_real_artifact"; + let artifact_get_url = format!("/artifacts/{}", filename); + + client + .make_request_error( + Method::GET, + &artifact_get_url, + StatusCode::NOT_FOUND, + ) + .await; + + cptestctx.teardown().await; +} + +// Tests that ".." paths are disallowed by dropshot. +#[tokio::test] +async fn test_download_with_dots_fails() { + let cptestctx = test_setup("test_download_with_dots_fails").await; + let client = &cptestctx.internal_client; + + let filename = "hey/can/you/look/../../../../up/the/directory/tree"; + let artifact_get_url = format!("/artifacts/{}", filename); + + client + .make_request_error( + Method::GET, + &artifact_get_url, + StatusCode::BAD_REQUEST, + ) + .await; + + cptestctx.teardown().await; +} diff --git a/sled-agent/Cargo.toml b/sled-agent/Cargo.toml index 3387a83dde6..fcfa1f3ff79 100644 --- a/sled-agent/Cargo.toml +++ b/sled-agent/Cargo.toml @@ -38,6 +38,7 @@ zone = "0.1" [dev-dependencies] expectorate = "1.0.4" +http = "0.2.5" mockall = "0.10" omicron-test-utils = { path = "../test-utils" } openapi-lint = { git = "https://github.com/oxidecomputer/openapi-lint", branch = "main" } diff --git a/sled-agent/src/bin/sled-agent.rs b/sled-agent/src/bin/sled-agent.rs index 63fdac63fed..d56d6287f63 100644 --- a/sled-agent/src/bin/sled-agent.rs +++ b/sled-agent/src/bin/sled-agent.rs @@ -121,8 +121,6 @@ async fn do_run() -> Result<(), CmdError> { nexus_address: nexus_addr, dropshot: ConfigDropshot { bind_address: sled_agent_addr, - // TODO: Check me? - // request_body_max_bytes: 1 << 20, ..Default::default() }, log: ConfigLogging::StderrTerminal { diff --git a/sled-agent/src/mocks/mod.rs b/sled-agent/src/mocks/mod.rs index fdd8363a115..297a0c693f3 100644 --- a/sled-agent/src/mocks/mod.rs +++ b/sled-agent/src/mocks/mod.rs @@ -4,9 +4,9 @@ //! Mock structures for testing. +use anyhow::Error; use mockall::mock; use nexus_client::types::{InstanceRuntimeState, SledAgentStartupInfo}; -use omicron_common::api::external::Error; use reqwest::Response; use slog::Logger; use uuid::Uuid; diff --git a/sled-agent/src/updates.rs b/sled-agent/src/updates.rs index 830c9734e93..6691f792bf8 100644 --- a/sled-agent/src/updates.rs +++ b/sled-agent/src/updates.rs @@ -6,7 +6,7 @@ use schemars::JsonSchema; use serde::Deserialize; -use std::path::PathBuf; +use std::path::Path; #[cfg(test)] use crate::mocks::MockNexusClient as NexusClient; @@ -25,12 +25,9 @@ pub enum Error { Response(reqwest::Error), } -#[derive(Clone, Debug, Deserialize, JsonSchema)] -pub enum UpdateArtifactKind { - Zone, -} - // TODO: De-duplicate this struct with the one in iliana's PR? +// +// This should likely be a wrapper around that type. #[derive(Clone, Debug, Deserialize, JsonSchema)] pub struct UpdateArtifact { pub name: String, @@ -38,14 +35,33 @@ pub struct UpdateArtifact { pub kind: UpdateArtifactKind, } +// TODO: De-dup me too. +#[derive(Clone, Debug, Deserialize, JsonSchema)] +pub enum UpdateArtifactKind { + Zone, +} + impl UpdateArtifact { - fn artifact_directory(&self) -> &str { + fn artifact_directory(&self) -> &'static Path { match self.kind { - UpdateArtifactKind::Zone => "/var/tmp/zones", + UpdateArtifactKind::Zone => Path::new("/var/tmp/zones"), } } /// Downloads an update artifact. + /// + /// The artifact is eventually stored in the path: + /// / + /// + /// Such as: + /// /var/tmp/zones/myzone + /// + /// While being downloaded, it is stored in a path also containing the + /// version: + /// / - + /// + /// Such as: + /// /var/tmp/zones/myzone-3 pub async fn download(&self, nexus: &NexusClient) -> Result<(), Error> { let file_name = format!("{}-{}", self.name, self.version); let response = nexus @@ -53,7 +69,7 @@ impl UpdateArtifact { .await .map_err(|e| Error::Nexus(e))?; - let mut path = PathBuf::from(self.artifact_directory()); + let mut path = self.artifact_directory().to_path_buf(); tokio::fs::create_dir_all(&path).await?; // We download the file to a location named "-". @@ -73,3 +89,48 @@ impl UpdateArtifact { Ok(()) } } + +#[cfg(test)] +mod test { + use super::*; + use crate::mocks::MockNexusClient; + use http::{Response, StatusCode}; + + #[tokio::test] + #[serial_test::serial] + async fn test_write_artifact_to_filesystem() { + // The (completely fabricated) artifact we'd like to download. + let expected_name = "test_artifact"; + let expected_contents = "test_artifact contents"; + let artifact = UpdateArtifact { + name: expected_name.to_string(), + version: 3, + kind: UpdateArtifactKind::Zone, + }; + let expected_path = artifact.artifact_directory().join(expected_name); + + // Remove the file if it already exists. + let _ = tokio::fs::remove_file(expected_path).await; + + // Let's pretend this is an artifact Nexus can actually give us. + let mut nexus_client = MockNexusClient::default(); + nexus_client.expect_cpapi_artifact_download().times(1).return_once( + |name| { + assert_eq!(name, expected_name); + let response = Response::builder() + .status(StatusCode::OK) + .body(expected_contents) + .unwrap(); + Ok(response.into()) + }, + ); + + // This should download the file to our local filesystem. + artifact.download(&nexus_client).await.unwrap(); + + // Confirm the download succeeded. + assert!(expected_path.exists()); + let contents = tokio::fs::read(&expected_path).await.unwrap(); + assert_eq!(std::str::from_utf8(&contents).unwrap(), expected_contents); + } +} From e82885c6a3ee99c61c3696a81174f3bfa23fe62c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 1 Dec 2021 14:50:05 -0500 Subject: [PATCH 6/8] I guess compiling code is better than the alternative --- sled-agent/src/updates.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sled-agent/src/updates.rs b/sled-agent/src/updates.rs index 6691f792bf8..e21cc070412 100644 --- a/sled-agent/src/updates.rs +++ b/sled-agent/src/updates.rs @@ -110,13 +110,13 @@ mod test { let expected_path = artifact.artifact_directory().join(expected_name); // Remove the file if it already exists. - let _ = tokio::fs::remove_file(expected_path).await; + let _ = tokio::fs::remove_file(&expected_path).await; // Let's pretend this is an artifact Nexus can actually give us. let mut nexus_client = MockNexusClient::default(); nexus_client.expect_cpapi_artifact_download().times(1).return_once( - |name| { - assert_eq!(name, expected_name); + move |name| { + assert_eq!(name, "test_artifact-3"); let response = Response::builder() .status(StatusCode::OK) .body(expected_contents) From dc5897a4a90d97814104282069c0541ffee0797f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 1 Dec 2021 16:29:11 -0500 Subject: [PATCH 7/8] I EXPECTORATEd that my JSON would be more up-to-date --- openapi/sled-agent.json | 46 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index abb60cdacc1..f3706eb70bf 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -87,6 +87,26 @@ } } } + }, + "/update": { + "post": { + "operationId": "update_artifact", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/UpdateArtifact" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation" + } + } + } } }, "components": { @@ -605,6 +625,32 @@ "subnet_id", "vpc_id" ] + }, + "UpdateArtifact": { + "type": "object", + "properties": { + "kind": { + "$ref": "#/components/schemas/UpdateArtifactKind" + }, + "name": { + "type": "string" + }, + "version": { + "type": "integer", + "format": "int64" + } + }, + "required": [ + "kind", + "name", + "version" + ] + }, + "UpdateArtifactKind": { + "type": "string", + "enum": [ + "Zone" + ] } } } From 9116b0797173da26909059a5b2e397c777717b73 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 20 Dec 2021 12:37:57 -0500 Subject: [PATCH 8/8] Keep merging --- common/src/api/external/mod.rs | 1 + .../artifact_download.rs} | 6 +----- nexus/tests/integration_tests/mod.rs | 1 + 3 files changed, 3 insertions(+), 5 deletions(-) rename nexus/tests/{test_artifact_download.rs => integration_tests/artifact_download.rs} (97%) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 0a14e8f2cfc..99f3c6277d4 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -569,6 +569,7 @@ pub enum ResourceType { Project, Dataset, Disk, + DownloadArtifact, Instance, NetworkInterface, Rack, diff --git a/nexus/tests/test_artifact_download.rs b/nexus/tests/integration_tests/artifact_download.rs similarity index 97% rename from nexus/tests/test_artifact_download.rs rename to nexus/tests/integration_tests/artifact_download.rs index d7c697bb442..3b790756580 100644 --- a/nexus/tests/test_artifact_download.rs +++ b/nexus/tests/integration_tests/artifact_download.rs @@ -4,11 +4,7 @@ use http::method::Method; use http::StatusCode; - -pub mod common; -use common::test_setup; - -extern crate slog; +use nexus_test_utils::test_setup; // Tests the "normal" case of downloading an artifact. // diff --git a/nexus/tests/integration_tests/mod.rs b/nexus/tests/integration_tests/mod.rs index 1c581dca5c7..1de7f818c04 100644 --- a/nexus/tests/integration_tests/mod.rs +++ b/nexus/tests/integration_tests/mod.rs @@ -3,6 +3,7 @@ //! See the driver in the parent directory for how and why this is structured //! the way it is. +mod artifact_download; mod authn_http; mod authz; mod basic;