Merge pull request #3178 from Bravo555/improve/dont-leave-tmpfiles-wh…

…en-downloading

Use `tempfile` in config-manager and downloader to ensure tmpfiles are always deleted

crates/common/download/Cargo.toml

@@ -22,14 +22,14 @@ rustls = { workspace = true }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 tedge_utils = { workspace = true }
+tempfile = { workspace = true }
 thiserror = { workspace = true }
 tokio = { workspace = true, features = ["fs"] }
 url = { workspace = true }
 
 [dev-dependencies]
 mockito = { workspace = true }
 regex = { workspace = true }
-tempfile = { workspace = true }
 test-case = { workspace = true }
 tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }
 

crates/common/download/src/download.rs

@@ -25,9 +25,7 @@ use std::os::unix::prelude::AsRawFd;
 use std::path::Path;
 use std::path::PathBuf;
 use std::time::Duration;
-use tedge_utils::file::move_file;
 use tedge_utils::file::FileError;
-use tedge_utils::file::PermissionEntry;
 
 #[cfg(target_os = "linux")]
 use nix::fcntl::fallocate;
@@ -106,7 +104,6 @@ impl Auth {
 #[derive(Debug)]
 pub struct Downloader {
     target_filename: PathBuf,
-    target_permission: PermissionEntry,
     backoff: ExponentialBackoff,
     client: Client,
 }
@@ -126,28 +123,6 @@ impl Downloader {
         let client = client_builder.build().expect("Client builder is valid");
         Self {
             target_filename: target_path,
-            target_permission: PermissionEntry::default(),
-            backoff: default_backoff(),
-            client,
-        }
-    }
-
-    /// Creates a new downloader which downloads to a target directory and sets
-    /// specified permissions the downloaded file.
-    pub fn with_permission(
-        target_path: PathBuf,
-        target_permission: PermissionEntry,
-        identity: Option<Identity>,
-        cloud_root_certs: CloudRootCerts,
-    ) -> Self {
-        let mut client_builder = cloud_root_certs.client_builder();
-        if let Some(identity) = identity {
-            client_builder = client_builder.identity(identity);
-        }
-        let client = client_builder.build().expect("Client builder is valid");
-        Self {
-            target_filename: target_path,
-            target_permission,
             backoff: default_backoff(),
             client,
         }
@@ -175,8 +150,13 @@ impl Downloader {
         let tmp_target_path = self.temp_filename().await?;
         let target_file_path = self.target_filename.as_path();
 
-        let mut file: File = File::create(&tmp_target_path)
-            .context(format!("Can't create a temporary file {tmp_target_path:?}"))?;
+        let temp_dir = self
+            .target_filename
+            .parent()
+            .unwrap_or(&self.target_filename);
+
+        let mut file = tempfile::NamedTempFile::new_in(temp_dir)
+            .context("Could not write to temporary file".to_string())?;
 
         let mut response = self.request_range_from(url, 0).await?;
 
@@ -187,21 +167,21 @@ impl Downloader {
         );
 
         if file_len > 0 {
-            try_pre_allocate_space(&file, &tmp_target_path, file_len)?;
+            try_pre_allocate_space(file.as_file(), &tmp_target_path, file_len)?;
             debug!("preallocated space for file {tmp_target_path:?}, len={file_len}");
         }
 
-        if let Err(err) = save_chunks_to_file_at(&mut response, &mut file, 0).await {
+        if let Err(err) = save_chunks_to_file_at(&mut response, file.as_file_mut(), 0).await {
             match err {
                 SaveChunksError::Network(err) => {
                     warn!("Error while downloading response: {err}.\nRetrying...");
 
                     match response.headers().get(header::ACCEPT_RANGES) {
                         Some(unit) if unit == "bytes" => {
-                            self.download_remaining(url, &mut file).await?;
+                            self.download_remaining(url, file.as_file_mut()).await?;
                         }
                         _ => {
-                            self.retry(url, &mut file).await?;
+                            self.retry(url, file.as_file_mut()).await?;
                         }
                     }
                 }
@@ -219,13 +199,10 @@ impl Downloader {
             "Moving downloaded file from {:?} to {:?}",
             &tmp_target_path, &target_file_path
         );
-        move_file(
-            tmp_target_path,
-            target_file_path,
-            self.target_permission.clone(),
-        )
-        .await
-        .map_err(FileError::from)?;
+
+        file.persist(target_file_path)
+            .map_err(|p| p.error)
+            .context("Could not persist temporary file".to_string())?;
 
         Ok(())
     }
@@ -740,6 +717,30 @@ mod tests {
         assert_eq!("".as_bytes(), std::fs::read(downloader.filename()).unwrap());
     }
 
+    #[tokio::test]
+    async fn doesnt_leave_tmpfiles_on_errors() {
+        let server = mockito::Server::new();
+
+        let target_dir_path = TempDir::new().unwrap();
+        let target_path = target_dir_path.path().join("test_doesnt_leave_tmpfiles");
+
+        let mut target_url = server.url();
+        target_url.push_str("/some_file.txt");
+
+        let url = DownloadInfo::new(&target_url);
+
+        let mut downloader = Downloader::new(target_path, None, CloudRootCerts::from([]));
+        downloader.set_backoff(ExponentialBackoff {
+            current_interval: Duration::ZERO,
+            max_interval: Duration::ZERO,
+            max_elapsed_time: Some(Duration::ZERO),
+            ..Default::default()
+        });
+        downloader.download(&url).await.unwrap_err();
+
+        assert_eq!(fs::read_dir(target_dir_path.path()).unwrap().count(), 0);
+    }
+
     /// This test simulates HTTP response where a connection just drops and a
     /// client hits a timeout, having downloaded only part of the response.
     ///

crates/extensions/c8y_http_proxy/src/actor.rs

@@ -513,9 +513,8 @@ impl C8YHttpProxyActor {
         }
 
         info!(target: self.name(), "Downloading from: {:?}", download_info.url());
-        let downloader: Downloader = Downloader::with_permission(
+        let downloader: Downloader = Downloader::new(
             request.file_path,
-            request.file_permissions,
             self.config.identity.clone(),
             self.config.cloud_root_certs.clone(),
         );

crates/extensions/c8y_http_proxy/src/handle.rs

@@ -13,7 +13,6 @@ use std::path::Path;
 use std::path::PathBuf;
 use tedge_actors::ClientMessageBox;
 use tedge_actors::Service;
-use tedge_utils::file::PermissionEntry;
 
 use super::messages::DownloadFile;
 
@@ -112,12 +111,10 @@ impl C8YHttpProxy {
         &mut self,
         download_url: &str,
         file_path: PathBuf,
-        file_permissions: PermissionEntry,
     ) -> Result<(), C8YRestError> {
         let request: C8YRestRequest = DownloadFile {
             download_url: download_url.into(),
             file_path,
-            file_permissions,
         }
         .into();
         match self.c8y.await_response(request).await? {

crates/extensions/c8y_http_proxy/src/messages.rs

@@ -5,7 +5,6 @@ use std::path::PathBuf;
 use tedge_actors::fan_in_message_type;
 use tedge_actors::ChannelError;
 use tedge_http_ext::HttpError;
-use tedge_utils::file::PermissionEntry;
 
 fan_in_message_type!(C8YRestRequest[GetJwtToken, GetFreshJwtToken, CreateEvent, SoftwareListResponse, UploadLogBinary, UploadFile, DownloadFile]: Debug, PartialEq, Eq);
 //HIPPO Rename EventId to String as there could be many other String responses as well and this macro doesn't allow another String variant
@@ -90,7 +89,6 @@ pub struct UploadFile {
 pub struct DownloadFile {
     pub download_url: String,
     pub file_path: PathBuf,
-    pub file_permissions: PermissionEntry,
 }
 
 pub type EventId = String;

crates/extensions/tedge_config_manager/src/actor.rs

@@ -350,14 +350,15 @@ impl ConfigManagerWorker {
         let download_response =
             download_result.context("config-manager failed downloading a file")?;
 
-        let from = Utf8Path::from_path(download_response.file_path.as_path()).unwrap();
+        let from = tempfile::TempPath::from_path(download_response.file_path);
+
+        let from_path = Utf8Path::from_path(&from)
+            .with_context(|| format!("path is not utf-8: '{}'", from.to_string_lossy()))?;
+
         let deployed_to_path = self
-            .deploy_config_file(from, &request.config_type)
+            .deploy_config_file(from_path, &request.config_type)
             .context("failed to deploy configuration file")?;
 
-        // TODO: source temporary file should be cleaned up automatically
-        let _ = std::fs::remove_file(from);
-
         Ok(deployed_to_path)
     }
 

crates/extensions/tedge_downloader_ext/src/actor.rs

@@ -118,20 +118,11 @@ impl<T: Message> Server for DownloaderActor<T> {
             DownloadInfo::new(&request.url)
         };
 
-        let downloader = if let Some(permission) = request.permission {
-            Downloader::with_permission(
-                request.file_path.clone(),
-                permission,
-                self.identity.clone(),
-                self.cloud_root_certs.clone(),
-            )
-        } else {
-            Downloader::new(
-                request.file_path.clone(),
-                self.identity.clone(),
-                self.cloud_root_certs.clone(),
-            )
-        };
+        let downloader = Downloader::new(
+            request.file_path.clone(),
+            self.identity.clone(),
+            self.cloud_root_certs.clone(),
+        );
 
         info!(
             "Downloading from url {} to location {}",