From 72d76a50eed0b212da0cffbc0459f6af7614f4e2 Mon Sep 17 00:00:00 2001 From: Leonardo Cecchi Date: Tue, 21 May 2024 17:58:02 +0200 Subject: [PATCH 1/4] fix: avoid reloading ident map when not needed Before this patch, the instance manager could write the ident map using two different formats even if the content was semantically the same. This was triggering an unneeded configuration reload after the postmaster was started up. Fixes: #4626 Signed-off-by: Leonardo Cecchi --- .../controller/instance_controller.go | 2 +- pkg/management/postgres/configuration.go | 20 ++++++++++++------- pkg/management/postgres/ident.go | 17 ---------------- pkg/management/postgres/instance.go | 3 ++- pkg/management/postgres/restore.go | 5 +++-- 5 files changed, 19 insertions(+), 28 deletions(-) diff --git a/internal/management/controller/instance_controller.go b/internal/management/controller/instance_controller.go index 81100f7322..209c5497da 100644 --- a/internal/management/controller/instance_controller.go +++ b/internal/management/controller/instance_controller.go @@ -298,7 +298,7 @@ func (r *InstanceReconciler) refreshConfigurationFiles( return false, err } - reloadIdent, err := r.instance.RefreshPGIdent(cluster) + reloadIdent, err := r.instance.RefreshPGIdent(cluster.Spec.PostgresConfiguration.PgIdent) if err != nil { return false, err } diff --git a/pkg/management/postgres/configuration.go b/pkg/management/postgres/configuration.go index 638f5eddae..a159b4de7d 100644 --- a/pkg/management/postgres/configuration.go +++ b/pkg/management/postgres/configuration.go @@ -198,16 +198,22 @@ func quoteHbaLiteral(literal string) string { return fmt.Sprintf(`"%s"`, literal) } -// GeneratePostgresqlIdent generates the pg_ident.conf content -func (instance *Instance) GeneratePostgresqlIdent(cluster *apiv1.Cluster) (string, error) { - return postgres.CreateIdentRules(cluster.Spec.PostgresConfiguration.PgIdent, - getCurrentUserOrDefaultToInsecureMapping()) +// generatePostgresqlIdent generates the pg_ident.conf content given +// a set of additional pg_ident lines that is usually taken from the +// Cluster configuration +func (instance *Instance) generatePostgresqlIdent(additionalLines []string) (string, error) { + return postgres.CreateIdentRules( + additionalLines, + getCurrentUserOrDefaultToInsecureMapping(), + ) } -// RefreshPGIdent generates and writes down the pg_ident.conf file -func (instance *Instance) RefreshPGIdent(cluster *apiv1.Cluster) (postgresIdentChanged bool, err error) { +// RefreshPGIdent generates and writes down the pg_ident.conf file given +// a set of additional pg_ident lines that is usually taken from the +// Cluster configuration +func (instance *Instance) RefreshPGIdent(additionalLines []string) (postgresIdentChanged bool, err error) { // Generate pg_hba.conf file - pgIdentContent, err := instance.GeneratePostgresqlIdent(cluster) + pgIdentContent, err := instance.generatePostgresqlIdent(additionalLines) if err != nil { return false, nil } diff --git a/pkg/management/postgres/ident.go b/pkg/management/postgres/ident.go index d080215d2d..4b3d07a032 100644 --- a/pkg/management/postgres/ident.go +++ b/pkg/management/postgres/ident.go @@ -17,28 +17,11 @@ limitations under the License. package postgres import ( - "fmt" "os/user" - "path/filepath" - "github.com/cloudnative-pg/cloudnative-pg/pkg/fileutils" "github.com/cloudnative-pg/cloudnative-pg/pkg/management/log" - "github.com/cloudnative-pg/cloudnative-pg/pkg/management/postgres/constants" ) -// WritePostgresUserMaps creates a pg_ident.conf file containing only one map called "local" that -// maps the current user to "postgres" user. -func WritePostgresUserMaps(pgData string) error { - username := getCurrentUserOrDefaultToInsecureMapping() - _, err := fileutils.WriteStringToFile(filepath.Join(pgData, constants.PostgresqlIdentFile), - fmt.Sprintf("local %s postgres\n", username)) - if err != nil { - return err - } - - return nil -} - // getCurrentUserOrDefaultToInsecureMapping retrieves the current system user's username. // If the retrieval fails, it falls back to an insecure mapping using the root ("/") as the default username. // diff --git a/pkg/management/postgres/instance.go b/pkg/management/postgres/instance.go index f26b5e6cb7..4afdb61036 100644 --- a/pkg/management/postgres/instance.go +++ b/pkg/management/postgres/instance.go @@ -299,7 +299,8 @@ func (instance *Instance) VerifyPgDataCoherence(ctx context.Context) error { return err } - return WritePostgresUserMaps(instance.PgData) + _, err := instance.RefreshPGIdent(nil) + return err } // InstanceCommand are commands for the goroutine managing postgres diff --git a/pkg/management/postgres/restore.go b/pkg/management/postgres/restore.go index e9ff25b86c..cd426094d9 100644 --- a/pkg/management/postgres/restore.go +++ b/pkg/management/postgres/restore.go @@ -717,7 +717,7 @@ func (info InitInfo) WriteInitialPostgresqlConf(cluster *apiv1.Cluster) error { if err != nil { return fmt.Errorf("while reading configuration files from ConfigMap: %w", err) } - _, err = temporaryInstance.RefreshPGIdent(cluster) + _, err = temporaryInstance.RefreshPGIdent(nil) if err != nil { return fmt.Errorf("while reading configuration files from ConfigMap: %w", err) } @@ -771,7 +771,8 @@ func (info InitInfo) WriteRestoreHbaConf() error { } // Create the local map referred in the HBA configuration - return WritePostgresUserMaps(info.PgData) + _, err = info.GetInstance().RefreshPGIdent(nil) + return err } // ConfigureInstanceAfterRestore changes the superuser password From 888f26696eb24ab8c1c5b30a07662fa785216930 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Fri, 24 May 2024 10:11:49 +0000 Subject: [PATCH 2/4] review: initial configuration should apply cluster config Signed-off-by: Marco Nenciarini --- pkg/management/postgres/restore.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/management/postgres/restore.go b/pkg/management/postgres/restore.go index cd426094d9..137ed50068 100644 --- a/pkg/management/postgres/restore.go +++ b/pkg/management/postgres/restore.go @@ -717,7 +717,7 @@ func (info InitInfo) WriteInitialPostgresqlConf(cluster *apiv1.Cluster) error { if err != nil { return fmt.Errorf("while reading configuration files from ConfigMap: %w", err) } - _, err = temporaryInstance.RefreshPGIdent(nil) + _, err = temporaryInstance.RefreshPGIdent(cluster.Spec.PostgresConfiguration.PgIdent) if err != nil { return fmt.Errorf("while reading configuration files from ConfigMap: %w", err) } From 6cdbf6bb9b621d69add3c01834bb97981ffa2149 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Fri, 24 May 2024 10:23:26 +0000 Subject: [PATCH 3/4] review: error messages Signed-off-by: Marco Nenciarini --- pkg/management/postgres/restore.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/management/postgres/restore.go b/pkg/management/postgres/restore.go index 137ed50068..cfa4a5c346 100644 --- a/pkg/management/postgres/restore.go +++ b/pkg/management/postgres/restore.go @@ -715,36 +715,36 @@ func (info InitInfo) WriteInitialPostgresqlConf(cluster *apiv1.Cluster) error { _, err = temporaryInstance.RefreshPGHBA(cluster, "") if err != nil { - return fmt.Errorf("while reading configuration files from ConfigMap: %w", err) + return fmt.Errorf("while generating pg_hba.conf: %w", err) } _, err = temporaryInstance.RefreshPGIdent(cluster.Spec.PostgresConfiguration.PgIdent) if err != nil { - return fmt.Errorf("while reading configuration files from ConfigMap: %w", err) + return fmt.Errorf("while generating pg_ident.conf: %w", err) } _, err = temporaryInstance.RefreshConfigurationFilesFromCluster(cluster, false) if err != nil { - return fmt.Errorf("while reading configuration files from ConfigMap: %w", err) + return fmt.Errorf("while generating Postgres configuration: %w", err) } err = fileutils.CopyFile( path.Join(temporaryInitInfo.PgData, "postgresql.conf"), path.Join(info.PgData, "postgresql.conf")) if err != nil { - return fmt.Errorf("while creating postgresql.conf: %w", err) + return fmt.Errorf("while installing postgresql.conf: %w", err) } err = fileutils.CopyFile( path.Join(temporaryInitInfo.PgData, constants.PostgresqlCustomConfigurationFile), path.Join(info.PgData, constants.PostgresqlCustomConfigurationFile)) if err != nil { - return fmt.Errorf("while creating custom.conf: %w", err) + return fmt.Errorf("while installing %v: %w", constants.PostgresqlCustomConfigurationFile, err) } err = fileutils.CopyFile( path.Join(temporaryInitInfo.PgData, constants.PostgresqlOverrideConfigurationFile), path.Join(info.PgData, constants.PostgresqlOverrideConfigurationFile)) if err != nil { - return fmt.Errorf("while creating %v: %w", constants.PostgresqlOverrideConfigurationFile, err) + return fmt.Errorf("while installing %v: %w", constants.PostgresqlOverrideConfigurationFile, err) } // Disable SSL as we still don't have the required certificates From b191565e29685695b385270d7b22c41f99cd2ad8 Mon Sep 17 00:00:00 2001 From: Jaime Silvela Date: Mon, 27 May 2024 15:00:30 +0200 Subject: [PATCH 4/4] chore: clarify some comments Signed-off-by: Jaime Silvela --- pkg/management/postgres/configuration.go | 2 +- pkg/management/postgres/instance.go | 1 + pkg/management/postgres/restore.go | 6 +++--- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/management/postgres/configuration.go b/pkg/management/postgres/configuration.go index a159b4de7d..8cc34e6310 100644 --- a/pkg/management/postgres/configuration.go +++ b/pkg/management/postgres/configuration.go @@ -212,7 +212,7 @@ func (instance *Instance) generatePostgresqlIdent(additionalLines []string) (str // a set of additional pg_ident lines that is usually taken from the // Cluster configuration func (instance *Instance) RefreshPGIdent(additionalLines []string) (postgresIdentChanged bool, err error) { - // Generate pg_hba.conf file + // Generate pg_ident.conf file pgIdentContent, err := instance.generatePostgresqlIdent(additionalLines) if err != nil { return false, nil diff --git a/pkg/management/postgres/instance.go b/pkg/management/postgres/instance.go index 4afdb61036..0a639f960e 100644 --- a/pkg/management/postgres/instance.go +++ b/pkg/management/postgres/instance.go @@ -299,6 +299,7 @@ func (instance *Instance) VerifyPgDataCoherence(ctx context.Context) error { return err } + // creates a bare pg_ident.conf that only grants local access _, err := instance.RefreshPGIdent(nil) return err } diff --git a/pkg/management/postgres/restore.go b/pkg/management/postgres/restore.go index cfa4a5c346..f962d968cb 100644 --- a/pkg/management/postgres/restore.go +++ b/pkg/management/postgres/restore.go @@ -758,8 +758,8 @@ func (info InitInfo) WriteInitialPostgresqlConf(cluster *apiv1.Cluster) error { return err } -// WriteRestoreHbaConf writes a pg_hba.conf allowing access without password from localhost. -// this is needed to set the PostgreSQL password after the postgres server is started and active +// WriteRestoreHbaConf writes basic pg_hba.conf and pg_ident.conf allowing access without password from localhost. +// This is needed to set the PostgreSQL password after the postgres server is started and active func (info InitInfo) WriteRestoreHbaConf() error { // We allow every access from localhost, and this is needed to correctly restore // the database @@ -770,7 +770,7 @@ func (info InitInfo) WriteRestoreHbaConf() error { return err } - // Create the local map referred in the HBA configuration + // Create only the local map referred in the HBA configuration _, err = info.GetInstance().RefreshPGIdent(nil) return err }