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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions cmd/pinniped/cmd/whoami.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

type whoamiFlags struct {
outputFormat string // e.g., yaml, json, text
timeout time.Duration

kubeconfigPath string
kubeconfigContextOverride string
Expand Down Expand Up @@ -58,6 +59,7 @@
f.StringVar(&flags.kubeconfigPath, "kubeconfig", os.Getenv("KUBECONFIG"), "Path to kubeconfig file")
f.StringVar(&flags.kubeconfigContextOverride, "kubeconfig-context", "", "Kubeconfig context name (default: current active context)")
f.StringVar(&flags.apiGroupSuffix, "api-group-suffix", groupsuffix.PinnipedDefaultSuffix, "Concierge API group suffix")
f.DurationVar(&flags.timeout, "timeout", 0, "Timeout for the WhoAmI API request (default: 0, meaning no timeout)")

cmd.RunE = func(cmd *cobra.Command, _ []string) error {
return runWhoami(cmd.OutOrStdout(), getClientset, flags)
Expand All @@ -78,8 +80,22 @@
return fmt.Errorf("could not get current cluster info: %w", err)
}

ctx, cancelFunc := context.WithTimeout(context.Background(), time.Second*20)
defer cancelFunc()
// Making the WhoAmI request may cause client-go to invoke the credential plugin, which may
// ask the user to interactively authenticate. The time that the user takes to authenticate
// is included in the timeout time, but their authentication is not cancelled by exceeding
// this timeout. Only the subsequent whoami API request is cancelled. Using a short timeout
// causes the odd behavior of a successful login immediately followed by a whoami request failure
// due to timeout. For comparison, kubectl uses an infinite timeout by default on API requests
// but also allows the user to adjust this timeout with the `--request-timeout` CLI option,
// so we will take a similar approach. Note that kubectl has the same behavior when a client-go
// credential plugin is invoked and the user takes longer then the timeout to authenticate.
ctx := context.Background()
if flags.timeout > 0 {
var cancelFunc context.CancelFunc
ctx, cancelFunc = context.WithTimeout(ctx, flags.timeout)
defer cancelFunc()
}

Check warning on line 97 in cmd/pinniped/cmd/whoami.go

View check run for this annotation

Codecov / codecov/patch

cmd/pinniped/cmd/whoami.go#L94-L97

Added lines #L94 - L97 were not covered by tests

whoAmI, err := clientset.IdentityV1alpha1().WhoAmIRequests().Create(ctx, &identityv1alpha1.WhoAmIRequest{}, metav1.CreateOptions{})
if err != nil {
hint := ""
Expand Down
10 changes: 8 additions & 2 deletions cmd/pinniped/cmd/whoami_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2021 the Pinniped contributors. All Rights Reserved.
// Copyright 2023 the Pinniped contributors. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

package cmd
Expand Down Expand Up @@ -45,6 +45,7 @@ func TestWhoami(t *testing.T) {
--kubeconfig string Path to kubeconfig file
--kubeconfig-context string Kubeconfig context name (default: current active context)
-o, --output string Output format (e.g., 'yaml', 'json', 'text') (default "text")
--timeout duration Timeout for the WhoAmI API request (default: 0, meaning no timeout)
`),
},
{
Expand Down Expand Up @@ -312,7 +313,12 @@ func TestWhoami(t *testing.T) {
stdout, stderr := bytes.NewBuffer([]byte{}), bytes.NewBuffer([]byte{})
cmd.SetOut(stdout)
cmd.SetErr(stderr)
cmd.SetArgs(test.args)
if test.args == nil {
// cobra uses os.Args[1:] when SetArgs is called with nil, so avoid using nil for tests.
cmd.SetArgs([]string{})
} else {
cmd.SetArgs(test.args)
}

err := cmd.Execute()
if test.wantError {
Expand Down