Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite default timeout for WhoAmI API call and add --timeout CLI flag #1774

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

cfryanr
Copy link
Member

@cfryanr cfryanr commented Nov 13, 2023

The pinniped whoami CLI command has always had a hardcoded timeout of 20 seconds for the WhoAmI API call. However, this has always been a problem. When a user needs to interactively authenticate, and when they take more than 20 seconds to do so, then the WhoAmI request always fails due to the timeout being exceeded, even though they successfully logged in.

To fix this, copy how kubectl timeouts work. Introduce a new --timeout flag (name of flag copied from the pre-existing pinniped get kubeconfg flag) and default it to 0 which means no timeout (i.e. infinite timeout).

The fake Kube client used in the unit tests ignores the context param that is passed to it, so unfortunately there is no easy way to unit test this timeout.

Release note:

`pinniped whoami` has a new `--timeout` parameter, which defaults to no timeout.

@joshuatcasey
Copy link
Member

Approved, check review comments.

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (61c630c) 78.96% compared to head (47f6de5) 78.94%.

Files Patch % Lines
cmd/pinniped/cmd/whoami.go 28.57% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1774      +/-   ##
==========================================
- Coverage   78.96%   78.94%   -0.03%     
==========================================
  Files         166      166              
  Lines       15933    15938       +5     
==========================================
  Hits        12582    12582              
- Misses       3036     3040       +4     
- Partials      315      316       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cfryanr cfryanr merged commit aad8dc5 into main Nov 14, 2023
39 checks passed
@cfryanr cfryanr deleted the whoami_timeout branch November 14, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants