From 105350b78b56c0a18d395baa46bb6344d89f753a Mon Sep 17 00:00:00 2001 From: Ralthos <161431341+Ralthos@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:03:02 +0530 Subject: [PATCH] Make restore logs testable by returning errors (#10234) pkg/cmd/cli/restore/logs.go was the last command in the CLI still calling cmd.Exit, which calls os.Exit directly. Two of its own tests were skipped because of it, and said so: t.Skip("Cannot test restore not complete case due to cmd.Exit() call") This gives restore logs the LogsOptions shape that backup logs already uses: Complete, BindFlags and Run returning an error, with the cobra command passing that to cmd.CheckError. Both skipped tests now run and assert on the returned errors. Exit status is unchanged; cmd.CheckError also exits 1. The two refusal messages now carry the standard "An error occurred:" prefix and match the wording backup logs uses. Signed-off-by: saral --- changelogs/unreleased/10234-Ralthos | 1 + pkg/cmd/cli/restore/logs.go | 108 ++++++++++++++++++---------- pkg/cmd/cli/restore/logs_test.go | 30 +++++--- 3 files changed, 95 insertions(+), 44 deletions(-) create mode 100644 changelogs/unreleased/10234-Ralthos diff --git a/changelogs/unreleased/10234-Ralthos b/changelogs/unreleased/10234-Ralthos new file mode 100644 index 000000000..70ac3000a --- /dev/null +++ b/changelogs/unreleased/10234-Ralthos @@ -0,0 +1 @@ +Make velero restore logs return errors instead of calling os.Exit directly, matching velero backup logs, and enable the two previously skipped tests diff --git a/pkg/cmd/cli/restore/logs.go b/pkg/cmd/cli/restore/logs.go index 26d3123ac..366fd511e 100644 --- a/pkg/cmd/cli/restore/logs.go +++ b/pkg/cmd/cli/restore/logs.go @@ -23,8 +23,9 @@ import ( "time" "github.com/spf13/cobra" + "github.com/spf13/pflag" apierrors "k8s.io/apimachinery/pkg/api/errors" - ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/client" @@ -34,59 +35,94 @@ import ( "github.com/vmware-tanzu/velero/pkg/cmd/util/downloadrequest" ) -func NewLogsCommand(f client.Factory) *cobra.Command { +// LogsOptions holds the state for the restore logs command, mirroring +// pkg/cmd/cli/backup.LogsOptions so both commands are shaped the same way. +type LogsOptions struct { + Timeout time.Duration + InsecureSkipTLSVerify bool + CaCertFile string + Client kbclient.Client + RestoreName string +} + +func NewLogsOptions() LogsOptions { config, err := client.LoadConfig() if err != nil { fmt.Fprintf(os.Stderr, "WARNING: Error reading config file: %v\n", err) } - timeout := time.Minute - insecureSkipTLSVerify := false - caCertFile := config.CACertFile() + return LogsOptions{ + Timeout: time.Minute, + InsecureSkipTLSVerify: false, + CaCertFile: config.CACertFile(), + } +} + +func (l *LogsOptions) BindFlags(flags *pflag.FlagSet) { + flags.DurationVar(&l.Timeout, "timeout", l.Timeout, "How long to wait to receive logs.") + flags.BoolVar(&l.InsecureSkipTLSVerify, "insecure-skip-tls-verify", l.InsecureSkipTLSVerify, "If true, the object store's TLS certificate will not be checked for validity. This is insecure and susceptible to man-in-the-middle attacks. Not recommended for production.") + flags.StringVar(&l.CaCertFile, "cacert", l.CaCertFile, "Path to a certificate bundle to use when verifying TLS connections. If not specified, the CA certificate from the BackupStorageLocation will be used if available.") +} + +func (l *LogsOptions) Run(c *cobra.Command, f client.Factory) error { + restore := new(velerov1api.Restore) + err := l.Client.Get(context.Background(), kbclient.ObjectKey{Namespace: f.Namespace(), Name: l.RestoreName}, restore) + if apierrors.IsNotFound(err) { + return fmt.Errorf("restore %q does not exist", l.RestoreName) + } else if err != nil { + return fmt.Errorf("error checking for restore %q: %v", l.RestoreName, err) + } + + switch restore.Status.Phase { + case velerov1api.RestorePhaseCompleted, velerov1api.RestorePhaseFailed, velerov1api.RestorePhasePartiallyFailed, velerov1api.RestorePhaseWaitingForPluginOperations, velerov1api.RestorePhaseWaitingForPluginOperationsPartiallyFailed: + // terminal and waiting for plugin operations phases, do nothing. + default: + return fmt.Errorf("logs for restore %q are not available until it's finished processing, please wait "+ + "until the restore has a phase of Completed or Failed and try again", l.RestoreName) + } + + // Get BSL cacert if available + bslCACert, err := cacert.GetCACertFromRestore(context.Background(), l.Client, f.Namespace(), restore) + if err != nil { + // Log the error but don't fail - we can still try to download without the BSL cacert + fmt.Fprintf(os.Stderr, "WARNING: Error getting cacert from BSL: %v\n", err) + bslCACert = "" + } + + return downloadrequest.StreamWithBSLCACert(context.Background(), l.Client, f.Namespace(), l.RestoreName, velerov1api.DownloadTargetKindRestoreLog, os.Stdout, l.Timeout, l.InsecureSkipTLSVerify, l.CaCertFile, bslCACert) +} + +func (l *LogsOptions) Complete(args []string, f client.Factory) error { + if len(args) > 0 { + l.RestoreName = args[0] + } + + kbClient, err := f.KubebuilderClient() + if err != nil { + return err + } + l.Client = kbClient + return nil +} + +func NewLogsCommand(f client.Factory) *cobra.Command { + l := NewLogsOptions() c := &cobra.Command{ Use: "logs RESTORE", Short: "Get restore logs", Args: cobra.ExactArgs(1), Run: func(c *cobra.Command, args []string) { - restoreName := args[0] - - kbClient, err := f.KubebuilderClient() + err := l.Complete(args, f) cmd.CheckError(err) - restore := new(velerov1api.Restore) - err = kbClient.Get(context.Background(), ctrlclient.ObjectKey{Namespace: f.Namespace(), Name: restoreName}, restore) - if apierrors.IsNotFound(err) { - cmd.Exit("Restore %q does not exist.", restoreName) - } else if err != nil { - cmd.Exit("Error checking for restore %q: %v", restoreName, err) - } - - switch restore.Status.Phase { - case velerov1api.RestorePhaseCompleted, velerov1api.RestorePhaseFailed, velerov1api.RestorePhasePartiallyFailed, velerov1api.RestorePhaseWaitingForPluginOperations, velerov1api.RestorePhaseWaitingForPluginOperationsPartiallyFailed: - // terminal and waiting for plugin operations phases, don't exit. - default: - cmd.Exit("Logs for restore %q are not available until it's finished processing. Please wait "+ - "until the restore has a phase of Completed or Failed and try again.", restoreName) - } - - // Get BSL cacert if available - bslCACert, err := cacert.GetCACertFromRestore(context.Background(), kbClient, f.Namespace(), restore) - if err != nil { - // Log the error but don't fail - we can still try to download without the BSL cacert - fmt.Fprintf(os.Stderr, "WARNING: Error getting cacert from BSL: %v\n", err) - bslCACert = "" - } - - err = downloadrequest.StreamWithBSLCACert(context.Background(), kbClient, f.Namespace(), restoreName, velerov1api.DownloadTargetKindRestoreLog, os.Stdout, timeout, insecureSkipTLSVerify, caCertFile, bslCACert) + err = l.Run(c, f) cmd.CheckError(err) }, } c.ValidArgsFunction = cli.CompleteRestoreNames(f) - c.Flags().DurationVar(&timeout, "timeout", timeout, "How long to wait to receive logs.") - c.Flags().BoolVar(&insecureSkipTLSVerify, "insecure-skip-tls-verify", insecureSkipTLSVerify, "If true, the object store's TLS certificate will not be checked for validity. This is insecure and susceptible to man-in-the-middle attacks. Not recommended for production.") - c.Flags().StringVar(&caCertFile, "cacert", caCertFile, "Path to a certificate bundle to use when verifying TLS connections. If not specified, the CA certificate from the BackupStorageLocation will be used if available.") + l.BindFlags(c.Flags()) return c } diff --git a/pkg/cmd/cli/restore/logs_test.go b/pkg/cmd/cli/restore/logs_test.go index 61c2392b6..5e020bf43 100644 --- a/pkg/cmd/cli/restore/logs_test.go +++ b/pkg/cmd/cli/restore/logs_test.go @@ -17,10 +17,12 @@ limitations under the License. package restore import ( + "fmt" "os" "testing" "time" + flag "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" kbclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -77,13 +79,20 @@ func TestNewLogsCommand(t *testing.T) { c := NewLogsCommand(f) assert.Equal(t, "Get restore logs", c.Short) - // The restore command exits with an error message when restore is not complete - // We can't easily test this since it calls cmd.Exit, which exits the process - // So we'll skip this test case - t.Skip("Cannot test restore not complete case due to cmd.Exit() call") + l := NewLogsOptions() + flags := new(flag.FlagSet) + l.BindFlags(flags) + err = l.Complete([]string{restoreName}, f) + require.NoError(t, err) + + err = l.Run(c, f) + require.Error(t, err) + require.ErrorContains(t, err, fmt.Sprintf("logs for restore %q are not available until it's finished processing", restoreName)) }) t.Run("Restore not exist test", func(t *testing.T) { + restoreName := "not-exist" + // create a factory f := &factorymocks.Factory{} @@ -95,10 +104,15 @@ func TestNewLogsCommand(t *testing.T) { c := NewLogsCommand(f) assert.Equal(t, "Get restore logs", c.Short) - // The restore command exits with an error message when restore doesn't exist - // We can't easily test this since it calls cmd.Exit, which exits the process - // So we'll skip this test case - t.Skip("Cannot test restore not exist case due to cmd.Exit() call") + l := NewLogsOptions() + flags := new(flag.FlagSet) + l.BindFlags(flags) + err := l.Complete([]string{restoreName}, f) + require.NoError(t, err) + + err = l.Run(c, f) + require.Error(t, err) + require.Equal(t, fmt.Sprintf("restore %q does not exist", restoreName), err.Error()) }) t.Run("Restore with BSL cacert test", func(t *testing.T) {