fix(instance): improve error handling for RefreshSecrets (cloudnative-pg#7260)

Enhance error handling for the `RefreshSecrets` function across multiple
components. Errors are now properly propagated and logged, ensuring that
issues during secret refresh operations are surfaced and handled
appropriately.

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>

Author: armru

internal/cmd/manager/instance/join/cmd.go

@@ -123,16 +123,10 @@ func joinSubCommand(ctx context.Context, instance *postgres.Instance, info postg
 		return err
 	}
 
-	// Since we're directly using the reconciler here, we cannot
-	// tell if the secrets were correctly downloaded or not.
-	// If they were the following "pg_basebackup" command will work, if
-	// they don't "pg_basebackup" with fail, complaining that the
-	// cryptographic material is not available.
-	// So it doesn't make a real difference.
-	//
-	// Besides this, we should improve this situation to have
-	// a real error handling.
-	reconciler.RefreshSecrets(ctx, &cluster)
+	if _, err := reconciler.RefreshSecrets(ctx, &cluster); err != nil {
+		contextLogger.Error(err, "Error while refreshing secrets")
+		return err
+	}
 
 	// Run "pg_basebackup" to download the data directory from the primary
 	if err := info.Join(ctx, &cluster); err != nil {

internal/cmd/manager/instance/upgrade/execute/cmd.go

@@ -107,6 +107,7 @@ func NewCmd() *cobra.Command {
 	return cmd
 }
 
+// nolint:gocognit
 func upgradeSubCommand(
 	ctx context.Context,
 	instance *postgres.Instance,
@@ -139,12 +140,9 @@ func upgradeSubCommand(
 		return err
 	}
 
-	// Since we're directly using the reconciler here, we cannot
-	// tell if the secrets were correctly downloaded or not.
-	// If they were the following "pg_upgrade" command will work, if
-	// they don't "pg_upgrade" with fail, complaining that the
-	// cryptographic material is not available.
-	reconciler.RefreshSecrets(ctx, &cluster)
+	if _, err := reconciler.RefreshSecrets(ctx, &cluster); err != nil {
+		return fmt.Errorf("error while downloading secrets: %w", err)
+	}
 
 	if err := reconciler.ReconcileWalStorage(ctx); err != nil {
 		return fmt.Errorf("error while reconciling the WAL storage: %w", err)

internal/management/controller/instance_controller.go

@@ -159,7 +159,10 @@ func (r *InstanceReconciler) Reconcile(
 
 	// Reconcile secrets and cryptographic material
 	// This doesn't need the PG connection, but it needs to reload it in case of changes
-	reloadNeeded := r.RefreshSecrets(ctx, cluster)
+	reloadNeeded, err := r.RefreshSecrets(ctx, cluster)
+	if err != nil {
+		return reconcile.Result{}, fmt.Errorf("while refreshing secrets: %w", err)
+	}
 
 	reloadConfigNeeded, err := r.refreshConfigurationFiles(ctx, cluster)
 	if err != nil {
@@ -899,65 +902,53 @@ func (r *InstanceReconciler) reconcileMonitoringQueries(
 // It returns a boolean flag telling if something changed. Usually
 // the invoker will check that flag and reload the PostgreSQL
 // instance it is up.
-//
-// This function manages its own errors by logging them, so the
-// user cannot easily tell if the operation has been done completely.
-// The rationale behind this is:
-//
-//  1. when invoked at the startup of the instance manager, PostgreSQL
-//     is not up. If this raise an error, then PostgreSQL won't
-//     be able to start correctly (TLS certs are missing, i.e.),
-//     making no difference between returning an error or not
-//
-//  2. when invoked inside the reconciliation loop, if the operation
-//     raise an error, it's pointless to retry. The only way to recover
-//     from such an error is wait for the CNPG operator to refresh the
-//     resource version of the secrets to be used, and in that case a
-//     reconciliation loop will be started again.
 func (r *InstanceReconciler) RefreshSecrets(
 	ctx context.Context,
 	cluster *apiv1.Cluster,
-) bool {
+) (bool, error) {
+	type executor func(context.Context, *apiv1.Cluster) (bool, error)
+
 	contextLogger := log.FromContext(ctx)
 
-	changed := false
+	var changed bool
+
+	secretRefresher := func(cb executor) error {
+		localChanged, err := cb(ctx, cluster)
+		if err == nil {
+			changed = changed || localChanged
+			return nil
+		}
+
+		if !apierrors.IsNotFound(err) {
+			return err
+		}
 
-	serverSecretChanged, err := r.refreshServerCertificateFiles(ctx, cluster)
-	if err == nil {
-		changed = changed || serverSecretChanged
-	} else if !apierrors.IsNotFound(err) {
+		return nil
+	}
+
+	if err := secretRefresher(r.refreshServerCertificateFiles); err != nil {
 		contextLogger.Error(err, "Error while getting server secret")
+		return changed, err
 	}
 
-	replicationSecretChanged, err := r.refreshReplicationUserCertificate(ctx, cluster)
-	if err == nil {
-		changed = changed || replicationSecretChanged
-	} else if !apierrors.IsNotFound(err) {
+	if err := secretRefresher(r.refreshReplicationUserCertificate); err != nil {
 		contextLogger.Error(err, "Error while getting streaming replication secret")
+		return changed, err
 	}
-
-	clientCaSecretChanged, err := r.refreshClientCA(ctx, cluster)
-	if err == nil {
-		changed = changed || clientCaSecretChanged
-	} else if !apierrors.IsNotFound(err) {
+	if err := secretRefresher(r.refreshClientCA); err != nil {
 		contextLogger.Error(err, "Error while getting cluster CA Client secret")
+		return changed, err
 	}
-
-	serverCaSecretChanged, err := r.refreshServerCA(ctx, cluster)
-	if err == nil {
-		changed = changed || serverCaSecretChanged
-	} else if !apierrors.IsNotFound(err) {
+	if err := secretRefresher(r.refreshServerCA); err != nil {
 		contextLogger.Error(err, "Error while getting cluster CA Server secret")
+		return changed, err
 	}
-
-	barmanEndpointCaSecretChanged, err := r.refreshBarmanEndpointCA(ctx, cluster)
-	if err == nil {
-		changed = changed || barmanEndpointCaSecretChanged
-	} else if !apierrors.IsNotFound(err) {
+	if err := secretRefresher(r.refreshBarmanEndpointCA); err != nil {
 		contextLogger.Error(err, "Error while getting barman endpoint CA secret")
+		return changed, err
 	}
 
-	return changed
+	return changed, nil
 }
 
 // reconcileInstance sets PostgreSQL instance parameters to current values