From f070d447efcf442cd6be93a4df6985623c654e31 Mon Sep 17 00:00:00 2001
From: Kevin Wan <wanjunfeng@gmail.com>
Date: Thu, 9 Sep 2021 15:43:33 +0800
Subject: [PATCH] refactor for better error reporting on sql error (#1016)

* refactor for better error reporting on sql error

* fix errors

* add docs
---
 core/stores/sqlx/sqlconn.go                   | 29 ++++++++++---------
 core/stores/sqlx/tx.go                        |  2 +-
 .../resolver/kube/deploy/clusterrole.yaml     |  9 ++++++
 .../kube/deploy/clusterrolebinding.yaml       | 12 ++++++++
 .../resolver/kube/deploy/serviceaccount.yaml  |  5 ++++
 5 files changed, 43 insertions(+), 14 deletions(-)
 create mode 100644 zrpc/internal/resolver/kube/deploy/clusterrole.yaml
 create mode 100644 zrpc/internal/resolver/kube/deploy/clusterrolebinding.yaml
 create mode 100644 zrpc/internal/resolver/kube/deploy/serviceaccount.yaml

diff --git a/core/stores/sqlx/sqlconn.go b/core/stores/sqlx/sqlconn.go
index cb4f8861..ce380487 100644
--- a/core/stores/sqlx/sqlconn.go
+++ b/core/stores/sqlx/sqlconn.go
@@ -4,11 +4,9 @@ import (
 	"database/sql"
 
 	"github.com/tal-tech/go-zero/core/breaker"
+	"github.com/tal-tech/go-zero/core/logx"
 )
 
-// datasource placeholder for logging error.
-const rawDB = "sql.DB"
-
 // ErrNotFound is an alias of sql.ErrNoRows
 var ErrNotFound = sql.ErrNoRows
 
@@ -26,6 +24,7 @@ type (
 	// SqlConn only stands for raw connections, so Transact method can be called.
 	SqlConn interface {
 		Session
+		// RawDB is for other ORM to operate with, use it with caution.
 		RawDB() (*sql.DB, error)
 		Transact(func(session Session) error) error
 	}
@@ -47,11 +46,11 @@ type (
 	// Because CORBA doesn't support PREPARE, so we need to combine the
 	// query arguments into one string and do underlying query without arguments
 	commonSqlConn struct {
-		datasource string
-		connProv   connProvider
-		beginTx    beginnable
-		brk        breaker.Breaker
-		accept     func(error) bool
+		connProv connProvider
+		onError  func(error)
+		beginTx  beginnable
+		brk      breaker.Breaker
+		accept   func(error) bool
 	}
 
 	connProvider func() (*sql.DB, error)
@@ -75,10 +74,12 @@ type (
 // NewSqlConn returns a SqlConn with given driver name and datasource.
 func NewSqlConn(driverName, datasource string, opts ...SqlOption) SqlConn {
 	conn := &commonSqlConn{
-		datasource: datasource,
 		connProv: func() (*sql.DB, error) {
 			return getSqlConn(driverName, datasource)
 		},
+		onError: func(err error) {
+			logInstanceError(datasource, err)
+		},
 		beginTx: begin,
 		brk:     breaker.NewBreaker(),
 	}
@@ -93,10 +94,12 @@ func NewSqlConn(driverName, datasource string, opts ...SqlOption) SqlConn {
 // Use it with caution, it's provided for other ORM to interact with.
 func NewSqlConnFromDB(db *sql.DB, opts ...SqlOption) SqlConn {
 	conn := &commonSqlConn{
-		datasource: rawDB,
 		connProv: func() (*sql.DB, error) {
 			return db, nil
 		},
+		onError: func(err error) {
+			logx.Errorf("Error on getting sql instance: %v", err)
+		},
 		beginTx: begin,
 		brk:     breaker.NewBreaker(),
 	}
@@ -112,7 +115,7 @@ func (db *commonSqlConn) Exec(q string, args ...interface{}) (result sql.Result,
 		var conn *sql.DB
 		conn, err = db.connProv()
 		if err != nil {
-			logInstanceError(db.datasource, err)
+			db.onError(err)
 			return err
 		}
 
@@ -128,7 +131,7 @@ func (db *commonSqlConn) Prepare(query string) (stmt StmtSession, err error) {
 		var conn *sql.DB
 		conn, err = db.connProv()
 		if err != nil {
-			logInstanceError(db.datasource, err)
+			db.onError(err)
 			return err
 		}
 
@@ -195,7 +198,7 @@ func (db *commonSqlConn) queryRows(scanner func(*sql.Rows) error, q string, args
 	return db.brk.DoWithAcceptable(func() error {
 		conn, err := db.connProv()
 		if err != nil {
-			logInstanceError(db.datasource, err)
+			db.onError(err)
 			return err
 		}
 
diff --git a/core/stores/sqlx/tx.go b/core/stores/sqlx/tx.go
index bf7d280e..0ae35ff9 100644
--- a/core/stores/sqlx/tx.go
+++ b/core/stores/sqlx/tx.go
@@ -73,7 +73,7 @@ func begin(db *sql.DB) (trans, error) {
 func transact(db *commonSqlConn, b beginnable, fn func(Session) error) (err error) {
 	conn, err := db.connProv()
 	if err != nil {
-		logInstanceError(db.datasource, err)
+		db.onError(err)
 		return err
 	}
 
diff --git a/zrpc/internal/resolver/kube/deploy/clusterrole.yaml b/zrpc/internal/resolver/kube/deploy/clusterrole.yaml
new file mode 100644
index 00000000..d95e7378
--- /dev/null
+++ b/zrpc/internal/resolver/kube/deploy/clusterrole.yaml
@@ -0,0 +1,9 @@
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: endpoints-reader
+rules:
+  - apiGroups: [""]
+    resources: ["endpoints"]
+    verbs: ["get", "watch", "list"]
+
diff --git a/zrpc/internal/resolver/kube/deploy/clusterrolebinding.yaml b/zrpc/internal/resolver/kube/deploy/clusterrolebinding.yaml
new file mode 100644
index 00000000..6d0b4c6a
--- /dev/null
+++ b/zrpc/internal/resolver/kube/deploy/clusterrolebinding.yaml
@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: endpoints-reader
+subjects:
+  - kind: ServiceAccount
+    name: endpoints-reader
+    namespace: kevin # the namespace that the ServiceAccount resides in
+roleRef:
+  kind: ClusterRole
+  name: endpoints-reader
+  apiGroup: rbac.authorization.k8s.io
diff --git a/zrpc/internal/resolver/kube/deploy/serviceaccount.yaml b/zrpc/internal/resolver/kube/deploy/serviceaccount.yaml
new file mode 100644
index 00000000..8364d5aa
--- /dev/null
+++ b/zrpc/internal/resolver/kube/deploy/serviceaccount.yaml
@@ -0,0 +1,5 @@
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: endpoints-reader
+  namespace: kevin # the namespace to create the ServiceAccount