Configurable timeout & improved error handling in kgs login (#1097)

* Configurable timeout & improved error handling in kgs login

* Adjusted login-timeout flag description

CHANGELOG.md

@@ -7,9 +7,14 @@ and this project's packages adheres to [Semantic Versioning](http://semver.org/s
 
 ## [Unreleased]
 
+### Added
+
+- Add `--login-timeout` flag to control the time period of OIDC login timeout
+
 ### Changed
 
 - Graceful failure of the `login` command in case workload cluster API is not known
+- Improved error message after login timeout
 
 ## [2.39.0] - 2023-06-22
 

cmd/login/flag.go

@@ -26,6 +26,8 @@ const (
 
 	flagProxy     = "proxy"
 	flagProxyPort = "proxy-port"
+
+	flagLoginTimeout = "login-timeout"
 )
 
 type flag struct {
@@ -46,6 +48,8 @@ type flag struct {
 
 	Proxy     bool
 	ProxyPort int
+
+	LoginTimeout time.Duration
 }
 
 func (f *flag) Init(cmd *cobra.Command) {
@@ -67,6 +71,8 @@ func (f *flag) Init(cmd *cobra.Command) {
 	cmd.Flags().BoolVar(&f.Proxy, flagProxy, false, "Enable socks proxy configuration for the cluster. Only Supported for Workload Cluster using clientcert auth mode")
 	cmd.Flags().IntVar(&f.ProxyPort, flagProxyPort, 9000, "Port for the socks proxy configuration for the cluster")
 
+	cmd.Flags().DurationVar(&f.LoginTimeout, flagLoginTimeout, 60*time.Second, "Duration for which kubectl gs will wait for the OIDC login to complete. Once the timeout is reached, OIDC login will fail.")
+
 	_ = cmd.Flags().MarkHidden(flagWCInsecureNamespace)
 	_ = cmd.Flags().MarkHidden("namespace")
 }
@@ -81,5 +87,9 @@ func (f *flag) Validate() error {
 		return microerror.Maskf(invalidFlagError, `--%s cannot be negative or zero.`, flagWCCertTTL)
 	}
 
+	if f.LoginTimeout <= 0 {
+		return microerror.Maskf(invalidFlagError, `--%s cannot be negative or zero`, flagLoginTimeout)
+	}
+
 	return nil
 }

cmd/login/login.go

@@ -2,6 +2,7 @@ package login
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/fatih/color"
@@ -138,8 +139,15 @@ func (r *runner) loginWithInstallation(ctx context.Context, tokenOverride string
 			}
 		} else {
 
-			authResult, err = handleOIDC(ctx, r.stdout, r.stderr, i, r.flag.ConnectorID, r.flag.ClusterAdmin, r.flag.CallbackServerPort)
+			authResult, err = handleOIDC(ctx, r.stdout, r.stderr, i, r.flag.ConnectorID, r.flag.ClusterAdmin, r.flag.CallbackServerPort, r.flag.LoginTimeout)
 			if err != nil {
+				if errors.Is(err, context.DeadlineExceeded) || IsAuthResponseTimedOut(err) {
+					fmt.Fprintf(r.stderr, "\nYour authentication flow timed out after %s. Please execute the same command again.\n", r.flag.LoginTimeout.String())
+					fmt.Fprintf(r.stderr, "You can use the --login-timeout flag to configure a longer timeout interval, for example --login-timeout=%.0fs.\n", 2*r.flag.LoginTimeout.Seconds())
+					if errors.Is(err, context.DeadlineExceeded) {
+						return microerror.Maskf(authResponseTimedOutError, "failed to get an authentication response on time")
+					}
+				}
 				return microerror.Mask(err)
 			}
 

cmd/login/oidc.go

@@ -29,7 +29,6 @@ const (
 	customerConnectorType   = "customer"
 	giantswarmConnectorType = "giantswarm"
 
-	oidcResultTimeout     = 1 * time.Minute
 	oidcReadHeaderTimeout = 1 * time.Minute
 )
 
@@ -38,7 +37,7 @@ var (
 )
 
 // handleOIDC executes the OIDC authentication against an installation's authentication provider.
-func handleOIDC(ctx context.Context, out io.Writer, errOut io.Writer, i *installation.Installation, connectorID string, clusterAdmin bool, port int) (authInfo, error) {
+func handleOIDC(ctx context.Context, out io.Writer, errOut io.Writer, i *installation.Installation, connectorID string, clusterAdmin bool, port int, oidcResultTimeout time.Duration) (authInfo, error) {
 	ctx, cancel := context.WithTimeout(ctx, oidcResultTimeout)
 	defer cancel()
 

cmd/login/runner_test.go

@@ -13,7 +13,6 @@ import (
 	"net/url"
 	"os"
 	"reflect"
-	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -307,8 +306,8 @@ func TestLogin(t *testing.T) {
 		},
 	}
 
-	for i, tc := range testCases {
-		t.Run(strconv.Itoa(i), func(t *testing.T) {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			configDir, err := os.MkdirTemp("", "loginTest")
 			if err != nil {
 				t.Fatal(err)
@@ -356,7 +355,8 @@ func TestMCLoginWithInstallation(t *testing.T) {
 		token       string
 		skipInCI    bool
 
-		expectError *microerror.Error
+		expectError    *microerror.Error
+		expectedOutput string
 	}{
 		// empty start config
 		{
@@ -398,14 +398,27 @@ func TestMCLoginWithInstallation(t *testing.T) {
 			flags: &flag{
 				ClusterAdmin:       false,
 				CallbackServerPort: 8080,
+				LoginTimeout:       60 * time.Second,
 			},
 			startConfig: &clientcmdapi.Config{},
 			skipInCI:    true,
 		},
+		// Fail in case the OIDC flow does not finish within time period specified in the flag
+		{
+			name: "case 5",
+			flags: &flag{
+				ClusterAdmin:       false,
+				CallbackServerPort: 8080,
+				LoginTimeout:       1 * time.Millisecond,
+			},
+			startConfig:    &clientcmdapi.Config{},
+			expectError:    authResponseTimedOutError,
+			expectedOutput: "\nYour authentication flow timed out after 1ms. Please execute the same command again.\nYou can use the --login-timeout flag to configure a longer timeout interval, for example --login-timeout=0s.\n",
+		},
 	}
 
-	for i, tc := range testCases {
-		t.Run(strconv.Itoa(i), func(t *testing.T) {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			if _, ok := os.LookupEnv("CI"); ok {
 				if tc.skipInCI {
 					t.Skip()
@@ -423,11 +436,14 @@ func TestMCLoginWithInstallation(t *testing.T) {
 			if len(tc.flags.SelfContained) > 0 {
 				tc.flags.SelfContained = configDir + tc.flags.SelfContained
 			}
+
+			out := new(bytes.Buffer)
+
 			r := runner{
 				commonConfig: commonconfig.New(cf),
 				flag:         tc.flags,
-				stdout:       new(bytes.Buffer),
-				stderr:       new(bytes.Buffer),
+				stdout:       out,
+				stderr:       out,
 				fs:           afero.NewBasePathFs(fs, configDir),
 			}
 			k8sConfigAccess := r.commonConfig.GetConfigAccess()
@@ -515,6 +531,12 @@ func TestMCLoginWithInstallation(t *testing.T) {
 				}
 			}
 
+			if tc.expectedOutput != "" {
+				outStr := out.String()
+				if !strings.Contains(outStr, tc.expectedOutput) {
+					t.Fatalf("output does not contain expected string:\nvalue: %s\nexpected string: %s\n", outStr, tc.expectedOutput)
+				}
+			}
 		})
 	}
 }