database: fix reload to not fail early (#29519)

* database: fix reload to not fail early

* return logical.ErrorRresponse; add tests

* do not return noop warnings; add logs

* changelog

* use name for log; remove event doc

builtin/logical/database/path_config_connection.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"net/url"
 	"sort"
-	"strings"
 
 	"github.com/fatih/structs"
 	"github.com/hashicorp/go-uuid"
@@ -177,6 +176,7 @@ func (b *databaseBackend) reloadPlugin() framework.OperationFunc {
 			return nil, err
 		}
 		reloaded := []string{}
+		reloadFailed := []string{}
 		for _, connName := range connNames {
 			entry, err := req.Storage.Get(ctx, fmt.Sprintf("config/%s", connName))
 			if err != nil {
@@ -192,15 +192,14 @@ func (b *databaseBackend) reloadPlugin() framework.OperationFunc {
 			}
 			if config.PluginName == pluginName {
 				if err := b.reloadConnection(ctx, req.Storage, connName); err != nil {
-					var successfullyReloaded string
+					b.Logger().Error("failed to reload connection", "name", connName, "error", err)
-					if len(reloaded) > 0 {
+					b.dbEvent(ctx, "reload-connection-fail", req.Path, "", false, "name", connName)
-						successfullyReloaded = fmt.Sprintf("successfully reloaded %d connection(s): %s; ",
+					reloadFailed = append(reloadFailed, connName)
-							len(reloaded),
+				} else {
-							strings.Join(reloaded, ", "))
+					b.Logger().Debug("reloaded connection", "name", connName)
-					}
+					b.dbEvent(ctx, "reload-connection", req.Path, "", true, "name", connName)
-					return nil, fmt.Errorf("%sfailed to reload connection %q: %w", successfullyReloaded, connName, err)
+					reloaded = append(reloaded, connName)
 				}
-				reloaded = append(reloaded, connName)
 			}
 		}
 
@@ -211,10 +210,12 @@ func (b *databaseBackend) reloadPlugin() framework.OperationFunc {
 			},
 		}
 
-		if len(reloaded) == 0 {
+		if len(reloaded) > 0 {
-			resp.AddWarning(fmt.Sprintf("no connections were found with plugin_name %q", pluginName))
+			b.dbEvent(ctx, "reload", req.Path, "", true, "plugin_name", pluginName)
+		} else if len(reloaded) == 0 && len(reloadFailed) == 0 {
+			b.Logger().Debug("no connections were found", "plugin_name", pluginName)
 		}
-		b.dbEvent(ctx, "reload", req.Path, "", true, "plugin_name", pluginName)
+
 		return resp, nil
 	}
 }

changelog/29519.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/database: Fix a bug where a global database plugin reload exits if any of the database connections are not available
+```

vault/external_tests/plugin/external_plugin_test.go

@@ -817,56 +817,81 @@ func TestExternalPlugin_DatabaseReload(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	dbName := fmt.Sprintf("%s-%d", plugin.Name, 0)
+	roleName := "test-role"
-	roleName := "test-role-" + dbName
 
-	cleanupContainer, connURL := postgreshelper.PrepareTestContainerWithVaultUser(t, context.Background())
+	// create 4 databases to create connections for
-	t.Cleanup(cleanupContainer)
+	cleanupContainer0, connURL0 := postgreshelper.PrepareTestContainerWithVaultUser(t, context.Background())
+	t.Cleanup(cleanupContainer0)
+	cleanupContainer1, connURL1 := postgreshelper.PrepareTestContainerWithVaultUser(t, context.Background())
+	t.Cleanup(cleanupContainer1)
+	cleanupContainer2, connURL2 := postgreshelper.PrepareTestContainerWithVaultUser(t, context.Background())
+	t.Cleanup(cleanupContainer2)
 
-	_, err := client.Logical().Write("database/config/"+dbName, map[string]interface{}{
+	var roles []string
-		"connection_url": connURL,
+	// write the config and roles for the first 3 DBs
-		"plugin_name":    plugin.Name,
+	for i, url := range []string{connURL0, connURL1, connURL2} {
-		"allowed_roles":  []string{roleName},
+		dbName := fmt.Sprintf("%s-%d", plugin.Name, i)
-		"username":       "vaultadmin",
+		_, err := client.Logical().Write("database/config/"+dbName, map[string]interface{}{
-		"password":       "vaultpass",
+			"connection_url": url,
+			"plugin_name":    plugin.Name,
+			"allowed_roles":  []string{"*"},
+			"username":       "vaultadmin",
+			"password":       "vaultpass",
+		})
+		require.NoError(t, err)
+
+		r := fmt.Sprintf("%s-%d", roleName, i)
+		roles = append(roles, r)
+		_, err = client.Logical().Write("database/roles/"+r, map[string]interface{}{
+			"db_name":             dbName,
+			"creation_statements": testRole,
+			"max_ttl":             "10m",
+		})
+		require.NoError(t, err)
+	}
+
+	// the 4th db connection has a bad connURL on purpose, it should not fail
+	// the plugin reload
+	_, err := client.Logical().Write("database/config/"+plugin.Name+"-3", map[string]interface{}{
+		"connection_url":    "foobar",
+		"verify_connection": false, // this db connection should not fail the plugin reload
+		"plugin_name":       plugin.Name,
+		"allowed_roles":     []string{"*"},
+		"username":          "vaultadmin",
+		"password":          "vaultpass",
 	})
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatal(err)
-	}
 
-	_, err = client.Logical().Write("database/roles/"+roleName, map[string]interface{}{
+	for _, role := range roles {
-		"db_name":             dbName,
+		resp, err := client.Logical().Read("database/creds/" + role)
-		"creation_statements": testRole,
+		require.NoError(t, err)
-		"max_ttl":             "10m",
+		if resp == nil {
-	})
+			t.Fatal("read creds response is nil")
-	if err != nil {
+		}
-		t.Fatal(err)
-	}
-
-	resp, err := client.Logical().Read("database/creds/" + roleName)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp == nil {
-		t.Fatal("read creds response is nil")
 	}
 
 	// Reload plugin
-	if _, err := client.Sys().ReloadPlugin(&api.ReloadPluginInput{
+	_, err = client.Sys().ReloadPlugin(&api.ReloadPluginInput{
 		Plugin: plugin.Name,
-	}); err != nil {
+	})
-		t.Fatal(err)
+	require.NoError(t, err)
-	}
 
 	// Generate credentials after reload
-	resp, err = client.Logical().Read("database/creds/" + roleName)
+	for _, role := range roles {
-	if err != nil {
+		resp, err := client.Logical().Read("database/creds/" + role)
-		t.Fatal(err)
+		require.NoError(t, err)
-	}
+		if resp == nil {
-	if resp == nil {
+			t.Fatal("read creds response is nil")
-		t.Fatal("read creds response is nil")
+		}
 	}
 
+	// remove one postgres database so that the plugin reload will fail
+	cleanupContainer1()
+	_, err = client.Sys().ReloadPlugin(&api.ReloadPluginInput{
+		Plugin: plugin.Name,
+	})
+	require.NoError(t, err)
+
 	if err := client.Sys().Unmount(plugin.Name); err != nil {
 		t.Fatal(err)
 	}