feat(database): add validation to PluginFactoryConfig and improve documentation

- Add validate() method to PluginFactoryConfig to check for required fields (Sys and Logger) before plugin initialization
- Prevent nil dereference panics by surfacing validation errors early to callers
- Refactor PluginFactoryWithConfig to use cfg directly instead of local variable assignments, improving consistency
- Update comments in backend.go to clarify mount point telemetry label behavior when opt-in is disabled
- Update telemetry.go documentation to reflect namespace labels in addition to mount_point and connection_name
- Add comprehensive test suite (plugin_factory_test.go) covering config validation scenarios

Author: vfernandezg

builtin/logical/database/backend.go

@@ -151,9 +151,10 @@ func Backend(conf *logical.BackendConfig) *databaseBackend {
 }
 
 // metricsLabelsForConnection returns the telemetry labels to attach to metrics
-// emitted on behalf of the named connection. It returns the zero value unless
-// the operator opted in, in which case metrics stay unlabeled and aggregate
-// across mounts as they always have.
+// emitted on behalf of the named connection.
+//
+// When the operator has not opted in, it returns the zero value, which leaves
+// metrics unlabeled so they keep aggregating across mounts as they always have.
 func (b *databaseBackend) metricsLabelsForConnection(name string) databaseWrapperMetricsLabels {
 	if !b.includeMountPointInMetrics {
 		return databaseWrapperMetricsLabels{}

internalshared/configutil/telemetry.go

@@ -214,9 +214,10 @@ type Telemetry struct {
 	// metrics
 	RollbackMetricsIncludeMountPoint bool `hcl:"add_mount_point_rollback_metrics"`
 
-	// Whether or not telemetry should add mount_point and connection_name
-	// labels to the database secrets engine metrics. This is opt-in because it
-	// multiplies the series count by the number of configured connections.
+	// Whether or not telemetry should add namespace, mount_point, and
+	// connection_name labels to the database secrets engine metrics. This is
+	// opt-in because it multiplies the series count by the number of
+	// namespaces, mounts, and configured connections.
 	DatabaseMetricsIncludeMountPoint bool `hcl:"add_mount_point_database_metrics"`
 }
 

sdk/database/dbplugin/v5/plugin_factory.go

@@ -5,6 +5,7 @@ package dbplugin
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/hashicorp/errwrap"
@@ -37,6 +38,25 @@ type PluginFactoryConfig struct {
 	ConnectionName string
 }
 
+// validate reports whether the config carries the collaborators required to
+// build a plugin. It exists because callers construct PluginFactoryConfig as a
+// struct literal, so an omitted field would otherwise surface as a nil
+// dereference deep inside plugin startup rather than as an error the caller can
+// handle.
+//
+// Only the fields that would panic are checked. PluginName is deliberately not
+// validated: an unknown or empty name is already reported by the plugin catalog
+// lookup, which some callers rely on to resolve a default plugin.
+func (cfg PluginFactoryConfig) validate() error {
+	if cfg.Sys == nil {
+		return errors.New("plugin system view is required")
+	}
+	if cfg.Logger == nil {
+		return errors.New("logger is required")
+	}
+	return nil
+}
+
 // PluginFactory is used to build plugin database types. It wraps the database
 // object in a logging and metrics middleware.
 func PluginFactory(ctx context.Context, pluginName string, sys pluginutil.LookRunnerUtil, logger log.Logger) (Database, error) {
@@ -57,18 +77,17 @@ func PluginFactoryVersion(ctx context.Context, pluginName string, pluginVersion
 // PluginFactoryWithConfig is used to build plugin database types from a config
 // struct. It wraps the database object in a logging and metrics middleware.
 func PluginFactoryWithConfig(ctx context.Context, cfg PluginFactoryConfig) (Database, error) {
-	pluginName := cfg.PluginName
-	pluginVersion := cfg.PluginVersion
-	sys := cfg.Sys
-	logger := cfg.Logger
+	if err := cfg.validate(); err != nil {
+		return nil, err
+	}
 
 	// Look for plugin in the plugin catalog
-	pluginRunner, err := sys.LookupPluginVersion(ctx, pluginName, consts.PluginTypeDatabase, pluginVersion)
+	pluginRunner, err := cfg.Sys.LookupPluginVersion(ctx, cfg.PluginName, consts.PluginTypeDatabase, cfg.PluginVersion)
 	if err != nil {
 		return nil, err
 	}
 
-	namedLogger := logger.Named(pluginName)
+	namedLogger := cfg.Logger.Named(cfg.PluginName)
 
 	var transport string
 	var db Database
@@ -83,34 +102,34 @@ func PluginFactoryWithConfig(ctx context.Context, cfg PluginFactoryConfig) (Data
 		var ok bool
 		db, ok = dbRaw.(Database)
 		if !ok {
-			return nil, fmt.Errorf("unsupported database type: %q", pluginName)
+			return nil, fmt.Errorf("unsupported database type: %q", cfg.PluginName)
 		}
 
 		transport = "builtin"
 
 	} else {
 		if pluginRunner.Download {
-			if err = sys.DownloadExtractVerifyPlugin(ctx, pluginRunner); err != nil {
+			if err = cfg.Sys.DownloadExtractVerifyPlugin(ctx, pluginRunner); err != nil {
 				return nil, fmt.Errorf("failed to extract and verify plugin=%q version=%q: %w",
 					pluginRunner.Name, pluginRunner.Version, err)
 			}
 		}
 
 		config := pluginutil.PluginClientConfig{
-			Name:            pluginName,
+			Name:            cfg.PluginName,
 			PluginType:      consts.PluginTypeDatabase,
-			Version:         pluginVersion,
+			Version:         cfg.PluginVersion,
 			PluginSets:      PluginSets,
 			HandshakeConfig: HandshakeConfig,
 			Logger:          namedLogger,
 			IsMetadataMode:  false,
 			AutoMTLS:        true,
-			Wrapper:         sys,
+			Wrapper:         cfg.Sys,
 			Tier:            pluginRunner.Tier,
 		}
 
 		// create a DatabasePluginClient instance
-		db, err = NewPluginClient(ctx, sys, config)
+		db, err = NewPluginClient(ctx, cfg.Sys, config)
 		if err != nil {
 			return nil, err
 		}
@@ -128,7 +147,7 @@ func PluginFactoryWithConfig(ctx context.Context, cfg PluginFactoryConfig) (Data
 	if err != nil {
 		return nil, errwrap.Wrapf("error getting plugin type: {{err}}", err)
 	}
-	logger.Debug("got database plugin instance", "type", typeStr)
+	cfg.Logger.Debug("got database plugin instance", "type", typeStr)
 
 	// Wrap with metrics middleware
 	db = &databaseMetricsMiddleware{

sdk/database/dbplugin/v5/plugin_factory_test.go

@@ -0,0 +1,132 @@
+// Copyright IBM Corp. 2016, 2025
+// SPDX-License-Identifier: MPL-2.0
+
+package dbplugin
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/vault/sdk/helper/pluginutil"
+)
+
+// stubLookRunnerUtil is a non-functional pluginutil.LookRunnerUtil used only to
+// supply a non-nil Sys. The embedded nil interface is never dereferenced because
+// config validation rejects the input before any plugin lookup happens.
+type stubLookRunnerUtil struct {
+	pluginutil.LookRunnerUtil
+}
+
+func TestPluginFactoryConfig_Validate(t *testing.T) {
+	type testCase struct {
+		config PluginFactoryConfig
+
+		// expectedErrContains is empty when the config is expected to be valid.
+		expectedErrContains string
+	}
+
+	tests := map[string]testCase{
+		"missing sys": {
+			config: PluginFactoryConfig{
+				PluginName: "postgresql-database-plugin",
+				Logger:     hclog.NewNullLogger(),
+			},
+			expectedErrContains: "system view",
+		},
+		"missing logger": {
+			config: PluginFactoryConfig{
+				PluginName: "postgresql-database-plugin",
+				Sys:        stubLookRunnerUtil{},
+			},
+			expectedErrContains: "logger",
+		},
+		"fully populated config is valid": {
+			config: PluginFactoryConfig{
+				PluginName: "postgresql-database-plugin",
+				Sys:        stubLookRunnerUtil{},
+				Logger:     hclog.NewNullLogger(),
+			},
+			expectedErrContains: "",
+		},
+		// An empty plugin name is left for the plugin catalog lookup to report,
+		// because some callers rely on it to resolve a default plugin.
+		"empty plugin name is not rejected here": {
+			config: PluginFactoryConfig{
+				Sys:    stubLookRunnerUtil{},
+				Logger: hclog.NewNullLogger(),
+			},
+			expectedErrContains: "",
+		},
+		"telemetry labels are optional": {
+			config: PluginFactoryConfig{
+				PluginName:     "postgresql-database-plugin",
+				Sys:            stubLookRunnerUtil{},
+				Logger:         hclog.NewNullLogger(),
+				Namespace:      "root",
+				MountPoint:     "database/",
+				ConnectionName: "prod-primary",
+			},
+			expectedErrContains: "",
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			err := test.config.validate()
+
+			if test.expectedErrContains == "" {
+				if err != nil {
+					t.Fatalf("Expected no error, but got: %s", err)
+				}
+				return
+			}
+
+			if err == nil {
+				t.Fatal("Expected an error, but got none")
+			}
+			if !strings.Contains(err.Error(), test.expectedErrContains) {
+				t.Fatalf("Expected error mentioning %q, but got: %s", test.expectedErrContains, err)
+			}
+		})
+	}
+}
+
+// TestPluginFactoryWithConfig_InvalidConfigReturnsError asserts that an
+// incomplete config produces an error from the exported factory rather than a
+// nil dereference panic. The factory takes a struct literal, so a caller can
+// omit a required field and still compile.
+func TestPluginFactoryWithConfig_InvalidConfigReturnsError(t *testing.T) {
+	tests := map[string]PluginFactoryConfig{
+		"empty config":   {},
+		"missing sys":    {PluginName: "postgresql-database-plugin", Logger: hclog.NewNullLogger()},
+		"missing logger": {PluginName: "postgresql-database-plugin", Sys: stubLookRunnerUtil{}},
+	}
+
+	for name, config := range tests {
+		t.Run(name, func(t *testing.T) {
+			// A panic here is a test failure, which is the regression this
+			// guards against.
+			db, err := PluginFactoryWithConfig(context.Background(), config)
+			if err == nil {
+				t.Fatal("Expected an error, but got none")
+			}
+			if db != nil {
+				t.Fatalf("Expected a nil Database on error, but got: %#v", db)
+			}
+		})
+	}
+}
+
+// TestPluginFactoryVersion_RejectsNilInput asserts the positional wrappers
+// inherit the same validation, since they previously panicked on nil input too.
+func TestPluginFactoryVersion_RejectsNilInput(t *testing.T) {
+	db, err := PluginFactoryVersion(context.Background(), "postgresql-database-plugin", "", nil, hclog.NewNullLogger())
+	if err == nil {
+		t.Fatal("Expected an error, but got none")
+	}
+	if db != nil {
+		t.Fatalf("Expected a nil Database on error, but got: %#v", db)
+	}
+}