Merge branch 'main' into issue/1898/non-blocking-connect

changelog.d/2518.changed.md

@@ -0,0 +1 @@
+Enable readlink hook by default.

mirrord/agent/src/file.rs

@@ -412,7 +412,7 @@ impl FileManager {
     }
 
     /// Handles our `readlink_detour` with [`std::fs::read_link`].
-    #[tracing::instrument(level = "trace", skip_all)]
+    #[tracing::instrument(level = Level::TRACE, skip_all)]
     pub(crate) fn read_link(&mut self, path: PathBuf) -> RemoteResult<ReadLinkFileResponse> {
         let path = path
             .strip_prefix("/")

mirrord/config/src/lib.rs

@@ -524,6 +524,14 @@ impl LayerConfig {
         self.feature.network.dns.verify(context)?;
         self.feature.network.outgoing.verify(context)?;
 
+        if self.experimental.readlink {
+            context.add_warning(
+                "experimental.readlink config has been deprecated, and `readlink` is now\
+                    enabled by default! You may remove it from your config."
+                    .into(),
+            );
+        }
+
         Ok(())
     }
 }

mirrord/intproxy/src/proxies/simple.rs

@@ -217,8 +217,34 @@ impl BackgroundTask for SimpleProxy {
                             .await;
                     }
                 }
-                SimpleProxyMessage::FileReq(message_id, session_id, req) => {
-                    self.file_reqs.insert(message_id, session_id);
+                // TODO(alex): We can remove this case when users are up-to-date for `readlink`.
+                SimpleProxyMessage::FileReq(
+                    message_id,
+                    layer_id,
+                    req @ FileRequest::ReadLink(_),
+                ) => {
+                    if protocol_version
+                        .as_ref()
+                        .is_some_and(|version| READDIR_BATCH_VERSION.matches(version))
+                    {
+                        self.file_reqs.insert(message_id, layer_id);
+                        message_bus
+                            .send(ProxyMessage::ToAgent(ClientMessage::FileRequest(req)))
+                            .await;
+                    } else {
+                        message_bus
+                            .send(ToLayer {
+                                message_id,
+                                message: ProxyToLayerMessage::File(FileResponse::ReadLink(Err(
+                                    ResponseError::NotImplemented,
+                                ))),
+                                layer_id,
+                            })
+                            .await;
+                    }
+                }
+                SimpleProxyMessage::FileReq(message_id, layer_id, req) => {
+                    self.file_reqs.insert(message_id, layer_id);
                     message_bus
                         .send(ProxyMessage::ToAgent(ClientMessage::FileRequest(req)))
                         .await;

mirrord/layer/src/detour.rs

@@ -209,6 +209,12 @@ pub(crate) enum Bypass {
 
     /// DNS query should be done locally.
     LocalDns,
+
+    /// Operation is not implemented, but it should not be a hard error.
+    ///
+    /// Useful for operations that are version gated, and we want to bypass when the protocol
+    /// doesn't support them.
+    NotImplemented,
 }
 
 impl Bypass {

mirrord/layer/src/file/ops.rs

@@ -5,12 +5,16 @@ use std::{env, ffi::CString, io::SeekFrom, os::unix::io::RawFd, path::PathBuf};
 #[cfg(target_os = "linux")]
 use libc::{c_char, statx, statx_timestamp};
 use libc::{c_int, iovec, unlink, AT_FDCWD};
-use mirrord_protocol::file::{
-    OpenFileRequest, OpenFileResponse, OpenOptionsInternal, ReadFileResponse, ReadLinkFileRequest,
-    ReadLinkFileResponse, SeekFileResponse, WriteFileResponse, XstatFsResponse, XstatResponse,
+use mirrord_protocol::{
+    file::{
+        OpenFileRequest, OpenFileResponse, OpenOptionsInternal, ReadFileResponse,
+        ReadLinkFileRequest, ReadLinkFileResponse, SeekFileResponse, WriteFileResponse,
+        XstatFsResponse, XstatResponse,
+    },
+    ResponseError,
 };
 use rand::distributions::{Alphanumeric, DistString};
-use tracing::{error, trace};
+use tracing::{error, trace, Level};
 
 use super::{hooks::FN_OPEN, open_dirs::OPEN_DIRS, *};
 #[cfg(target_os = "linux")]
@@ -295,23 +299,21 @@ pub(crate) fn pread(local_fd: RawFd, buffer_size: u64, offset: u64) -> Detour<Re
 }
 
 /// Resolves the symbolic link `path`.
-///
-/// Bypassed if the `experimental.readlink` config is not set to `true`.
-#[mirrord_layer_macro::instrument(level = "trace", ret)]
+#[mirrord_layer_macro::instrument(level = Level::TRACE, ret)]
 pub(crate) fn read_link(path: Detour<PathBuf>) -> Detour<ReadLinkFileResponse> {
-    if crate::setup().experimental().readlink {
-        let path = remap_path!(path?);
+    let path = remap_path!(path?);
 
-        check_relative_paths!(path);
+    check_relative_paths!(path);
 
-        ensure_not_ignored!(path, false);
+    ensure_not_ignored!(path, false);
 
-        let requesting_path = ReadLinkFileRequest { path };
-        let response = common::make_proxy_request_with_response(requesting_path)??;
+    let requesting_path = ReadLinkFileRequest { path };
 
-        Detour::Success(response)
-    } else {
-        None?
+    // `NotImplemented` error here means that the protocol doesn't support it.
+    match common::make_proxy_request_with_response(requesting_path)? {
+        Ok(response) => Detour::Success(response),
+        Err(ResponseError::NotImplemented) => Detour::Bypass(Bypass::NotImplemented),
+        Err(fail) => Detour::Error(fail.into()),
     }
 }
 

mirrord/layer/tests/readlink.rs

@@ -10,22 +10,11 @@ pub use common::*;
 #[rstest]
 #[tokio::test]
 #[timeout(Duration::from_secs(60))]
-async fn readlink(
-    #[values(Some("readlink.json"))] with_config: Option<&str>,
-    dylib_path: &Path,
-    config_dir: &Path,
-) {
+async fn readlink(dylib_path: &Path) {
     let application = Application::ReadLink;
 
-    let config = with_config.map(|config| {
-        let mut config_path = config_dir.to_path_buf();
-        config_path.push(config);
-        config_path
-    });
-    let config = config.as_ref().map(|path_buf| path_buf.to_str().unwrap());
-
     let (mut test_process, mut intproxy) = application
-        .start_process_with_layer(dylib_path, Default::default(), config)
+        .start_process_with_layer(dylib_path, Default::default(), None)
         .await;
 
     println!("waiting for file request.");