Merge pull request #537 from runcom/fix-execsync

execsync: rewrite to fix a bug in conmon

conmon/conmon.c

@@ -87,6 +87,7 @@ static char *runtime_path = NULL;
 static char *bundle_path = NULL;
 static char *pid_file = NULL;
 static bool systemd_cgroup = false;
+static char *exec_process_spec = NULL;
 static bool exec = false;
 static char *log_path = NULL;
 static GOptionEntry entries[] =
@@ -98,6 +99,7 @@ static GOptionEntry entries[] =
   { "pidfile", 'p', 0, G_OPTION_ARG_STRING, &pid_file, "PID file", NULL },
   { "systemd-cgroup", 's', 0, G_OPTION_ARG_NONE, &systemd_cgroup, "Enable systemd cgroup manager", NULL },
   { "exec", 'e', 0, G_OPTION_ARG_NONE, &exec, "Exec a command in a running container", NULL },
+  { "exec-process-spec", 0, 0, G_OPTION_ARG_STRING, &exec_process_spec, "Path to the process spec for exec", NULL },
   { "log-path", 'l', 0, G_OPTION_ARG_STRING, &log_path, "Log file path", NULL },
   { NULL }
 };
@@ -356,6 +358,10 @@ int main(int argc, char *argv[])
 		bundle_path = cwd;
 	}
 
+	if (exec && exec_process_spec == NULL) {
+		nexit("Exec process spec path not provided. Use --exec-process-spec");
+	}
+
 	if (pid_file == NULL) {
 		if (snprintf(default_pid_file, sizeof(default_pid_file),
 			     "%s/pidfile-%s", cwd, cid) < 0) {
@@ -458,23 +464,14 @@ int main(int argc, char *argv[])
 	if (terminal)
 		g_string_append_printf(cmd, " --console-socket %s", csname);
 
-	/* Container name comes last. */
-	g_string_append_printf(cmd, " %s", cid);
-
 	/* Set the exec arguments. */
 	if (exec) {
-		/*
+		g_string_append_printf(cmd, " --process %s", exec_process_spec);
-		 * FIXME: This code is broken if argv[1] contains spaces or other
-		 *        similar characters that shells don't like. It's a bit silly
-		 *        that we're doing things inside a shell at all -- this should
-		 *        all be done in arrays.
-		 */
-
-		int i;
-		for (i = 1; i < argc; i++)
-			g_string_append_printf(cmd, " %s", argv[i]);
 	}
 
+	/* Container name comes last. */
+	g_string_append_printf(cmd, " %s", cid);
+
 	/*
 	 * We have to fork here because the current runC API dups the stdio of the
 	 * calling process over the container's fds. This is actually *very bad*

oci/oci.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/Sirupsen/logrus"
 	"github.com/kubernetes-incubator/cri-o/utils"
+	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	"golang.org/x/sys/unix"
 )
 
@@ -329,6 +330,15 @@ func (r *Runtime) ExecSync(c *Container, command []string, timeout int64) (resp
 		os.RemoveAll(logPath)
 	}()
 
+	f, err := ioutil.TempFile("", "exec-process")
+	if err != nil {
+		return nil, ExecSyncError{
+			ExitCode: -1,
+			Err:      err,
+		}
+	}
+	defer os.RemoveAll(f.Name())
+
 	var args []string
 	args = append(args, "-c", c.name)
 	args = append(args, "-r", r.Path(c))
@@ -339,7 +349,27 @@ func (r *Runtime) ExecSync(c *Container, command []string, timeout int64) (resp
 	}
 	args = append(args, "-l", logPath)
 
-	args = append(args, command...)
+	pspec := rspec.Process{
+		Env:  r.conmonEnv,
+		Args: command,
+		Cwd:  "/",
+	}
+	processJSON, err := json.Marshal(pspec)
+	if err != nil {
+		return nil, ExecSyncError{
+			ExitCode: -1,
+			Err:      err,
+		}
+	}
+
+	if err := ioutil.WriteFile(f.Name(), processJSON, 0644); err != nil {
+		return nil, ExecSyncError{
+			ExitCode: -1,
+			Err:      err,
+		}
+	}
+
+	args = append(args, "--exec-process-spec", f.Name())
 
 	cmd := exec.Command(r.conmonPath, args...)
 

test/ctr.bats

@@ -401,6 +401,28 @@ function teardown() {
 	stop_crio
 }
 
+@test "ctr execsync conflicting with conmon flags parsing" {
+	start_crio
+	run crioctl pod run --config "$TESTDATA"/sandbox_config.json
+	echo "$output"
+	[ "$status" -eq 0 ]
+	pod_id="$output"
+	run crioctl ctr create --config "$TESTDATA"/container_redis.json --pod "$pod_id"
+	echo "$output"
+	[ "$status" -eq 0 ]
+	ctr_id="$output"
+	run crioctl ctr start --id "$ctr_id"
+	echo "$output"
+	[ "$status" -eq 0 ]
+	run crioctl ctr execsync --id "$ctr_id" sh -c "echo hello world"
+	echo "$output"
+	[ "$status" -eq 0 ]
+	[[ "$output" =~ "hello world" ]]
+	cleanup_ctrs
+	cleanup_pods
+	stop_crio
+}
+
 @test "ctr execsync" {
 	start_crio
 	run crioctl pod run --config "$TESTDATA"/sandbox_config.json
@@ -511,19 +533,22 @@ function teardown() {
 	run crioctl ctr start --id "$ctr_id"
 	echo "$output"
 	[ "$status" -eq 0 ]
-	run crioctl ctr execsync --id "$ctr_id" "echo hello0 stdout"
+	run crioctl ctr execsync --id "$ctr_id" echo hello0 stdout
 	echo "$output"
 	[ "$status" -eq 0 ]
 	[[ "$output" == *"$(printf "Stdout:\nhello0 stdout")"* ]]
-	run crioctl ctr execsync --id "$ctr_id" "echo hello1 stderr >&2"
+
+	stderrconfig=$(cat "$TESTDATA"/container_config.json | python -c 'import json,sys;obj=json.load(sys.stdin);obj["image"]["image"] = "runcom/stderr-test"; json.dump(obj, sys.stdout)')
+	echo "$stderrconfig" > "$TESTDIR"/container_config_stderr.json
+	run crioctl ctr create --config "$TESTDIR"/container_config_stderr.json --pod "$pod_id"
 	echo "$output"
 	[ "$status" -eq 0 ]
-	[[ "$output" == *"$(printf "Stderr:\nhello1 stderr")"* ]]
+	ctr_id="$output"
-	run crioctl ctr execsync --id "$ctr_id" "echo hello2 stderr >&2; echo hello3 stdout"
+	run crioctl ctr execsync --id "$ctr_id" stderr
 	echo "$output"
 	[ "$status" -eq 0 ]
-	[[ "$output" == *"$(printf "Stderr:\nhello2 stderr")"* ]]
+	[[ "$output" == *"$(printf "Stderr:\nthis goes to stderr")"* ]]
-	[[ "$output" == *"$(printf "Stdout:\nhello3 stdout")"* ]]
+
 	run crioctl pod remove --id "$pod_id"
 	echo "$output"
 	[ "$status" -eq 0 ]

test/helpers.bash

@@ -77,6 +77,16 @@ if ! [ -d "$ARTIFACTS_PATH"/redis-image ]; then
     fi
 fi
 
+# Make sure we have a copy of the runcom/stderr-test image.
+if ! [ -d "$ARTIFACTS_PATH"/stderr-test ]; then
+    mkdir -p "$ARTIFACTS_PATH"/stderr-test
+    if ! "$COPYIMG_BINARY" --import-from=docker://runcom/stderr-test:latest --export-to=dir:"$ARTIFACTS_PATH"/stderr-test --signature-policy="$INTEGRATION_ROOT"/policy.json ; then
+        echo "Error pulling docker://stderr-test"
+        rm -fr "$ARTIFACTS_PATH"/stderr-test
+        exit 1
+    fi
+fi
+
 # Make sure we have a copy of the busybox:latest image.
 if ! [ -d "$ARTIFACTS_PATH"/busybox-image ]; then
     mkdir -p "$ARTIFACTS_PATH"/busybox-image
@@ -159,6 +169,8 @@ function start_crio() {
 	fi
 	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=redis:alpine --import-from=dir:"$ARTIFACTS_PATH"/redis-image --add-name=docker.io/library/redis:alpine --signature-policy="$INTEGRATION_ROOT"/policy.json
 	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=mrunalp/oom --import-from=dir:"$ARTIFACTS_PATH"/oom-image --add-name=docker.io/library/mrunalp/oom --signature-policy="$INTEGRATION_ROOT"/policy.json
+	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=busybox:latest --import-from=dir:"$ARTIFACTS_PATH"/busybox-image --add-name=docker.io/library/busybox:latest --signature-policy="$INTEGRATION_ROOT"/policy.json
+	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=runcom/stderr-test:latest --import-from=dir:"$ARTIFACTS_PATH"/stderr-test --add-name=docker.io/runcom/stderr-test:latest --signature-policy="$INTEGRATION_ROOT"/policy.json
 	"$CRIO_BINARY" --conmon "$CONMON_BINARY" --listen "$CRIO_SOCKET" --cgroup-manager "$CGROUP_MANAGER" --runtime "$RUNTIME_BINARY" --root "$TESTDIR/crio" --runroot "$TESTDIR/crio-run" $STORAGE_OPTS --seccomp-profile "$seccomp" --apparmor-profile "$apparmor" --cni-config-dir "$CRIO_CNI_CONFIG" --signature-policy "$INTEGRATION_ROOT"/policy.json --config /dev/null config >$CRIO_CONFIG
 
 	# Prepare the CNI configuration files, we're running with non host networking by default
@@ -177,15 +189,10 @@ function start_crio() {
 		crioctl image pull redis:alpine
 	fi
 	REDIS_IMAGEID=$(crioctl image status --id=redis:alpine | head -1 | sed -e "s/ID: //g")
-	run crioctl image status --id=busybox
-	if [ "$status" -ne 0 ] ; then
-		crioctl image pull busybox:latest
-	fi
 	run crioctl image status --id=mrunalp/oom
 	if [ "$status" -ne 0 ] ; then
 		  crioctl image pull mrunalp/oom
 	fi
-
 	#
 	#
 	#
@@ -204,6 +211,15 @@ function start_crio() {
 	#
 	#
 	#
+	run crioctl image status --id=runcom/stderr-test
+	if [ "$status" -ne 0 ] ; then
+		crioctl image pull runcom/stderr-test:latest
+	fi
+	STDERR_IMAGEID=$(crioctl image status --id=runcom/stderr-test | head -1 | sed -e "s/ID: //g")
+	run crioctl image status --id=busybox
+	if [ "$status" -ne 0 ] ; then
+		crioctl image pull busybox:latest
+	fi
 	BUSYBOX_IMAGEID=$(crioctl image status --id=busybox | head -1 | sed -e "s/ID: //g")
 }