Add --request-timeout flag with TCP dial support for all commands

This commit adds a --request-timeout flag (following kubectl conventions)
to all OADP CLI commands, with full support for cluster unreachability:

- Add --request-timeout flag to nonadmin backup logs and describe commands
  (takes precedence over OADP_CLI_REQUEST_TIMEOUT env var)
- Add timeoutFactory wrapper that applies dial timeout to all Velero
  commands by overriding all client-creating methods (KubeClient,
  DynamicClient, DiscoveryClient, KubebuilderClient, KubebuilderWatchClient)
- Add renameTimeoutFlag to rename Velero's --timeout to --request-timeout
  for consistent kubectl-style CLI experience
- Set custom net.Dialer with timeout to handle TCP dial timeouts
- Add context-based timeout handling with proper cancellation detection
- Add FormatDownloadRequestTimeoutError for helpful timeout diagnostics
- Add tests for timeout functionality (global timeout, config application,
  dialer timeout behavior)

The timeout now applies to both HTTP requests and TCP connection
attempts across all commands, ensuring the CLI times out quickly when
the cluster is unreachable instead of waiting for the default ~30s TCP
timeout.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Signed-off-by: Michal Pryc <[email protected]>

Author: mpryc

cmd/non-admin/backup/describe.go

@@ -18,35 +18,52 @@ import (
 )
 
 func NewDescribeCommand(f client.Factory, use string) *cobra.Command {
+	var requestTimeout time.Duration
+
 	c := &cobra.Command{
 		Use:   use + " NAME",
 		Short: "Describe a non-admin backup",
 		Args:  cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			backupName := args[0]
 
+			// Get effective timeout (flag takes precedence over env var)
+			effectiveTimeout := shared.GetHTTPTimeoutWithOverride(requestTimeout)
+
+			// Create context with the effective timeout
+			ctx, cancel := context.WithTimeout(context.Background(), effectiveTimeout)
+			defer cancel()
+
 			// Get the current namespace from kubectl context
 			userNamespace, err := shared.GetCurrentNamespace()
 			if err != nil {
 				return fmt.Errorf("failed to determine current namespace: %w", err)
 			}
 
-			// Create client with required scheme types
+			// Create client with required scheme types and timeout
 			kbClient, err := shared.NewClientWithScheme(f, shared.ClientOptions{
 				IncludeNonAdminTypes: true,
 				IncludeVeleroTypes:   true,
 				IncludeCoreTypes:     true,
+				Timeout:              effectiveTimeout,
 			})
 			if err != nil {
 				return err
 			}
 
 			// Get the specific backup
 			var nab nacv1alpha1.NonAdminBackup
-			if err := kbClient.Get(context.Background(), kbclient.ObjectKey{
+			if err := kbClient.Get(ctx, kbclient.ObjectKey{
 				Namespace: userNamespace,
 				Name:      backupName,
 			}, &nab); err != nil {
+				// Check for context cancellation
+				if ctx.Err() == context.DeadlineExceeded {
+					return fmt.Errorf("timed out after %v getting NonAdminBackup %q", effectiveTimeout, backupName)
+				}
+				if ctx.Err() == context.Canceled {
+					return fmt.Errorf("operation cancelled: %w", ctx.Err())
+				}
 				return fmt.Errorf("NonAdminBackup %q not found in namespace %q: %w", backupName, userNamespace, err)
 			}
 
@@ -55,9 +72,12 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command {
 
 			return nil
 		},
-		Example: `  kubectl oadp nonadmin backup describe my-backup`,
+		Example: `  kubectl oadp nonadmin backup describe my-backup
+  kubectl oadp nonadmin backup describe my-backup --request-timeout=30m`,
 	}
 
+	c.Flags().DurationVar(&requestTimeout, "request-timeout", 0, fmt.Sprintf("The length of time to wait before giving up on a single server request (e.g., 30s, 5m, 1h). Overrides %s env var. Default: %v", shared.TimeoutEnvVar, shared.DefaultHTTPTimeout))
+
 	output.BindFlags(c.Flags())
 	output.ClearOutputFlagDefault(c)
 
@@ -349,9 +369,16 @@ func colorizePhase(phase string) string {
 }
 
 // NonAdminDescribeBackup mirrors Velero's output.DescribeBackup functionality
-// but works within non-admin RBAC boundaries using NonAdminDownloadRequest
-func NonAdminDescribeBackup(cmd *cobra.Command, kbClient kbclient.Client, nab *nacv1alpha1.NonAdminBackup, userNamespace string) error {
-	ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second)
+// but works within non-admin RBAC boundaries using NonAdminDownloadRequest.
+// The timeout parameter controls how long to wait for download requests to complete.
+// If timeout is 0, DefaultOperationTimeout is used.
+func NonAdminDescribeBackup(cmd *cobra.Command, kbClient kbclient.Client, nab *nacv1alpha1.NonAdminBackup, userNamespace string, timeout time.Duration) error {
+	// Use provided timeout or fall back to default
+	if timeout == 0 {
+		timeout = shared.DefaultOperationTimeout
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
 
 	// Print basic backup information
@@ -401,39 +428,43 @@ func NonAdminDescribeBackup(cmd *cobra.Command, kbClient kbclient.Client, nab *n
 
 		// Get backup results using NonAdminDownloadRequest (most important data)
 		if results, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{
-			BackupName: veleroBackupName,
-			DataType:   "BackupResults",
-			Namespace:  userNamespace,
+			BackupName:  veleroBackupName,
+			DataType:    "BackupResults",
+			Namespace:   userNamespace,
+			HTTPTimeout: timeout,
 		}); err == nil {
 			fmt.Fprintf(cmd.OutOrStdout(), "\nBackup Results:\n")
 			fmt.Fprintf(cmd.OutOrStdout(), "%s", indent(results, "  "))
 		}
 
 		// Get backup details using NonAdminDownloadRequest for BackupResourceList
 		if resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{
-			BackupName: veleroBackupName,
-			DataType:   "BackupResourceList",
-			Namespace:  userNamespace,
+			BackupName:  veleroBackupName,
+			DataType:    "BackupResourceList",
+			Namespace:   userNamespace,
+			HTTPTimeout: timeout,
 		}); err == nil {
 			fmt.Fprintf(cmd.OutOrStdout(), "\nBackup Resource List:\n")
 			fmt.Fprintf(cmd.OutOrStdout(), "%s", indent(resourceList, "  "))
 		}
 
 		// Get backup volume info using NonAdminDownloadRequest
 		if volumeInfo, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{
-			BackupName: veleroBackupName,
-			DataType:   "BackupVolumeInfos",
-			Namespace:  userNamespace,
+			BackupName:  veleroBackupName,
+			DataType:    "BackupVolumeInfos",
+			Namespace:   userNamespace,
+			HTTPTimeout: timeout,
 		}); err == nil {
 			fmt.Fprintf(cmd.OutOrStdout(), "\nBackup Volume Info:\n")
 			fmt.Fprintf(cmd.OutOrStdout(), "%s", indent(volumeInfo, "  "))
 		}
 
 		// Get backup item operations using NonAdminDownloadRequest
 		if itemOps, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{
-			BackupName: veleroBackupName,
-			DataType:   "BackupItemOperations",
-			Namespace:  userNamespace,
+			BackupName:  veleroBackupName,
+			DataType:    "BackupItemOperations",
+			Namespace:   userNamespace,
+			HTTPTimeout: timeout,
 		}); err == nil {
 			fmt.Fprintf(cmd.OutOrStdout(), "\nBackup Item Operations:\n")
 			fmt.Fprintf(cmd.OutOrStdout(), "%s", indent(itemOps, "  "))

cmd/non-admin/backup/logs.go

@@ -19,6 +19,7 @@ limitations under the License.
 import (
 	"context"
 	"fmt"
+	"net"
 	"time"
 
 	"github.com/migtools/oadp-cli/cmd/shared"
@@ -31,12 +32,18 @@ import (
 )
 
 func NewLogsCommand(f client.Factory, use string) *cobra.Command {
-	return &cobra.Command{
+	var requestTimeout time.Duration
+
+	c := &cobra.Command{
 		Use:   use + " NAME",
 		Short: "Show logs for a non-admin backup",
 		Args:  cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second)
+			// Get effective timeout (flag takes precedence over env var)
+			effectiveTimeout := shared.GetHTTPTimeoutWithOverride(requestTimeout)
+
+			// Create context with the effective timeout for the entire operation
+			ctx, cancel := context.WithTimeout(context.Background(), effectiveTimeout)
 			defer cancel()
 
 			// Get the current namespace from kubectl context
@@ -59,6 +66,18 @@ func NewLogsCommand(f client.Factory, use string) *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("failed to get rest config: %w", err)
 			}
+			// Set timeout on REST config to prevent hanging when cluster is unreachable
+			restConfig.Timeout = effectiveTimeout
+
+			// Set a custom dial function with timeout to ensure TCP connection attempts
+			// also respect the timeout (the default TCP dial timeout is ~30s)
+			dialer := &net.Dialer{
+				Timeout: effectiveTimeout,
+			}
+			restConfig.Dial = func(ctx context.Context, network, address string) (net.Conn, error) {
+				return dialer.DialContext(ctx, network, address)
+			}
+
 			kbClient, err := kbclient.New(restConfig, kbclient.Options{Scheme: scheme})
 			if err != nil {
 				return fmt.Errorf("failed to create controller-runtime client: %w", err)
@@ -97,26 +116,34 @@ func NewLogsCommand(f client.Factory, use string) *cobra.Command {
 				_ = kbClient.Delete(deleteCtx, req)
 			}()
 
-			fmt.Fprintf(cmd.OutOrStdout(), "Waiting for backup logs to be processed...\n")
+			fmt.Fprintf(cmd.OutOrStdout(), "Waiting for backup logs to be processed (timeout: %v)...\n", effectiveTimeout)
 
-			// Wait for the download request to be processed using shared utility
-			// Note: We create a custom waiting implementation here to provide user feedback
-			timeout := time.After(120 * time.Second)
-			tick := time.Tick(2 * time.Second)
+			// Wait for the download request to be processed
+			ticker := time.NewTicker(2 * time.Second)
+			defer ticker.Stop()
 
 			var signedURL string
 		Loop:
 			for {
 				select {
-				case <-timeout:
-					return fmt.Errorf("timed out waiting for NonAdminDownloadRequest to be processed")
-				case <-tick:
+				case <-ctx.Done():
+					// Check if context was cancelled due to timeout or other reason
+					if ctx.Err() == context.DeadlineExceeded {
+						return shared.FormatDownloadRequestTimeoutError(kbClient, req, effectiveTimeout)
+					}
+					// Context cancelled for other reason (e.g., user interruption)
+					return fmt.Errorf("operation cancelled: %w", ctx.Err())
+				case <-ticker.C:
 					fmt.Fprintf(cmd.OutOrStdout(), ".")
 					var updated nacv1alpha1.NonAdminDownloadRequest
 					if err := kbClient.Get(ctx, kbclient.ObjectKey{
 						Namespace: req.Namespace,
 						Name:      req.Name,
 					}, &updated); err != nil {
+						// If context expired during Get, handle it in next iteration
+						if ctx.Err() != nil {
+							continue
+						}
 						return fmt.Errorf("failed to get NonAdminDownloadRequest: %w", err)
 					}
 
@@ -141,12 +168,18 @@ func NewLogsCommand(f client.Factory, use string) *cobra.Command {
 			}
 
 			// Use the shared StreamDownloadContent function to download and stream logs
-			if err := shared.StreamDownloadContent(signedURL, cmd.OutOrStdout()); err != nil {
+			// Note: We use the same effective timeout for the HTTP download
+			if err := shared.StreamDownloadContentWithTimeout(signedURL, cmd.OutOrStdout(), effectiveTimeout); err != nil {
 				return fmt.Errorf("failed to download and stream logs: %w", err)
 			}
 
 			return nil
 		},
-		Example: `  kubectl oadp nonadmin backup logs my-backup`,
+		Example: `  kubectl oadp nonadmin backup logs my-backup
+  kubectl oadp nonadmin backup logs my-backup --request-timeout=30m`,
 	}
+
+	c.Flags().DurationVar(&requestTimeout, "request-timeout", 0, fmt.Sprintf("The length of time to wait before giving up on a single server request (e.g., 30s, 5m, 1h). Overrides %s env var. Default: %v", shared.TimeoutEnvVar, shared.DefaultHTTPTimeout))
+
+	return c
 }