Tweak mirrord container command (#2645)

* Tweak mirrord container command

* Add unit-test for parsing container commands

.github/workflows/ci.yaml

@@ -345,6 +345,8 @@ jobs:
         run: cargo test --target x86_64-unknown-linux-gnu -p mirrord-auth
       - name: mirrord operator UT
         run: cargo test --target x86_64-unknown-linux-gnu -p mirrord-operator --features "crd, client"
+      - name: mirrord cli UT
+        run: cargo test --target x86_64-unknown-linux-gnu -p mirrord
 
   macos_tests:
     runs-on: macos-13

changelog.d/+tweak-container-command.internal.md

@@ -0,0 +1,12 @@
+Update the location of `--` in container command
+```
+mirrord container [options] <docker/podman/nerdctl> run -- ...
+```
+Now will be
+```
+mirrord container [options] -- <docker/podman/nerdctl> run ...
+```
+or simply
+```
+mirrord container [options] <docker/podman/nerdctl> run ...
+```

mirrord/cli/src/config.rs

@@ -9,7 +9,7 @@ use mirrord_operator::setup::OperatorNamespace;
 
 use crate::error::CliError;
 
-#[derive(Parser)]
+#[derive(Debug, Parser)]
 #[command(
     author,
     version,
@@ -23,7 +23,7 @@ pub(super) struct Cli {
     pub(super) commands: Commands,
 }
 
-#[derive(Subcommand)]
+#[derive(Debug, Subcommand)]
 pub(super) enum Commands {
     /// Create and run a new container from an image with mirrord loaded
     Container(Box<ContainerArgs>),
@@ -461,7 +461,7 @@ pub(super) enum DiagnoseCommand {
     },
 }
 
-#[derive(Clone, Copy, Debug, ValueEnum)]
+#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
 /// Runtimes supported by the `mirrord container` command.
 pub(super) enum ContainerRuntime {
     Docker,
@@ -486,6 +486,24 @@ pub(super) struct ContainerArgs {
     /// Parameters to be passed to mirrord.
     pub params: ExecParams,
 
+    /// Container command to be executed
+    #[arg(trailing_var_arg = true)]
+    pub exec: Vec<String>,
+}
+
+impl ContainerArgs {
+    pub fn into_parts(self) -> (RuntimeArgs, ExecParams) {
+        let ContainerArgs { params, exec } = self;
+
+        let runtime_args =
+            RuntimeArgs::parse_from(std::iter::once("mirrord container --".into()).chain(exec));
+
+        (runtime_args, params)
+    }
+}
+
+#[derive(Parser, Debug)]
+pub struct RuntimeArgs {
     /// Which kind of container runtime to use.
     #[arg(value_enum)]
     pub runtime: ContainerRuntime,
@@ -495,13 +513,13 @@ pub(super) struct ContainerArgs {
     pub command: ContainerCommand,
 }
 
-/// Commands for using mirrord with container runtimes.
+/// Supported command for using mirrord with container runtimes.
 #[derive(Subcommand, Debug, Clone)]
 pub(super) enum ContainerCommand {
     /// Execute a `<RUNTIME> run` command with mirrord loaded.
     Run {
         /// Arguments that will be propogated to underlying `<RUNTIME> run` command.
-        #[arg(raw = true)]
+        #[arg(allow_hyphen_values = true, trailing_var_arg = true)]
         runtime_args: Vec<String>,
     },
 }
@@ -513,3 +531,44 @@ impl ContainerCommand {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn runtime_args_parsing() {
+        let command = "mirrord container -t deploy/test podman run -it --rm debian";
+        let result = Cli::parse_from(command.split(' '));
+
+        let Commands::Container(continaer) = result.commands else {
+            panic!("cli command didn't parse into container command, got: {result:#?}")
+        };
+
+        let (runtime_args, _) = continaer.into_parts();
+
+        assert_eq!(runtime_args.runtime, ContainerRuntime::Podman);
+
+        let ContainerCommand::Run { runtime_args } = runtime_args.command;
+
+        assert_eq!(runtime_args, vec!["-it", "--rm", "debian"]);
+    }
+
+    #[test]
+    fn runtime_args_parsing_with_seperator() {
+        let command = "mirrord container -t deploy/test -- podman run -it --rm debian";
+        let result = Cli::parse_from(command.split(' '));
+
+        let Commands::Container(continaer) = result.commands else {
+            panic!("cli command didn't parse into container command, got: {result:#?}")
+        };
+
+        let (runtime_args, _) = continaer.into_parts();
+
+        assert_eq!(runtime_args.runtime, ContainerRuntime::Podman);
+
+        let ContainerCommand::Run { runtime_args } = runtime_args.command;
+
+        assert_eq!(runtime_args, vec!["-it", "--rm", "debian"]);
+    }
+}

mirrord/cli/src/container.rs

@@ -23,7 +23,7 @@ use tokio::process::Command;
 use tracing::Level;
 
 use crate::{
-    config::{ContainerArgs, ContainerCommand},
+    config::{ContainerCommand, ExecParams, RuntimeArgs},
     connection::AGENT_CONNECT_INFO_ENV_KEY,
     container::command_builder::RuntimeCommandBuilder,
     error::{ContainerError, Result},
@@ -155,10 +155,14 @@ async fn create_sidecar_intproxy(
 
 /// Main entry point for the `mirrord container` command.
 /// This spawns: "agent" - "external proxy" - "intproxy sidecar" - "execution container"
-pub(crate) async fn container_command(args: ContainerArgs, watch: drain::Watch) -> Result<()> {
+pub(crate) async fn container_command(
+    runtime_args: RuntimeArgs,
+    exec_params: ExecParams,
+    watch: drain::Watch,
+) -> Result<()> {
     let progress = ProgressTracker::from_env("mirrord container");
 
-    for (name, value) in args.params.as_env_vars()? {
+    for (name, value) in exec_params.as_env_vars()? {
         std::env::set_var(name, value);
     }
 
@@ -244,7 +248,7 @@ pub(crate) async fn container_command(args: ContainerArgs, watch: drain::Watch)
 
     sub_progress.success(None);
 
-    let mut runtime_command = RuntimeCommandBuilder::new(args.runtime);
+    let mut runtime_command = RuntimeCommandBuilder::new(runtime_args.runtime);
 
     if let Ok(console_addr) = std::env::var(MIRRORD_CONSOLE_ADDR_ENV) {
         if console_addr
@@ -308,7 +312,7 @@ pub(crate) async fn container_command(args: ContainerArgs, watch: drain::Watch)
     );
 
     let (binary, binary_args) = runtime_command
-        .with_command(args.command)
+        .with_command(runtime_args.command)
         .into_execvp_args();
 
     let err = execvp(binary, binary_args);

mirrord/cli/src/main.rs

@@ -501,7 +501,10 @@ fn main() -> miette::Result<()> {
             }
             Commands::Teams => teams::navigate_to_intro().await,
             Commands::Diagnose(args) => diagnose_command(*args).await?,
-            Commands::Container(args) => container_command(*args, watch).await?,
+            Commands::Container(args) => {
+                let (runtime_args, exec_params) = args.into_parts();
+                container_command(runtime_args, exec_params, watch).await?
+            }
             Commands::ExternalProxy => external_proxy::proxy(watch).await?,
         };