Retry database reads (#747)

Fixes GH-652

internal/admin/authorizedapps/save.go

@@ -54,7 +54,7 @@ func (h *saveController) Execute(c *gin.Context) {
 		authApp := model.NewAuthorizedApp()
 		priorKey := form.PriorKey()
 		if priorKey != "" {
-			authApp, err = aadb.GetAuthorizedApp(ctx, h.env.SecretManager(), priorKey)
+			authApp, err = aadb.GetAuthorizedApp(ctx, priorKey)
 			if err != nil {
 				admin.ErrorPage(c, "Invalid request, app to edit not found.")
 				return

internal/admin/authorizedapps/view.go

@@ -67,7 +67,7 @@ func (h *viewController) Execute(c *gin.Context) {
 	} else {
 		aadb := database.New(h.env.Database())
 		var err error
-		authorizedApp, err = aadb.GetAuthorizedApp(ctx, h.env.SecretManager(), appID)
+		authorizedApp, err = aadb.GetAuthorizedApp(ctx, appID)
 		if err != nil {
 			admin.ErrorPage(c, err.Error())
 			return

internal/authorizedapp/database/authorized_app.go

@@ -23,7 +23,6 @@ import (
 
 	"github.com/google/exposure-notifications-server/internal/authorizedapp/model"
 	"github.com/google/exposure-notifications-server/internal/database"
-	"github.com/google/exposure-notifications-server/pkg/secrets"
 	pgx "github.com/jackc/pgx/v4"
 )
 
@@ -110,64 +109,71 @@ func (aa *AuthorizedAppDB) DeleteAuthorizedApp(ctx context.Context, name string)
 }
 
 func (aa *AuthorizedAppDB) ListAuthorizedApps(ctx context.Context) ([]*model.AuthorizedApp, error) {
-	conn, err := aa.db.Pool.Acquire(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("acquiring connection: %w", err)
-	}
-	defer conn.Release()
-
-	query := `
-		SELECT
-			LOWER(app_package_name), allowed_regions,
-			allowed_health_authority_ids, bypass_health_authority_verification
-		FROM
-			AuthorizedApp
-		ORDER BY LOWER(app_package_name) ASC`
-
-	rows, err := conn.Query(ctx, query)
-	if err != nil {
-		return nil, err
-	}
-	defer rows.Close()
-
-	var result []*model.AuthorizedApp
-	for rows.Next() {
-		if err := rows.Err(); err != nil {
-			return nil, fmt.Errorf("iterating rows: %w", err)
-		}
-
-		app, err := scanOneAuthorizedApp(ctx, rows)
+	var apps []*model.AuthorizedApp
+
+	if err := aa.db.InTx(ctx, pgx.ReadCommitted, func(tx pgx.Tx) error {
+		rows, err := tx.Query(ctx, `
+			SELECT
+				LOWER(app_package_name), allowed_regions,
+				allowed_health_authority_ids, bypass_health_authority_verification
+			FROM
+				AuthorizedApp
+			ORDER BY LOWER(app_package_name) ASC
+		`)
 		if err != nil {
-			return nil, fmt.Errorf("error reading authorized apps: %w", err)
+			return fmt.Errorf("failed to list: %w", err)
+		}
+		defer rows.Close()
+
+		for rows.Next() {
+			if err := rows.Err(); err != nil {
+				return fmt.Errorf("failed to iterate: %w", err)
+			}
+
+			app, err := scanOneAuthorizedApp(rows)
+			if err != nil {
+				return fmt.Errorf("failed to parse: %w", err)
+			}
+			apps = append(apps, app)
 		}
-		result = append(result, app)
+
+		return nil
+	}); err != nil {
+		return nil, fmt.Errorf("list authorized apps: %w", err)
 	}
-	return result, nil
+
+	return apps, nil
 }
 
 // GetAuthorizedApp loads a single AuthorizedApp for the given name. If no row
 // exists, this returns nil.
-func (aa *AuthorizedAppDB) GetAuthorizedApp(ctx context.Context, sm secrets.SecretManager, name string) (*model.AuthorizedApp, error) {
-	conn, err := aa.db.Pool.Acquire(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("acquiring connection: %v", err)
-	}
-	defer conn.Release()
-
-	query := `
-		SELECT
-			LOWER(app_package_name), allowed_regions,
-			allowed_health_authority_ids, bypass_health_authority_verification
-		FROM
-			AuthorizedApp
-		WHERE LOWER(app_package_name) = LOWER($1)`
+func (aa *AuthorizedAppDB) GetAuthorizedApp(ctx context.Context, name string) (*model.AuthorizedApp, error) {
+	var app *model.AuthorizedApp
+
+	if err := aa.db.InTx(ctx, pgx.ReadCommitted, func(tx pgx.Tx) error {
+		row := tx.QueryRow(ctx, `
+			SELECT
+				LOWER(app_package_name), allowed_regions,
+				allowed_health_authority_ids, bypass_health_authority_verification
+			FROM
+				AuthorizedApp
+			WHERE LOWER(app_package_name) = LOWER($1)
+		`, name)
 
-	row := conn.QueryRow(ctx, query, name)
+		var err error
+		app, err = scanOneAuthorizedApp(row)
+		if err != nil {
+			return fmt.Errorf("failed to parse: %w", err)
+		}
+		return nil
+	}); err != nil {
+		return nil, fmt.Errorf("get authorized app: %w", err)
+	}
 
-	return scanOneAuthorizedApp(ctx, row)
+	return app, nil
 }
 
-func scanOneAuthorizedApp(ctx context.Context, row pgx.Row) (*model.AuthorizedApp, error) {
+func scanOneAuthorizedApp(row pgx.Row) (*model.AuthorizedApp, error) {
 	config := model.NewAuthorizedApp()
 	var allowedRegions []string
 	var allowedHealthAuthorityIDs []int64

internal/authorizedapp/database/authorized_app_test.go

@@ -17,11 +17,6 @@ package database
 import (
 	"context"
 	"crypto/ecdsa"
-	"crypto/elliptic"
-	"crypto/rand"
-	"crypto/x509"
-	"encoding/pem"
-	"fmt"
 	"testing"
 
 	"github.com/google/exposure-notifications-server/internal/authorizedapp/model"
@@ -30,27 +25,12 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 )
 
-type testSecretManager struct {
-	values map[string]string
-}
-
-func (s *testSecretManager) GetSecretValue(ctx context.Context, name string) (string, error) {
-	v, ok := s.values[name]
-	if !ok {
-		return "", fmt.Errorf("missing %q", name)
-	}
-	return v, nil
-}
-
 func TestAuthorizedAppLifecycle(t *testing.T) {
 	t.Parallel()
 
 	testDB := database.NewTestDatabase(t)
 	ctx := context.Background()
 	aadb := New(testDB)
-	sm := &testSecretManager{
-		values: map[string]string{},
-	}
 
 	source := &model.AuthorizedApp{
 		AppPackageName:            "myapp",
@@ -62,7 +42,7 @@ func TestAuthorizedAppLifecycle(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	readBack, err := aadb.GetAuthorizedApp(ctx, sm, source.AppPackageName)
+	readBack, err := aadb.GetAuthorizedApp(ctx, source.AppPackageName)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -75,7 +55,7 @@ func TestAuthorizedAppLifecycle(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	readBack, err = aadb.GetAuthorizedApp(ctx, sm, source.AppPackageName)
+	readBack, err = aadb.GetAuthorizedApp(ctx, source.AppPackageName)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -87,7 +67,7 @@ func TestAuthorizedAppLifecycle(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	readBack, err = aadb.GetAuthorizedApp(ctx, sm, source.AppPackageName)
+	readBack, err = aadb.GetAuthorizedApp(ctx, source.AppPackageName)
 	if err != nil {
 		t.Errorf("unexpected error seen: %v", err)
 	}
@@ -101,26 +81,6 @@ func TestGetAuthorizedApp(t *testing.T) {
 
 	ctx := context.Background()
 
-	// Create private key for parsing later
-	p8PrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	if err != nil {
-		t.Fatal(err)
-	}
-	derKey, err := x509.MarshalPKCS8PrivateKey(p8PrivateKey)
-	if err != nil {
-		t.Fatal(err)
-	}
-	pemBytes := pem.EncodeToMemory(&pem.Block{
-		Type:  "PRIVATE KEY",
-		Bytes: derKey,
-	})
-
-	sm := &testSecretManager{
-		values: map[string]string{
-			"private_key": string(pemBytes),
-		},
-	}
-
 	cases := []struct {
 		name string
 		sql  string
@@ -182,7 +142,7 @@ func TestGetAuthorizedApp(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			config, err := New(testDB).GetAuthorizedApp(ctx, sm, "myapp")
+			config, err := New(testDB).GetAuthorizedApp(ctx, "myapp")
 			if (err != nil) != c.err {
 				t.Fatal(err)
 			}

internal/authorizedapp/database_provider.go

@@ -27,7 +27,6 @@ import (
 	"github.com/google/exposure-notifications-server/pkg/cache"
 
 	"github.com/google/exposure-notifications-server/internal/logging"
-	"github.com/google/exposure-notifications-server/pkg/secrets"
 )
 
 // Compile-time check to assert implementation.
@@ -37,7 +36,6 @@ var _ Provider = (*DatabaseProvider)(nil)
 // refreshes values on failure.
 type DatabaseProvider struct {
 	database      *database.DB
-	secretManager secrets.SecretManager
 	cacheDuration time.Duration
 
 	cache *cache.Cache
@@ -46,14 +44,6 @@ type DatabaseProvider struct {
 // DatabaseProviderOption is used as input to the database provider.
 type DatabaseProviderOption func(*DatabaseProvider) *DatabaseProvider
 
-// WithSecretManager sets the secret manager for resolving secrets.
-func WithSecretManager(sm secrets.SecretManager) DatabaseProviderOption {
-	return func(p *DatabaseProvider) *DatabaseProvider {
-		p.secretManager = sm
-		return p
-	}
-}
-
 // NewDatabaseProvider creates a new Provider that reads from a database.
 func NewDatabaseProvider(ctx context.Context, db *database.DB, config *Config, opts ...DatabaseProviderOption) (Provider, error) {
 	cache, err := cache.New(config.CacheDuration)
@@ -116,7 +106,7 @@ func (p *DatabaseProvider) loadAuthorizedAppFromDatabase(ctx context.Context, na
 	logger := logging.FromContext(ctx)
 
 	logger.Infof("authorizedapp: loading %v from database", name)
-	config, err := authorizedappdb.New(p.database).GetAuthorizedApp(ctx, p.secretManager, name)
+	config, err := authorizedappdb.New(p.database).GetAuthorizedApp(ctx, name)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read %v from database: %w", name, err)
 	}