fix: cli prompt termination exit code #4888

Benehiko · 2024-02-21T15:47:33Z

⋊> ~/g/s/g/d/cli on fix-prompt-termination ⨯ ./build/docker volume prune      16:35:17
WARNING! This will remove anonymous local volumes not used by at least one container.
Are you sure you want to continue? [y/N] ^C
⋊> ~/g/s/g/d/cli on fix-prompt-termination ⨯ echo $status                     16:35:21
0
⋊> ~/g/s/g/d/cli on fix-prompt-termination ⨯ bash                             16:35:23
[benehiko@benehiko cli]$ ./build/docker volume prune
WARNING! This will remove anonymous local volumes not used by at least one container.
Are you sure you want to continue? [y/N] ^C
[benehiko@benehiko cli]$ echo $?
0
⋊> ~/g/s/g/d/cli on fix-prompt-termination ⨯ zsh                              16:35:32
benehiko% ./build/docker volume prune
WARNING! This will remove anonymous local volumes not used by at least one container.
Are you sure you want to continue? [y/N] ^C
benehiko% echo $?
0

- What I did

The docker command/utils.go provides a function for prompting the user for a confirmation [Y/n]. To fix the termination of the CLI while the prompt is active, the reader is blocked inside a gouroutine instead allowing for checks on context cancellation from the parent or from a SIGINT/SIGTERM. An error is then propagated up the stack when the prompt is terminated instead of an immediate program termination. The caller should respect the error and either close the reader or exit the CLI to prevent goroutine leaks and unexpected behavior on re-prompting.

- How I did it

Catch the termination signal on prompt.

- How to verify it

Manually and with tests.

- Description for the changelog

CLI confirmation prompts will now safely cancel the operation on termination (SIGINT/SIGTERM) producing an exit code of 0.

These are the affected CLI commands:

docker system prune
docker volume prune
docker plugin upgrade
docker image prune
docker network prune
docker container prune
docker network rm
docker builder prune
docker trust revoke

Some commands above are replaced by plugins, which will not be affected.

- A picture of a cute animal (not mandatory but encouraged)

(https://www.rawpixel.com/image/6041428/photo-image-background-public-domain-cat)

codecov-commenter · 2024-02-22T13:26:16Z

Codecov Report

Merging #4888 (4824d80) into master (f2e98f9) will increase coverage by 0.15%.
Report is 28 commits behind head on master.
The diff coverage is 87.32%.

❗ Current head 4824d80 differs from pull request most recent head 385e049. Consider uploading reports for the commit 385e049 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4888      +/-   ##
==========================================
+ Coverage   61.31%   61.46%   +0.15%     
==========================================
  Files         287      289       +2     
  Lines       20063    20228     +165     
==========================================
+ Hits        12301    12434     +133     
- Misses       6868     6894      +26     
- Partials      894      900       +6

cli/command/filename

cli/command/plugin/upgrade_test.go

cli/command/utils.go

Benehiko · 2024-02-23T09:38:05Z

cli/command/utils_test.go

@@ -1,36 +1,45 @@
-package command
+package command_test


had cyclical import between the command package and test.

Fine if we do this, but I wonder if the cyclical import was a smell that we should move something.

cli/command/utils.go

Benehiko · 2024-02-27T10:05:05Z

cli/command/plugin/install.go

@@ -142,6 +142,7 @@ func acceptPrivileges(dockerCli command.Cli, name string) func(privileges types.
 		for _, privilege := range privileges {
 			fmt.Fprintf(dockerCli.Out(), " - %s: %v\n", privilege.Name, privilege.Value)
 		}
-		return command.PromptForConfirmation(dockerCli.In(), dockerCli.Out(), "Do you grant the above permissions?"), nil
+		ctx := context.TODO()


here the context isn't apparent. Either we use the top level dockerCli context or create a new context within the function. The moby project does not support passing context on this function call https://github.com/moby/moby/blob/d19d98b136f6a7e80029b65c5ae8886eacdf43d7/api/types/client.go#L292

FWIW; if we would benefit from passing contexts, we should consider updating the moby code. For a lot of code in moby, context didn't exist yet when it was written, and we started passing through contexts more, but there's still many areas where we didn't.

I'll take a look at also adding context in the moby api client. It will help greatly with otel and just task cleanup in general.

cli/command/utils_test.go

krissetto · 2024-02-27T11:01:40Z

cli/command/trust/testdata/trust-revoke-prompt-termination.golden

@@ -0,0 +1,2 @@
+Please confirm you would like to delete all signature data for example/trust-demo? [y/N] 
+Aborting action.


aah i guess this could be another confirmation prompt to "fix" by standardizing the output (aka not printing "Aborting action" and making it behave more like everything else). For another moment of course

ah yeah true

internal/test/cmd.go

cli/command/utils.go

laurazard · 2024-03-04T11:09:47Z

cli/command/plugin/testdata/plugin-upgrade-terminate.golden

@@ -0,0 +1,2 @@
+Upgrading plugin foo/bar from localhost:5000/foo/bar:v0.1.0 to localhost:5000/foo/bar:v1.0.0
+Plugin images do not match, are you sure? [y/N] 


Missing a newline at the end here? (The other new golden files have them 🤔)

@Benehiko could we fix this?

hmm, i'm not sure why this is. The file is generated by the -update command.

Right, this is likely due to us missing a newline when printing things somewhere

laurazard

Really nice, thank you! Left a few comments (mostly nits), but overall a good change.

Not sure if I missed it, but are we explicitly testing the exit code anywhere?

internal/test/writer.go

cli/command/utils.go

Benehiko · 2024-03-04T13:53:16Z

Really nice, thank you! Left a few comments (mostly nits), but overall a good change.

Not sure if I missed it, but are we explicitly testing the exit code anywhere?

Hmm, I suppose we could check the status code, but the status code seems to be added to a specific error type. It seems the main package handles some exit codes based on the error type here https://github.com/docker/cli/blob/master/cmd/docker/docker.go#L35-L50

Since we gracefully propagate the error up we check for it inside the test, which I think should be enough. Wdyt?

laurazard · 2024-03-04T14:00:11Z

imo those tests are useful, but if we want to preserve the "exit code is 0 when user CTRL-C's a confirmation prompt" in the future, we could add this check in particular for the e2e tests for these commands. (But fine if you'd prefer to do it in another PR).

Benehiko · 2024-03-04T14:09:18Z

Seems like this test is quite flaky

56.34 === FAIL: cli-plugins/socket TestConnectAndWait/connect_goroutine_exits_after_EOF

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

laurazard · 2024-03-04T14:58:06Z

internal/test/writer.go

+// NewWriterWithHook returns a new WriterWithHook that still writes to the actualWriter
+// but also calls the hook function after every write.
+// The hook function is useful for testing, or waiting for a write to complete.
+func NewWriterWithHook(actualWriter io.Writer, hook func([]byte)) *WriterWithHook {


Just noticed we're only using this in internal/test and one other usage, I wonder if this needs to be public at all (thinking about this because otherwise we can relax a bit with the docstrings.

Anyway, can address this (and e2e tests) in a follow-up (cc @Benehiko)

tonistiigi · 2024-03-15T01:05:37Z

cli/command/utils.go

+
+	// Catch the termination signal and exit the prompt gracefully.
+	// The caller is responsible for properly handling the termination.
+	notifyCtx, notifyCancel := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM)


I'm not sure a utility function should set up signal handlers for the process as a side effect. I think this should just use the passed in context for cancellation, and if that context is tied to the os signals is up to the caller. Normally, a process already has a signal handler and this will conflict with it.

Benehiko force-pushed the fix-prompt-termination branch 2 times, most recently from 0bffacc to c532931 Compare February 21, 2024 16:57

Benehiko marked this pull request as ready for review February 21, 2024 16:59

Benehiko requested a review from a team February 22, 2024 10:44

thaJeztah reviewed Feb 22, 2024

View reviewed changes

cli/command/filename Outdated Show resolved Hide resolved

thaJeztah reviewed Feb 22, 2024

View reviewed changes

cli/command/plugin/upgrade_test.go Outdated Show resolved Hide resolved

Benehiko force-pushed the fix-prompt-termination branch 6 times, most recently from 7ddd3b8 to e67927c Compare February 23, 2024 09:18

Benehiko commented Feb 23, 2024

View reviewed changes

cli/command/utils.go Outdated Show resolved Hide resolved

Benehiko commented Feb 23, 2024

View reviewed changes

Benehiko force-pushed the fix-prompt-termination branch from e67927c to 5e14416 Compare February 23, 2024 09:40

Benehiko requested review from thaJeztah, krissetto and laurazard February 23, 2024 10:09

Benehiko commented Feb 23, 2024

View reviewed changes

cli/command/utils.go Outdated Show resolved Hide resolved

Benehiko force-pushed the fix-prompt-termination branch 5 times, most recently from de7081a to e662a1e Compare February 27, 2024 10:00

Benehiko commented Feb 27, 2024

View reviewed changes

krissetto reviewed Feb 27, 2024

View reviewed changes

Benehiko force-pushed the fix-prompt-termination branch 3 times, most recently from f4e617d to 77ad0c2 Compare February 28, 2024 12:14

Benehiko force-pushed the fix-prompt-termination branch from 77ad0c2 to d4d2552 Compare February 28, 2024 12:16

Benehiko commented Feb 28, 2024

View reviewed changes

internal/test/cmd.go Outdated Show resolved Hide resolved

Benehiko force-pushed the fix-prompt-termination branch 2 times, most recently from ee32a21 to ef1624c Compare February 28, 2024 14:13

Benehiko requested a review from krissetto February 28, 2024 14:19

krissetto reviewed Feb 29, 2024

View reviewed changes

cli/command/utils.go Outdated Show resolved Hide resolved

cli/command/utils.go Outdated Show resolved Hide resolved

cli/command/utils.go Outdated Show resolved Hide resolved

thaJeztah added the status/2-code-review label Feb 29, 2024

thaJeztah assigned Benehiko Feb 29, 2024

laurazard reviewed Mar 4, 2024

View reviewed changes

laurazard approved these changes Mar 4, 2024

View reviewed changes

internal/test/writer.go Outdated Show resolved Hide resolved

internal/test/writer.go Show resolved Hide resolved

cli/command/utils.go Outdated Show resolved Hide resolved

cli/command/utils.go Outdated Show resolved Hide resolved

Benehiko force-pushed the fix-prompt-termination branch 2 times, most recently from 4824d80 to 385e049 Compare March 4, 2024 11:49

krissetto approved these changes Mar 4, 2024

View reviewed changes

neersighted approved these changes Mar 4, 2024

View reviewed changes

fix: cli prompt termination exit code

10bf91a

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

Benehiko force-pushed the fix-prompt-termination branch from f21679e to 10bf91a Compare March 4, 2024 14:28

neersighted merged commit 181575b into docker:master Mar 4, 2024
88 checks passed

Benehiko deleted the fix-prompt-termination branch March 4, 2024 14:57

laurazard reviewed Mar 4, 2024

View reviewed changes

Benehiko mentioned this pull request Mar 12, 2024

feat: standardize error for prompt #4939

Merged

tonistiigi reviewed Mar 15, 2024

View reviewed changes

thaJeztah mentioned this pull request Mar 17, 2024

flaky test: TestPromptForConfirmation/case=reader_closed #4948

Closed

crazy-max mentioned this pull request Mar 27, 2024

vendor: github.com/docker/docker and github.com/docker/cli v26.0.0 docker/buildx#2359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cli prompt termination exit code #4888

fix: cli prompt termination exit code #4888

Benehiko commented Feb 21, 2024 •

edited

codecov-commenter commented Feb 22, 2024 •

edited

Benehiko Feb 23, 2024

laurazard Mar 4, 2024

Benehiko Feb 27, 2024 •

edited by thaJeztah

thaJeztah Feb 29, 2024

Benehiko Feb 29, 2024

krissetto Feb 27, 2024

Benehiko Feb 27, 2024

laurazard Mar 4, 2024

laurazard Mar 4, 2024

Benehiko Mar 4, 2024

laurazard Mar 4, 2024

laurazard left a comment

Benehiko commented Mar 4, 2024

laurazard commented Mar 4, 2024

Benehiko commented Mar 4, 2024

laurazard Mar 4, 2024

laurazard Mar 4, 2024

tonistiigi Mar 15, 2024

		@@ -0,0 +1,2 @@
		Please confirm you would like to delete all signature data for example/trust-demo? [y/N]
		Aborting action.

		@@ -0,0 +1,2 @@
		Upgrading plugin foo/bar from localhost:5000/foo/bar:v0.1.0 to localhost:5000/foo/bar:v1.0.0
		Plugin images do not match, are you sure? [y/N]

fix: cli prompt termination exit code #4888

fix: cli prompt termination exit code #4888

Conversation

Benehiko commented Feb 21, 2024 • edited

codecov-commenter commented Feb 22, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko Feb 27, 2024 • edited by thaJeztah

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

Benehiko commented Mar 4, 2024

laurazard commented Mar 4, 2024

Benehiko commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko commented Feb 21, 2024 •

edited

codecov-commenter commented Feb 22, 2024 •

edited

Benehiko Feb 27, 2024 •

edited by thaJeztah