Merge pull request #4202 from fidencio/topic/second-round-of-backports-for-2.4.1

stable-2.4 | Second round of backports for the 2.4.1 release

src/agent/src/watcher.rs

@@ -6,6 +6,7 @@
 #![allow(unknown_lints)]
 
 use std::collections::HashMap;
+use std::os::unix::fs::MetadataExt;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 use std::time::SystemTime;
@@ -13,6 +14,7 @@ use std::time::SystemTime;
 use anyhow::{ensure, Context, Result};
 use async_recursion::async_recursion;
 use nix::mount::{umount, MsFlags};
+use nix::unistd::{Gid, Uid};
 use slog::{debug, error, info, warn, Logger};
 use thiserror::Error;
 use tokio::fs;
@@ -80,7 +82,8 @@ impl Drop for Storage {
 }
 
 async fn copy(from: impl AsRef<Path>, to: impl AsRef<Path>) -> Result<()> {
-    if fs::symlink_metadata(&from).await?.file_type().is_symlink() {
+    let metadata = fs::symlink_metadata(&from).await?;
+    if metadata.file_type().is_symlink() {
         // if source is a symlink, create new symlink with same link source. If
         // the symlink exists, remove and create new one:
         if fs::symlink_metadata(&to).await.is_ok() {
@@ -88,8 +91,15 @@ async fn copy(from: impl AsRef<Path>, to: impl AsRef<Path>) -> Result<()> {
         }
         fs::symlink(fs::read_link(&from).await?, &to).await?;
     } else {
-        fs::copy(from, to).await?;
+        fs::copy(&from, &to).await?;
     }
+    // preserve the source uid and gid to the destination.
+    nix::unistd::chown(
+        to.as_ref(),
+        Some(Uid::from_raw(metadata.uid())),
+        Some(Gid::from_raw(metadata.gid())),
+    )?;
+
     Ok(())
 }
 
@@ -106,14 +116,29 @@ impl Storage {
 
     async fn update_target(&self, logger: &Logger, source_path: impl AsRef<Path>) -> Result<()> {
         let source_file_path = source_path.as_ref();
+        let metadata = source_file_path.symlink_metadata()?;
 
         // if we are creating a directory: just create it, nothing more to do
-        if source_file_path.symlink_metadata()?.file_type().is_dir() {
+        if metadata.file_type().is_dir() {
             let dest_file_path = self.make_target_path(&source_file_path)?;
 
             fs::create_dir_all(&dest_file_path)
                 .await
                 .with_context(|| format!("Unable to mkdir all for {}", dest_file_path.display()))?;
+            // set the directory permissions to match the source directory permissions
+            fs::set_permissions(&dest_file_path, metadata.permissions())
+                .await
+                .with_context(|| {
+                    format!("Unable to set permissions for {}", dest_file_path.display())
+                })?;
+            // preserve the source directory uid and gid to the destination.
+            nix::unistd::chown(
+                &dest_file_path,
+                Some(Uid::from_raw(metadata.uid())),
+                Some(Gid::from_raw(metadata.gid())),
+            )
+            .with_context(|| format!("Unable to set ownership for {}", dest_file_path.display()))?;
+
             return Ok(());
         }
 
@@ -504,6 +529,7 @@ mod tests {
     use super::*;
     use crate::mount::is_mounted;
     use crate::skip_if_not_root;
+    use nix::unistd::{Gid, Uid};
     use std::fs;
     use std::thread;
 
@@ -895,20 +921,28 @@ mod tests {
 
     #[tokio::test]
     async fn test_copy() {
+        skip_if_not_root!();
+
         // prepare tmp src/destination
         let source_dir = tempfile::tempdir().unwrap();
         let dest_dir = tempfile::tempdir().unwrap();
+        let uid = Uid::from_raw(10);
+        let gid = Gid::from_raw(200);
 
         // verify copy of a regular file
         let src_file = source_dir.path().join("file.txt");
         let dst_file = dest_dir.path().join("file.txt");
         fs::write(&src_file, "foo").unwrap();
+        nix::unistd::chown(&src_file, Some(uid), Some(gid)).unwrap();
+
         copy(&src_file, &dst_file).await.unwrap();
         // verify destination:
-        assert!(!fs::symlink_metadata(dst_file)
+        assert!(!fs::symlink_metadata(&dst_file)
             .unwrap()
             .file_type()
             .is_symlink());
+        assert_eq!(fs::metadata(&dst_file).unwrap().uid(), uid.as_raw());
+        assert_eq!(fs::metadata(&dst_file).unwrap().gid(), gid.as_raw());
 
         // verify copy of a symlink
         let src_symlink_file = source_dir.path().join("symlink_file.txt");
@@ -916,7 +950,7 @@ mod tests {
         tokio::fs::symlink(&src_file, &src_symlink_file)
             .await
             .unwrap();
-        copy(src_symlink_file, &dst_symlink_file).await.unwrap();
+        copy(&src_symlink_file, &dst_symlink_file).await.unwrap();
         // verify destination:
         assert!(fs::symlink_metadata(&dst_symlink_file)
             .unwrap()
@@ -924,6 +958,8 @@ mod tests {
             .is_symlink());
         assert_eq!(fs::read_link(&dst_symlink_file).unwrap(), src_file);
         assert_eq!(fs::read_to_string(&dst_symlink_file).unwrap(), "foo");
+        assert_ne!(fs::metadata(&dst_symlink_file).unwrap().uid(), uid.as_raw());
+        assert_ne!(fs::metadata(&dst_symlink_file).unwrap().gid(), gid.as_raw());
     }
 
     #[tokio::test]
@@ -1069,6 +1105,8 @@ mod tests {
 
     #[tokio::test]
     async fn watch_directory() {
+        skip_if_not_root!();
+
         // Prepare source directory:
         // ./tmp/1.txt
         // ./tmp/A/B/2.txt
@@ -1079,7 +1117,9 @@ mod tests {
 
         // A/C is an empty directory
         let empty_dir = "A/C";
-        fs::create_dir_all(source_dir.path().join(empty_dir)).unwrap();
+        let path = source_dir.path().join(empty_dir);
+        fs::create_dir_all(&path).unwrap();
+        nix::unistd::chown(&path, Some(Uid::from_raw(10)), Some(Gid::from_raw(200))).unwrap();
 
         // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique
         thread::sleep(Duration::from_millis(20));
@@ -1123,7 +1163,9 @@ mod tests {
 
         // create another empty directory A/C/D
         let empty_dir = "A/C/D";
-        fs::create_dir_all(source_dir.path().join(empty_dir)).unwrap();
+        let path = source_dir.path().join(empty_dir);
+        fs::create_dir_all(&path).unwrap();
+        nix::unistd::chown(&path, Some(Uid::from_raw(10)), Some(Gid::from_raw(200))).unwrap();
         assert_eq!(entry.scan(&logger).await.unwrap(), 1);
         assert!(dest_dir.path().join(empty_dir).exists());
     }

src/runtime/virtcontainers/clh.go

@@ -165,6 +165,7 @@ type cloudHypervisor struct {
 	APIClient      clhClient
 	ctx            context.Context
 	id             string
+	devicesIds     map[string]string
 	vmconfig       chclient.VmConfig
 	state          CloudHypervisorState
 	config         HypervisorConfig
@@ -360,6 +361,7 @@ func (clh *cloudHypervisor) CreateVM(ctx context.Context, id string, network Net
 
 	clh.id = id
 	clh.state.state = clhNotReady
+	clh.devicesIds = make(map[string]string)
 
 	clh.Logger().WithField("function", "CreateVM").Info("creating Sandbox")
 
@@ -667,7 +669,6 @@ func (clh *cloudHypervisor) hotplugAddBlockDevice(drive *config.BlockDrive) erro
 	clhDisk := *chclient.NewDiskConfig(drive.File)
 	clhDisk.Readonly = &drive.ReadOnly
 	clhDisk.VhostUser = func(b bool) *bool { return &b }(false)
-	clhDisk.Id = &driveID
 
 	pciInfo, _, err := cl.VmAddDiskPut(ctx, clhDisk)
 
@@ -675,6 +676,7 @@ func (clh *cloudHypervisor) hotplugAddBlockDevice(drive *config.BlockDrive) erro
 		return fmt.Errorf("failed to hotplug block device %+v %s", drive, openAPIClientError(err))
 	}
 
+	clh.devicesIds[driveID] = pciInfo.GetId()
 	drive.PCIPath, err = clhPciInfoToPath(pciInfo)
 
 	return err
@@ -688,11 +690,11 @@ func (clh *cloudHypervisor) hotPlugVFIODevice(device *config.VFIODev) error {
 	// Create the clh device config via the constructor to ensure default values are properly assigned
 	clhDevice := *chclient.NewVmAddDevice()
 	clhDevice.Path = &device.SysfsDev
-	clhDevice.Id = &device.ID
 	pciInfo, _, err := cl.VmAddDevicePut(ctx, clhDevice)
 	if err != nil {
 		return fmt.Errorf("Failed to hotplug device %+v %s", device, openAPIClientError(err))
 	}
+	clh.devicesIds[device.ID] = pciInfo.GetId()
 
 	// clh doesn't use bridges, so the PCI path is simply the slot
 	// number of the device.  This will break if clh starts using
@@ -753,13 +755,15 @@ func (clh *cloudHypervisor) HotplugRemoveDevice(ctx context.Context, devInfo int
 	ctx, cancel := context.WithTimeout(context.Background(), clhHotPlugAPITimeout*time.Second)
 	defer cancel()
 
+	originalDeviceID := clh.devicesIds[deviceID]
 	remove := *chclient.NewVmRemoveDevice()
-	remove.Id = &deviceID
+	remove.Id = &originalDeviceID
 	_, err := cl.VmRemoveDevicePut(ctx, remove)
 	if err != nil {
 		err = fmt.Errorf("failed to hotplug remove (unplug) device %+v: %s", devInfo, openAPIClientError(err))
 	}
 
+	delete(clh.devicesIds, deviceID)
 	return nil, err
 }
 

src/runtime/virtcontainers/clh_test.go

@@ -384,6 +384,7 @@ func TestCloudHypervisorHotplugAddBlockDevice(t *testing.T) {
 	clh := &cloudHypervisor{}
 	clh.config = clhConfig
 	clh.APIClient = &clhClientMock{}
+	clh.devicesIds = make(map[string]string)
 
 	clh.config.BlockDeviceDriver = config.VirtioBlock
 	err = clh.hotplugAddBlockDevice(&config.BlockDrive{Pmem: false})
@@ -406,6 +407,7 @@ func TestCloudHypervisorHotplugRemoveDevice(t *testing.T) {
 	clh := &cloudHypervisor{}
 	clh.config = clhConfig
 	clh.APIClient = &clhClientMock{}
+	clh.devicesIds = make(map[string]string)
 
 	_, err = clh.HotplugRemoveDevice(context.Background(), &config.BlockDrive{}, BlockDev)
 	assert.NoError(err, "Hotplug remove block device expected no error")