Fix overwriting the Healthcheck configuration from the image

If the --health-cmd flag is not specified, other flags such as --health-interval, --health-timeout, --health-retries, and --health-start-period are ignored if the image contains a Healthcheck. This makes it impossible to modify these Healthcheck configuration when a container is created.

Fixes: #20212
Fixes: https://issues.redhat.com/browse/RUN-2629

Signed-off-by: Jan Rodák <[email protected]>

Author: Honny1

cmd/podman/common/create.go

@@ -1,9 +1,13 @@
 package common
 
 import (
+	"errors"
+	"time"
+
 	"github.com/containers/common/pkg/auth"
 	"github.com/containers/common/pkg/completion"
 	commonFlag "github.com/containers/common/pkg/flag"
+	"github.com/containers/image/v5/manifest"
 	"github.com/containers/podman/v5/cmd/podman/registry"
 	"github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/pkg/domain/entities"
@@ -1053,3 +1057,43 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 	)
 	_ = cmd.RegisterFlagCompletionFunc(blkioWeightDeviceFlagName, completion.AutocompleteDefault)
 }
+
+func GetHealthCheckOverrideConfig(cmd *cobra.Command, vals *entities.ContainerCreateOptions) (*manifest.Schema2HealthConfig, error) {
+	healthcheck := manifest.Schema2HealthConfig{}
+
+	if cmd.Flags().Changed("health-interval") {
+		if vals.HealthInterval == "disable" {
+			vals.HealthInterval = "-1s"
+		}
+		hct, err := time.ParseDuration(vals.HealthInterval)
+		if err != nil {
+			return nil, err
+		}
+		healthcheck.Interval = hct
+	}
+	if cmd.Flags().Changed("health-retries") {
+		healthcheck.Retries = int(vals.HealthRetries)
+	}
+	if cmd.Flags().Changed("health-timeout") {
+		hct, err := time.ParseDuration(vals.HealthTimeout)
+		if err != nil {
+			return nil, err
+		}
+		if hct < time.Duration(1) {
+			return nil, errors.New("healthcheck-timeout must be at least 1 second")
+		}
+		healthcheck.Timeout = hct
+	}
+	if cmd.Flags().Changed("health-start-period") {
+		hct, err := time.ParseDuration(vals.HealthStartPeriod)
+		if err != nil {
+			return nil, err
+		}
+		if hct < time.Duration(0) {
+			return nil, errors.New("healthcheck-start-period must be 0 seconds or greater")
+		}
+		healthcheck.StartPeriod = hct
+	}
+
+	return &healthcheck, nil
+}

cmd/podman/containers/create.go

@@ -174,6 +174,13 @@ func create(cmd *cobra.Command, args []string) error {
 		}
 	}
 
+	if s.HealthConfig == nil {
+		s.HealthConfig, err = common.GetHealthCheckOverrideConfig(cmd, &cliVals)
+		if err != nil {
+			return err
+		}
+	}
+
 	report, err := registry.ContainerEngine().ContainerCreate(registry.Context(), s)
 	if err != nil {
 		// if pod was created as part of run

cmd/podman/containers/run.go

@@ -225,6 +225,13 @@ func run(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	if s.HealthConfig == nil {
+		s.HealthConfig, err = common.GetHealthCheckOverrideConfig(cmd, &cliVals)
+		if err != nil {
+			return err
+		}
+	}
+
 	report, err := registry.ContainerEngine().ContainerRun(registry.Context(), runOpts)
 	// report.ExitCode is set by ContainerRun even it returns an error
 	if report != nil {

docs/source/markdown/options/health-cmd.md

@@ -10,3 +10,5 @@ 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 is interpreted
 as an argument to **/bin/sh -c**.
+
+Note: The default values are used even if healthcheck is configured in the image.

docs/source/markdown/options/health-interval.md

@@ -5,3 +5,5 @@
 #### **--health-interval**=*interval*
 
 Set an interval for the healthchecks. An _interval_ of **disable** results in no automatic timer setup. The default is **30s**.
+
+Note: This parameter will overwrite related healthcheck configuration from the image.

docs/source/markdown/options/health-retries.md

@@ -5,3 +5,5 @@
 #### **--health-retries**=*retries*
 
 The number of retries allowed before a healthcheck is considered to be unhealthy. The default value is **3**.
+
+Note: This parameter can overwrite the healthcheck configuration from the image.

docs/source/markdown/options/health-start-period.md

@@ -12,3 +12,5 @@ the container's health state will be updated to `healthy`. However, if the healt
 stay as `starting` until either the health check is successful or until the `--health-start-period` time is over. If the
 health check command fails after the `--health-start-period` time is over, the health state will be updated to `unhealthy`.
 The health check command is executed periodically based on the value of `--health-interval`.
+
+Note: This parameter will overwrite related healthcheck configuration from the image.

docs/source/markdown/options/health-timeout.md

@@ -10,3 +10,5 @@ value can be expressed in a time format such as **1m22s**. The default value is
 Note: A timeout marks the healthcheck as failed but does not terminate the running process.
 This ensures that a slow but eventually successful healthcheck does not disrupt the container
 but is still accounted for in the health status.
+
+Note: This parameter will overwrite related healthcheck configuration from the image.

pkg/specgen/generate/container.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/containers/common/libimage"
 	"github.com/containers/common/pkg/config"
+	"github.com/containers/image/v5/manifest"
 	"github.com/containers/podman/v5/libpod"
 	"github.com/containers/podman/v5/libpod/define"
 	ann "github.com/containers/podman/v5/pkg/annotations"
@@ -61,6 +62,66 @@ func getImageFromSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGen
 	return image, resolvedName, inspectData, err
 }
 
+func applyHealthCheckOverrides(s *specgen.SpecGenerator, healthCheckFromImage *manifest.Schema2HealthConfig) error {
+	overrideHealthCheckConfig := s.HealthConfig
+	s.HealthConfig = healthCheckFromImage
+
+	if s.HealthConfig == nil {
+		return nil
+	}
+
+	if overrideHealthCheckConfig != nil {
+		if overrideHealthCheckConfig.Interval != 0 {
+			s.HealthConfig.Interval = overrideHealthCheckConfig.Interval
+		}
+		if overrideHealthCheckConfig.Retries != 0 {
+			s.HealthConfig.Retries = overrideHealthCheckConfig.Retries
+		}
+		if overrideHealthCheckConfig.Timeout != 0 {
+			s.HealthConfig.Timeout = overrideHealthCheckConfig.Timeout
+		}
+		if overrideHealthCheckConfig.StartPeriod != 0 {
+			s.HealthConfig.StartPeriod = overrideHealthCheckConfig.StartPeriod
+		}
+	}
+
+	disableInterval := false
+	if s.HealthConfig.Interval < 0 {
+		s.HealthConfig.Interval = 0
+		disableInterval = true
+	}
+
+	// NOTE: Zero means inherit.
+	if s.HealthConfig.Timeout == 0 {
+		hct, err := time.ParseDuration(define.DefaultHealthCheckTimeout)
+		if err != nil {
+			return err
+		}
+		s.HealthConfig.Timeout = hct
+	}
+	if s.HealthConfig.Interval == 0 && !disableInterval {
+		hct, err := time.ParseDuration(define.DefaultHealthCheckInterval)
+		if err != nil {
+			return err
+		}
+		s.HealthConfig.Interval = hct
+	}
+
+	if s.HealthConfig.Retries == 0 {
+		s.HealthConfig.Retries = int(define.DefaultHealthCheckRetries)
+	}
+
+	if s.HealthConfig.StartPeriod == 0 {
+		hct, err := time.ParseDuration(define.DefaultHealthCheckStartPeriod)
+		if err != nil {
+			return err
+		}
+		s.HealthConfig.StartPeriod = hct
+	}
+
+	return nil
+}
+
 // Fill any missing parts of the spec generator (e.g. from the image).
 // Returns a set of warnings or any fatal error that occurred.
 func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerator) ([]string, error) {
@@ -70,25 +131,9 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
 		return nil, err
 	}
 	if inspectData != nil {
-		if s.HealthConfig == nil {
-			// NOTE: the health check is only set for Docker images
-			// but inspect will take care of it.
-			s.HealthConfig = inspectData.HealthCheck
-			if s.HealthConfig != nil {
-				if s.HealthConfig.Timeout == 0 {
-					hct, err := time.ParseDuration(define.DefaultHealthCheckTimeout)
-					if err != nil {
-						return nil, err
-					}
-					s.HealthConfig.Timeout = hct
-				}
-				if s.HealthConfig.Interval == 0 {
-					hct, err := time.ParseDuration(define.DefaultHealthCheckInterval)
-					if err != nil {
-						return nil, err
-					}
-					s.HealthConfig.Interval = hct
-				}
+		if s.HealthConfig == nil || len(s.HealthConfig.Test) == 0 {
+			if err := applyHealthCheckOverrides(s, inspectData.HealthCheck); err != nil {
+				return nil, err
 			}
 		}
 

test/e2e/healthcheck_run_test.go

@@ -32,6 +32,20 @@ var _ = Describe("Podman healthcheck run", func() {
 		Expect(hc).Should(ExitWithError(125, "has no defined healthcheck"))
 	})
 
+	It("podman run/create override image healthcheck configuration", func() {
+		podmanTest.PodmanExitCleanly("run", "-dt", "--name", "hc", "--health-start-period", "10s", "--health-interval", "10s", "--health-timeout", "10s", "--health-retries", "2", HEALTHCHECK_IMAGE)
+		hc := podmanTest.PodmanExitCleanly("container", "inspect", "--format", "{{.Config.Healthcheck.StartPeriod}}--{{.Config.Healthcheck.Interval}}--{{.Config.Healthcheck.Timeout}}--{{.Config.Healthcheck.Retries}}", "hc")
+		Expect(hc.OutputToString()).To(Equal("10s--10s--10s--2"))
+
+		podmanTest.PodmanExitCleanly("create", "-q", "--name", "hc1", "--health-start-period", "10s", "--health-interval", "10s", "--health-timeout", "10s", "--health-retries", "2", "quay.io/libpod/healthcheck:config-only", "ls")
+		hc1 := podmanTest.PodmanExitCleanly("container", "inspect", "--format", "{{.Config.Healthcheck.StartPeriod}}--{{.Config.Healthcheck.Interval}}--{{.Config.Healthcheck.Timeout}}--{{.Config.Healthcheck.Retries}}", "hc1")
+		Expect(hc1.OutputToString()).To(Equal("10s--10s--10s--2"))
+
+		podmanTest.PodmanExitCleanly("run", "-dt", "--name", "hc2", "--health-start-period", "10s", "--health-interval", "disable", "--health-timeout", "10s", "--health-retries", "2", HEALTHCHECK_IMAGE)
+		hc2 := podmanTest.PodmanExitCleanly("container", "inspect", "--format", "{{.Config.Healthcheck.StartPeriod}}--{{.Config.Healthcheck.Interval}}--{{.Config.Healthcheck.Timeout}}--{{.Config.Healthcheck.Retries}}", "hc2")
+		Expect(hc2.OutputToString()).To(Equal("10s--0s--10s--2"))
+	})
+
 	It("podman disable healthcheck with --no-healthcheck must not show starting on status", func() {
 		session := podmanTest.Podman([]string{"run", "-dt", "--no-healthcheck", "--name", "hc", HEALTHCHECK_IMAGE})
 		session.WaitWithDefaultTimeout()
@@ -70,7 +84,7 @@ var _ = Describe("Podman healthcheck run", func() {
 		hc := podmanTest.Podman([]string{"container", "inspect", "--format", "{{.Config.Healthcheck}}", "hc"})
 		hc.WaitWithDefaultTimeout()
 		Expect(hc).Should(ExitCleanly())
-		Expect(hc.OutputToString()).To(Equal("{[CMD-SHELL curl -f http://localhost/ || exit 1] 0s 0s 5m0s 3s 0}"))
+		Expect(hc.OutputToString()).To(Equal("{[CMD-SHELL curl -f http://localhost/ || exit 1] 0s 0s 5m0s 3s 3}"))
 	})
 
 	It("podman disable healthcheck with --health-cmd=none on valid container", func() {