fix: reject an empty --plugin-config and stage plugin restore directories

backup/delete_backup.go

@@ -117,6 +117,7 @@ func DoDeleteBackupInit(cmd *cobra.Command) {
 
 func DoDeleteBackup(timestamp string) {
 	SetLoggerVerbosity()
+	ValidatePluginConfigFlag()
 
 	if !filepath.IsValidTimestamp(timestamp) {
 		gplog.Fatal(errors.Errorf("Invalid timestamp: %s", timestamp), "")

backup/delete_backup_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/greenplum-db/gpbackup/history"
 	"github.com/greenplum-db/gpbackup/utils"
+	"github.com/spf13/cobra"
 	"github.com/warehouse-pg/common-go-libs/cluster"
 	"github.com/warehouse-pg/common-go-libs/testhelper"
 
@@ -442,6 +443,38 @@ var _ = Describe("delete-backup internal tests", func() {
 		})
 	})
 
+	Describe("ValidatePluginConfigFlag", func() {
+		// delete-backup and delete-backups-before never run DoFlagValidation, so
+		// they call this guard themselves; exercise it through their own flag set.
+		runWithArgs := func(args []string) error {
+			testCmd := &cobra.Command{
+				Use:  "delete-backup",
+				Args: cobra.NoArgs,
+				Run: func(cmd *cobra.Command, _ []string) {
+					UseCmdFlags(cmd.Flags())
+					ValidatePluginConfigFlag()
+				}}
+			RegisterDeleteBackupFlags(testCmd.Flags())
+			testCmd.SetArgs(args)
+			return testCmd.Execute()
+		}
+
+		It("fails when --plugin-config is specified with an empty value", func() {
+			defer testhelper.ShouldPanicWithMessage("The --plugin-config flag was specified with an empty value")
+			if err := runWithArgs([]string{"--plugin-config", ""}); err == nil {
+				Fail("empty --plugin-config value passed validation check")
+			}
+		})
+
+		It("passes when --plugin-config is specified with a path", func() {
+			Expect(runWithArgs([]string{"--plugin-config", "/etc/ddboost_config.yaml"})).To(Succeed())
+		})
+
+		It("passes when --plugin-config is not specified", func() {
+			Expect(runWithArgs([]string{})).To(Succeed())
+		})
+	})
+
 	Describe("newlyAddedDependents", func() {
 		It("returns nothing when recheck matches the original set", func() {
 			original := []*history.BackupConfig{{Timestamp: "20260101000000"}, {Timestamp: "20260101010000"}}

backup/delete_backups_before.go

@@ -29,6 +29,7 @@ func DoDeleteBackupsBeforeInit(cmd *cobra.Command) {
 // delete-backup --cascade.
 func DoDeleteBackupsBefore(cutoff string) {
 	SetLoggerVerbosity()
+	ValidatePluginConfigFlag()
 
 	if !filepath.IsValidTimestamp(cutoff) {
 		gplog.Fatal(errors.Errorf("Invalid timestamp: %s", cutoff), "")

backup/validate.go

@@ -164,7 +164,26 @@ func validateFlagCombinations(flags *pflag.FlagSet) {
 	}
 }
 
+/*
+ * ValidatePluginConfigFlag rejects --plugin-config given with an empty value.
+ * ValidateFullPath accepts the empty string and every command that takes this
+ * flag treats an empty plugin config as "no plugin at all", so an unset or
+ * mistyped shell variable would otherwise silently write the backup to local
+ * disk instead of to the plugin's storage -- which can fill up the segment
+ * hosts -- or delete only the local half of a backup set.
+ *
+ * Call this from any command that registers --plugin-config: gpbackup itself
+ * (via validateFlagValues), delete-backup and delete-backups-before.  It must
+ * run after UseCmdFlags has pointed cmdFlags at the subcommand's flag set.
+ */
+func ValidatePluginConfigFlag() {
+	if FlagChanged(options.PLUGIN_CONFIG) && MustGetFlagString(options.PLUGIN_CONFIG) == "" {
+		gplog.Fatal(errors.Errorf("The --plugin-config flag was specified with an empty value. Specify the absolute path to the plugin configuration file, or omit the flag to use local storage."), "")
+	}
+}
+
 func validateFlagValues() {
+	ValidatePluginConfigFlag()
 	err := utils.ValidateFullPath(MustGetFlagString(options.BACKUP_DIR))
 	gplog.FatalOnError(err)
 	err = utils.ValidateFullPath(MustGetFlagString(options.PLUGIN_CONFIG))

backup/validate_test.go

@@ -215,6 +215,8 @@ var _ = Describe("backup/validate tests", func() {
 				}
 			},
 			Entry("--backup-dir combo", "--backup-dir /tmp --plugin-config /tmp/config", false),
+			Entry("--plugin-config with an empty value", "--plugin-config ", false),
+			Entry("--plugin-config with a path", "--plugin-config /tmp/config", true),
 
 			/*
 			 * Below are all the different filter combinations

restore/remote.go

@@ -17,25 +17,54 @@ import (
  * Functions to run commands on entire cluster during restore
  */
 
-func VerifyBackupDirectoriesExistOnAllHosts() {
-	_, err := globalCluster.ExecuteLocalCommand(fmt.Sprintf("test -d %s", globalFPInfo.GetDirForContent(-1)))
-	gplog.FatalOnError(err, "Backup directory %s missing or inaccessible", globalFPInfo.GetDirForContent(-1))
-	if MustGetFlagString(options.PLUGIN_CONFIG) == "" || backupConfig.SingleDataFile {
-		origSize, destSize, isResizeRestore, _ := GetResizeClusterInfo()
-
-		remoteOutput := globalCluster.GenerateAndExecuteCommand("Verifying backup directories exist", cluster.ON_SEGMENTS, func(contentID int) string {
-			if isResizeRestore { // Map origin content to destination content to find where the original files have been placed
-				if contentID >= origSize { // Don't check for directories for contents that aren't part of the backup set
-					return ""
-				}
-				contentID = contentID % destSize
-			}
-			return fmt.Sprintf("test -d %s", globalFPInfo.GetDirForContent(contentID))
-		})
-		globalCluster.CheckClusterError(remoteOutput, "Backup directories missing or inaccessible", func(contentID int) string {
-			return fmt.Sprintf("Backup directory %s missing or inaccessible", globalFPInfo.GetDirForContent(contentID))
-		})
+/*
+ * EnsureBackupDirectoriesExistOnAllHosts checks, or creates, the backup
+ * directories the restore is about to use.
+ *
+ * Without a plugin those directories hold the backup itself, so a missing one
+ * means the data is gone and the restore must stop.  With a plugin the backup
+ * lives on the plugin's storage and the local directories are only staging
+ * areas for files gprestore downloads into them, so they are created here
+ * instead of being required.  That is what lets a backup be restored onto a
+ * host that was rebuilt or replaced after the backup was taken, without having
+ * to reconstruct the old directory tree by hand first.
+ */
+func EnsureBackupDirectoriesExistOnAllHosts() {
+	usingPlugin := MustGetFlagString(options.PLUGIN_CONFIG) != ""
+
+	coordinatorDir := globalFPInfo.GetDirForContent(-1)
+	if usingPlugin {
+		_, err := globalCluster.ExecuteLocalCommand(fmt.Sprintf("mkdir -p %s", coordinatorDir))
+		gplog.FatalOnError(err, "Unable to create backup directory %s", coordinatorDir)
+	} else {
+		_, err := globalCluster.ExecuteLocalCommand(fmt.Sprintf("test -d %s", coordinatorDir))
+		gplog.FatalOnError(err, "Backup directory %s missing or inaccessible", coordinatorDir)
 	}
+
+	// The segment directories are needed for every plugin restore, not just the
+	// single-data-file ones: a resize restore stages the helper's pipes, oid
+	// files and segment TOCs there as well.
+	dirCommand, verboseMsg, errMsg := "test -d", "Verifying backup directories exist", "Backup directories missing or inaccessible"
+	if usingPlugin {
+		dirCommand, verboseMsg, errMsg = "mkdir -p", "Creating backup directories", "Unable to create backup directories"
+	}
+	origSize, destSize, isResizeRestore, _ := GetResizeClusterInfo()
+
+	remoteOutput := globalCluster.GenerateAndExecuteCommand(verboseMsg, cluster.ON_SEGMENTS, func(contentID int) string {
+		if isResizeRestore { // Map origin content to destination content to find where the original files have been placed
+			if contentID >= origSize { // Don't check for directories for contents that aren't part of the backup set
+				return ""
+			}
+			contentID = contentID % destSize
+		}
+		return fmt.Sprintf("%s %s", dirCommand, globalFPInfo.GetDirForContent(contentID))
+	})
+	globalCluster.CheckClusterError(remoteOutput, errMsg, func(contentID int) string {
+		if usingPlugin {
+			return fmt.Sprintf("Unable to create backup directory %s", globalFPInfo.GetDirForContent(contentID))
+		}
+		return fmt.Sprintf("Backup directory %s missing or inaccessible", globalFPInfo.GetDirForContent(contentID))
+	})
 }
 
 func VerifyBackupFileCountOnSegments() {

restore/remote_test.go

@@ -117,4 +117,49 @@ var _ = Describe("restore/remote tests", func() {
 			restore.VerifyBackupFileCountOnSegments()
 		})
 	})
+	Describe("EnsureBackupDirectoriesExistOnAllHosts", func() {
+		BeforeEach(func() {
+			testExecutor.ClusterOutput = &cluster.RemoteOutput{NumErrors: 0}
+			testCluster.Executor = testExecutor
+			restore.SetCluster(testCluster)
+			restore.SetBackupConfig(&history.BackupConfig{SegmentCount: 2})
+		})
+		It("requires the directories to exist when no plugin is in use", func() {
+			restore.EnsureBackupDirectoriesExistOnAllHosts()
+
+			Expect(testExecutor.LocalCommands[0]).To(Equal("test -d /data/gpseg-1/backups/20170101/20170101010101"))
+			cc := testExecutor.ClusterCommands[0]
+			Expect(cc).To(HaveLen(2))
+			Expect(cc[0].CommandString).To(ContainSubstring("test -d /data/gpseg0/backups/20170101/20170101010101"))
+			Expect(cc[1].CommandString).To(ContainSubstring("test -d /data/gpseg1/backups/20170101/20170101010101"))
+		})
+		It("creates the directories when a plugin is in use", func() {
+			// With a plugin these directories only stage files downloaded from the
+			// plugin's storage, so a host rebuilt since the backup must not fail here.
+			cmdFlags.Set(options.PLUGIN_CONFIG, "/tmp/plugin_config.yaml")
+			restore.SetBackupConfig(&history.BackupConfig{SegmentCount: 2, SingleDataFile: true})
+
+			restore.EnsureBackupDirectoriesExistOnAllHosts()
+
+			Expect(testExecutor.LocalCommands[0]).To(Equal("mkdir -p /data/gpseg-1/backups/20170101/20170101010101"))
+			cc := testExecutor.ClusterCommands[0]
+			Expect(cc).To(HaveLen(2))
+			Expect(cc[0].CommandString).To(ContainSubstring("mkdir -p /data/gpseg0/backups/20170101/20170101010101"))
+			Expect(cc[1].CommandString).To(ContainSubstring("mkdir -p /data/gpseg1/backups/20170101/20170101010101"))
+		})
+		It("creates the segment directories for a plugin backup taken without --single-data-file", func() {
+			// This combination used to skip the segments entirely, leaving the staging
+			// directory missing for the helper files a resize restore puts there.
+			cmdFlags.Set(options.PLUGIN_CONFIG, "/tmp/plugin_config.yaml")
+			restore.SetBackupConfig(&history.BackupConfig{SegmentCount: 2, SingleDataFile: false})
+
+			restore.EnsureBackupDirectoriesExistOnAllHosts()
+
+			Expect(testExecutor.NumClusterExecutions).To(Equal(1))
+			cc := testExecutor.ClusterCommands[0]
+			Expect(cc).To(HaveLen(2))
+			Expect(cc[0].CommandString).To(ContainSubstring("mkdir -p /data/gpseg0/backups/20170101/20170101010101"))
+			Expect(cc[1].CommandString).To(ContainSubstring("mkdir -p /data/gpseg1/backups/20170101/20170101010101"))
+		})
+	})
 })

restore/restore.go

@@ -39,6 +39,7 @@ func DoInit(cmd *cobra.Command) {
  */
 func DoValidation(cmd *cobra.Command) {
 	ValidateFlagCombinations(cmd)
+	ValidatePluginConfigFlag()
 	err := utils.ValidateFullPath(MustGetFlagString(options.BACKUP_DIR))
 	gplog.FatalOnError(err)
 	err = utils.ValidateFullPath(MustGetFlagString(options.PLUGIN_CONFIG))
@@ -93,8 +94,10 @@ func DoSetup() {
 
 	// Get restore metadata from plugin
 	if MustGetFlagString(options.PLUGIN_CONFIG) != "" {
+		gplog.Info("Restoring metadata files using plugin config %s", MustGetFlagString(options.PLUGIN_CONFIG))
 		RecoverMetadataFilesUsingPlugin()
 	} else {
+		gplog.Info("No plugin configured; expecting all backup files on local disk under %s", globalFPInfo.GetDirForContent(-1))
 		InitializeBackupConfig()
 	}
 

restore/validate.go

@@ -282,10 +282,29 @@ func ValidateBackupFlagCombinations() {
 	if !backupConfig.SingleDataFile && FlagChanged(options.COPY_QUEUE_SIZE) {
 		gplog.Fatal(errors.Errorf("The --copy-queue-size flag can only be used if the backup was taken with --single-data-file"), "")
 	}
-	validateBackupFlagPluginCombinations()
 }
 
-func validateBackupFlagPluginCombinations() {
+/*
+ * ValidatePluginConfigFlag rejects --plugin-config given with an empty value.
+ * ValidateFullPath accepts the empty string, and DoSetup treats an empty plugin
+ * config as "no plugin at all", so without this check an unset or mistyped shell
+ * variable silently downgrades a plugin restore to a local-filesystem restore
+ * and then fails on the first backup file that is not on local disk.
+ */
+func ValidatePluginConfigFlag() {
+	if FlagChanged(options.PLUGIN_CONFIG) && MustGetFlagString(options.PLUGIN_CONFIG) == "" {
+		gplog.Fatal(errors.Errorf("The --plugin-config flag was specified with an empty value. Specify the absolute path to the plugin configuration file, or omit the flag to restore from local disk."), "")
+	}
+}
+
+/*
+ * ValidateBackupFlagPluginCombinations must be called before any check that
+ * assumes the backup files are on local disk.  A backup taken with a plugin
+ * stores its files on the plugin's storage, so if the plugin is missing here the
+ * useful error is "the --plugin-config flag must be used", not a report of a
+ * missing local directory or file.
+ */
+func ValidateBackupFlagPluginCombinations() {
 	if MustGetFlagBool(options.IGNORE_PLUGIN_CONFIG) {
 		// No-op against a backup taken without a plugin; the flag just means
 		// "do not invoke any plugin during restore", which is already the

restore/validate_test.go

@@ -421,7 +421,7 @@ var _ = Describe("restore/validate tests", func() {
 				Use:  "flag validation",
 				Args: cobra.NoArgs,
 				Run: func(cmd *cobra.Command, args []string) {
-					restore.ValidateBackupFlagCombinations()
+					restore.ValidateBackupFlagPluginCombinations()
 				}}
 			testCmd.SetArgs([]string{"--ignore-plugin-config"})
 			restore.SetCmdFlags(testCmd.Flags())
@@ -435,11 +435,72 @@ var _ = Describe("restore/validate tests", func() {
 				Use:  "flag validation",
 				Args: cobra.NoArgs,
 				Run: func(cmd *cobra.Command, args []string) {
-					restore.ValidateBackupFlagCombinations()
+					restore.ValidateBackupFlagPluginCombinations()
 				}}
 			testCmd.SetArgs([]string{"--ignore-plugin-config"})
 			restore.SetCmdFlags(testCmd.Flags())
 
+			err := testCmd.Execute()
+			Expect(err).ToNot(HaveOccurred())
+		})
+		It("should fatal when the backup was taken with a plugin but no plugin config is given", func() {
+			restore.SetBackupConfig(&history.BackupConfig{Plugin: "/tmp/gpbackup_fake_plugin"})
+			testCmd := &cobra.Command{
+				Use:  "flag validation",
+				Args: cobra.NoArgs,
+				Run: func(cmd *cobra.Command, args []string) {
+					restore.ValidateBackupFlagPluginCombinations()
+				}}
+			testCmd.SetArgs([]string{})
+			restore.SetCmdFlags(testCmd.Flags())
+
+			defer testhelper.ShouldPanicWithMessage("The --plugin-config flag must be used to restore")
+			err := testCmd.Execute()
+			if err == nil {
+				Fail("restore of a plugin backup without --plugin-config passed validation check")
+			}
+		})
+	})
+	Describe("ValidatePluginConfigFlag", func() {
+		It("should fatal when --plugin-config is specified with an empty value", func() {
+			testCmd := &cobra.Command{
+				Use:  "flag validation",
+				Args: cobra.NoArgs,
+				Run: func(cmd *cobra.Command, args []string) {
+					restore.ValidatePluginConfigFlag()
+				}}
+			testCmd.SetArgs([]string{"--plugin-config", ""})
+			restore.SetCmdFlags(testCmd.Flags())
+
+			defer testhelper.ShouldPanicWithMessage("The --plugin-config flag was specified with an empty value")
+			err := testCmd.Execute()
+			if err == nil {
+				Fail("empty --plugin-config value passed validation check")
+			}
+		})
+		It("should pass when --plugin-config is specified with a path", func() {
+			testCmd := &cobra.Command{
+				Use:  "flag validation",
+				Args: cobra.NoArgs,
+				Run: func(cmd *cobra.Command, args []string) {
+					restore.ValidatePluginConfigFlag()
+				}}
+			testCmd.SetArgs([]string{"--plugin-config", "/tmp/plugin_config.yaml"})
+			restore.SetCmdFlags(testCmd.Flags())
+
+			err := testCmd.Execute()
+			Expect(err).ToNot(HaveOccurred())
+		})
+		It("should pass when --plugin-config is not specified at all", func() {
+			testCmd := &cobra.Command{
+				Use:  "flag validation",
+				Args: cobra.NoArgs,
+				Run: func(cmd *cobra.Command, args []string) {
+					restore.ValidatePluginConfigFlag()
+				}}
+			testCmd.SetArgs([]string{})
+			restore.SetCmdFlags(testCmd.Flags())
+
 			err := testCmd.Execute()
 			Expect(err).ToNot(HaveOccurred())
 		})

restore/wrappers.go

@@ -163,9 +163,14 @@ func InitializeBackupConfig() {
 }
 
 func BackupConfigurationValidation() {
+	// This must run first: with no plugin in effect the checks below assume the
+	// backup files are on local disk, and would report a missing directory or
+	// file instead of the missing --plugin-config flag.
+	ValidateBackupFlagPluginCombinations()
+
 	if !backupConfig.MetadataOnly {
 		gplog.Verbose("Gathering information on backup directories")
-		VerifyBackupDirectoriesExistOnAllHosts()
+		EnsureBackupDirectoriesExistOnAllHosts()
 	}
 
 	VerifyMetadataFilePaths(MustGetFlagBool(options.WITH_STATS))