Repository: cloudnative-pg/cloudnative-pg

Issue:
[Bug]: initialization order has changed in initdb job
In #7811, we modified the order of the initialization operations by moving the creation of the owner role to the end of the process, just before the database is created. 

This change could potentially disrupt any initialization scripts that assume the application user has already been created.

_Originally posted by @mnencia in https://github.com/cloudnative-pg/cloudnative-pg/pull/7811#discussion_r2161446771_
            

Hints from issue comments:

Solution patch:
diff --git a/pkg/management/postgres/initdb.go b/pkg/management/postgres/initdb.go
index e49cdb17..21d250fd 100644
--- a/pkg/management/postgres/initdb.go
+++ b/pkg/management/postgres/initdb.go
@@ -53,6 +53,15 @@ import (
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/system"
 )
 
+type connectionProvider interface {
+	// GetSuperUserDB returns the superuser database connection
+	GetSuperUserDB() (*sql.DB, error)
+	// GetTemplateDB returns the template database connection
+	GetTemplateDB() (*sql.DB, error)
+	// ConnectionPool returns the connection pool for this instance
+	ConnectionPool() pool.Pooler
+}
+
 // InitInfo contains all the info needed to bootstrap a new PostgreSQL instance
 type InitInfo struct {
 	// The data directory where to generate the new cluster
@@ -295,7 +304,7 @@ func (info InitInfo) GetInstance() *Instance {
 
 // ConfigureNewInstance creates the expected users and databases in a new
 // PostgreSQL instance. If any error occurs, we return it
-func (info InitInfo) ConfigureNewInstance(instance *Instance) error {
+func (info InitInfo) ConfigureNewInstance(instance connectionProvider) error {
 	log.Info("Configuring new PostgreSQL instance")
 
 	dbSuperUser, err := instance.GetSuperUserDB()
@@ -303,6 +312,23 @@ func (info InitInfo) ConfigureNewInstance(instance *Instance) error {
 		return fmt.Errorf("while getting superuser database: %w", err)
 	}
 
+	if info.ApplicationUser != "" {
+		var existsRole bool
+		userRow := dbSuperUser.QueryRow("SELECT COUNT(*) > 0 FROM pg_catalog.pg_roles WHERE rolname = $1",
+			info.ApplicationUser)
+		if err = userRow.Scan(&existsRole); err != nil {
+			return err
+		}
+
+		if !existsRole {
+			if _, err = dbSuperUser.Exec(fmt.Sprintf(
+				"CREATE ROLE %v LOGIN",
+				pgx.Identifier{info.ApplicationUser}.Sanitize())); err != nil {
+				return err
+			}
+		}
+	}
+
 	// Execute the custom set of init queries for the `postgres` database
 	log.Info("Executing post-init SQL instructions")
 	if err = info.executeQueries(dbSuperUser, info.PostInitSQL); err != nil {
@@ -332,26 +358,7 @@ func (info InitInfo) ConfigureNewInstance(instance *Instance) error {
 		return fmt.Errorf("could not create %v file: %w", filePath, err)
 	}
 
-	if info.ApplicationUser == "" {
-		return nil
-	}
-
-	var existsRole bool
-	userRow := dbSuperUser.QueryRow("SELECT COUNT(*) > 0 FROM pg_catalog.pg_roles WHERE rolname = $1",
-		info.ApplicationUser)
-	if err = userRow.Scan(&existsRole); err != nil {
-		return err
-	}
-
-	if !existsRole {
-		if _, err = dbSuperUser.Exec(fmt.Sprintf(
-			"CREATE ROLE %v LOGIN",
-			pgx.Identifier{info.ApplicationUser}.Sanitize())); err != nil {
-			return err
-		}
-	}
-
-	if info.ApplicationDatabase == "" {
+	if info.ApplicationUser == "" || info.ApplicationDatabase == "" {
 		return nil
 	}
 
diff --git a/pkg/management/postgres/instance.go b/pkg/management/postgres/instance.go
index 6aa7f4b9..bada828e 100644
--- a/pkg/management/postgres/instance.go
+++ b/pkg/management/postgres/instance.go
@@ -786,7 +786,7 @@ func (instance *Instance) GetPgVersion() (semver.Version, error) {
 }
 
 // ConnectionPool gets or initializes the connection pool for this instance
-func (instance *Instance) ConnectionPool() *pool.ConnectionPool {
+func (instance *Instance) ConnectionPool() pool.Pooler {
 	const applicationName = "cnpg-instance-manager"
 	if instance.pool == nil {
 		socketDir := GetSocketDir()



Test patch:
diff --git a/pkg/management/postgres/initdb_test.go b/pkg/management/postgres/initdb_test.go
index d6a6d5d9..bc81cfd3 100644
--- a/pkg/management/postgres/initdb_test.go
+++ b/pkg/management/postgres/initdb_test.go
@@ -21,7 +21,11 @@ package postgres
 
 import (
 	"os"
+	"path"
 	"path/filepath"
+	"regexp"
+
+	"github.com/DATA-DOG/go-sqlmock"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -125,3 +129,73 @@ var _ = Describe("renameExistingTargetDataDirectories", func() {
 		Expect(filelist).To(BeEmpty())
 	})
 })
+
+var _ = Describe("ConfigureNewInstance role creation", func() {
+	var (
+		info          InitInfo
+		mi            *mockInstance
+		mockSuperUser sqlmock.Sqlmock
+		testDir       string
+	)
+
+	BeforeEach(func() {
+		var err error
+
+		testDir = path.Join(GinkgoT().TempDir(), "initdb_test")
+
+		Expect(os.MkdirAll(testDir, 0o700)).To(Succeed())
+
+		mi = &mockInstance{}
+		mi.superUserDB, mockSuperUser, err = sqlmock.New()
+		Expect(err).NotTo(HaveOccurred())
+
+		mi.appDB, _, err = sqlmock.New()
+		Expect(err).NotTo(HaveOccurred())
+
+		info = InitInfo{
+			ApplicationUser: "app_user",
+			PostInitSQL:     []string{"CREATE ROLE post_init_role LOGIN"},
+			PgData:          testDir,
+		}
+	})
+
+	AfterEach(func() {
+		Expect(mockSuperUser.ExpectationsWereMet()).NotTo(HaveOccurred())
+	})
+
+	It("ensures that we create the application user before postIniSQL", func() {
+		// Expect check if application role exists
+		mockSuperUser.ExpectQuery(regexp.QuoteMeta("SELECT COUNT(*) > 0 FROM pg_catalog.pg_roles WHERE rolname = $1")).
+			WithArgs("app_user").
+			WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(false))
+
+		mockSuperUser.ExpectExec(`CREATE ROLE \"app_user\" LOGIN`).
+			WillReturnResult(sqlmock.NewResult(1, 1))
+
+		mockSuperUser.ExpectExec("CREATE ROLE post_init_role LOGIN").
+			WillReturnResult(sqlmock.NewResult(1, 1))
+
+		err := info.ConfigureNewInstance(mi)
+
+		Expect(err).NotTo(HaveOccurred())
+	})
+
+	It("ensures that we do not create the application user if already exists", func() {
+		// Expect check if application role exists - return true this time
+		mockSuperUser.ExpectQuery(regexp.QuoteMeta("SELECT COUNT(*) > 0 FROM pg_catalog.pg_roles WHERE rolname = $1")).
+			WithArgs("app_user").
+			WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true))
+
+		// No direct role creation expected
+
+		mockSuperUser.ExpectExec("CREATE ROLE post_init_role LOGIN").
+			WillReturnResult(sqlmock.NewResult(1, 1))
+
+		// Execute function under test
+		err := info.ConfigureNewInstance(mi)
+
+		// Verify results
+		Expect(err).NotTo(HaveOccurred())
+		Expect(mockSuperUser.ExpectationsWereMet()).NotTo(HaveOccurred())
+	})
+})
diff --git a/pkg/management/postgres/suite_test.go b/pkg/management/postgres/suite_test.go
index 1d88c1b8..b602dcf5 100644
--- a/pkg/management/postgres/suite_test.go
+++ b/pkg/management/postgres/suite_test.go
@@ -20,8 +20,11 @@ SPDX-License-Identifier: Apache-2.0
 package postgres
 
 import (
+	"database/sql"
 	"testing"
 
+	"github.com/cloudnative-pg/cloudnative-pg/pkg/management/postgres/pool"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
@@ -30,3 +33,37 @@ func TestPostgres(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "PostgreSQL instance manager test suite")
 }
+
+// mockInstance implements the minimum required functionality to test ConfigureNewInstance
+type mockInstance struct {
+	superUserDB *sql.DB
+	templateDB  *sql.DB
+	appDB       *sql.DB
+}
+
+type fakePooler struct {
+	db *sql.DB
+}
+
+func (f fakePooler) Connection(_ string) (*sql.DB, error) {
+	return f.db, nil
+}
+
+func (f fakePooler) GetDsn(dbName string) string {
+	return dbName
+}
+
+func (f fakePooler) ShutdownConnections() {
+}
+
+func (m *mockInstance) GetSuperUserDB() (*sql.DB, error) {
+	return m.superUserDB, nil
+}
+
+func (m *mockInstance) GetTemplateDB() (*sql.DB, error) {
+	return m.templateDB, nil
+}
+
+func (m *mockInstance) ConnectionPool() pool.Pooler {
+	return &fakePooler{db: m.appDB}
+}