Show non-PgError errors when connecting to db (flyteorg#445)

Signed-off-by: Sam Lai <[email protected]>

When connecting to a database instance that doesn't exist, that error is currently masked due to an invalid assumption that all wrapped errors returned from gorm are of type `*pgconn.PgError`. When a network error occurs, the returned error is of type `*net.OpError`. This causes the following panic -

```
{"json":{"src":"base.go:73"},"level":"fatal","msg":"caught panic: interface conversion: error is *net.OpError, not *pgconn.PgError [goroutine 1 [running]:\nruntime/debug.Stack()\n\t/home/user/.local/go/src/runtime/debug/stack.go:24
...
```

... and doesn't show what the actual error is.

This PR fixes that by using the functions in the `errors` package to check if the error chain contains an error of type `*pgconn.PgError`, and if not, simply returns the error.

It also removes the ConnectError interface, as it is no longer needed - the `errors.As` function will automatically check the whole error chain and unwrap as necessary.

With this fix, the error becomes -

```
{"json":{"src":"base.go:82"},"level":"fatal","msg":"failed to connect to `host=localhost user=postgres database=flyte`: dial error (dial tcp 127.0.0.1:5432: connect: connection refused)","ts":"2022-06-11T20:48:28+01:00"}
```

pkg/repositories/database.go

@@ -2,15 +2,14 @@ package repositories
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
-	"reflect"
 	"strings"
 
 	"github.com/flyteorg/flytestdlib/database"
 
-	repoErrors "github.com/flyteorg/flyteadmin/pkg/repositories/errors"
 	"gorm.io/driver/sqlite"
 
 	"github.com/flyteorg/flytestdlib/logger"
@@ -116,15 +115,7 @@ func createPostgresDbIfNotExists(ctx context.Context, gormConfig *gorm.Config, p
 		return gormDb, nil
 	}
 
-	// Check if its invalid db code error
-	cErr, ok := err.(repoErrors.ConnectError)
-	if !ok {
-		logger.Errorf(ctx, "Failed to cast error of type: %v, err: %v", reflect.TypeOf(err),
-			err)
-		return nil, err
-	}
-	pqError := cErr.Unwrap().(*pgconn.PgError)
-	if pqError.Code != pqInvalidDBCode {
+	if !isInvalidDBPgError(err) {
 		return nil, err
 	}
 
@@ -154,6 +145,17 @@ func createPostgresDbIfNotExists(ctx context.Context, gormConfig *gorm.Config, p
 	return gorm.Open(dialector, gormConfig)
 }
 
+func isInvalidDBPgError(err error) bool {
+	pgErr := &pgconn.PgError{}
+	if !errors.As(err, &pgErr) {
+		// err chain does not contain a pgconn.PgError
+		return false
+	}
+
+	// pgconn.PgError found in chain and set to pgErr
+	return pgErr.Code == pqInvalidDBCode
+}
+
 func setupDbConnectionPool(ctx context.Context, gormDb *gorm.DB, dbConfig *database.DbConfig) error {
 	genericDb, err := gormDb.DB()
 	if err != nil {

pkg/repositories/database_test.go

@@ -2,14 +2,17 @@ package repositories
 
 import (
 	"context"
+	"errors"
 	"io/ioutil"
+	"net"
 	"os"
 	"path"
 	"path/filepath"
 	"testing"
 	"time"
 
 	"github.com/flyteorg/flytestdlib/database"
+	"github.com/jackc/pgconn"
 
 	"github.com/flyteorg/flytestdlib/config"
 	"github.com/flyteorg/flytestdlib/logger"
@@ -73,6 +76,57 @@ func TestGetPostgresDsn(t *testing.T) {
 	})
 }
 
+type wrappedError struct {
+	err error
+}
+
+func (e *wrappedError) Error() string {
+	return e.err.Error()
+}
+
+func (e *wrappedError) Unwrap() error {
+	return e.err
+}
+
+func TestIsInvalidDBPgError(t *testing.T) {
+	// wrap error with wrappedError when testing to ensure the function checks the whole error chain
+
+	testCases := []struct {
+		Name           string
+		Err            error
+		ExpectedResult bool
+	}{
+		{
+			Name:           "nil error",
+			Err:            nil,
+			ExpectedResult: false,
+		},
+		{
+			Name:           "not a PgError",
+			Err:            &wrappedError{err: &net.OpError{Op: "connect", Err: errors.New("connection refused")}},
+			ExpectedResult: false,
+		},
+		{
+			Name:           "PgError but not invalid DB",
+			Err:            &wrappedError{&pgconn.PgError{Severity: "FATAL", Message: "out of memory", Code: "53200"}},
+			ExpectedResult: false,
+		},
+		{
+			Name:           "PgError and is invalid DB",
+			Err:            &wrappedError{&pgconn.PgError{Severity: "FATAL", Message: "database \"flyte\" does not exist", Code: "3D000"}},
+			ExpectedResult: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.Name, func(t *testing.T) {
+			assert.Equal(t, tc.ExpectedResult, isInvalidDBPgError(tc.Err))
+		})
+	}
+}
+
 func TestSetupDbConnectionPool(t *testing.T) {
 	ctx := context.TODO()
 	t.Run("successful", func(t *testing.T) {

pkg/repositories/errors/postgres.go

@@ -104,8 +104,3 @@ func NewPostgresErrorTransformer(scope promutils.Scope) ErrorTransformer {
 		metrics: metrics,
 	}
 }
-
-type ConnectError interface {
-	Unwrap() error
-	Error() string
-}