create: improve parser for --healthcheck-command

Fix Docker CLI compatibility issue: the "--healthcheck-command" option value should not be split but instead be passed as single string to "CMD-SHELL", i.e. "/bin/sh -c <opt>". On the other hand implement the same extension as is already available for "--entrypoint", i.e. allow the option value to be a JSON array of strings. This will make life easier for tools like podman-compose. Updated "--healthcheck-command" option values in tests accordingly. Continuation of containers#3455 & containers#3507 Signed-off-by: Stefan Becker <[email protected]>
stefanb2 · Jul 14, 2019 · e4cba7d · e4cba7d
1 parent c59d08b
commit e4cba7d
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 14 deletions.
diff --git a/cmd/podman/shared/create.go b/cmd/podman/shared/create.go
@@ -26,7 +26,6 @@ import (
 	"github.com/docker/docker/pkg/signal"
 	"github.com/docker/go-connections/nat"
 	"github.com/docker/go-units"
-	"github.com/google/shlex"
 	"github.com/opencontainers/selinux/go-selinux/label"
 	"github.com/opentracing/opentracing-go"
 	"github.com/pkg/errors"
@@ -788,9 +787,12 @@ func makeHealthCheckFromCli(c *GenericCLIResults) (*manifest.Schema2HealthConfig
 		return nil, errors.New("Must define a healthcheck command for all healthchecks")
 	}
 
-	cmd, err := shlex.Split(inCommand)
+	// first try to parse option value as JSON array of strings...
+	cmd := []string{}
+	err := json.Unmarshal([]byte(inCommand), &cmd)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to parse healthcheck command")
+		// ...otherwise pass it to "/bin/sh -c" inside the container
+		cmd = []string{"CMD-SHELL", inCommand}
 	}
 	hc := manifest.Schema2HealthConfig{
 		Test: cmd,

diff --git a/docs/podman-create.1.md b/docs/podman-create.1.md
@@ -272,12 +272,15 @@ The following example maps uids 0-2000 in the container to the uids 30000-31999
 
 Add additional groups to run as
 
-**--healthcheck-command**=*command*
+**--healthcheck-command**=*"command"* | *'["command", "arg1", ...]'*
 
 Set or alter a healthcheck command for a container. The command is a command to be executed inside your
 container that determines your container health.  The command is required for other healthcheck options
 to be applied.  A value of `none` disables existing healthchecks.
 
+Multiple options can be passed in the form of a JSON array; otherwise, the command will be interpreted
+as an argument to `/bin/sh -c`.
+
 **--healthcheck-interval**=*interval*
 
 Set an interval for the healthchecks (a value of `disable` results in no automatic timer setup) (default "30s")

diff --git a/docs/podman-run.1.md b/docs/podman-run.1.md
@@ -279,12 +279,15 @@ The example maps gids 0-2000 in the container to the gids 30000-31999 on the hos
 
 Add additional groups to run as
 
-**--healthcheck-command**=*command*
+**--healthcheck-command**=*"command"* | *'["command", "arg1", ...]'*
 
 Set or alter a healthcheck command for a container. The command is a command to be executed inside your
 container that determines your container health.  The command is required for other healthcheck options
 to be applied.  A value of `none` disables existing healthchecks.
 
+Multiple options can be passed in the form of a JSON array; otherwise, the command will be interpreted
+as an argument to `/bin/sh -c`.
+
 **--healthcheck-interval**=*interval*
 
 Set an interval for the healthchecks (a value of `disable` results in no automatic timer setup) (default "30s")

diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go
@@ -375,7 +375,7 @@ func (p *PodmanTestIntegration) RunNginxWithHealthCheck(name string) (*PodmanSes
 	if name != "" {
 		podmanArgs = append(podmanArgs, "--name", name)
 	}
-	podmanArgs = append(podmanArgs, "-dt", "-P", "--healthcheck-command", "CMD-SHELL curl http://localhost/", nginx)
+	podmanArgs = append(podmanArgs, "-dt", "-P", "--healthcheck-command", "curl http://localhost/", nginx)
 	session := p.Podman(podmanArgs)
 	session.WaitWithDefaultTimeout()
 	return session, session.OutputToString()

diff --git a/test/e2e/healthcheck_run_test.go b/test/e2e/healthcheck_run_test.go
@@ -95,15 +95,15 @@ var _ = Describe("Podman healthcheck run", func() {
 	})
 
 	It("podman healthcheck should be starting", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-retries", "2", "--healthcheck-command", "\"CMD-SHELL ls /foo || exit 1\"", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-retries", "2", "--healthcheck-command", "ls /foo || exit 1", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 		inspect := podmanTest.InspectContainer("hc")
 		Expect(inspect[0].State.Healthcheck.Status).To(Equal("starting"))
 	})
 
 	It("podman healthcheck failed checks in start-period should not change status", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-start-period", "2m", "--healthcheck-retries", "2", "--healthcheck-command", "\"CMD-SHELL ls /foo || exit 1\"", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-start-period", "2m", "--healthcheck-retries", "2", "--healthcheck-command", "ls /foo || exit 1", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 
@@ -124,7 +124,7 @@ var _ = Describe("Podman healthcheck run", func() {
 	})
 
 	It("podman healthcheck failed checks must reach retries before unhealthy ", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-retries", "2", "--healthcheck-command", "\"CMD-SHELL ls /foo || exit 1\"", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-retries", "2", "--healthcheck-command", "ls /foo || exit 1", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 
@@ -145,7 +145,7 @@ var _ = Describe("Podman healthcheck run", func() {
 	})
 
 	It("podman healthcheck good check results in healthy even in start-period", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-start-period", "2m", "--healthcheck-retries", "2", "--healthcheck-command", "\"CMD-SHELL\" \"ls\" \"||\" \"exit\" \"1\"", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-start-period", "2m", "--healthcheck-retries", "2", "--healthcheck-command", "ls || exit 1", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 
@@ -158,7 +158,7 @@ var _ = Describe("Podman healthcheck run", func() {
 	})
 
 	It("podman healthcheck single healthy result changes failed to healthy", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-retries", "2", "--healthcheck-command", "\"CMD-SHELL\" \"ls\" \"/foo\" \"||\" \"exit\" \"1\"", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--healthcheck-retries", "2", "--healthcheck-command", "ls /foo || exit 1", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 

diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go
@@ -750,21 +750,21 @@ USER mail`
 	})
 
 	It("podman run with bad healthcheck retries", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--healthcheck-command", "foo", "--healthcheck-retries", "0", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--healthcheck-command", "[\"foo\"]", "--healthcheck-retries", "0", ALPINE, "top"})
 		session.Wait()
 		Expect(session.ExitCode()).ToNot(Equal(0))
 		Expect(session.ErrorToString()).To(ContainSubstring("healthcheck-retries must be greater than 0"))
 	})
 
 	It("podman run with bad healthcheck timeout", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--healthcheck-command", "foo", "--healthcheck-timeout", "0s", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--healthcheck-command", "[\"foo\"]", "--healthcheck-timeout", "0s", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).ToNot(Equal(0))
 		Expect(session.ErrorToString()).To(ContainSubstring("healthcheck-timeout must be at least 1 second"))
 	})
 
 	It("podman run with bad healthcheck start-period", func() {
-		session := podmanTest.Podman([]string{"run", "-dt", "--healthcheck-command", "foo", "--healthcheck-start-period", "-1s", ALPINE, "top"})
+		session := podmanTest.Podman([]string{"run", "-dt", "--healthcheck-command", "[\"foo\"]", "--healthcheck-start-period", "-1s", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).ToNot(Equal(0))
 		Expect(session.ErrorToString()).To(ContainSubstring("healthcheck-start-period must be 0 seconds or greater"))