fix: don't panic in GetHealth during a dry run - #6146
Open
ankit090701 wants to merge 1 commit into
Open
Conversation
containerReference.GetHealth called cr.cli.ContainerInspect unconditionally. cr.cli is only ever set by connect(), which is called from pipelines that are all gated with .IfNot(common.Dryrun) (Create, Start, Exec, etc.) - so during a dry run, cr.cli stays nil. GetHealth itself is called directly (not through one of those gated pipelines) by RunContext.waitForServiceContainer, so any workflow with `services` panicked with a nil pointer dereference as soon as a dry run (`act -n`) reached the "wait for service containers to be healthy" step. Add a dry-run check at the top of GetHealth, matching the pattern GetContainerArchive already uses, and matching how HostEnvironment's GetHealth already unconditionally returns HealthHealthy - a dry run doesn't create real containers, so pretending they're healthy is consistent with how the rest of the dry-run codepaths behave. fixes nektos#2607 Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Any workflow with
services, run with dry-run (act -n), panics withinvalid memory address or nil pointer dereference(confirmed by 4 separate users on this issue, across versions 0.2.71-0.2.78, on macOS and WSL2).Root cause
containerReference.GetHealth(pkg/container/docker_run.go) callscr.cli.ContainerInspect(...)unconditionally.cr.cliis only ever assigned byconnect(), and every pipeline that callsconnect()(Create,Start,Exec,Copy,Remove) is wrapped with.IfNot(common.Dryrun)- so during a dry run,cr.cliis never set and staysnil.GetHealthis the odd one out: it's called directly byRunContext.waitForServiceContainer(pkg/runner/run_context.go), not through one of those dry-run-gated pipelines. So as soon as a dry run withservicesreaches the "wait for service containers to become healthy" step,cr.cli.ContainerInspectpanics on the nil client.The fix
Add a dry-run check at the top of
GetHealththat returnsHealthHealthyimmediately, before touchingcr.cli. This mirrors two existing patterns in the same file/package:GetContainerArchivealready has its ownif common.Dryrun(ctx) { ... }early return.HostEnvironment.GetHealth(the no-DockerExecutionsEnvironmentimplementation) already unconditionally returnsHealthHealthy- a dry run doesn't create real containers, so reporting them as healthy is consistent with how every other dry-run codepath behaves (they no-op and report success).waitForServiceContainertreats any non-HealthStartingresult as terminal and only succeeds onHealthHealthy, so this also makes the dry-run wait loop resolve immediately instead of looping 30 times / 5 minutes.Testing
TestDockerGetHealthDryrun(pkg/container/docker_run_test.go), which constructs acontainerReferencewith a nilcli(matching dry run's actual state) and confirmsGetHealthunder a dry-run context returnsHealthHealthywithout panicking.docker_run.gochange: the test fails with the exact panic reported in this issue (invalid memory address or nil pointer dereference,docker_run.go:175, insideGetHealth).go build ./...(clean) andgo test ./pkg/container/.... The only other failure in that package,TestImageExistsLocally, is pre-existing and unrelated - it needs a live Docker daemon, which isn't available in the sandbox I used to verify this (no Docker-in-Docker).Fixes #2607