oauth/api: drain timer channel on each iteration by laurazard · Pull Request #5376 · docker/cli

laurazard · 2024-08-28T13:20:42Z

- What I did

Previously, if while polling for oauth device-code login results a user suspended the process (such as with CTRL-Z) and then restored it with fg, an error might occur in the form of:

failed waiting for authentication: You are polling faster than the specified interval of 5 seconds.

This is due to our use of a time.Ticker here - if no receiver drains the ticker channel (and timers/tickers use a buffered channel behind the scenes), more than one tick will pile up, causing the program to "tick" twice, in fast succession, after it is resumed.

- How I did it

The new implementation replaces the time.Ticker with a time.Timer (time.Ticker is just a nice wrapper) and introduces a helper function resetTimer to ensure that before every select, the timer is stopped and it's channel is drained.

- How to verify it

Reproduce as described in #5375

- Description for the changelog

Fix issue that will sometimes cause the browser-login flow to fail if the CLI process is suspended and then resumed while waiting for the user to authenticate.

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

codecov-commenter · 2024-08-28T13:23:19Z

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.97%. Comparing base (3826f5a) to head (60d0450).
Report is 13 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@## master #5376 +/- ##
==========================================
+ Coverage 60.93% 60.97% +0.03% 
==========================================
Files 304 304 Lines 21348 21358 +10 ==========================================
+ Hits 13009 13022 +13 + Misses 7409 7407 -2 + Partials 930 929 -1

dvdksn

works in my local test 🎉

laurazard · 2024-09-02T10:16:54Z

@thaJeztah @vvoland PTAL

thaJeztah

Thanks for updating! Left two comments 🙈

thaJeztah · 2024-09-03T09:33:12Z

+ // tick, check for user login
 res, err := a.getDeviceToken(ctx, state)
 if err != nil {
+ if errors.Is(err, context.Canceled) {


Do we need to handle context.DeadlineExceeded here? We could even use the errdefs helper, but not sure if we want to import it here;
cli/vendor/github.com/docker/docker/errdefs/is.go
Lines 120 to 123 in 092b5f0
// IsContext returns if the passed in error is due to context cancellation or deadline exceeded.
funcIsContext(errerror) bool {
returnerrors.Is(err, context.Canceled) ||errors.Is(err, context.DeadlineExceeded)
}

Hmm, the purpose of that:
// if the caller canceled the context, continue// and let the select hit the ctx.Done() branch
is just to make sure we hit the user canceled login error if the user cancels execution while API.getDeviceToken is going rather than the context canceled error from there, so I don't think we need it. This is also all inside /internal, so we don't expect others to import this, so I think it's fine. If the user does get a timeout while trying to connect to the tenant, I think we should error out and let them know.

Ah, right; yeah, was curious if we wanted to retry if the connection timed out. I guess that would be a bit of an exceptional case, so returning early in that case makes sense as well.
Thanks!

Previously, if while polling for oauth device-code login results a user suspended the process (such as with CTRL-Z) and then restored it with `fg`, an error might occur in the form of: ``` failed waiting for authentication: You are polling faster than the specified interval of 5 seconds. ``` This is due to our use of a `time.Ticker` here - if no receiver drains the ticker channel (and timers/tickers use a buffered channel behind the scenes), more than one tick will pile up, causing the program to "tick" twice, in fast succession, after it is resumed. The new implementation replaces the `time.Ticker` with a `time.Timer` (`time.Ticker` is just a nice wrapper) and introduces a helper function `resetTimer` to ensure that before every `select`, the timer is stopped and it's channel is drained. Signed-off-by: Laura Brehm <laurabrehm@hey.com>

thaJeztah

LGTM

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/docker](https://redirect.github.com/docker/docker) | patch | `27.2.0` -> `27.2.1` | --- ### Release Notes <details> <summary>docker/docker (docker/docker)</summary> ### [`v27.2.1`](https://redirect.github.com/moby/moby/releases/tag/v27.2.1) [Compare Source](https://redirect.github.com/docker/docker/compare/v27.2.0-rc.1...v27.2.1) #### 27.2.1 For a full list of pull requests and changes in this release, refer to the relevant GitHub milestones: - [docker/cli, 27.2.1 milestone](https://redirect.github.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.2.1) - [moby/moby, 27.2.1 milestone](https://redirect.github.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.2.1) ##### Bug fixes and enhancements - containerd image store: Fix non-container images being hidden in the `docker images` output. [moby/moby#48402](https://redirect.github.com/moby/moby/pull/48402) - containerd image store: Improve `docker pull` error message when the image platform doesn't match. [moby/moby#48415](https://redirect.github.com/moby/moby/pull/48415) - CLI: Fix issue causing login to not remove repository names from passed in registry addresses, resulting in credentials being stored under the wrong key. [docker/cli#5385](https://redirect.github.com/docker/cli/pull/5385) - CLI: Fix issue that will sometimes cause the browser-login flow to fail if the CLI process is suspended and then resumed while waiting for the user to authenticate. [docker/cli#5376](https://redirect.github.com/docker/cli/pull/5376) - CLI: `docker login` now returns an error instead of hanging if called non-interactively with `--password` or `--password-stdin` but without `--user`. [docker/cli#5402](https://redirect.github.com/docker/cli/pull/5402) ##### Packaging updates - Update `runc` to [v1.1.14](https://redirect.github.com/opencontainers/runc/releases/tag/v1.1.14), which contains a fix for [CVE-2024-45310](https://redirect.github.com/opencontainers/runc/security/advisories/GHSA-jfvp-7x6p-h2pv). [moby/moby#48426](https://redirect.github.com/moby/moby/pull/48426) - Update Go runtime to 1.22.7. [moby/moby#48433](https://redirect.github.com/moby/moby/pull/48433), [docker/cli#5411](https://redirect.github.com/docker/cli/pull/5411), [docker/docker-ce-packaging#1068](https://redirect.github.com/docker/docker-ce-packaging/pull/1068) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/earthly/dind).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/docker](https://redirect.github.com/docker/docker) | patch | `27.2.0` -> `27.2.1` | --- ### Release Notes <details> <summary>docker/docker (docker/docker)</summary> ### [`v27.2.1`](https://redirect.github.com/moby/moby/releases/tag/v27.2.1) [Compare Source](https://redirect.github.com/docker/docker/compare/v27.2.0-rc.1...v27.2.1) #### 27.2.1 For a full list of pull requests and changes in this release, refer to the relevant GitHub milestones: - [docker/cli, 27.2.1 milestone](https://redirect.github.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.2.1) - [moby/moby, 27.2.1 milestone](https://redirect.github.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.2.1) ##### Bug fixes and enhancements - containerd image store: Fix non-container images being hidden in the `docker image ls` output. [moby/moby#48402](https://redirect.github.com/moby/moby/pull/48402) - containerd image store: Improve `docker pull` error message when the image platform doesn't match. [moby/moby#48415](https://redirect.github.com/moby/moby/pull/48415) - CLI: Fix issue causing `docker login` to not remove repository names from passed in registry addresses, resulting in credentials being stored under the wrong key. [docker/cli#5385](https://redirect.github.com/docker/cli/pull/5385) - CLI: Fix issue that will sometimes cause the browser-login flow to fail if the CLI process is suspended and then resumed while waiting for the user to authenticate. [docker/cli#5376](https://redirect.github.com/docker/cli/pull/5376) - CLI: `docker login` now returns an error instead of hanging if called non-interactively with `--password` or `--password-stdin` but without `--user`. [docker/cli#5402](https://redirect.github.com/docker/cli/pull/5402) ##### Packaging updates - Update `runc` to [v1.1.14](https://redirect.github.com/opencontainers/runc/releases/tag/v1.1.14), which contains a fix for [CVE-2024-45310](https://redirect.github.com/opencontainers/runc/security/advisories/GHSA-jfvp-7x6p-h2pv). [moby/moby#48426](https://redirect.github.com/moby/moby/pull/48426) - Update Go runtime to 1.22.7. [moby/moby#48433](https://redirect.github.com/moby/moby/pull/48433), [docker/cli#5411](https://redirect.github.com/docker/cli/pull/5411), [docker/docker-ce-packaging#1068](https://redirect.github.com/docker/docker-ce-packaging/pull/1068) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/earthly/dind).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

laurazard added status/2-code-review kind/bugfix PR's that fix bugs labels Aug 28, 2024

laurazard self-assigned this Aug 28, 2024

dvdksn reviewed Aug 28, 2024
View reviewed changes

Comment threadcli/internal/oauth/api/api.go

dvdksn approved these changes Aug 28, 2024
View reviewed changes

thaJeztah added area/authentication process/cherry-pick labels Aug 28, 2024

laurazard force-pushed the fix-oauth-login-timer branch 4 times, most recently from a29bec2 to 63ef63dCompare August 29, 2024 14:35

laurazard requested review from thaJeztah and vvoland August 29, 2024 16:34

vvoland approved these changes Aug 30, 2024
View reviewed changes

vvoland added the process/cherry-pick/27.2 label Aug 30, 2024

thaJeztah reviewed Aug 30, 2024
View reviewed changes

Comment threadcli/internal/oauth/api/api.go Outdated

laurazard force-pushed the fix-oauth-login-timer branch from 63ef63d to 290cf9eCompare September 2, 2024 10:16

laurazard force-pushed the fix-oauth-login-timer branch from 290cf9e to f2a3abeCompare September 2, 2024 10:30

thaJeztah reviewed Sep 2, 2024
View reviewed changes

Comment threadcli/internal/oauth/api/api.go Outdated

laurazard force-pushed the fix-oauth-login-timer branch 2 times, most recently from 614fb9c to ad483cfCompare September 2, 2024 14:43

thaJeztah reviewed Sep 3, 2024
View reviewed changes

laurazard force-pushed the fix-oauth-login-timer branch from ad483cf to 60d0450Compare September 3, 2024 10:31

laurazard requested a review from thaJeztah September 3, 2024 10:31

thaJeztah approved these changes Sep 3, 2024
View reviewed changes

thaJeztah merged commit 51f320f into docker:masterSep 3, 2024

vvoland mentioned this pull request Sep 3, 2024
[27.x backport] oauth/api: drain timer channel on each iteration #5399
Merged

vvoland added process/cherry-picked and removed process/cherry-pick process/cherry-pick/27.2 labels Sep 3, 2024

laurazard added this to the 27.2.1 milestone Sep 12, 2024

Navigation Menu

Uh oh!

oauth/api: drain timer channel on each iteration - #5376

oauth/api: drain timer channel on each iteration#5376
thaJeztah merged 1 commit into
docker:masterfrom
laurazard:fix-oauth-login-timer

laurazard commented Aug 28, 2024

Uh oh!

Uh oh!

codecov-commenter commented Aug 28, 2024•
edited
Loading

Uh oh!

dvdksn left a comment

Uh oh!

Uh oh!

laurazard commented Sep 2, 2024

Uh oh!

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztahSep 3, 2024

Uh oh!

laurazardSep 3, 2024

Uh oh!

thaJeztahSep 3, 2024

Uh oh!

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	// IsContext returns if the passed in error is due to context cancellation or deadline exceeded.
	funcIsContext(errerror) bool {
	returnerrors.Is(err, context.Canceled) \|\|errors.Is(err, context.DeadlineExceeded)
	}

Uh oh!

Conversation

laurazard commented Aug 28, 2024

Uh oh!

Uh oh!

codecov-commenter commented Aug 28, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dvdksn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laurazard commented Sep 2, 2024

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztahSep 3, 2024

Choose a reason for hiding this comment

Uh oh!

laurazardSep 3, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztahSep 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 28, 2024•
edited
Loading