Add policy for file ops. (#2997)

* Add policy to exclude env vars. * docs * changelog * fix tests * Make policy optional for backwards compat. * typo * other typo * default serde and more explicit docs * fix mismatching policy * make it look more like config * make it pub * fix test * better docs Co-authored-by: Michał Smolarek <[email protected]> * better docs due Co-authored-by: Michał Smolarek <[email protected]> * rustfmt * Add policy for file ops. * no exclude * update protocol with open_local_version * bump protocol * lint test * docs * fix test * change min protocol version * e2e test for fspolicy * Ignore fs policy test/ * namespaced test * hopefully fixed policy test * the children get to live * fix test policy name Co-authored-by: t4lz <[email protected]> * fix go fs test * remove read_write * add newline to python test * changelog --------- Co-authored-by: Michał Smolarek <[email protected]> Co-authored-by: t4lz <[email protected]>
metalbear-co · Jan 9, 2025 · a51e981 · a51e981
1 parent 2ec5ab3
commit a51e981
Show file tree

Hide file tree

Showing 16 changed files with 437 additions and 174 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/changelog.d/+104-policy-fs.added.md b/changelog.d/+104-policy-fs.added.md
@@ -0,0 +1 @@
+Add policy to control file ops.
diff --git a/mirrord/layer/src/detour.rs b/mirrord/layer/src/detour.rs
@@ -215,6 +215,10 @@ pub(crate) enum Bypass {
     /// Useful for operations that are version gated, and we want to bypass when the protocol
     /// doesn't support them.
     NotImplemented,
+
+    /// File `open` (any `open`-ish operation) was forced to be local, instead of remote, most
+    /// likely due to an operator fs policy.
+    OpenLocal,
 }
 
 impl Bypass {

diff --git a/mirrord/layer/src/error.rs b/mirrord/layer/src/error.rs
@@ -248,6 +248,7 @@ impl From<HookError> for i64 {
             HookError::BincodeEncode(_) => libc::EINVAL,
             HookError::ResponseError(response_fail) => match response_fail {
                 ResponseError::IdsExhausted(_) => libc::ENOMEM,
+                ResponseError::OpenLocal => libc::ENOENT,
                 ResponseError::NotFound(_) => libc::ENOENT,
                 ResponseError::NotDirectory(_) => libc::ENOTDIR,
                 ResponseError::NotFile(_) => libc::EISDIR,

diff --git a/mirrord/layer/src/file/ops.rs b/mirrord/layer/src/file/ops.rs
@@ -206,7 +206,12 @@ pub(crate) fn open(path: Detour<PathBuf>, open_options: OpenOptionsInternal) ->
 
     ensure_not_ignored!(path, open_options.is_write());
 
-    let OpenFileResponse { fd: remote_fd } = RemoteFile::remote_open(path.clone(), open_options)?;
+    let OpenFileResponse { fd: remote_fd } = RemoteFile::remote_open(path.clone(), open_options)
+        .or_else(|fail| match fail {
+            // The operator has a policy that matches this `path` as local-only.
+            HookError::ResponseError(ResponseError::OpenLocal) => Detour::Bypass(Bypass::OpenLocal),
+            other => Detour::Error(other),
+        })?;
 
     // TODO: Need a way to say "open a directory", right now `is_dir` always returns false.
     // This requires having a fake directory name (`/fake`, for example), instead of just converting

diff --git a/mirrord/operator/src/crd/policy.rs b/mirrord/operator/src/crd/policy.rs
@@ -58,6 +58,11 @@ pub struct MirrordPolicySpec {
     /// target.
     #[serde(default)]
     pub env: EnvPolicy,
+
+    /// Overrides fs ops behaviour, granting control over them to the operator policy, instead of
+    /// the user config.
+    #[serde(default)]
+    pub fs: FsPolicy,
 }
 
 /// Custom cluster-wide resource for policies that limit what mirrord features users can use.
@@ -90,6 +95,11 @@ pub struct MirrordClusterPolicySpec {
     /// target.
     #[serde(default)]
     pub env: EnvPolicy,
+
+    /// Overrides fs ops behaviour, granting control over them to the operator policy, instead of
+    /// the user config.
+    #[serde(default)]
+    pub fs: FsPolicy,
 }
 
 /// Policy for controlling environment variables access from mirrord instances.
@@ -104,9 +114,33 @@ pub struct EnvPolicy {
     /// Variable names can be matched using `*` and `?` where `?` matches exactly one occurrence of
     /// any character and `*` matches arbitrary many (including zero) occurrences of any character,
     /// e.g. `DATABASE_*` will match `DATABASE_URL` and `DATABASE_PORT`.
+    #[serde(default)]
     pub exclude: HashSet<String>,
 }
 
+/// File operations policy that mimics the mirrord fs config.
+///
+/// Allows the operator control over remote file ops behaviour, overriding what the user has set in
+/// their mirrord config file, if it matches something in one of the lists (regex sets) of this
+/// struct.
+#[derive(Clone, Default, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)]
+#[serde(rename_all = "kebab-case")]
+pub struct FsPolicy {
+    /// The file can only be opened in read-only mode, otherwise the operator returns an IO error.
+    #[serde(default)]
+    pub read_only: HashSet<String>,
+
+    /// The file cannot be opened in the remote target.
+    ///
+    /// `open` calls that match this are forced to be opened in the local user's machine.
+    #[serde(default)]
+    pub local: HashSet<String>,
+
+    /// Any file that matches this returns a file not found error from the operator.
+    #[serde(default)]
+    pub not_found: HashSet<String>,
+}
+
 #[test]
 fn check_one_api_group() {
     use kube::Resource;

diff --git a/mirrord/protocol/Cargo.toml b/mirrord/protocol/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "mirrord-protocol"
-version = "1.13.2"
+version = "1.13.3"
 authors.workspace = true
 description.workspace = true
 documentation.workspace = true

diff --git a/mirrord/protocol/src/error.rs b/mirrord/protocol/src/error.rs
@@ -67,6 +67,9 @@ pub enum ResponseError {
 
     #[error("Failed stripping path with `{0}`!")]
     StripPrefix(String),
+
+    #[error("File has to be opened locally!")]
+    OpenLocal,
 }
 
 impl From<StripPrefixError> for ResponseError {

diff --git a/mirrord/protocol/src/file.rs b/mirrord/protocol/src/file.rs
@@ -25,6 +25,9 @@ pub static READDIR_BATCH_VERSION: LazyLock<VersionReq> =
 pub static MKDIR_VERSION: LazyLock<VersionReq> =
     LazyLock::new(|| ">=1.13.0".parse().expect("Bad Identifier"));
 
+pub static OPEN_LOCAL_VERSION: LazyLock<VersionReq> =
+    LazyLock::new(|| ">=1.13.3".parse().expect("Bad Identifier"));
+
 /// Internal version of Metadata across operating system (macOS, Linux)
 /// Only mutual attributes
 #[derive(Encode, Decode, Debug, PartialEq, Clone, Copy, Eq, Default)]

diff --git a/tests/go-e2e-dir/main.go b/tests/go-e2e-dir/main.go
@@ -15,13 +15,20 @@ func main() {
 		os.Exit(-1)
 	}
 	fmt.Printf("DirEntries: %s\n", dir)
+
 	// `os.ReadDir` does not include `.` and `..`.
-	if len(dir) != 2 {
+	if len(dir) < 2 {
 		os.Exit(-1)
 	}
-	// `os.ReadDir` sorts the result by file name.
-	if dir[0].Name() != "app.py" || dir[1].Name() != "test.txt" {
-		os.Exit(-1)
+
+	// Iterate over the files in this dir, exiting if it's not an expected file name.
+	for i := 0; i < len(dir); i++ {
+		dirName := dir[i].Name()
+
+		if dirName != "app.py" && dirName != "test.txt" && dirName != "file.local" && dirName != "file.not-found" && dirName != "file.read-only" && dirName != "file.read-write" {
+			os.Exit(-1)
+		}
+
 	}
 
 	err = os.Mkdir("/app/test_mkdir", 0755)

diff --git a/tests/node-e2e/fspolicy/test_operator_fs_policy.mjs b/tests/node-e2e/fspolicy/test_operator_fs_policy.mjs
@@ -0,0 +1,54 @@
+import fs from 'fs';
+
+fs.open("/app/file.local", (fail, fd) => {
+  console.log(`open file.local ${fd}`);
+  if (fd) {
+    console.log(`SUCCESS /app/file.local ${fd}`);
+  }
+
+  if (fail) {
+    console.error(`FAIL /app/file.local ${fail}`);
+  }
+});
+
+fs.open("/app/file.not-found", (fail, fd) => {
+  console.log(`open file.not-found ${fd}`);
+  if (fd) {
+    console.log(`SUCCESS /app/file.not-found ${fd}`);
+  }
+
+  if (fail) {
+    console.error(`FAIL /app/file.not-found ${fail}`);
+  }
+});
+
+fs.open("/app/file.read-only", (fail, fd) => {
+  if (fd) {
+    console.log(`SUCCESS /app/file.read-only ${fd}`);
+  }
+
+  if (fail) {
+    console.error(`FAIL /app/file.read-only ${fail}`);
+  }
+});
+
+fs.open("/app/file.read-only", "r+", (fail, fd) => {
+  if (fd) {
+    console.log(`SUCCESS r+ /app/file.read-only ${fd}`);
+  }
+
+  if (fail) {
+    console.error(`FAIL r+ /app/file.read-only ${fail}`);
+  }
+});
+
+fs.open("/app/file.read-write", "r+", (fail, fd) => {
+  if (fd) {
+    console.log(`SUCCESS /app/file.read-write ${fd}`);
+  }
+
+  if (fail) {
+    console.error(`FAIL /app/file.read-write ${fail}`);
+  }
+});
+
diff --git a/tests/python-e2e/files_ro.py b/tests/python-e2e/files_ro.py
@@ -3,7 +3,7 @@
 import uuid
 import unittest
 
-TEXT = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
+TEXT = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.\n"
 
 
 class FileOpsTest(unittest.TestCase):
@@ -22,4 +22,4 @@ def test_read_only(self):
 
 
 if __name__ == "__main__":
-    unittest.main()
+    unittest.main()
diff --git a/tests/python-e2e/ops.py b/tests/python-e2e/ops.py
@@ -2,7 +2,7 @@
 import uuid
 import unittest
 
-TEXT = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
+TEXT = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.\n"
 
 
 class FileOpsTest(unittest.TestCase):

diff --git a/tests/src/operator/policies.rs b/tests/src/operator/policies.rs
@@ -18,6 +18,8 @@ use crate::utils::{
     config_dir, kube_client, service, Application, KubeService, ResourceGuard, TestProcess,
 };
 
+mod fs;
+
 /// Guard that deletes a mirrord policy when dropped.
 struct PolicyGuard {
     _inner: ResourceGuard,
@@ -128,6 +130,7 @@ fn block_steal_without_qualifiers() -> PolicyTestCase {
                 selector: None,
                 block: vec![BlockedFeature::Steal],
                 env: Default::default(),
+                fs: Default::default(),
             },
         ),
         service_b_can_steal: No,
@@ -147,6 +150,7 @@ fn block_steal_with_path_pattern() -> PolicyTestCase {
                 selector: None,
                 block: vec![BlockedFeature::Steal],
                 env: Default::default(),
+                fs: Default::default(),
             },
         ),
         service_b_can_steal: EvenWithoutFilter,
@@ -166,6 +170,7 @@ fn block_unfiltered_steal_with_path_pattern() -> PolicyTestCase {
                 selector: None,
                 block: vec![BlockedFeature::StealWithoutFilter],
                 env: Default::default(),
+                fs: Default::default(),
             },
         ),
         service_b_can_steal: EvenWithoutFilter,
@@ -185,6 +190,7 @@ fn block_unfiltered_steal_with_deployment_path_pattern() -> PolicyTestCase {
                 selector: None,
                 block: vec![BlockedFeature::StealWithoutFilter],
                 env: Default::default(),
+                fs: Default::default(),
             },
         ),
         service_a_can_steal: OnlyWithFilter,
@@ -210,6 +216,7 @@ fn block_steal_with_label_selector() -> PolicyTestCase {
                 }),
                 block: vec![BlockedFeature::Steal],
                 env: Default::default(),
+                fs: Default::default(),
             },
         ),
         service_b_can_steal: EvenWithoutFilter,
@@ -236,6 +243,7 @@ fn block_steal_with_unmatching_policy() -> PolicyTestCase {
                 }),
                 block: vec![BlockedFeature::Steal],
                 env: Default::default(),
+                fs: Default::default(),
             },
         ),
         service_b_can_steal: EvenWithoutFilter,
@@ -377,6 +385,7 @@ pub async fn create_cluster_policy_and_try_to_mirror(
                 selector: None,
                 block: vec![BlockedFeature::Mirror],
                 env: Default::default(),
+                fs: Default::default(),
             },
         ),
     )

diff --git a/tests/src/operator/policies/fs.rs b/tests/src/operator/policies/fs.rs
@@ -0,0 +1,87 @@
+use std::{collections::HashSet, time::Duration};
+
+use mirrord_operator::crd::policy::{FsPolicy, MirrordPolicy, MirrordPolicySpec};
+use rstest::{fixture, rstest};
+
+use crate::{
+    operator::policies::PolicyGuard,
+    utils::{kube_client, service, Application, KubeService},
+};
+
+#[fixture]
+async fn fs_service(#[future] kube_client: kube::Client) -> KubeService {
+    let namespace = format!("e2e-tests-fs-policies-{}", crate::utils::random_string());
+
+    service(
+        &namespace,
+        "NodePort",
+        "ghcr.io/metalbear-co/mirrord-pytest:latest",
+        "fs-policy-e2e-test-service",
+        false,
+        kube_client,
+    )
+    .await
+}
+
+#[rstest]
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+#[timeout(Duration::from_secs(60))]
+pub async fn create_cluster_fs_policy_and_try_file_operations(
+    #[future] service: KubeService,
+    #[future] kube_client: kube::Client,
+) {
+    let kube_client = kube_client.await;
+    let service = service.await;
+
+    // Create policy, delete it when test exits.
+    let _policy_guard = PolicyGuard::namespaced(
+        kube_client,
+        &MirrordPolicy::new(
+            "e2e-test-fs-policy-with-path-pattern",
+            MirrordPolicySpec {
+                target_path: Some("fs_policy_e2e-test-*".into()),
+                selector: None,
+                block: Default::default(),
+                env: Default::default(),
+                fs: FsPolicy {
+                    read_only: HashSet::from_iter(vec!["file.read-only".to_string()]),
+                    local: HashSet::from_iter(vec!["file.local".to_string()]),
+                    not_found: HashSet::from_iter(vec!["file.not-found".to_string()]),
+                },
+            },
+        ),
+        &service.namespace,
+    )
+    .await;
+
+    let application = Application::NodeFsPolicy;
+    println!("Running mirrord {application:?} against {}", &service.name);
+
+    let mut test_process = application
+        .run(
+            &service.target,
+            Some(&service.namespace),
+            Some(vec!["--fs-mode=write"]),
+            None,
+        )
+        .await;
+
+    test_process.wait_assert_success().await;
+
+    test_process
+        .assert_stderr_contains("FAIL /app/file.local")
+        .await;
+    test_process
+        .assert_stderr_contains("FAIL /app/file.not-found")
+        .await;
+    test_process
+        .assert_stderr_contains("FAIL r+ /app/file.read-only")
+        .await;
+
+    test_process
+        .assert_stdout_contains("SUCCESS /app/file.read-only")
+        .await;
+    test_process
+        .assert_stdout_contains("SUCCESS /app/file.read-write")
+        .await;
+}