From d28539cb6dcb7e7f362584007e0c752a82fbdcaf Mon Sep 17 00:00:00 2001
From: Justin Trudell <jtru@meta.com>
Date: Thu, 20 Jul 2023 15:59:10 -0700
Subject: [PATCH] [antlir2] Fix nspawn subtarget mount handling

Summary:
This was a tricky failure to track down, and I don't love the solution so am open to other ideas, but TL;DR:

antlir2_isolate takes a collection of "inputs" and converts them to escpaed `--bind-ro` CLI args (where `:` is significant). We thus can't escape the mount arg from this wrapper binary as we're doing today.

Some options I'd considered:
- Make antlir2_isolate also take "already-escaped" inputs - could work, but would require a fair amount of extra boilerplate for all the `Into<_>` conversions, and complicates the API
- Make the wrapper binary parse its mount args in the same way that systemd-nspawn does (i.e. using optional `:` as delimiter, allowing escaped colons) - requires a fair amount of implementation complexity
- Do what was done here and just explicitly allow space-separated SRC -> DST bind mount mapping arg

I opted to do the latter as it's by far the simplest and matches all known input types to the lib today, but open to ideas.

Test Plan:
For regressions:
```
$ buck2 run "//os_foundation/images/impl/v2/centos8:base.os[nspawn]"
...
[root@subvol-compilecompile-448a91e009bf978a ~]# ls /
bin  boot  data  dev  etc  home  lib  lib64  media  mnt  opt  proc  root  run  sbin  srv  sys  tmp  usr  var
```

also tested in above diffs in stack using src -> dst mounts.

Reviewed By: vmagro

Differential Revision: D47533620

fbshipit-source-id: 4be3f4887f05a10f8b1664a24ef147933bfad886
---
 antlir/antlir2/bzl/image/layer.bzl          |  2 +-
 antlir/antlir2/bzl/image/mounts.bzl         | 10 ++--------
 antlir/antlir2/nspawn_in_subvol/src/main.rs | 21 +++++++++++++++++----
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/antlir/antlir2/bzl/image/layer.bzl b/antlir/antlir2/bzl/image/layer.bzl
index efc310964fe..9edafecf135 100644
--- a/antlir/antlir2/bzl/image/layer.bzl
+++ b/antlir/antlir2/bzl/image/layer.bzl
@@ -67,7 +67,7 @@ def _nspawn_sub_target(nspawn_binary: "dependency", subvol: "artifact", mounts:
     if REPO_CFG.artifacts_require_repo:
         dev_mode_args = cmd_args(
             "--artifacts-require-repo",
-            cmd_args([["--bind-ro", x] for x in REPO_CFG.host_mounts_for_repo_artifacts]),
+            cmd_args([cmd_args("--bind-mount-ro", p, p) for p in REPO_CFG.host_mounts_for_repo_artifacts]),
         )
     return [
         DefaultInfo(),
diff --git a/antlir/antlir2/bzl/image/mounts.bzl b/antlir/antlir2/bzl/image/mounts.bzl
index e184a6173bc..66af30f31ee 100644
--- a/antlir/antlir2/bzl/image/mounts.bzl
+++ b/antlir/antlir2/bzl/image/mounts.bzl
@@ -52,14 +52,8 @@ def all_mounts(
 
 def nspawn_mount_args(mount: mount_record.type) -> cmd_args.type:
     if mount.layer:
-        return cmd_args(
-            "--bind-ro",
-            cmd_args(mount.layer.src.subvol_symlink, format = "{{}}:{}".format(mount.layer.mountpoint.replace(":", "\\:"))),
-        )
+        return cmd_args("--bind-mount-ro", mount.layer.src.subvol_symlink, mount.layer.mountpoint)
     elif mount.host:
-        return cmd_args(
-            "--bind-ro",
-            mount.host.src + ":" + mount.host.mountpoint.replace(":", "\\:"),
-        )
+        return cmd_args("--bind-mount-ro", mount.host.src, mount.host.mountpoint)
     else:
         fail("neither host nor layer mount, what is it?!")
diff --git a/antlir/antlir2/nspawn_in_subvol/src/main.rs b/antlir/antlir2/nspawn_in_subvol/src/main.rs
index 6720c528c7e..c06507acfa0 100644
--- a/antlir/antlir2/nspawn_in_subvol/src/main.rs
+++ b/antlir/antlir2/nspawn_in_subvol/src/main.rs
@@ -5,13 +5,14 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-use std::collections::HashSet;
+use std::collections::HashMap;
 use std::os::unix::process::CommandExt;
 use std::path::PathBuf;
 
 use antlir2_isolate::isolate;
 use antlir2_isolate::InvocationType;
 use antlir2_isolate::IsolationContext;
+use anyhow::anyhow;
 use anyhow::Context;
 use clap::Parser;
 use tracing_subscriber::filter;
@@ -22,8 +23,9 @@ use tracing_subscriber::prelude::*;
 struct Args {
     #[clap(long)]
     subvol: PathBuf,
-    #[clap(long)]
-    bind_ro: Vec<PathBuf>,
+    /// `--bind-mount-ro src dst` creates an RO bind-mount of src to dst in the subvol
+    #[clap(long, num_args = 2)]
+    bind_mount_ro: Vec<PathBuf>,
     #[clap(long)]
     artifacts_require_repo: bool,
 }
@@ -50,9 +52,20 @@ fn main() -> anyhow::Result<()> {
     )
     .context("while looking for repo root")?;
 
+    // antlir2_isolate re-parses these into --bind-ro args and escapes any colons, so we
+    // instead take an explicit pair to not have to deal with the added complexity of
+    // de-and-re-serializing.
+    let bind_ro_inputs = args
+        .bind_mount_ro
+        .chunks(2)
+        .map(|pair| match pair {
+            [src, dst] => Ok((dst.clone(), src.clone())),
+            _ => Err(anyhow!("Unrecognized --mount arg: {:?}", pair)),
+        })
+        .collect::<anyhow::Result<HashMap<_, _>>>()?;
     let mut cmd_builder = IsolationContext::builder(args.subvol);
     cmd_builder
-        .inputs(args.bind_ro.into_iter().collect::<HashSet<_>>())
+        .inputs(bind_ro_inputs)
         .ephemeral(true)
         .invocation_type(InvocationType::Pid2Interactive);
     if args.artifacts_require_repo {