chore: cleanup of unit tests pt 1

Signed-off-by: Jaime Silvela <[email protected]>

internal/management/controller/roles/postgres.go

@@ -46,7 +46,9 @@ func (sm PostgresRoleManager) List(
 	db *sql.DB,
 ) ([]DatabaseRole, error) {
 	logger := log.FromContext(ctx).WithName("roles_reconciler")
-	wrapErr := func(err error) error { return fmt.Errorf("while listing DB roles for DRM: %w", err) }
+	wrapErr := func(err error) error {
+		return fmt.Errorf("while listing DB roles for role reconciler: %w", err)
+	}
 
 	rows, err := db.QueryContext(
 		ctx,
@@ -114,7 +116,7 @@ func (sm PostgresRoleManager) Update(ctx context.Context, db *sql.DB, role Datab
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Trace("Invoked", "role", role)
 	wrapErr := func(err error) error {
-		return fmt.Errorf("while updating role %s with DRM: %w", role.Name, err)
+		return fmt.Errorf("while updating role %s with role reconciler: %w", role.Name, err)
 	}
 	var query strings.Builder
 
@@ -138,7 +140,7 @@ func (sm PostgresRoleManager) Create(ctx context.Context, db *sql.DB, role Datab
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Trace("Invoked", "role", role)
 	wrapErr := func(err error) error {
-		return fmt.Errorf("while creating role %s with DRM: %w", role.Name, err)
+		return fmt.Errorf("while creating role %s with role reconciler: %w", role.Name, err)
 	}
 
 	var query strings.Builder
@@ -173,7 +175,7 @@ func (sm PostgresRoleManager) Delete(ctx context.Context, db *sql.DB, role Datab
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Trace("Invoked", "role", role)
 	wrapErr := func(err error) error {
-		return fmt.Errorf("while deleting role %s with DRM: %w", role.Name, err)
+		return fmt.Errorf("while deleting role %s with role reconciler: %w", role.Name, err)
 	}
 
 	query := fmt.Sprintf("DROP ROLE %s", pgx.Identifier{role.Name}.Sanitize())
@@ -192,7 +194,7 @@ func (sm PostgresRoleManager) GetLastTransactionID(ctx context.Context, db *sql.
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Trace("Invoked", "role", role)
 	wrapErr := func(err error) error {
-		return fmt.Errorf("while getting last xmin for role %s with DRM: %w", role.Name, err)
+		return fmt.Errorf("while getting last xmin for role %s with role reconciler: %w", role.Name, err)
 	}
 
 	var xmin int64
@@ -214,7 +216,7 @@ func (sm PostgresRoleManager) UpdateComment(ctx context.Context, db *sql.DB, rol
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Trace("Invoked", "role", role)
 	wrapErr := func(err error) error {
-		return fmt.Errorf("while updating comment for role %s with DRM: %w", role.Name, err)
+		return fmt.Errorf("while updating comment for role %s with role reconciler: %w", role.Name, err)
 	}
 
 	query := fmt.Sprintf("COMMENT ON ROLE %s IS %s",
@@ -243,7 +245,7 @@ func (sm PostgresRoleManager) UpdateMembership(
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Trace("Invoked", "role", role)
 	wrapErr := func(err error) error {
-		return fmt.Errorf("while updating memberships for role %s with DRM: %w", role.Name, err)
+		return fmt.Errorf("while updating memberships for role %s with role reconciler: %w", role.Name, err)
 	}
 	if len(rolesToRevoke)+len(rolesToGrant) == 0 {
 		contextLog.Debug("No membership change query to execute for role")
@@ -293,7 +295,7 @@ func (sm PostgresRoleManager) GetParentRoles(
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Trace("Invoked", "role", role)
 	wrapErr := func(err error) error {
-		return fmt.Errorf("while getting parents for role %s with DRM: %w", role.Name, err)
+		return fmt.Errorf("while getting parents for role %s with role reconciler: %w", role.Name, err)
 	}
 	query := `SELECT mem.inroles 
 		FROM pg_catalog.pg_authid as auth

internal/management/controller/roles/postgres_test.go

@@ -36,6 +36,18 @@ import (
 	. "github.com/onsi/gomega"
 )
 
+const (
+	expectedMembershipStmt = `SELECT mem.inroles 
+	FROM pg_catalog.pg_authid as auth
+	LEFT JOIN (
+		SELECT array_agg(pg_get_userbyid(roleid)) as inroles, member
+		FROM pg_auth_members GROUP BY member
+	) mem ON member = oid
+	WHERE rolname = $1`
+
+	wantedRoleCommentTpl = "COMMENT ON ROLE \"%s\" IS %s"
+)
+
 var _ = Describe("Postgres RoleManager implementation test", func() {
 	falseValue := false
 	validUntil := metav1.Date(2100, 0o1, 0o1, 0o0, 0o0, 0o0, 0o0, time.UTC)
@@ -127,7 +139,7 @@ var _ = Describe("Postgres RoleManager implementation test", func() {
 		wantedRoleWithDefaultConnectionLimit.Name)
 
 	wantedRoleCommentStmt := fmt.Sprintf(
-		"COMMENT ON ROLE \"%s\" IS %s",
+		wantedRoleCommentTpl,
 		wantedRole.Name, pq.QuoteLiteral(wantedRole.Comment))
 
 	wantedRoleExpectedAltStmt := fmt.Sprintf(
@@ -145,14 +157,6 @@ var _ = Describe("Postgres RoleManager implementation test", func() {
 		) mem ON member = oid
 		WHERE rolname not like 'pg\_%'`
 
-	expectedMembershipStmt := `SELECT mem.inroles 
-		FROM pg_catalog.pg_authid as auth
-		LEFT JOIN (
-			SELECT array_agg(pg_get_userbyid(roleid)) as inroles, member
-			FROM pg_auth_members GROUP BY member
-		) mem ON member = oid
-		WHERE rolname = $1`
-
 	// Testing List
 	It("List can read the list of roles from the DB", func(ctx context.Context) {
 		db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherEqual))
@@ -237,7 +241,7 @@ var _ = Describe("Postgres RoleManager implementation test", func() {
 		mock.ExpectQuery(expectedSelStmt).WillReturnError(dbError)
 		roles, err := prm.List(ctx, db)
 		Expect(err).To(HaveOccurred())
-		Expect(err.Error()).To(BeEquivalentTo("while listing DB roles for DRM: Kaboom"))
+		Expect(err.Error()).To(BeEquivalentTo("while listing DB roles for role reconciler: Kaboom"))
 		Expect(roles).To(BeEmpty())
 	})
 	// Testing Create

internal/management/controller/roles/reconciler_test.go

@@ -55,7 +55,7 @@ var _ = Describe("Role reconciler test", func() {
 				},
 			},
 		}
-		pgStringError := "while listing DB roles for DRM: " +
+		pgStringError := "while listing DB roles for role reconciler: " +
 			"failed to connect to `user=postgres database=postgres`: " +
 			"/controller/run/.s.PGSQL.5432 (/controller/run): " +
 			"dial error: dial unix /controller/run/.s.PGSQL.5432: connect: no such file or directory"

internal/management/controller/roles/runnable.go

@@ -19,6 +19,7 @@ package roles
 import (
 	"context"
 	"database/sql"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -49,12 +50,21 @@ const (
 	roleUpdateMemberships roleAction = "UPDATE_MEMBERSHIPS"
 )
 
+type instanceInterface interface {
+	GetSuperUserDB() (*sql.DB, error)
+	IsPrimary() (bool, error)
+	RoleSynchronizerChan() <-chan *apiv1.ManagedConfiguration
+	IsServerHealthy() error
+	GetClusterName() string
+	GetNamespaceName() string
+}
+
 // A RoleSynchronizer is a Kubernetes manager.Runnable
 // that makes sure the Roles in the PostgreSQL databases are in sync with the spec
 //
 // c.f. https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/manager#Runnable
 type RoleSynchronizer struct {
-	instance *postgres.Instance
+	instance instanceInterface
 	client   client.Client
 }
 
@@ -174,6 +184,10 @@ 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 unexpeted error
 func (sr *RoleSynchronizer) synchronizeRoles(
 	ctx context.Context,
 	roleManager RoleManager,
@@ -195,16 +209,16 @@ func (sr *RoleSynchronizer) synchronizeRoles(
 	}
 	rolesByAction := evaluateNextRoleActions(
 		ctx, config, rolesInDB, storedPasswordState, latestSecretResourceVersion)
-	if err != nil {
-		return nil, nil, fmt.Errorf("while syncrhonizing managed roles: %w", err)
-	}
 
-	passwordStates, irreconcilableRoles := sr.applyRoleActions(
+	passwordStates, irreconcilableRoles, err := sr.applyRoleActions(
 		ctx,
 		db,
 		roleManager,
 		rolesByAction,
 	)
+	if err != nil {
+		return nil, nil, err
+	}
 
 	// 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
@@ -218,32 +232,34 @@ func (sr *RoleSynchronizer) synchronizeRoles(
 // 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 not error out if a single role operation fails.
-// This is designed so that a role configuration that cannot be honored by PostgreSQL
-// cannot stop the reconciliation loop and prevent other roles from being applied
+// 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.
 func (sr *RoleSynchronizer) applyRoleActions(
 	ctx context.Context,
 	db *sql.DB,
 	roleManager RoleManager,
 	rolesByAction rolesByAction,
-) (map[string]apiv1.PasswordState, map[string][]string) {
+) (map[string]apiv1.PasswordState, map[string][]string, error) {
 	contextLog := log.FromContext(ctx).WithName("roles_reconciler")
 	contextLog.Debug("applying role actions")
 
 	irreconcilableRoles := make(map[string][]string)
 	appliedChanges := make(map[string]apiv1.PasswordState)
-	handleRoleError := func(errToEvaluate error, roleName string, action roleAction) {
+	handleRoleError := func(errToEvaluate error, roleName string, action roleAction) error {
 		// log unexpected errors, collect expectable PostgreSQL errors
 		if errToEvaluate == nil {
-			return
+			return nil
 		}
 		roleError, err := parseRoleError(errToEvaluate, roleName, action)
 		if err != nil {
 			contextLog.Error(err, "while performing "+string(action), "role", roleName)
-			return
+			return err
 		}
 
 		irreconcilableRoles[roleName] = append(irreconcilableRoles[roleName], roleError.Error())
+		return nil
 	}
 
 	for action, roles := range rolesByAction {
@@ -257,35 +273,39 @@ func (sr *RoleSynchronizer) applyRoleActions(
 			"roles", getRoleNames(roles), "action", action)
 
 		for _, role := range roles {
+			var (
+				err             error
+				appliedState    apiv1.PasswordState
+				grants, revokes []string
+			)
 			switch action {
 			case roleCreate, roleUpdate:
-				appliedState, err := sr.applyRoleCreateUpdate(ctx, db, roleManager, role, action)
+				appliedState, err = sr.applyRoleCreateUpdate(ctx, db, roleManager, role, action)
 				if err == nil {
 					appliedChanges[role.Name] = appliedState
 				}
-				handleRoleError(err, role.Name, action)
 			case roleDelete:
-				err := roleManager.Delete(ctx, db, role.toDatabaseRole())
-				handleRoleError(err, role.Name, action)
+				err = roleManager.Delete(ctx, db, role.toDatabaseRole())
 			case roleSetComment:
 				// NOTE: adding/updating a comment on a role does not alter its TransactionID
-				err := roleManager.UpdateComment(ctx, db, role.toDatabaseRole())
-				handleRoleError(err, role.Name, action)
+				err = roleManager.UpdateComment(ctx, db, role.toDatabaseRole())
 			case roleUpdateMemberships:
 				// NOTE: revoking / granting to a role does not alter its TransactionID
 				dbRole := role.toDatabaseRole()
-				grants, revokes, err := getRoleMembershipDiff(ctx, db, roleManager, role, dbRole)
-				if err != nil {
-					contextLog.Error(err, "while performing "+string(action), "role", role.Name)
-					continue
+				grants, revokes, err = getRoleMembershipDiff(ctx, db, roleManager, role, dbRole)
+				if unhandledErr := handleRoleError(err, role.Name, action); unhandledErr != nil {
+					return nil, nil, unhandledErr
 				}
+
 				err = roleManager.UpdateMembership(ctx, db, dbRole, grants, revokes)
-				handleRoleError(err, role.Name, action)
+			}
+			if unhandledErr := handleRoleError(err, role.Name, action); unhandledErr != nil {
+				return nil, nil, unhandledErr
 			}
 		}
 	}
 
-	return appliedChanges, irreconcilableRoles
+	return appliedChanges, irreconcilableRoles, nil
 }
 
 func getRoleMembershipDiff(
@@ -296,7 +316,7 @@ func getRoleMembershipDiff(
 	dbRole DatabaseRole,
 ) ([]string, []string, error) {
 	inRoleInDB, err := roleManager.GetParentRoles(ctx, db, dbRole)
-	if err != nil {
+	if err != nil && !errors.Is(err, sql.ErrNoRows) {
 		return nil, nil, err
 	}
 	rolesToGrant := getRolesToGrant(inRoleInDB, role.InRoles)