fix(roles): preserve password when the referenced Secret is missing (cloudnative-pg#10053)

Previously, when a role's passwordSecret could not be fetched, the
reconciler would clear the password on the PostgreSQL role. The role is
now left untouched until the Secret becomes available.

Role reconciliation no longer stops at the first failing action: every
action is attempted and the resulting errors are aggregated, giving
better visibility into partial failures.

Closes cloudnative-pg#9677

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
(cherry picked from commit 1854d20)

Author: 3 people

api/v1/cluster_types.go

@@ -681,8 +681,10 @@ type ManagedRoles struct {
 	// +optional
 	ByStatus map[RoleStatus][]string `json:"byStatus,omitempty"`
 
-	// CannotReconcile lists roles that cannot be reconciled in PostgreSQL,
-	// with an explanation of the cause
+	// CannotReconcile lists roles that cannot be reconciled, with an
+	// explanation of the cause. Failures may originate in PostgreSQL
+	// (e.g. dropping a role that owns objects) or in Kubernetes (e.g.
+	// the referenced password Secret cannot be fetched).
 	// +optional
 	CannotReconcile map[string][]string `json:"cannotReconcile,omitempty"`
 
@@ -2260,8 +2262,10 @@ type RoleConfiguration struct {
 	// +optional
 	Ensure EnsureOption `json:"ensure,omitempty"`
 
-	// Secret containing the password of the role (if present)
-	// If null, the password will be ignored unless DisablePassword is set
+	// Secret containing the password of the role (if present).
+	// If null, the password will be ignored unless DisablePassword is set.
+	// When set, the secret must follow the `kubernetes.io/basic-auth` format
+	// and contain both a `username` and a `password` field.
 	// +optional
 	PasswordSecret *LocalObjectReference `json:"passwordSecret,omitempty"`
 

config/crd/bases/postgresql.cnpg.io_clusters.yaml

@@ -3284,8 +3284,10 @@ spec:
                           type: string
                         passwordSecret:
                           description: |-
-                            Secret containing the password of the role (if present)
-                            If null, the password will be ignored unless DisablePassword is set
+                            Secret containing the password of the role (if present).
+                            If null, the password will be ignored unless DisablePassword is set.
+                            When set, the secret must follow the `kubernetes.io/basic-auth` format
+                            and contain both a `username` and a `password` field.
                           properties:
                             name:
                               description: Name of the referent.
@@ -6342,8 +6344,10 @@ spec:
                         type: string
                       type: array
                     description: |-
-                      CannotReconcile lists roles that cannot be reconciled in PostgreSQL,
-                      with an explanation of the cause
+                      CannotReconcile lists roles that cannot be reconciled, with an
+                      explanation of the cause. Failures may originate in PostgreSQL
+                      (e.g. dropping a role that owns objects) or in Kubernetes (e.g.
+                      the referenced password Secret cannot be fetched).
                     type: object
                   passwordStatus:
                     additionalProperties:

docs/src/cloudnative-pg.v1.md

@@ -1208,7 +1208,7 @@ _Appears in:_
 | Field | Description | Required | Default | Validation |
 | --- | --- | --- | --- | --- |
 | `byStatus` _object (keys:[RoleStatus](#rolestatus), values:string array)_ | ByStatus gives the list of roles in each state |  |  |  |
-| `cannotReconcile` _object (keys:string, values:string array)_ | CannotReconcile lists roles that cannot be reconciled in PostgreSQL,<br />with an explanation of the cause |  |  |  |
+| `cannotReconcile` _object (keys:string, values:string array)_ | CannotReconcile lists roles that cannot be reconciled, with an<br />explanation of the cause. Failures may originate in PostgreSQL<br />(e.g. dropping a role that owns objects) or in Kubernetes (e.g.<br />the referenced password Secret cannot be fetched). |  |  |  |
 | `passwordStatus` _object (keys:string, values:[PasswordState](#passwordstate))_ | PasswordStatus gives the last transaction id and password secret version for each managed role |  |  |  |
 
 
@@ -2042,7 +2042,7 @@ _Appears in:_
 | `name` _string_ | Name of the role | True |  |  |
 | `comment` _string_ | Description of the role |  |  |  |
 | `ensure` _[EnsureOption](#ensureoption)_ | Ensure the role is `present` or `absent` - defaults to "present" |  | present | Enum: [present absent] <br /> |
-| `passwordSecret` _[LocalObjectReference](https://pkg.go.dev/github.com/cloudnative-pg/machinery/pkg/api#LocalObjectReference)_ | Secret containing the password of the role (if present)<br />If null, the password will be ignored unless DisablePassword is set |  |  |  |
+| `passwordSecret` _[LocalObjectReference](https://pkg.go.dev/github.com/cloudnative-pg/machinery/pkg/api#LocalObjectReference)_ | Secret containing the password of the role (if present).<br />If null, the password will be ignored unless DisablePassword is set.<br />When set, the secret must follow the `kubernetes.io/basic-auth` format<br />and contain both a `username` and a `password` field. |  |  |  |
 | `connectionLimit` _integer_ | If the role can log in, this specifies how many concurrent<br />connections the role can make. `-1` (the default) means no limit. |  | -1 |  |
 | `validUntil` _[Time](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.34/#time-v1-meta)_ | Date and time after which the role's password is no longer valid.<br />When omitted, the password will never expire (default). |  |  |  |
 | `inRoles` _string array_ | List of one or more existing roles to which this role will be<br />immediately added as a new member. Default empty. |  |  |  |

internal/management/controller/roles/reconciler.go

@@ -52,11 +52,8 @@ func Reconcile(
 	}
 
 	// get current passwords from spec/secrets
-	latestPasswordResourceVersion, err := getPasswordSecretResourceVersion(
+	latestPasswordResourceVersion := getPasswordSecretResourceVersion(
 		ctx, c, cluster.Spec.Managed.Roles, cluster.Namespace)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
 
 	contextLogger.Debug("getting the managed roles status")
 	rolesInDB, err := List(ctx, db)
@@ -79,7 +76,8 @@ func Reconcile(
 
 	if len(rolesByStatus[apiv1.RoleStatusPendingReconciliation]) != 0 {
 		instance.TriggerRoleSynchronizer(cluster.Spec.Managed)
-		contextLogger.Info("Triggered a managed role reconciliation")
+		contextLogger.Info("Triggered a managed role reconciliation",
+			"pending-roles", roleNamesByStatus[apiv1.RoleStatusPendingReconciliation])
 	}
 
 	updatedCluster := cluster.DeepCopy()

internal/management/controller/roles/roles.go

@@ -114,6 +114,22 @@ func (r rolesByAction) convertToRolesByStatus() rolesByStatus {
 	return rolesByStatus
 }
 
+// passwordSecretIsRequiredButMissing reports whether the spec asks for a
+// password secret that getPasswordSecretResourceVersion was unable to fetch.
+// Such roles must be routed through the update path so the synchronizer can
+// surface the failure in CannotReconcile rather than silently classifying
+// them as reconciled.
+func passwordSecretIsRequiredButMissing(
+	inSpec apiv1.RoleConfiguration,
+	latestSecretResourceVersion map[string]string,
+) bool {
+	if inSpec.PasswordSecret == nil || inSpec.DisablePassword {
+		return false
+	}
+	_, ok := latestSecretResourceVersion[inSpec.Name]
+	return !ok
+}
+
 // evaluateNextRoleActions evaluates the action needed for each role in the DB and/or the Spec.
 // It has no side effects
 func evaluateNextRoleActions(
@@ -148,7 +164,8 @@ func evaluateNextRoleActions(
 				roleAdapterFromName(role.Name))
 		case isInSpec &&
 			(!role.isEquivalentTo(inSpec) ||
-				role.passwordNeedsUpdating(lastPasswordState, latestSecretResourceVersion)):
+				role.passwordNeedsUpdating(lastPasswordState, latestSecretResourceVersion) ||
+				passwordSecretIsRequiredButMissing(inSpec, latestSecretResourceVersion)):
 			internalRole := roleConfigurationAdapter{
 				RoleConfiguration:        inSpec,
 				validUntilNullIsInfinity: role.ValidUntil.Valid,

internal/management/controller/roles/runnable.go

@@ -28,7 +28,6 @@ import (
 
 	"github.com/cloudnative-pg/machinery/pkg/log"
 	corev1 "k8s.io/api/core/v1"
-	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -132,13 +131,6 @@ func (sr *RoleSynchronizer) Start(ctx context.Context) error {
 // with the spec. It also updates the cluster Status with the latest applied changes
 func (sr *RoleSynchronizer) reconcile(ctx context.Context, config *apiv1.ManagedConfiguration) error {
 	var err error
-
-	defer func() {
-		if r := recover(); r != nil {
-			err = fmt.Errorf("recovered from a panic: %s", r)
-		}
-	}()
-
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Debug("reconciling managed roles")
 
@@ -163,7 +155,7 @@ func (sr *RoleSynchronizer) reconcile(ctx context.Context, config *apiv1.Managed
 	if err != nil {
 		return fmt.Errorf("while getting superuser connection: %w", err)
 	}
-	appliedState, irreconcilableRoles, err := sr.synchronizeRoles(ctx, superUserDB, config, rolePasswords)
+	appliedState, unreconciledRoles, err := sr.synchronizeRoles(ctx, superUserDB, config, rolePasswords)
 	if err != nil {
 		return fmt.Errorf("while syncrhonizing managed roles: %w", err)
 	}
@@ -176,7 +168,7 @@ func (sr *RoleSynchronizer) reconcile(ctx context.Context, config *apiv1.Managed
 	}
 	updatedCluster := remoteCluster.DeepCopy()
 	updatedCluster.Status.ManagedRolesStatus.PasswordStatus = appliedState
-	updatedCluster.Status.ManagedRolesStatus.CannotReconcile = irreconcilableRoles
+	updatedCluster.Status.ManagedRolesStatus.CannotReconcile = unreconciledRoles
 	return sr.client.Status().Patch(ctx, updatedCluster, client.MergeFrom(&remoteCluster))
 }
 
@@ -191,70 +183,61 @@ func getRoleNames(roles []roleConfigurationAdapter) []string {
 // synchronizeRoles aligns roles in the database to the spec
 // It returns
 //   - the PasswordState for any updated roles
-//   - any roles that had expectable postgres errors
-//   - any unexpected error
+//   - the per-role errors encountered while applying role actions (e.g.
+//     unrealizable postgres operations, missing password secret)
+//   - any error that prevents the whole synchronization from running
+//     (e.g. listing the roles in the database failed)
 func (sr *RoleSynchronizer) synchronizeRoles(
 	ctx context.Context,
 	db *sql.DB,
 	config *apiv1.ManagedConfiguration,
 	storedPasswordState map[string]apiv1.PasswordState,
 ) (map[string]apiv1.PasswordState, map[string][]string, error) {
-	latestSecretResourceVersion, err := getPasswordSecretResourceVersion(
+	latestSecretResourceVersion := getPasswordSecretResourceVersion(
 		ctx, sr.client, config.Roles, sr.instance.GetNamespaceName())
-	if err != nil {
-		return nil, nil, err
-	}
 	rolesInDB, err := List(ctx, db)
 	if err != nil {
 		return nil, nil, err
 	}
 	rolesByAction := evaluateNextRoleActions(
 		ctx, config, rolesInDB, storedPasswordState, latestSecretResourceVersion)
 
-	passwordStates, irreconcilableRoles, err := sr.applyRoleActions(ctx, db, rolesByAction)
-	if err != nil {
-		return nil, nil, err
-	}
+	passwordStates, unreconciledRoles := sr.applyRoleActions(ctx, db, rolesByAction)
 
 	// Merge the status from database into spec. We should keep all the status
 	// otherwise in the next loop the user without status will be marked as need update
 	for role, stateInDatabase := range passwordStates {
 		storedPasswordState[role] = stateInDatabase
 	}
-	return storedPasswordState, irreconcilableRoles, nil
+	return storedPasswordState, unreconciledRoles, nil
 }
 
-// applyRoleActions applies the actions to reconcile roles in the DB with the Spec
-// It returns the apiv1.PasswordState for each role, as well as a map of roles that
-// cannot be reconciled for expectable errors, e.g. dropping a role owning content
-//
-// NOTE: applyRoleActions will carry on after an expectable error, i.e. an error
-// due to an invalid request for postgres. This is so that other actions will not
-// be blocked by a user error.
-// It will, however, error out on unexpected errors.
+// applyRoleActions applies the actions to reconcile roles in the DB with the Spec.
+// It returns the apiv1.PasswordState for each successfully applied role, and a
+// map collecting the errors encountered per role. Both expectable errors
+// (e.g. dropping a role that owns content) and unexpected errors are recorded
+// in the map, so that an error on one role does not block the reconciliation
+// of the others.
 func (sr *RoleSynchronizer) applyRoleActions(
 	ctx context.Context,
 	db *sql.DB,
 	rolesByAction rolesByAction,
-) (map[string]apiv1.PasswordState, map[string][]string, error) {
+) (map[string]apiv1.PasswordState, map[string][]string) {
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Debug("applying role actions")
 
-	irreconcilableRoles := make(map[string][]string)
+	unreconciledRoles := make(map[string][]string)
 	appliedChanges := make(map[string]apiv1.PasswordState)
-	handleRoleError := func(errToEvaluate error, roleName string, action roleAction) error {
-		// log unexpected errors, collect expectable PostgreSQL errors
+	handleRoleError := func(errToEvaluate error, roleName string, action roleAction) {
 		if errToEvaluate == nil {
-			return nil
+			return
 		}
 		roleError, err := parseRoleError(errToEvaluate, roleName, action)
-		if err != nil {
-			contextLog.Error(err, "while performing "+string(action), "role", roleName)
-			return err
+		if err == nil {
+			unreconciledRoles[roleName] = append(unreconciledRoles[roleName], roleError.Error())
+		} else {
+			unreconciledRoles[roleName] = append(unreconciledRoles[roleName], errToEvaluate.Error())
 		}
-
-		irreconcilableRoles[roleName] = append(irreconcilableRoles[roleName], roleError.Error())
-		return nil
 	}
 
 	actionsCreateUpdate := []roleAction{roleCreate, roleUpdate}
@@ -264,42 +247,32 @@ func (sr *RoleSynchronizer) applyRoleActions(
 			if err == nil {
 				appliedChanges[role.Name] = appliedState
 			}
-			if unhandledErr := handleRoleError(err, role.Name, action); unhandledErr != nil {
-				return nil, nil, unhandledErr
-			}
+			handleRoleError(err, role.Name, action)
 		}
 	}
 
 	for _, role := range rolesByAction[roleSetComment] {
 		// NOTE: adding/updating a comment on a role does not alter its TransactionID
 		err := UpdateComment(ctx, db, role.toDatabaseRole())
-		if unhandledErr := handleRoleError(err, role.Name, roleSetComment); unhandledErr != nil {
-			return nil, nil, unhandledErr
-		}
+		handleRoleError(err, role.Name, roleSetComment)
 	}
 
 	for _, role := range rolesByAction[roleUpdateMemberships] {
 		// NOTE: revoking / granting to a role does not alter its TransactionID
 		dbRole := role.toDatabaseRole()
 		grants, revokes, err := getRoleMembershipDiff(ctx, db, role, dbRole)
-		if unhandledErr := handleRoleError(err, role.Name, roleUpdateMemberships); unhandledErr != nil {
-			return nil, nil, unhandledErr
-		}
+		handleRoleError(err, role.Name, roleUpdateMemberships)
 
 		err = UpdateMembership(ctx, db, dbRole, grants, revokes)
-		if unhandledErr := handleRoleError(err, role.Name, roleUpdateMemberships); unhandledErr != nil {
-			return nil, nil, unhandledErr
-		}
+		handleRoleError(err, role.Name, roleUpdateMemberships)
 	}
 
 	for _, role := range rolesByAction[roleDelete] {
 		err := Delete(ctx, db, role.toDatabaseRole())
-		if unhandledErr := handleRoleError(err, role.Name, roleDelete); unhandledErr != nil {
-			return nil, nil, unhandledErr
-		}
+		handleRoleError(err, role.Name, roleDelete)
 	}
 
-	return appliedChanges, irreconcilableRoles, nil
+	return appliedChanges, unreconciledRoles
 }
 
 func getRoleMembershipDiff(
@@ -397,10 +370,8 @@ func getPassword(
 		client.ObjectKey{Namespace: namespace, Name: secretName},
 		&secret)
 	if err != nil {
-		if apierrs.IsNotFound(err) {
-			return passwordSecret{}, nil
-		}
-		return passwordSecret{}, err
+		return passwordSecret{},
+			fmt.Errorf("failed to get password secret %s: %w", secretName, err)
 	}
 	usernameFromSecret, passwordFromSecret, err := utils.GetUserPasswordFromSecret(&secret)
 	if err != nil {
@@ -418,26 +389,31 @@ func getPassword(
 		nil
 }
 
-// getPasswordSecretResourceVersion returns a list of resource version of the passwords secrets for managed roles
-// stored as Kubernetes secrets
+// getPasswordSecretResourceVersion returns a list of resource version of the password secrets for managed roles
+// stored as Kubernetes secrets. Roles whose secret cannot be fetched are
+// omitted from the result; the action evaluator will route them through the
+// update path so the failure surfaces in CannotReconcile.
 func getPasswordSecretResourceVersion(
 	ctx context.Context,
-	client client.Client,
+	cl client.Client,
 	rolesInSpec []apiv1.RoleConfiguration,
 	namespace string,
-) (map[string]string, error) {
+) map[string]string {
+	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	re := make(map[string]string)
 	for _, role := range rolesInSpec {
 		if role.PasswordSecret == nil || role.DisablePassword {
 			continue
 		}
-		passwordSecret, err := getPassword(ctx, client, roleConfigurationAdapter{RoleConfiguration: role}, namespace)
+		passwordSecret, err := getPassword(ctx, cl, roleConfigurationAdapter{RoleConfiguration: role}, namespace)
 		if err != nil {
-			return nil, err
+			contextLog.Debug("could not fetch password secret for role; will be flagged for reconciliation",
+				"role", role.Name, "secret", role.PasswordSecret.Name, "err", err.Error())
+			continue
 		}
 		re[role.Name] = passwordSecret.version
 	}
-	return re, nil
+	return re
 }
 
 func getRolesToGrant(inRoleInDB, inRoleInSpec []string) []string {