From 6ad15b0c4078c68e1409275848b65469ee71d1bb Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 3 Feb 2026 19:40:39 -0500 Subject: [PATCH 1/4] Fix gzip decompression for backup downloads and enhance describe command This commit addresses issues with compressed backup data and adds new functionality for detailed backup information: Download improvements: - Fix gzip detection to check file magic bytes (0x1f 0x8b) in addition to Content-Encoding header. Object storage signed URLs often serve .gz files without the Content-Encoding header, causing decompressed content to be displayed as binary gibberish - Add comprehensive debug logging to download functions for easier troubleshooting - Use buffered reader with Peek() to detect gzip format without consuming the stream Describe command enhancements: - Add --details flag to fetch additional backup information including volume snapshots, resource lists, backup results, and item operations - Exclude describe command from output wrapper (similar to logs command) to prevent buffering issues with large downloads - Update tests to reflect describe command exclusion Installation improvements: - Enhance make install output formatting with clearer status messages - Show version info during installation verification Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- Makefile | 30 +++++- cmd/non-admin/backup/describe.go | 161 ++++++++++++++++++++++++++++++- cmd/root.go | 9 +- cmd/root_test.go | 25 +++-- cmd/shared/download.go | 109 ++++++++++++++++++--- 5 files changed, 304 insertions(+), 30 deletions(-) diff --git a/Makefile b/Makefile index 3f0b264..9106165 100644 --- a/Makefile +++ b/Makefile @@ -228,8 +228,17 @@ install: build ## Build and install the kubectl plugin to ~/.local/bin (no sudo echo " ├─ Temporarily updating PATH for verification"; \ if PATH="$(INSTALL_PATH):$$PATH" command -v kubectl >/dev/null 2>&1; then \ if PATH="$(INSTALL_PATH):$$PATH" kubectl plugin list 2>/dev/null | grep -q "kubectl-oadp"; then \ - echo " ├─ ✅ Installation verified: kubectl oadp plugin is accessible"; \ - PATH="$(INSTALL_PATH):$$PATH" kubectl oadp version 2>/dev/null || echo " │ └─ (Note: version command requires cluster access)"; \ + echo " └─ ✅ Installation verified: kubectl oadp plugin is accessible"; \ + echo ""; \ + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"; \ + echo "🎉 Installation complete!"; \ + echo ""; \ + echo " ⚠️ To use in this terminal session, run:"; \ + echo " export PATH=\"$(INSTALL_PATH):$$PATH\""; \ + echo ""; \ + echo " Quick start:"; \ + echo " • kubectl oadp --help # Show available commands"; \ + echo " • kubectl oadp backup get # List backups"; \ else \ echo " ├─ ❌ Installation verification failed: kubectl oadp plugin not found"; \ echo " │ └─ Try running: export PATH=\"$(INSTALL_PATH):$$PATH\""; \ @@ -242,9 +251,22 @@ install: build ## Build and install the kubectl plugin to ~/.local/bin (no sudo if command -v kubectl >/dev/null 2>&1; then \ if kubectl plugin list 2>/dev/null | grep -q "kubectl-oadp"; then \ echo " ├─ ✅ Installation verified: kubectl oadp plugin is accessible"; \ - echo " ├─ Running version command..."; \ + echo " └─ Running version command..."; \ + echo ""; \ + version_output=$$(kubectl oadp version 2>&1 | grep -v "WARNING: the client version does not match"); \ + if [ $$? -eq 0 ] && [ -n "$$version_output" ]; then \ + echo "$$version_output" | sed 's/^/ /'; \ + else \ + echo " (Note: version command requires cluster access)"; \ + fi; \ + echo ""; \ + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"; \ + echo "🎉 Installation complete!"; \ echo ""; \ - kubectl oadp version 2>/dev/null || echo " │ └─ (Note: version command requires cluster access)"; \ + echo " Quick start:"; \ + echo " • kubectl oadp --help # Show available commands"; \ + echo " • kubectl oadp backup get # List backups"; \ + echo " • kubectl oadp version # Show version info"; \ else \ echo " └─ ❌ Installation verification failed: kubectl oadp plugin not found"; \ fi; \ diff --git a/cmd/non-admin/backup/describe.go b/cmd/non-admin/backup/describe.go index c772e4c..30798e6 100644 --- a/cmd/non-admin/backup/describe.go +++ b/cmd/non-admin/backup/describe.go @@ -18,7 +18,10 @@ import ( ) func NewDescribeCommand(f client.Factory, use string) *cobra.Command { - var requestTimeout time.Duration + var ( + requestTimeout time.Duration + details bool + ) c := &cobra.Command{ Use: use + " NAME", @@ -70,13 +73,22 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { // Print in Velero-style format printNonAdminBackupDetails(cmd, &nab) + // Add detailed output if --details flag is set + if details { + if err := printDetailedBackupInfo(cmd, kbClient, &nab, userNamespace, effectiveTimeout); err != nil { + return fmt.Errorf("failed to fetch detailed backup information: %w", err) + } + } + return nil }, Example: ` kubectl oadp nonadmin backup describe my-backup - kubectl oadp nonadmin backup describe my-backup --request-timeout=30m`, + kubectl oadp nonadmin backup describe my-backup --details + kubectl oadp nonadmin backup describe my-backup --details --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)) + c.Flags().BoolVar(&details, "details", false, "Display additional backup details including volume snapshots, resource lists, and item operations") output.BindFlags(c.Flags()) output.ClearOutputFlagDefault(c) @@ -347,6 +359,151 @@ func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBac } } +// printDetailedBackupInfo fetches and displays additional backup details when --details flag is used. +// It uses NonAdminDownloadRequest to fetch: +// - BackupResults (errors, warnings) +// - BackupResourceList (inventory by GroupVersionKind) +// - BackupVolumeInfos (snapshot details) +// - BackupItemOperations (plugin operations) +func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, nab *nacv1alpha1.NonAdminBackup, userNamespace string, timeout time.Duration) error { + out := cmd.OutOrStdout() + + // Check if VeleroBackup exists + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.Name == "" { + fmt.Fprintf(out, "\nDetailed information not available. Backup may not have been processed yet.\n") + return nil + } + + veleroBackupName := nab.Status.VeleroBackup.Name + + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + fmt.Fprintf(out, "\nFetching detailed backup information...\n\n") + + // 1. Fetch BackupVolumeInfos (most useful for users) + volumeInfo, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ + BackupName: veleroBackupName, + DataType: "BackupVolumeInfos", + Namespace: userNamespace, + HTTPTimeout: timeout, + }) + + if err == nil && volumeInfo != "" { + fmt.Fprintf(out, "Volume Snapshot Details:\n") + if formattedInfo := formatVolumeInfo(volumeInfo); formattedInfo != "" { + fmt.Fprintf(out, "%s", formattedInfo) + } else { + fmt.Fprintf(out, " \n") + } + fmt.Fprintf(out, "\n") + } else if err != nil { + fmt.Fprintf(out, "Volume Info: \n\n", err) + } + + // 2. Fetch BackupResourceList + resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ + BackupName: veleroBackupName, + DataType: "BackupResourceList", + Namespace: userNamespace, + HTTPTimeout: timeout, + }) + + if err == nil && resourceList != "" { + fmt.Fprintf(out, "Resource List:\n") + if formattedList := formatResourceList(resourceList); formattedList != "" { + fmt.Fprintf(out, "%s", formattedList) + } else { + fmt.Fprintf(out, " \n") + } + fmt.Fprintf(out, "\n") + } else if err != nil { + fmt.Fprintf(out, "Resource List: \n\n", err) + } + + // 3. Fetch BackupResults + results, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ + BackupName: veleroBackupName, + DataType: "BackupResults", + Namespace: userNamespace, + HTTPTimeout: timeout, + }) + + if err == nil && results != "" { + fmt.Fprintf(out, "Backup Results:\n") + if formattedResults := formatBackupResults(results); formattedResults != "" { + fmt.Fprintf(out, "%s", formattedResults) + } else { + fmt.Fprintf(out, " \n") + } + fmt.Fprintf(out, "\n") + } else if err != nil { + fmt.Fprintf(out, "Backup Results: \n\n", err) + } + + // 4. Fetch BackupItemOperations + itemOps, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ + BackupName: veleroBackupName, + DataType: "BackupItemOperations", + Namespace: userNamespace, + HTTPTimeout: timeout, + }) + + if err == nil && itemOps != "" { + fmt.Fprintf(out, "Backup Item Operations:\n") + if formattedOps := formatItemOperations(itemOps); formattedOps != "" { + fmt.Fprintf(out, "%s", formattedOps) + } else { + fmt.Fprintf(out, " \n") + } + fmt.Fprintf(out, "\n") + } else if err != nil { + fmt.Fprintf(out, "Backup Item Operations: \n\n", err) + } + + return nil +} + +// formatVolumeInfo formats volume snapshot information for display +func formatVolumeInfo(volumeInfo string) string { + // Start with simple indented output + // TODO: Parse JSON/YAML and format nicely (future enhancement) + if strings.TrimSpace(volumeInfo) == "" { + return "" + } + return indent(volumeInfo, " ") +} + +// formatResourceList formats the resource list for display +func formatResourceList(resourceList string) string { + // Start with simple indented output + // TODO: Parse and group by GroupVersionKind (future enhancement) + if strings.TrimSpace(resourceList) == "" { + return "" + } + return indent(resourceList, " ") +} + +// formatBackupResults formats backup results (errors/warnings) for display +func formatBackupResults(results string) string { + // Start with simple indented output + // TODO: Parse JSON and show errors/warnings prominently (future enhancement) + if strings.TrimSpace(results) == "" { + return "" + } + return indent(results, " ") +} + +// formatItemOperations formats backup item operations for display +func formatItemOperations(itemOps string) string { + // Start with simple indented output + // TODO: Parse and show operation details (future enhancement) + if strings.TrimSpace(itemOps) == "" { + return "" + } + return indent(itemOps, " ") +} + // colorizePhase returns the phase string with ANSI color codes func colorizePhase(phase string) string { const ( diff --git a/cmd/root.go b/cmd/root.go index 56e046e..fb82fce 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -208,11 +208,14 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { // Replace in multiple command fields using context-aware replacement cmd.Example = replaceVeleroCommandWithOADP(cmd.Example) - // Skip wrapping logs commands to allow real-time streaming without buffering + // Skip wrapping commands that stream output or make long-running requests + // to allow real-time display without buffering isLogsCommand := cmd.Use == "logs" || strings.HasPrefix(cmd.Use, "logs ") + isDescribeCommand := cmd.Use == "describe" || strings.HasPrefix(cmd.Use, "describe ") + shouldSkipWrapper := isLogsCommand || isDescribeCommand // Wrap the Run function to replace velero in output - if cmd.Run != nil && !isLogsCommand { + if cmd.Run != nil && !shouldSkipWrapper { originalRun := cmd.Run cmd.Run = func(c *cobra.Command, args []string) { // Capture stdout temporarily @@ -239,7 +242,7 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { } // Wrap the RunE function to replace velero in output - if cmd.RunE != nil && !isLogsCommand { + if cmd.RunE != nil && !shouldSkipWrapper { originalRunE := cmd.RunE cmd.RunE = func(c *cobra.Command, args []string) error { // Capture stdout temporarily diff --git a/cmd/root_test.go b/cmd/root_test.go index cb0fb9b..aba87a9 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -591,8 +591,8 @@ func TestApplyTimeoutToConfig(t *testing.T) { } } -// TestReplaceVeleroWithOADP_LogsCommandNotWrapped tests that logs commands are never wrapped -func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { +// TestReplaceVeleroWithOADP_OutputWrapperExclusions tests that certain commands are excluded from output wrapping +func TestReplaceVeleroWithOADP_OutputWrapperExclusions(t *testing.T) { tests := []struct { name string use string @@ -616,7 +616,12 @@ func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { { name: "describe command", use: "describe", - shouldWrap: true, + shouldWrap: false, + }, + { + name: "describe with args", + use: "describe NAME", + shouldWrap: false, }, { name: "create command", @@ -677,16 +682,16 @@ func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { t.Errorf("Wrapped command output should have 'velero' replaced, got: %s", output) } } else { - // Logs commands should NOT have output replaced + // Excluded commands should NOT have output replaced if !strings.Contains(output, "velero backup create") { - t.Errorf("Logs command output should NOT be modified, got: %s", output) + t.Errorf("Excluded command output should NOT be modified, got: %s", output) } } }) } // Test with RunE function - t.Run("logs_command_runE_not_wrapped", func(t *testing.T) { + t.Run("excluded_command_runE_not_wrapped", func(t *testing.T) { runECalled := false cmd := &cobra.Command{ Use: "logs", @@ -700,10 +705,10 @@ func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { originalRunE := cmd.RunE replaceVeleroWithOADP(cmd) - // Logs command should not be wrapped + // Excluded command should not be wrapped isWrapped := fmt.Sprintf("%p", originalRunE) != fmt.Sprintf("%p", cmd.RunE) if isWrapped { - t.Error("Expected logs command RunE NOT to be wrapped, but it was") + t.Error("Expected excluded command RunE NOT to be wrapped, but it was") } // Verify output is not modified @@ -728,9 +733,9 @@ func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { } output := buf.String() - // Logs command output should NOT be modified + // Excluded command output should NOT be modified if !strings.Contains(output, "velero backup logs") { - t.Errorf("Logs command output should NOT be modified, got: %s", output) + t.Errorf("Excluded command output should NOT be modified, got: %s", output) } }) } diff --git a/cmd/shared/download.go b/cmd/shared/download.go index c17de81..309cef8 100644 --- a/cmd/shared/download.go +++ b/cmd/shared/download.go @@ -17,6 +17,7 @@ limitations under the License. package shared import ( + "bufio" "compress/gzip" "context" "fmt" @@ -98,6 +99,9 @@ type DownloadRequestOptions struct { // downloads the content from the signed URL, and returns it as a string. // This function automatically cleans up the download request when done. func ProcessDownloadRequest(ctx context.Context, kbClient kbclient.Client, opts DownloadRequestOptions) (string, error) { + log.Printf("[ProcessDownloadRequest] Starting for BackupName=%q, DataType=%q, Namespace=%q", + opts.BackupName, opts.DataType, opts.Namespace) + // Set defaults if opts.Timeout == 0 { opts.Timeout = 120 * time.Second @@ -105,6 +109,8 @@ func ProcessDownloadRequest(ctx context.Context, kbClient kbclient.Client, opts if opts.PollInterval == 0 { opts.PollInterval = 2 * time.Second } + log.Printf("[ProcessDownloadRequest] Using Timeout=%v, PollInterval=%v, HTTPTimeout=%v", + opts.Timeout, opts.PollInterval, opts.HTTPTimeout) // Create NonAdminDownloadRequest req := &nacv1alpha1.NonAdminDownloadRequest{ @@ -120,54 +126,86 @@ func ProcessDownloadRequest(ctx context.Context, kbClient kbclient.Client, opts }, } + log.Printf("[ProcessDownloadRequest] Creating NonAdminDownloadRequest with GenerateName=%q", req.ObjectMeta.GenerateName) if err := kbClient.Create(ctx, req); err != nil { + log.Printf("[ProcessDownloadRequest] ERROR: Failed to create NonAdminDownloadRequest: %v", err) return "", fmt.Errorf("failed to create NonAdminDownloadRequest for %s: %w", opts.DataType, err) } + log.Printf("[ProcessDownloadRequest] Successfully created NonAdminDownloadRequest with Name=%q", req.Name) // Clean up the download request when done defer func() { + log.Printf("[ProcessDownloadRequest] Cleaning up NonAdminDownloadRequest Name=%q", req.Name) deleteCtx, cancelDelete := context.WithTimeout(context.Background(), 5*time.Second) defer cancelDelete() - _ = kbClient.Delete(deleteCtx, req) + if err := kbClient.Delete(deleteCtx, req); err != nil { + log.Printf("[ProcessDownloadRequest] WARNING: Failed to delete NonAdminDownloadRequest: %v", err) + } else { + log.Printf("[ProcessDownloadRequest] Successfully deleted NonAdminDownloadRequest") + } }() // Wait for the download request to be processed + log.Printf("[ProcessDownloadRequest] Waiting for download URL...") signedURL, err := waitForDownloadURL(ctx, kbClient, req, opts.Timeout, opts.PollInterval) if err != nil { + log.Printf("[ProcessDownloadRequest] ERROR: Failed to get download URL: %v", err) return "", err } + log.Printf("[ProcessDownloadRequest] Received signed URL (length=%d)", len(signedURL)) // Download and return the content using the specified HTTP timeout httpTimeout := GetHTTPTimeoutWithOverride(opts.HTTPTimeout) - return DownloadContentWithTimeout(signedURL, httpTimeout) + log.Printf("[ProcessDownloadRequest] Downloading content with HTTP timeout=%v", httpTimeout) + content, err := DownloadContentWithTimeout(signedURL, httpTimeout) + if err != nil { + log.Printf("[ProcessDownloadRequest] ERROR: Failed to download content: %v", err) + return "", err + } + log.Printf("[ProcessDownloadRequest] Successfully downloaded content (length=%d bytes)", len(content)) + return content, nil } // waitForDownloadURL waits for a NonAdminDownloadRequest to be processed and returns the signed URL func waitForDownloadURL(ctx context.Context, kbClient kbclient.Client, req *nacv1alpha1.NonAdminDownloadRequest, timeout, pollInterval time.Duration) (string, error) { + log.Printf("[waitForDownloadURL] Starting wait for Name=%q, timeout=%v, pollInterval=%v", req.Name, timeout, pollInterval) timeoutCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() ticker := time.NewTicker(pollInterval) defer ticker.Stop() + pollCount := 0 for { select { case <-timeoutCtx.Done(): + log.Printf("[waitForDownloadURL] TIMEOUT after %d polls", pollCount) return "", fmt.Errorf("timed out waiting for NonAdminDownloadRequest to be processed") case <-ticker.C: + pollCount++ + log.Printf("[waitForDownloadURL] Poll #%d: Getting status for Name=%q", pollCount, req.Name) var updated nacv1alpha1.NonAdminDownloadRequest if err := kbClient.Get(ctx, kbclient.ObjectKey{ Namespace: req.Namespace, Name: req.Name, }, &updated); err != nil { + log.Printf("[waitForDownloadURL] ERROR on poll #%d: Failed to get NonAdminDownloadRequest: %v", pollCount, err) return "", fmt.Errorf("failed to get NonAdminDownloadRequest: %w", err) } + log.Printf("[waitForDownloadURL] Poll #%d: Got %d conditions", pollCount, len(updated.Status.Conditions)) + // Check if the download request was processed successfully - for _, condition := range updated.Status.Conditions { + for i, condition := range updated.Status.Conditions { + log.Printf("[waitForDownloadURL] Poll #%d: Condition[%d] Type=%q, Status=%q, Reason=%q, Message=%q", + pollCount, i, condition.Type, condition.Status, condition.Reason, condition.Message) + if condition.Type == "Processed" && condition.Status == "True" { if updated.Status.VeleroDownloadRequest.Status.DownloadURL != "" { + log.Printf("[waitForDownloadURL] SUCCESS on poll #%d: Got download URL", pollCount) return updated.Status.VeleroDownloadRequest.Status.DownloadURL, nil + } else { + log.Printf("[waitForDownloadURL] Poll #%d: Processed=True but no DownloadURL yet", pollCount) } } } @@ -175,6 +213,7 @@ func waitForDownloadURL(ctx context.Context, kbClient kbclient.Client, req *nacv // Check for failure conditions for _, condition := range updated.Status.Conditions { if condition.Status == "True" && condition.Reason == "Error" { + log.Printf("[waitForDownloadURL] FAILURE on poll #%d: Error condition found", pollCount) return "", fmt.Errorf("NonAdminDownloadRequest failed: %s - %s", condition.Type, condition.Message) } } @@ -192,23 +231,51 @@ func DownloadContent(url string) (string, error) { // DownloadContentWithTimeout fetches content from a signed URL with a specified timeout. // It handles both gzipped and non-gzipped content automatically. func DownloadContentWithTimeout(url string, timeout time.Duration) (string, error) { + log.Printf("[DownloadContentWithTimeout] Starting download with timeout=%v", timeout) client := httpClientWithTimeout(timeout) + + log.Printf("[DownloadContentWithTimeout] Sending HTTP GET request...") resp, err := client.Get(url) if err != nil { + log.Printf("[DownloadContentWithTimeout] ERROR: HTTP GET failed: %v", err) return "", fmt.Errorf("failed to download content from URL %q: %w", url, err) } defer resp.Body.Close() + log.Printf("[DownloadContentWithTimeout] HTTP response: Status=%s, Content-Encoding=%s", + resp.Status, resp.Header.Get("Content-Encoding")) + if resp.StatusCode != http.StatusOK { bodyBytes, _ := io.ReadAll(resp.Body) + log.Printf("[DownloadContentWithTimeout] ERROR: Non-OK status code: %s, body length=%d", resp.Status, len(bodyBytes)) return "", fmt.Errorf("failed to download content: status %s, body: %s", resp.Status, string(bodyBytes)) } - // Try to decompress if it's gzipped - var reader io.Reader = resp.Body - if strings.Contains(resp.Header.Get("Content-Encoding"), "gzip") { - gzr, err := gzip.NewReader(resp.Body) + // Use a buffered reader to peek at the content and detect gzip format + bufReader := bufio.NewReader(resp.Body) + var reader io.Reader = bufReader + + // Check if content is gzipped by: + // 1. Content-Encoding header (HTTP-level compression) + // 2. Magic bytes 0x1f 0x8b at start (file-level gzip) + // Object storage signed URLs often serve .gz files without Content-Encoding header, + // so we need to detect gzip by inspecting the actual file content. + isGzipped := strings.Contains(resp.Header.Get("Content-Encoding"), "gzip") + + if !isGzipped { + // Peek at first 2 bytes to check for gzip magic bytes (0x1f 0x8b) + magicBytes, err := bufReader.Peek(2) + if err == nil && len(magicBytes) == 2 && magicBytes[0] == 0x1f && magicBytes[1] == 0x8b { + isGzipped = true + log.Printf("[DownloadContentWithTimeout] Detected gzip format by magic bytes") + } + } + + if isGzipped { + log.Printf("[DownloadContentWithTimeout] Content is gzipped, creating gzip reader...") + gzr, err := gzip.NewReader(bufReader) if err != nil { + log.Printf("[DownloadContentWithTimeout] ERROR: Failed to create gzip reader: %v", err) return "", fmt.Errorf("failed to create gzip reader: %w", err) } defer gzr.Close() @@ -216,11 +283,14 @@ func DownloadContentWithTimeout(url string, timeout time.Duration) (string, erro } // Read all content + log.Printf("[DownloadContentWithTimeout] Reading content from response body...") content, err := io.ReadAll(reader) if err != nil { + log.Printf("[DownloadContentWithTimeout] ERROR: Failed to read content: %v", err) return "", fmt.Errorf("failed to read content: %w", err) } + log.Printf("[DownloadContentWithTimeout] Successfully read %d bytes", len(content)) return string(content), nil } @@ -246,10 +316,27 @@ func StreamDownloadContentWithTimeout(url string, writer io.Writer, timeout time return fmt.Errorf("failed to download content: status %s, body: %s", resp.Status, string(bodyBytes)) } - // Try to decompress if it's gzipped - var reader io.Reader = resp.Body - if strings.Contains(resp.Header.Get("Content-Encoding"), "gzip") { - gzr, err := gzip.NewReader(resp.Body) + // Use a buffered reader to peek at the content and detect gzip format + bufReader := bufio.NewReader(resp.Body) + var reader io.Reader = bufReader + + // Check if content is gzipped by: + // 1. Content-Encoding header (HTTP-level compression) + // 2. Magic bytes 0x1f 0x8b at start (file-level gzip) + // Object storage signed URLs often serve .gz files without Content-Encoding header, + // so we need to detect gzip by inspecting the actual file content. + isGzipped := strings.Contains(resp.Header.Get("Content-Encoding"), "gzip") + + if !isGzipped { + // Peek at first 2 bytes to check for gzip magic bytes (0x1f 0x8b) + magicBytes, err := bufReader.Peek(2) + if err == nil && len(magicBytes) == 2 && magicBytes[0] == 0x1f && magicBytes[1] == 0x8b { + isGzipped = true + } + } + + if isGzipped { + gzr, err := gzip.NewReader(bufReader) if err != nil { return fmt.Errorf("failed to create gzip reader: %w", err) } From 3a5e43b8cd534f8e037343984b8386a14fc07383 Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 3 Feb 2026 22:47:30 -0500 Subject: [PATCH 2/4] Refactor download request logic to eliminate code duplication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract common download request creation and polling logic into a new shared function CreateAndWaitForDownloadURL. This eliminates ~53 lines of duplicated code between the logs and describe commands. Key changes: - Add CreateAndWaitForDownloadURL function to handle request creation and polling with optional progress callbacks - Add OnProgress callback to DownloadRequestOptions for progress indication - Refactor logs command to use shared function (75 lines → 22 lines) - Simplify printDetailedBackupInfo to accept backup name string instead of full NonAdminBackup object - Remove debug log statements from download.go - Clean up unused imports in logs.go This maintains the key behavioral difference between commands: - logs: streams content for low memory usage - describe --details: buffers content for formatting Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- cmd/non-admin/backup/describe.go | 20 ++---- cmd/non-admin/backup/logs.go | 90 +++++++------------------ cmd/shared/download.go | 110 +++++++++++++------------------ 3 files changed, 75 insertions(+), 145 deletions(-) diff --git a/cmd/non-admin/backup/describe.go b/cmd/non-admin/backup/describe.go index 30798e6..13c0588 100644 --- a/cmd/non-admin/backup/describe.go +++ b/cmd/non-admin/backup/describe.go @@ -75,7 +75,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { // Add detailed output if --details flag is set if details { - if err := printDetailedBackupInfo(cmd, kbClient, &nab, userNamespace, effectiveTimeout); err != nil { + if err := printDetailedBackupInfo(cmd, kbClient, backupName, userNamespace, effectiveTimeout); err != nil { return fmt.Errorf("failed to fetch detailed backup information: %w", err) } } @@ -365,17 +365,9 @@ func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBac // - BackupResourceList (inventory by GroupVersionKind) // - BackupVolumeInfos (snapshot details) // - BackupItemOperations (plugin operations) -func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, nab *nacv1alpha1.NonAdminBackup, userNamespace string, timeout time.Duration) error { +func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration) error { out := cmd.OutOrStdout() - // Check if VeleroBackup exists - if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.Name == "" { - fmt.Fprintf(out, "\nDetailed information not available. Backup may not have been processed yet.\n") - return nil - } - - veleroBackupName := nab.Status.VeleroBackup.Name - ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -383,7 +375,7 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, nab * // 1. Fetch BackupVolumeInfos (most useful for users) volumeInfo, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ - BackupName: veleroBackupName, + BackupName: backupName, DataType: "BackupVolumeInfos", Namespace: userNamespace, HTTPTimeout: timeout, @@ -403,7 +395,7 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, nab * // 2. Fetch BackupResourceList resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ - BackupName: veleroBackupName, + BackupName: backupName, DataType: "BackupResourceList", Namespace: userNamespace, HTTPTimeout: timeout, @@ -423,7 +415,7 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, nab * // 3. Fetch BackupResults results, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ - BackupName: veleroBackupName, + BackupName: backupName, DataType: "BackupResults", Namespace: userNamespace, HTTPTimeout: timeout, @@ -443,7 +435,7 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, nab * // 4. Fetch BackupItemOperations itemOps, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ - BackupName: veleroBackupName, + BackupName: backupName, DataType: "BackupItemOperations", Namespace: userNamespace, HTTPTimeout: timeout, diff --git a/cmd/non-admin/backup/logs.go b/cmd/non-admin/backup/logs.go index 850699e..73dad09 100644 --- a/cmd/non-admin/backup/logs.go +++ b/cmd/non-admin/backup/logs.go @@ -25,9 +25,7 @@ import ( "github.com/migtools/oadp-cli/cmd/shared" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" "github.com/spf13/cobra" - velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/client" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -92,21 +90,32 @@ func NewLogsCommand(f client.Factory, use string) *cobra.Command { return fmt.Errorf("failed to get NonAdminBackup %q: %w", backupName, err) } - req := &nacv1alpha1.NonAdminDownloadRequest{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: backupName + "-logs-", - Namespace: userNamespace, - }, - Spec: nacv1alpha1.NonAdminDownloadRequestSpec{ - Target: velerov1.DownloadTarget{ - Kind: "BackupLog", - Name: backupName, // Use NonAdminBackup name, controller will resolve to Velero backup - }, + fmt.Fprintf(cmd.OutOrStdout(), "Waiting for backup logs to be processed (timeout: %v)...\n", effectiveTimeout) + + // Create download request and wait for signed URL + req, signedURL, err := shared.CreateAndWaitForDownloadURL(ctx, kbClient, shared.DownloadRequestOptions{ + BackupName: backupName, + DataType: "BackupLog", + Namespace: userNamespace, + Timeout: effectiveTimeout, + PollInterval: 2 * time.Second, + HTTPTimeout: effectiveTimeout, + OnProgress: func() { + fmt.Fprintf(cmd.OutOrStdout(), ".") }, - } + }) - if err := kbClient.Create(ctx, req); err != nil { - return fmt.Errorf("failed to create NonAdminDownloadRequest: %w", err) + if err != nil { + if req != nil { + // Clean up on error + if ctx.Err() == context.DeadlineExceeded { + return shared.FormatDownloadRequestTimeoutError(kbClient, req, effectiveTimeout) + } + deleteCtx, cancelDelete := context.WithTimeout(context.Background(), 5*time.Second) + defer cancelDelete() + _ = kbClient.Delete(deleteCtx, req) + } + return err } // Clean up the download request when done @@ -116,56 +125,7 @@ func NewLogsCommand(f client.Factory, use string) *cobra.Command { _ = kbClient.Delete(deleteCtx, req) }() - fmt.Fprintf(cmd.OutOrStdout(), "Waiting for backup logs to be processed (timeout: %v)...\n", effectiveTimeout) - - // Wait for the download request to be processed - ticker := time.NewTicker(2 * time.Second) - defer ticker.Stop() - - var signedURL string - Loop: - for { - select { - 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) - } - - // Check if the download request was processed successfully - for _, condition := range updated.Status.Conditions { - if condition.Type == "Processed" && condition.Status == "True" { - if updated.Status.VeleroDownloadRequest.Status.DownloadURL != "" { - signedURL = updated.Status.VeleroDownloadRequest.Status.DownloadURL - fmt.Fprintf(cmd.OutOrStdout(), "\nDownload URL received, fetching logs...\n") - break Loop - } - } - } - - // Check for failure conditions - for _, condition := range updated.Status.Conditions { - if condition.Status == "True" && condition.Reason == "Error" { - return fmt.Errorf("NonAdminDownloadRequest failed: %s - %s", condition.Type, condition.Message) - } - } - } - } + fmt.Fprintf(cmd.OutOrStdout(), "\nDownload URL received, fetching logs...\n") // Use the shared StreamDownloadContent function to download and stream logs // Note: We use the same effective timeout for the HTTP download diff --git a/cmd/shared/download.go b/cmd/shared/download.go index 309cef8..5e5a75c 100644 --- a/cmd/shared/download.go +++ b/cmd/shared/download.go @@ -22,7 +22,6 @@ import ( "context" "fmt" "io" - "log" "net/http" "os" "strings" @@ -54,17 +53,14 @@ func getHTTPTimeout() time.Duration { func GetHTTPTimeoutWithOverride(override time.Duration) time.Duration { // If an explicit override is provided (e.g., from --timeout flag), use it if override > 0 { - log.Printf("Using HTTP timeout from command-line flag: %v", override) return override } // Check for environment variable if envTimeout := os.Getenv(TimeoutEnvVar); envTimeout != "" { if parsed, err := time.ParseDuration(envTimeout); err == nil { - log.Printf("Using custom HTTP timeout from %s: %v", TimeoutEnvVar, parsed) return parsed } - log.Printf("Warning: Invalid duration in %s=%q, using default %v", TimeoutEnvVar, envTimeout, DefaultHTTPTimeout) } return DefaultHTTPTimeout @@ -93,15 +89,14 @@ type DownloadRequestOptions struct { // HTTPTimeout is the timeout for downloading content from the signed URL. // If zero, uses the default timeout (env var or DefaultHTTPTimeout). HTTPTimeout time.Duration + // OnProgress is an optional callback called on each polling iteration + OnProgress func() } -// ProcessDownloadRequest creates a NonAdminDownloadRequest, waits for it to be processed, -// downloads the content from the signed URL, and returns it as a string. -// This function automatically cleans up the download request when done. -func ProcessDownloadRequest(ctx context.Context, kbClient kbclient.Client, opts DownloadRequestOptions) (string, error) { - log.Printf("[ProcessDownloadRequest] Starting for BackupName=%q, DataType=%q, Namespace=%q", - opts.BackupName, opts.DataType, opts.Namespace) - +// CreateAndWaitForDownloadURL creates a NonAdminDownloadRequest, waits for it to be processed, +// and returns the signed URL. The request is NOT automatically cleaned up - caller is responsible. +// This is a lower-level function that allows callers to control cleanup timing. +func CreateAndWaitForDownloadURL(ctx context.Context, kbClient kbclient.Client, opts DownloadRequestOptions) (*nacv1alpha1.NonAdminDownloadRequest, string, error) { // Set defaults if opts.Timeout == 0 { opts.Timeout = 120 * time.Second @@ -109,8 +104,6 @@ func ProcessDownloadRequest(ctx context.Context, kbClient kbclient.Client, opts if opts.PollInterval == 0 { opts.PollInterval = 2 * time.Second } - log.Printf("[ProcessDownloadRequest] Using Timeout=%v, PollInterval=%v, HTTPTimeout=%v", - opts.Timeout, opts.PollInterval, opts.HTTPTimeout) // Create NonAdminDownloadRequest req := &nacv1alpha1.NonAdminDownloadRequest{ @@ -126,86 +119,85 @@ func ProcessDownloadRequest(ctx context.Context, kbClient kbclient.Client, opts }, } - log.Printf("[ProcessDownloadRequest] Creating NonAdminDownloadRequest with GenerateName=%q", req.ObjectMeta.GenerateName) if err := kbClient.Create(ctx, req); err != nil { - log.Printf("[ProcessDownloadRequest] ERROR: Failed to create NonAdminDownloadRequest: %v", err) - return "", fmt.Errorf("failed to create NonAdminDownloadRequest for %s: %w", opts.DataType, err) + return nil, "", fmt.Errorf("failed to create NonAdminDownloadRequest for %s: %w", opts.DataType, err) + } + + // Wait for the download request to be processed + signedURL, err := waitForDownloadURL(ctx, kbClient, req, opts.Timeout, opts.PollInterval, opts.OnProgress) + if err != nil { + return req, "", err + } + + return req, signedURL, nil +} + +// ProcessDownloadRequest creates a NonAdminDownloadRequest, waits for it to be processed, +// downloads the content from the signed URL, and returns it as a string. +// This function automatically cleans up the download request when done. +func ProcessDownloadRequest(ctx context.Context, kbClient kbclient.Client, opts DownloadRequestOptions) (string, error) { + req, signedURL, err := CreateAndWaitForDownloadURL(ctx, kbClient, opts) + if err != nil { + if req != nil { + // Clean up on error + deleteCtx, cancelDelete := context.WithTimeout(context.Background(), 5*time.Second) + defer cancelDelete() + _ = kbClient.Delete(deleteCtx, req) + } + return "", err } - log.Printf("[ProcessDownloadRequest] Successfully created NonAdminDownloadRequest with Name=%q", req.Name) // Clean up the download request when done defer func() { - log.Printf("[ProcessDownloadRequest] Cleaning up NonAdminDownloadRequest Name=%q", req.Name) deleteCtx, cancelDelete := context.WithTimeout(context.Background(), 5*time.Second) defer cancelDelete() - if err := kbClient.Delete(deleteCtx, req); err != nil { - log.Printf("[ProcessDownloadRequest] WARNING: Failed to delete NonAdminDownloadRequest: %v", err) - } else { - log.Printf("[ProcessDownloadRequest] Successfully deleted NonAdminDownloadRequest") - } + _ = kbClient.Delete(deleteCtx, req) }() - // Wait for the download request to be processed - log.Printf("[ProcessDownloadRequest] Waiting for download URL...") - signedURL, err := waitForDownloadURL(ctx, kbClient, req, opts.Timeout, opts.PollInterval) - if err != nil { - log.Printf("[ProcessDownloadRequest] ERROR: Failed to get download URL: %v", err) - return "", err - } - log.Printf("[ProcessDownloadRequest] Received signed URL (length=%d)", len(signedURL)) - // Download and return the content using the specified HTTP timeout httpTimeout := GetHTTPTimeoutWithOverride(opts.HTTPTimeout) - log.Printf("[ProcessDownloadRequest] Downloading content with HTTP timeout=%v", httpTimeout) content, err := DownloadContentWithTimeout(signedURL, httpTimeout) if err != nil { - log.Printf("[ProcessDownloadRequest] ERROR: Failed to download content: %v", err) return "", err } - log.Printf("[ProcessDownloadRequest] Successfully downloaded content (length=%d bytes)", len(content)) return content, nil } -// waitForDownloadURL waits for a NonAdminDownloadRequest to be processed and returns the signed URL -func waitForDownloadURL(ctx context.Context, kbClient kbclient.Client, req *nacv1alpha1.NonAdminDownloadRequest, timeout, pollInterval time.Duration) (string, error) { - log.Printf("[waitForDownloadURL] Starting wait for Name=%q, timeout=%v, pollInterval=%v", req.Name, timeout, pollInterval) +// waitForDownloadURL waits for a NonAdminDownloadRequest to be processed and returns the signed URL. +// If onProgress is provided, it will be called on each polling iteration. +func waitForDownloadURL(ctx context.Context, kbClient kbclient.Client, req *nacv1alpha1.NonAdminDownloadRequest, timeout, pollInterval time.Duration, onProgress func()) (string, error) { timeoutCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() ticker := time.NewTicker(pollInterval) defer ticker.Stop() - pollCount := 0 for { select { case <-timeoutCtx.Done(): - log.Printf("[waitForDownloadURL] TIMEOUT after %d polls", pollCount) return "", fmt.Errorf("timed out waiting for NonAdminDownloadRequest to be processed") case <-ticker.C: - pollCount++ - log.Printf("[waitForDownloadURL] Poll #%d: Getting status for Name=%q", pollCount, req.Name) + if onProgress != nil { + onProgress() + } + var updated nacv1alpha1.NonAdminDownloadRequest if err := kbClient.Get(ctx, kbclient.ObjectKey{ Namespace: req.Namespace, Name: req.Name, }, &updated); err != nil { - log.Printf("[waitForDownloadURL] ERROR on poll #%d: Failed to get NonAdminDownloadRequest: %v", pollCount, err) + // If context expired during Get, let next iteration handle it + if ctx.Err() != nil { + continue + } return "", fmt.Errorf("failed to get NonAdminDownloadRequest: %w", err) } - log.Printf("[waitForDownloadURL] Poll #%d: Got %d conditions", pollCount, len(updated.Status.Conditions)) - // Check if the download request was processed successfully - for i, condition := range updated.Status.Conditions { - log.Printf("[waitForDownloadURL] Poll #%d: Condition[%d] Type=%q, Status=%q, Reason=%q, Message=%q", - pollCount, i, condition.Type, condition.Status, condition.Reason, condition.Message) - + for _, condition := range updated.Status.Conditions { if condition.Type == "Processed" && condition.Status == "True" { if updated.Status.VeleroDownloadRequest.Status.DownloadURL != "" { - log.Printf("[waitForDownloadURL] SUCCESS on poll #%d: Got download URL", pollCount) return updated.Status.VeleroDownloadRequest.Status.DownloadURL, nil - } else { - log.Printf("[waitForDownloadURL] Poll #%d: Processed=True but no DownloadURL yet", pollCount) } } } @@ -213,7 +205,6 @@ func waitForDownloadURL(ctx context.Context, kbClient kbclient.Client, req *nacv // Check for failure conditions for _, condition := range updated.Status.Conditions { if condition.Status == "True" && condition.Reason == "Error" { - log.Printf("[waitForDownloadURL] FAILURE on poll #%d: Error condition found", pollCount) return "", fmt.Errorf("NonAdminDownloadRequest failed: %s - %s", condition.Type, condition.Message) } } @@ -231,23 +222,16 @@ func DownloadContent(url string) (string, error) { // DownloadContentWithTimeout fetches content from a signed URL with a specified timeout. // It handles both gzipped and non-gzipped content automatically. func DownloadContentWithTimeout(url string, timeout time.Duration) (string, error) { - log.Printf("[DownloadContentWithTimeout] Starting download with timeout=%v", timeout) client := httpClientWithTimeout(timeout) - log.Printf("[DownloadContentWithTimeout] Sending HTTP GET request...") resp, err := client.Get(url) if err != nil { - log.Printf("[DownloadContentWithTimeout] ERROR: HTTP GET failed: %v", err) return "", fmt.Errorf("failed to download content from URL %q: %w", url, err) } defer resp.Body.Close() - log.Printf("[DownloadContentWithTimeout] HTTP response: Status=%s, Content-Encoding=%s", - resp.Status, resp.Header.Get("Content-Encoding")) - if resp.StatusCode != http.StatusOK { bodyBytes, _ := io.ReadAll(resp.Body) - log.Printf("[DownloadContentWithTimeout] ERROR: Non-OK status code: %s, body length=%d", resp.Status, len(bodyBytes)) return "", fmt.Errorf("failed to download content: status %s, body: %s", resp.Status, string(bodyBytes)) } @@ -267,15 +251,12 @@ func DownloadContentWithTimeout(url string, timeout time.Duration) (string, erro magicBytes, err := bufReader.Peek(2) if err == nil && len(magicBytes) == 2 && magicBytes[0] == 0x1f && magicBytes[1] == 0x8b { isGzipped = true - log.Printf("[DownloadContentWithTimeout] Detected gzip format by magic bytes") } } if isGzipped { - log.Printf("[DownloadContentWithTimeout] Content is gzipped, creating gzip reader...") gzr, err := gzip.NewReader(bufReader) if err != nil { - log.Printf("[DownloadContentWithTimeout] ERROR: Failed to create gzip reader: %v", err) return "", fmt.Errorf("failed to create gzip reader: %w", err) } defer gzr.Close() @@ -283,14 +264,11 @@ func DownloadContentWithTimeout(url string, timeout time.Duration) (string, erro } // Read all content - log.Printf("[DownloadContentWithTimeout] Reading content from response body...") content, err := io.ReadAll(reader) if err != nil { - log.Printf("[DownloadContentWithTimeout] ERROR: Failed to read content: %v", err) return "", fmt.Errorf("failed to read content: %w", err) } - log.Printf("[DownloadContentWithTimeout] Successfully read %d bytes", len(content)) return string(content), nil } From 311e3b64514bf9a751035b9f8fe226972d27d283 Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 3 Feb 2026 23:16:25 -0500 Subject: [PATCH 3/4] Improve nonadmin backup describe output formatting Align nonadmin backup describe output with admin backup format: - Move Resource List before Backup Volumes section to match admin order - Parse and format Resource List JSON as hierarchical structure with bullets - Skip printing sections with no data (empty snapshots, no errors/warnings) - Parse and format Volume Snapshot Details, Backup Results, and Item Operations as readable JSON - Split Backup Results into separate Errors and Warnings subsections Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- cmd/non-admin/backup/describe.go | 212 ++++++++++++++++++++++--------- 1 file changed, 151 insertions(+), 61 deletions(-) diff --git a/cmd/non-admin/backup/describe.go b/cmd/non-admin/backup/describe.go index 13c0588..5df3a89 100644 --- a/cmd/non-admin/backup/describe.go +++ b/cmd/non-admin/backup/describe.go @@ -2,6 +2,7 @@ package backup import ( "context" + "encoding/json" "fmt" "sort" "strings" @@ -71,7 +72,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { } // Print in Velero-style format - printNonAdminBackupDetails(cmd, &nab) + printNonAdminBackupDetails(cmd, &nab, kbClient, backupName, userNamespace, effectiveTimeout) // Add detailed output if --details flag is set if details { @@ -97,7 +98,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { } // printNonAdminBackupDetails prints backup details in Velero admin describe format -func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup) { +func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration) { out := cmd.OutOrStdout() // Get Velero backup reference if available @@ -321,6 +322,25 @@ func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBac fmt.Fprintf(out, "\n") + // Fetch and display Resource List + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ + BackupName: backupName, + DataType: "BackupResourceList", + Namespace: userNamespace, + HTTPTimeout: timeout, + }) + + if err == nil && resourceList != "" { + if formattedList := formatResourceList(resourceList); formattedList != "" { + fmt.Fprintf(out, "Resource List:\n") + fmt.Fprintf(out, "%s\n", formattedList) + fmt.Fprintf(out, "\n") + } + } + // Backup Volumes fmt.Fprintf(out, "Backup Volumes:\n") @@ -361,9 +381,8 @@ func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBac // printDetailedBackupInfo fetches and displays additional backup details when --details flag is used. // It uses NonAdminDownloadRequest to fetch: -// - BackupResults (errors, warnings) -// - BackupResourceList (inventory by GroupVersionKind) // - BackupVolumeInfos (snapshot details) +// - BackupResults (errors, warnings) // - BackupItemOperations (plugin operations) func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration) error { out := cmd.OutOrStdout() @@ -371,9 +390,9 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backu ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - fmt.Fprintf(out, "\nFetching detailed backup information...\n\n") + hasOutput := false - // 1. Fetch BackupVolumeInfos (most useful for users) + // 1. Fetch BackupVolumeInfos volumeInfo, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ BackupName: backupName, DataType: "BackupVolumeInfos", @@ -382,38 +401,18 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backu }) if err == nil && volumeInfo != "" { - fmt.Fprintf(out, "Volume Snapshot Details:\n") if formattedInfo := formatVolumeInfo(volumeInfo); formattedInfo != "" { - fmt.Fprintf(out, "%s", formattedInfo) - } else { - fmt.Fprintf(out, " \n") - } - fmt.Fprintf(out, "\n") - } else if err != nil { - fmt.Fprintf(out, "Volume Info: \n\n", err) - } - - // 2. Fetch BackupResourceList - resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ - BackupName: backupName, - DataType: "BackupResourceList", - Namespace: userNamespace, - HTTPTimeout: timeout, - }) - - if err == nil && resourceList != "" { - fmt.Fprintf(out, "Resource List:\n") - if formattedList := formatResourceList(resourceList); formattedList != "" { - fmt.Fprintf(out, "%s", formattedList) - } else { - fmt.Fprintf(out, " \n") + if !hasOutput { + fmt.Fprintf(out, "\n") + hasOutput = true + } + fmt.Fprintf(out, "Volume Snapshot Details:\n") + fmt.Fprintf(out, "%s\n", formattedInfo) + fmt.Fprintf(out, "\n") } - fmt.Fprintf(out, "\n") - } else if err != nil { - fmt.Fprintf(out, "Resource List: \n\n", err) } - // 3. Fetch BackupResults + // 2. Fetch BackupResults results, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ BackupName: backupName, DataType: "BackupResults", @@ -422,18 +421,18 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backu }) if err == nil && results != "" { - fmt.Fprintf(out, "Backup Results:\n") if formattedResults := formatBackupResults(results); formattedResults != "" { - fmt.Fprintf(out, "%s", formattedResults) - } else { - fmt.Fprintf(out, " \n") + if !hasOutput { + fmt.Fprintf(out, "\n") + hasOutput = true + } + fmt.Fprintf(out, "Backup Results:\n") + fmt.Fprintf(out, "%s\n", formattedResults) + fmt.Fprintf(out, "\n") } - fmt.Fprintf(out, "\n") - } else if err != nil { - fmt.Fprintf(out, "Backup Results: \n\n", err) } - // 4. Fetch BackupItemOperations + // 3. Fetch BackupItemOperations itemOps, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ BackupName: backupName, DataType: "BackupItemOperations", @@ -442,15 +441,15 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backu }) if err == nil && itemOps != "" { - fmt.Fprintf(out, "Backup Item Operations:\n") if formattedOps := formatItemOperations(itemOps); formattedOps != "" { - fmt.Fprintf(out, "%s", formattedOps) - } else { - fmt.Fprintf(out, " \n") + if !hasOutput { + fmt.Fprintf(out, "\n") + hasOutput = true + } + fmt.Fprintf(out, "Backup Item Operations:\n") + fmt.Fprintf(out, "%s\n", formattedOps) + fmt.Fprintf(out, "\n") } - fmt.Fprintf(out, "\n") - } else if err != nil { - fmt.Fprintf(out, "Backup Item Operations: \n\n", err) } return nil @@ -458,42 +457,133 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backu // formatVolumeInfo formats volume snapshot information for display func formatVolumeInfo(volumeInfo string) string { - // Start with simple indented output - // TODO: Parse JSON/YAML and format nicely (future enhancement) if strings.TrimSpace(volumeInfo) == "" { return "" } - return indent(volumeInfo, " ") + + // Try to parse as JSON array + var snapshots []interface{} + if err := json.Unmarshal([]byte(volumeInfo), &snapshots); err != nil { + // If parsing fails, fall back to indented output + return indent(volumeInfo, " ") + } + + // If empty array, return empty string (will show "") + if len(snapshots) == 0 { + return "" + } + + // Format as indented JSON for readability + formatted, err := json.MarshalIndent(snapshots, " ", " ") + if err != nil { + return indent(volumeInfo, " ") + } + return indent(string(formatted), " ") } // formatResourceList formats the resource list for display func formatResourceList(resourceList string) string { - // Start with simple indented output - // TODO: Parse and group by GroupVersionKind (future enhancement) if strings.TrimSpace(resourceList) == "" { return "" } - return indent(resourceList, " ") + + // Try to parse as JSON map + var resources map[string][]string + if err := json.Unmarshal([]byte(resourceList), &resources); err != nil { + // If parsing fails, fall back to indented output + return indent(resourceList, " ") + } + + // Sort the keys (GroupVersionKind) + keys := make([]string, 0, len(resources)) + for k := range resources { + keys = append(keys, k) + } + sort.Strings(keys) + + // Build formatted output + var output strings.Builder + for _, gvk := range keys { + items := resources[gvk] + output.WriteString(fmt.Sprintf(" %s:\n", gvk)) + for _, item := range items { + output.WriteString(fmt.Sprintf(" - %s\n", item)) + } + } + + return strings.TrimSuffix(output.String(), "\n") } // formatBackupResults formats backup results (errors/warnings) for display func formatBackupResults(results string) string { - // Start with simple indented output - // TODO: Parse JSON and show errors/warnings prominently (future enhancement) if strings.TrimSpace(results) == "" { return "" } - return indent(results, " ") + + // Try to parse as JSON object with errors and warnings + var resultsObj struct { + Errors map[string]interface{} `json:"errors"` + Warnings map[string]interface{} `json:"warnings"` + } + if err := json.Unmarshal([]byte(results), &resultsObj); err != nil { + // If parsing fails, fall back to indented output + return indent(results, " ") + } + + // If both are empty, return empty string so section won't be printed + if len(resultsObj.Errors) == 0 && len(resultsObj.Warnings) == 0 { + return "" + } + + // Format nicely + var output strings.Builder + + // Show errors + output.WriteString(" Errors:\n") + if len(resultsObj.Errors) > 0 { + formatted, _ := json.MarshalIndent(resultsObj.Errors, " ", " ") + output.WriteString(indent(string(formatted), " ")) + } else { + output.WriteString(" ") + } + output.WriteString("\n\n") + + // Show warnings + output.WriteString(" Warnings:\n") + if len(resultsObj.Warnings) > 0 { + formatted, _ := json.MarshalIndent(resultsObj.Warnings, " ", " ") + output.WriteString(indent(string(formatted), " ")) + } else { + output.WriteString(" ") + } + + return strings.TrimSuffix(output.String(), "\n") } // formatItemOperations formats backup item operations for display func formatItemOperations(itemOps string) string { - // Start with simple indented output - // TODO: Parse and show operation details (future enhancement) if strings.TrimSpace(itemOps) == "" { return "" } - return indent(itemOps, " ") + + // Try to parse as JSON array + var operations []interface{} + if err := json.Unmarshal([]byte(itemOps), &operations); err != nil { + // If parsing fails, fall back to indented output + return indent(itemOps, " ") + } + + // If empty array, return empty string (will show "") + if len(operations) == 0 { + return "" + } + + // Format as indented JSON for readability + formatted, err := json.MarshalIndent(operations, " ", " ") + if err != nil { + return indent(itemOps, " ") + } + return indent(string(formatted), " ") } // colorizePhase returns the phase string with ANSI color codes From ab958327b37bf68b96e50f0d4a635955141d59d6 Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 4 Feb 2026 10:17:17 -0500 Subject: [PATCH 4/4] Fix ineffectual assignment in backup describe command Remove unused assignment to hasOutput variable in printDetailedBackupInfo function. The assignment at line 447 had no effect since it was in the last section and the variable was never used afterward. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- cmd/non-admin/backup/describe.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/non-admin/backup/describe.go b/cmd/non-admin/backup/describe.go index 5df3a89..22214b2 100644 --- a/cmd/non-admin/backup/describe.go +++ b/cmd/non-admin/backup/describe.go @@ -444,7 +444,6 @@ func printDetailedBackupInfo(cmd *cobra.Command, kbClient kbclient.Client, backu if formattedOps := formatItemOperations(itemOps); formattedOps != "" { if !hasOutput { fmt.Fprintf(out, "\n") - hasOutput = true } fmt.Fprintf(out, "Backup Item Operations:\n") fmt.Fprintf(out, "%s\n", formattedOps)