In SQLite state, use defaults for empty-string checks

As part of our database init, we perform a check of the current
values for a few fields (graph driver, graph root, static dir,
and a few more) to validate that Libpod is being started with a
sane & sensible config, and the user's containers can actually be
expected to work. Basically, we take the current runtime config
and compare against values cached in the database from the first
time Podman was run.

We've had some issues with this logic before this year around
symlink resolution, but this is a new edge case. Somehow, the
database is being loaded with the empty string for some fields
(at least graph driver) which is causing comparisons to fail
because we will never compare against "" for those fields - we
insert the default value instead, assuming we have one.

Having a value of "" in the database largely invalidates the
check so arguably we could just drop it, but what BoltDB did -
and what SQLite does after this patch - is to use the default
value for comparison instead of "". This should still catch some
edge cases, and shouldn't be too harmful.

What this does not do is identify or solve the reason that we are
seeing the empty string in the database at all. From my read on
the logic, it must mean that the graph driver is explicitly set
to "" in the c/storage config at the time Podman is first run and
I'm not precisely sure how that happens.

Fixes #24738

Signed-off-by: Matt Heon <[email protected]>

libpod/sqlite_state.go

@@ -383,6 +383,30 @@ func (s *SQLiteState) ValidateDBConfig(runtime *Runtime) (defErr error) {
 		return fmt.Errorf("retrieving DB config: %w", err)
 	}
 
+	// Sometimes, for as-yet unclear reasons, the database value ends up set
+	// to the empty string. If it does, this evaluation is always going to
+	// fail, and libpod will be unusable.
+	// At this point, the check is effectively meaningless - we don't
+	// actually know the settings we should be checking against. The best
+	// thing we can do (and what BoltDB did in this case) is to compare
+	// against the default, on the assumption that is what was in use.
+	// TODO: We can't remove this code without breaking existing SQLite DBs
+	// that already have incorrect values in the database, but we should
+	// investigate why this is happening and try and prevent the creation of
+	// new databases with these garbage checks.
+	if graphRoot == "" {
+		logrus.Debugf("Database uses empty-string graph root, substituting default %q", storeOpts.GraphRoot)
+		graphRoot = storeOpts.GraphRoot
+	}
+	if runRoot == "" {
+		logrus.Debugf("Database uses empty-string run root, substituting default %q", storeOpts.RunRoot)
+		runRoot = storeOpts.RunRoot
+	}
+	if graphDriver == "" {
+		logrus.Debugf("Database uses empty-string graph driver, substituting default %q", storeOpts.GraphDriverName)
+		graphDriver = storeOpts.GraphDriverName
+	}
+
 	checkField := func(fieldName, dbVal, ourVal string, isPath bool) error {
 		if isPath {
 			// Tolerate symlinks when possible - most relevant for OStree systems

test/e2e/info_test.go

@@ -291,4 +291,52 @@ var _ = Describe("Podman Info", func() {
 		Expect(info).ToNot(ExitCleanly())
 		podmanTest.StartRemoteService() // Start service again so teardown runs clean
 	})
+
+	It("podman startup: ensure database checks for graph driver accommodate empty string", func() {
+		// We need to start with a bare storage for this test
+		reset := podmanTest.Podman([]string{"system", "reset", "--force"})
+		reset.WaitWithDefaultTimeout()
+		Expect(reset).To(ExitCleanly())
+
+		// We will use two storage.conf files: one with no graph driver,
+		// one with overlay.
+
+		configOnePath := filepath.Join(podmanTest.TempDir, ".config", "containers", "storage.conf")
+		configTwoPath := filepath.Join(podmanTest.TempDir, ".config", "containers2", "storage.conf")
+		os.Setenv("CONTAINERS_STORAGE_CONF", configOnePath)
+		defer func() {
+			os.Unsetenv("CONTAINERS_STORAGE_CONF")
+		}()
+
+		err := os.RemoveAll(filepath.Dir(configOnePath))
+		Expect(err).ToNot(HaveOccurred())
+		err := os.RemoveAll(filepath.Dir(configTwoPath))
+		Expect(err).ToNot(HaveOccurred())
+
+		err = os.MkdirAll(filepath.Dir(configOnePath), os.ModePerm)
+		Expect(err).ToNot(HaveOccurred())
+		err = os.MkdirAll(filepath.Dir(configTwoPath), os.ModePerm)
+		Expect(err).ToNot(HaveOccurred())
+
+		storageConfOne := []byte(fmt.Sprintf("[storage]\n[storage.options]\n", driver, rootlessStoragePath))
+		err = os.WriteFile(configOnePath, storageConfOne, os.ModePerm)
+		Expect(err).ToNot(HaveOccurred())
+
+		storageConfTwo := []byte(fmt.Sprintf("[storage]\ndriver=\"overlay\"\n[storage.options]\n", driver, rootlessStoragePath))
+		err = os.WriteFile(configTwoPath, storageConfTwo, os.ModePerm)
+		Expect(err).ToNot(HaveOccurred())
+
+		// Run any command to init storage and the database with an empty string driver
+		info1 := podmanTest.Podman([]string{"info"})
+		info1.WaitWithDefaultTimeout()
+		Expect(info1).To(ExitCleanly())
+
+		// Change to second storage.conf with overlay driver
+		os.Setenv("CONTAINERS_STORAGE_CONF", configTwoPath)
+
+		// Run another command. It should succeed.
+		info2 := podmanTest.Podman([]string{"info"})
+		info2.WaitWithDefaultTimeout()
+		Expect(info2).To(ExitCleanly())
+	})
 })