diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6d1456c..9f5bd4f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -217,7 +217,7 @@ When adding collectors: 1. **Update unit tests** in `radar_test.go` 2. **Update integration tests** if permission-dependent -3. **Test all 4 permission scenarios** if applicable +3. **Test all 6 integration scenarios** if applicable 4. **Ensure silent handling** of missing tools/permissions ## Architecture @@ -334,31 +334,41 @@ The project uses GitHub Actions for CI/CD. See [.github/workflows/ci.yml](.githu ### Integration Test Scenarios -The integration test validates radar works correctly in all permission combinations: +The integration test runs six scenarios: the four permission combinations, then certificate and GSSAPI authentication: **Scenario 1: Root + PostgreSQL superuser** - Full system access (all commands available to root) - Full PostgreSQL access (all views, config files) -- Expected: ~66 system collectors, ~32 PostgreSQL collectors, pg_statviz data +- Expected: ~71 system collectors, ~55 PostgreSQL collectors, pg_statviz data **Scenario 2: Root + PostgreSQL pg_monitor role** - Full system access (root privileges) - Monitoring-level PostgreSQL access (most views, limited config) -- Expected: ~66 system collectors, ~29 PostgreSQL collectors, pg_statviz data +- Expected: ~71 system collectors, ~51 PostgreSQL collectors, pg_statviz data - Note: Some collectors unavailable (e.g., pg_hba_file_rules, subscriptions) **Scenario 3: Non-root + PostgreSQL superuser** - Limited system access (some commands fail without root) - Full PostgreSQL access (all views, config files) -- Expected: ~63 system collectors, ~32 PostgreSQL collectors, pg_statviz data +- Expected: ~68 system collectors, ~51 PostgreSQL collectors, pg_statviz data - Note: Some system collectors unavailable (e.g., ifconfig, sysctl) **Scenario 4: Non-root + PostgreSQL pg_monitor role** - Limited system access (non-root user) - Monitoring-level PostgreSQL access (pg_monitor role) -- Expected: ~63 system collectors, ~29 PostgreSQL collectors, pg_statviz data +- Expected: ~68 system collectors, ~47 PostgreSQL collectors, pg_statviz data - Note: Combines limitations of both non-root and pg_monitor +**Scenario 5: Certificate authentication** +- Root, connecting as the pg_monitor role with a client certificate over TLS +- Expected: ~71 system collectors, ~51 PostgreSQL collectors, pg_statviz data +- Note: Exercises `-sslmode verify-full` with `-sslcert`, `-sslkey` and `-sslrootcert` + +**Scenario 6: GSSAPI/Kerberos authentication** +- Root, connecting as the pg_monitor role with a Kerberos ticket from `kinit` +- Expected: ~71 system collectors, ~51 PostgreSQL collectors, pg_statviz data +- Note: `pg_hba.conf` requires `hostgssenc`, so the connection must be GSSAPI-encrypted + All scenarios verify that radar handles permission limitations and collects maximum available data. ### Collector Availability Differences by Permission Scenario @@ -460,7 +470,7 @@ docs: update README with new collectors - Code must be formatted (gofmt) - Linting must pass (golangci-lint) - Unit tests must pass -- Integration tests must pass (all 4 scenarios) +- Integration tests must pass (all 6 scenarios) - Documentation updated if applicable ## Getting Help diff --git a/README.md b/README.md index bf1794b..e36a87d 100644 --- a/README.md +++ b/README.md @@ -280,6 +280,7 @@ radar-hostname-20260115-133700.zip - Data streams directly to ZIP file without buffering in memory - Sequential execution with minimal memory footprint +- One connection per database, reused across all of that database's queries - Complete collection typically takes seconds ## Author diff --git a/docs/index.md b/docs/index.md index 9df1e0d..bdf7501 100644 --- a/docs/index.md +++ b/docs/index.md @@ -256,6 +256,7 @@ radar-hostname-20260115-133700.zip - Data streams directly to ZIP file without buffering in memory - Sequential execution with minimal memory footprint +- One connection per database, reused across all of that database's queries - Complete collection typically takes seconds ## Author diff --git a/postgres.go b/postgres.go index 65ce37e..1623c70 100644 --- a/postgres.go +++ b/postgres.go @@ -11,6 +11,7 @@ package main import ( + "context" "database/sql" "errors" "fmt" @@ -111,16 +112,17 @@ func collectPGConfigFile(db *sql.DB, cfg *Config, filename string, w io.Writer) return err } -// generateDatabaseTasks creates per-database collection tasks -func generateDatabaseTasks(db *sql.DB) ([]CollectionTask, error) { +// generateDatabaseTasks creates per-database collection tasks, returning the +// closer for the connection they share. +func generateDatabaseTasks(db *sql.DB) ([]CollectionTask, io.Closer, error) { if db == nil { - return nil, fmt.Errorf("PostgreSQL not initialized") + return nil, nil, fmt.Errorf("PostgreSQL not initialized") } // Get list of databases rows, err := db.Query("SELECT datname FROM pg_database WHERE datallowconn ORDER BY datname") if err != nil { - return nil, fmt.Errorf("querying databases: %w", err) + return nil, nil, fmt.Errorf("querying databases: %w", err) } defer closeErrCheck(rows, "database list query rows") @@ -128,7 +130,7 @@ func generateDatabaseTasks(db *sql.DB) ([]CollectionTask, error) { for rows.Next() { var dbname string if err := rows.Scan(&dbname); err != nil { - return nil, fmt.Errorf("scanning database name: %w", err) + return nil, nil, fmt.Errorf("scanning database name: %w", err) } // Always skip template0 and template1 if dbname == "template0" || dbname == "template1" { @@ -138,11 +140,12 @@ func generateDatabaseTasks(db *sql.DB) ([]CollectionTask, error) { } if err := rows.Err(); err != nil { - return nil, fmt.Errorf("iterating databases: %w", err) + return nil, nil, fmt.Errorf("iterating databases: %w", err) } // Generate tasks for each database var tasks []CollectionTask + conns := &dbConns{} allDBTasks := make([]SimpleQueryTask, 0, len(perDatabaseQueryTasks)+len(pgStatvizQueryTasks)+len(spockQueryTasks)) allDBTasks = append(allDBTasks, perDatabaseQueryTasks...) @@ -162,22 +165,84 @@ func generateDatabaseTasks(db *sql.DB) ([]CollectionTask, error) { Name: fmt.Sprintf("%s/%s", dbName, td.Name), ArchivePath: fmt.Sprintf(td.ArchivePath, dbName), Collector: func(cfg *Config, w io.Writer) error { - return execPGQueryOnDB(dbName, cfg, td.Query, w) + return conns.exec(cfg, dbName, td.Query, w) }, }) } } - return tasks, nil + return tasks, conns, nil } -// execPGQueryOnDB executes a query on a specific database -func execPGQueryOnDB(dbname string, cfg *Config, query string, w io.Writer) error { +// dbConns holds the connection the per-database tasks share. They are generated +// and run grouped by database, so holding one connection and closing it when +// collection moves on costs one connect and one authentication per database +// rather than one per task. +type dbConns struct { + db *sql.DB + name string + err error // why name could not be reached, if it could not +} + +// conn returns the connection for dbname, establishing one on first use and +// closing the connection to the database collection has left behind. A database +// that cannot be reached is recorded, so its remaining tasks skip. The database +// radar was invoked against is served by the connection opened at startup. +func (c *dbConns) conn(cfg *Config, dbname string) (*sql.DB, error) { + if dbname == c.name { + if c.err != nil { + return nil, NewSkipError(c.err.Error()) + } + return c.db, nil + } + // A different database, so the held connection is finished with. + closeErrCheck(c, "database connection") + + if dbname == cfg.Database && cfg.DB != nil { + return cfg.DB, nil + } + db, err := sql.Open("pgx", cfg.ConnectionString(dbname)) if err != nil { - return fmt.Errorf("connecting to %s: %w", dbname, err) + return nil, fmt.Errorf("connecting to %s: %w", dbname, err) + } + // Collectors run one at a time, so one connection is all the pool needs. + db.SetMaxOpenConns(1) + c.name = dbname + + // Connect here rather than leaving it to the first query. A database that + // allows connections but refuses this user then costs one rejected login + // instead of one per task, since c.err makes the rest of them skip. + pooled, err := db.Conn(context.Background()) + if err != nil { + closeErrCheck(db, "database connection") + c.err = fmt.Errorf("connecting to %s: %w", dbname, err) + return nil, c.err + } + // Back to the pool, which the tasks then draw it from. + closeErrCheck(pooled, "pooled connection") + + c.db = db + return db, nil +} + +// Close closes the connection the per-database tasks were sharing. +func (c *dbConns) Close() error { + c.name, c.err = "", nil + if c.db == nil { + return nil + } + db := c.db + c.db = nil + return db.Close() +} + +// exec executes a query on a specific database +func (c *dbConns) exec(cfg *Config, dbname, query string, w io.Writer) error { + db, err := c.conn(cfg, dbname) + if err != nil { + return err } - defer closeErrCheck(db, "database connection") rows, err := db.Query(query) if err != nil { diff --git a/postgres_test.go b/postgres_test.go index a70087a..1027791 100644 --- a/postgres_test.go +++ b/postgres_test.go @@ -13,9 +13,11 @@ package main import ( "bytes" "errors" + "net" "os" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -207,6 +209,126 @@ func TestPGQueryCollectorUnavailableAsSkip(t *testing.T) { } } +// TestDBConnsReusesOneConnectionPerDatabase pins the connection budget: a task +// on the database being collected reuses the connection already established for +// it, and the closer releases that connection at the end. +func TestDBConnsReusesOneConnectionPerDatabase(t *testing.T) { + held, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("failed to create mock: %v", err) + } + mock.ExpectClose() + + cfg := &Config{Host: "127.0.0.1", Port: 1, Database: "postgres", + Username: "radar", SSLMode: "disable"} + // Seeded as though collection is part way through mydb. + conns := &dbConns{db: held, name: "mydb"} + + got, err := conns.conn(cfg, "mydb") + if err != nil { + t.Fatalf("conn(mydb): %v", err) + } + if got != held { + t.Error("a second task on the same database opened another connection") + } + + if err := conns.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + // database/sql reports "sql: database is closed" once the pool is closed. + if err := held.Ping(); err == nil || !strings.Contains(err.Error(), "closed") { + t.Errorf("the last connection was left open: Ping = %v", err) + } + // Collection closes whether or not a database was ever opened. + if err := conns.Close(); err != nil { + t.Errorf("Close with nothing held: %v", err) + } +} + +// TestDBConnsClosesWhenCollectionReachesTheStartupDatabase covers the ordering +// where the database radar was invoked against is collected after another one. +// The startup connection serves it, and the connection to the database just +// finished must not be left open while it does. +func TestDBConnsClosesWhenCollectionReachesTheStartupDatabase(t *testing.T) { + initial, initialMock, err := sqlmock.New() + if err != nil { + t.Fatalf("failed to create mock: %v", err) + } + initialMock.ExpectClose() + defer closeErrCheck(initial, "mock database") + + held, heldMock, err := sqlmock.New() + if err != nil { + t.Fatalf("failed to create mock: %v", err) + } + heldMock.ExpectClose() + + cfg := &Config{Host: "127.0.0.1", Port: 1, Database: "postgres", + Username: "radar", SSLMode: "disable", DB: initial} + conns := &dbConns{db: held, name: "mydb"} + + got, err := conns.conn(cfg, "postgres") + if err != nil { + t.Fatalf("conn(postgres): %v", err) + } + if got != initial { + t.Error("the startup database opened a second connection") + } + if err := held.Ping(); err == nil || !strings.Contains(err.Error(), "closed") { + t.Errorf("connection to the finished database left open: Ping = %v", err) + } +} + +// TestDBConnsAttemptsAnUnreachableDatabaseOnce verifies that a database which +// refuses this user costs one connection attempt rather than one per task: the +// first task reports the failure, and the rest skip without connecting again. +func TestDBConnsAttemptsAnUnreachableDatabaseOnce(t *testing.T) { + // Accepts each connection and closes it, so the startup handshake fails the + // way a rejected login does, and counts what radar opened. + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer closeErrCheck(listener, "probe listener") + + var attempts atomic.Int64 + go func() { + for { + conn, err := listener.Accept() + if err != nil { + return + } + attempts.Add(1) + closeErrCheck(conn, "probe connection") + } + }() + + cfg := &Config{Host: "127.0.0.1", Port: listener.Addr().(*net.TCPAddr).Port, + Database: "postgres", Username: "radar", SSLMode: "disable"} + conns := &dbConns{} + defer closeErrCheck(conns, "database connection") + + var buf bytes.Buffer + var skipErr SkipError + if err := conns.exec(cfg, "mydb", "SELECT 1", &buf); err == nil || errors.As(err, &skipErr) { + t.Fatalf("first task = %v, want the failure reported", err) + } + // What the first task costs is not fixed, since database/sql retries a + // connection it finds broken. What matters is that the second task adds + // nothing. + attempted := attempts.Load() + if attempted == 0 { + t.Fatal("the first task never connected") + } + + if err := conns.exec(cfg, "mydb", "SELECT 1", &buf); !errors.As(err, &skipErr) { + t.Errorf("second task = %v, want SkipError", err) + } + if got := attempts.Load(); got != attempted { + t.Errorf("second task connected again: %d attempts, want %d", got, attempted) + } +} + // writeFixture creates a file holding contents, failing the test if it cannot. func writeFixture(t *testing.T, path, contents string) { t.Helper() diff --git a/radar.go b/radar.go index 353622f..74bffe6 100644 --- a/radar.go +++ b/radar.go @@ -543,10 +543,11 @@ func collectAll(cfg *Config, zipWriter *zip.Writer) int { if !cfg.SkipPostgres { // Pass cfg.DB to PostgreSQL task generators pgTasks = append(pgTasks, getPostgreSQLTasks(cfg.DB)...) - dbTasks, err := generateDatabaseTasks(cfg.DB) + dbTasks, conns, err := generateDatabaseTasks(cfg.DB) if err != nil { errorLog.Printf("Failed to generate database tasks: %v", err) } else { + defer closeErrCheck(conns, "database connection") pgTasks = append(pgTasks, dbTasks...) } } diff --git a/radar_test.go b/radar_test.go index 895a67b..7180012 100644 --- a/radar_test.go +++ b/radar_test.go @@ -821,10 +821,11 @@ func TestGenerateDatabaseTasksRegistersAllRegistries(t *testing.T) { WillReturnRows(sqlmock.NewRows([]string{"datname"}). AddRow("mydb").AddRow("template0").AddRow("template1")) - tasks, err := generateDatabaseTasks(db) + tasks, conns, err := generateDatabaseTasks(db) if err != nil { t.Fatalf("generateDatabaseTasks: %v", err) } + defer closeErrCheck(conns, "database connection") paths := archivePathsByName(t, tasks) diff --git a/test-radar.sh b/test-radar.sh index 2a7aa24..0bff0eb 100755 --- a/test-radar.sh +++ b/test-radar.sh @@ -1,10 +1,12 @@ #!/bin/bash # Test script for radar in a Debian container with PostgreSQL 18 -# Tests all 4 permission scenarios: +# Tests all 6 scenarios: # 1. Root + superuser # 2. Root + pg_monitor # 3. Non-root + superuser # 4. Non-root + pg_monitor +# 5. Certificate authentication +# 6. GSSAPI/Kerberos authentication set -e @@ -243,6 +245,30 @@ validate_freeze_age() { return 0 } +# Total sessions ever opened to this instance, from the cumulative counter in +# pg_stat_database. +count_sessions() { + su - postgres -c "/usr/lib/postgresql/18/bin/psql -Atqc 'SELECT sum(sessions) FROM pg_stat_database'" +} + +# Verifies the connection budget. Every database contributes tens of queries, so +# a connection per query would be a connect and authentication storm on an +# instance holding many databases. Expected here is 2: one to the instance, and +# one to postgres, since testdb reuses the instance connection. +validate_session_count() { + local before="$1" + local scenario="$2" + # The psql call reading the total opens a session of its own, hence the -1. + local used=$(( $(count_sessions) - before - 1 )) + + echo " Sessions opened: $used" + if [ "$used" -gt 10 ]; then + echo -e "${RED}✗ $scenario FAILED: opened $used sessions, expected at most 10${NC}" + return 1 + fi + return 0 +} + # Helper function to validate ZIP contents validate_zip() { local zip_file="$1" @@ -304,7 +330,11 @@ echo "" echo "========================================" echo -e "${YELLOW}Scenario 1: Root + superuser${NC}" echo "========================================" +SESSIONS_BEFORE=$(count_sessions) ./radar -h localhost -d testdb -U postgres -vv +if ! validate_session_count "$SESSIONS_BEFORE" "Scenario 1"; then + exit 1 +fi ZIP1=$(ls -t radar-*.zip | head -1) if ! validate_zip "$ZIP1" "Scenario 1" "yes"; then exit 1