docker / cli

The Docker CLI
Apache License 2.0
4.87k stars 1.91k forks source link

fix: ctx cancellation on login prompt #5168

Closed Benehiko closed 3 months ago

Benehiko commented 3 months ago

- What I did

Implemented a new Prompt function to capture user input called PromptForInput. This function works virtually similar to PromptForConfirmation and handles context cancellation.

- How I did it

Replicate PromptForConfirmation.

- How to verify it Run docker login and cancel it midway with a SIGINT or SIGTERM or ctrl+c.

docker login

Username:^C

PromptForInput also allows for hiding user input - for example when the user enters their password.

The original behavior of docker login should still be in-tact with trimming spaces and not showing user input on the password prompt.

- Description for the changelog

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

laurazard commented 3 months ago

Before this, when receiving a SIGINT during the login prompt, the CLI would exit with code 140

 ⭑ docker login      
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆。・:*:・゚⭑ laurabrehm ✿ cli [e06bee6999] ⭑ echo $?     
130

However, now it's exiting with 0:

⭑ ./build/docker login                 
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆。・:*:・゚⭑ laurabrehm ✿ cli [fix-cli-login] ⭑ echo $?                              
0

I don't think this is necessarily wrong (on one hand, the user is cancelling the operation) but not sure.

Benehiko commented 3 months ago

Before this, when receiving a SIGINT during the login prompt, the CLI would exit with code 140

 ⭑ docker login      
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆。・:*:・゚⭑ laurabrehm ✿ cli [e06bee6999] ⭑ echo $?     
130

However, now it's exiting with 0:

⭑ ./build/docker login                 
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆。・:*:・゚⭑ laurabrehm ✿ cli [fix-cli-login] ⭑ echo $?                              
0

I don't think this is necessarily wrong (on one hand, the user is cancelling the operation) but not sure.

Error code 130 used to occur on the previous Prompt confirmations that were cancelled as well. I can't remember if this was an error that Go decides or that we did with os.Exit. But see here https://github.com/docker/cli/issues/4850

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 43.75000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 61.47%. Comparing base (01dd6ab) to head (c15ade0). Report is 11 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5168 +/- ## ========================================== + Coverage 61.43% 61.47% +0.04% ========================================== Files 298 298 Lines 20799 20808 +9 ========================================== + Hits 12777 12791 +14 + Misses 7109 7105 -4 + Partials 913 912 -1 ```
Benehiko commented 3 months ago

Please see this PR which would also be required in the case of other commands/features not respecting context cancellation https://github.com/docker/cli/pull/5171

Benehiko commented 3 months ago

@vvoland could we get this one merged? The windows specific code can be addressed in a follow-up. I'll be running some tests to verify if we need this on older (supported) versions of windows 10 https://github.com/docker/cli/blob/b83cf582cd3b932b6fce900a54cf0e1e6f61ac17/cli/command/registry.go#L93-L104