Repository: crossplane-contrib/provider-sql (go)

Issue:
Disabling MySQL binlog writing does not work
<!--
Thank you for helping to improve Crossplane!

Please be sure to search for open issues before raising a new one. We use issues
for bug reports and feature requests. Please find us at https://slack.crossplane.io
for questions, support, and discussion.
-->

### What happened?
Setting `binlog: false` to any of MySQL resources does not prevent MySQL server from writing to binlogs. Looks like this option is not applied at all.

### How can we reproduce it?

- Run a MySQL server with binlogs enabled
- Create a kube resource `User` for which `spec.forProvider.binlog = false`
- On MySQL server, run `mysqlbinlog -vv /var/lib/mysql/mysql-bin.XXXX`

→ You can see `CREATE USER` statement in binlogs

### What environment did it happen in?
Crossplane version: v1.14.5
Provider-sql version: v0.9.0

<!--
Include at least the version or commit of Crossplane you were running. Consider
also including your:

* Cloud provider or hardware configuration
* Kubernetes version (use `kubectl version`)
* Kubernetes distribution (e.g. Tectonic, GKE, OpenShift)
* OS (e.g. from /etc/os-release)
* Kernel (e.g. `uname -a`)
-->

### Analysis

By looking into the code, the `SET sql_log_bin = 0` statement is executed in a different MySQL connection from the one used for create/update/delete resource. In fact, as `mySQLDB.Exec()` creates a new `sql.Database` object, it also opens/closes a new every time it 's called, which explain why this option is not taken into account.

See [MySQL doc](https://dev.mysql.com/doc/refman/8.0/en/set-sql-log-bin.html) about this option.

I've been working on a patch to fix this, a PR will follow.

Pull request:
Give MySQL sql_log_bin at DSN level to take parameter into account
<!--
Thank you for helping to improve Crossplane!

Please read through https://git.io/fj2m9 if this is your first time opening a
Crossplane pull request. Find us in https://slack.crossplane.io/messages/dev if
you need any help contributing.
-->

### Description of your changes

<!--
Briefly describe what this pull request does. Be sure to direct your reviewers'
attention to anything that needs special consideration.

We love pull requests that resolve an open Crossplane issue. If yours does, you
can uncomment the below line to indicate which issue your PR fixes, for example
"Fixes #500":

-->
Fixes #170

As actual MySQL code is not designed to execute several statements in the same connection, the `sql_log_bin` parameter value is now given at [DSN level](https://github.com/go-sql-driver/mysql/tree/v1.5.0?tab=readme-ov-file#system-variables). This ensures this parameter is always taken into account, no matter how MySQL connections are managed under the hood.

I guess this could be fixed differently, but it's the simpler way I found to manage this. Any feedbacks are welcome.

I have:

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `make reviewable` to ensure this PR is ready for review.

### How has this code been tested

<!--
Before reviewers can be confident in the correctness of this pull request, it
needs to tested and shown to be correct. Briefly describe the testing that has
already been done or which is planned for this change.
-->

I created several `User` and `Grant` resources against a MySQL 8.0 server, with and without binlog option enabled, and then I check what was written in binlog files. It worked as expected.

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/pkg/clients/mysql/mysql.go b/pkg/clients/mysql/mysql.go
index f0d6206..dbcaa28 100644
--- a/pkg/clients/mysql/mysql.go
+++ b/pkg/clients/mysql/mysql.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
+	"strconv"
 	"strings"
 
 	"github.com/crossplane-contrib/provider-sql/pkg/clients/xsql"
@@ -16,7 +17,6 @@ import (
 
 const (
 	errNotSupported = "%s not supported by mysql client"
-	errSetSQLLogBin = "cannot set sql_log_bin = 0"
 	errFlushPriv    = "cannot flush privileges"
 )
 
@@ -28,7 +28,7 @@ type mySQLDB struct {
 }
 
 // New returns a new MySQL database client.
-func New(creds map[string][]byte, tls *string) xsql.DB {
+func New(creds map[string][]byte, tls *string, binlog *bool) xsql.DB {
 	// TODO(negz): Support alternative connection secret formats?
 	endpoint := string(creds[xpv1.ResourceCredentialsSecretEndpointKey])
 	port := string(creds[xpv1.ResourceCredentialsSecretPortKey])
@@ -38,7 +38,11 @@ func New(creds map[string][]byte, tls *string) xsql.DB {
 		defaultTLS := "preferred"
 		tls = &defaultTLS
 	}
-	dsn := DSN(username, password, endpoint, port, *tls)
+	if binlog == nil {
+		defaultBinlog := true
+		binlog = &defaultBinlog
+	}
+	dsn := DSN(username, password, endpoint, port, *tls, *binlog)
 
 	return mySQLDB{
 		dsn:      dsn,
@@ -49,16 +53,17 @@ func New(creds map[string][]byte, tls *string) xsql.DB {
 }
 
 // DSN returns the DSN URL
-func DSN(username, password, endpoint, port, tls string) string {
+func DSN(username, password, endpoint, port, tls string, binlog bool) string {
 	// Use net/url UserPassword to encode the username and password
 	// This will ensure that any special characters in the username or password
 	// are percent-encoded for use in the user info portion of the DSN URL
-	return fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s",
+	return fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s&sql_log_bin=%s",
 		username,
 		password,
 		endpoint,
 		port,
-		tls)
+		tls,
+		strconv.FormatBool(binlog))
 }
 
 // ExecTx is unsupported in MySQL.
@@ -143,30 +148,16 @@ type ExecQuery struct {
 
 // ExecOptions parametrizes which optional statements will be executed before or after ExecQuery.Query
 type ExecOptions struct {
-	// Binlog defines whether storing binlogs will be disabled before executing the query. Defaults to true
-	Binlog *bool
 	// Flush defines whether privileges will be flushed after executing the query. Defaults to true
 	Flush *bool
 }
 
-// ExecWithBinlogAndFlush is a wrapper function for xsql.DB.Exec() that allows the execution of optional queries before and after the provided query
-func ExecWithBinlogAndFlush(ctx context.Context, db xsql.DB, query ExecQuery, options ExecOptions) error {
-	if options.Binlog == nil {
-		options.Binlog = ptr.To(true)
-	}
-
+// ExecWithFlush is a wrapper function for xsql.DB.Exec() that allows the execution of optional queries before and after the provided query
+func ExecWithFlush(ctx context.Context, db xsql.DB, query ExecQuery, options ExecOptions) error {
 	if options.Flush == nil {
 		options.Flush = ptr.To(true)
 	}
 
-	if !*options.Binlog {
-		if err := db.Exec(ctx, xsql.Query{
-			String: "SET sql_log_bin = 0",
-		}); err != nil {
-			return errors.Wrap(err, errSetSQLLogBin)
-		}
-	}
-
 	if err := db.Exec(ctx, xsql.Query{
 		String: query.Query,
 	}); err != nil {
diff --git a/pkg/controller/mysql/database/reconciler.go b/pkg/controller/mysql/database/reconciler.go
index 2aab4db..9f60e78 100644
--- a/pkg/controller/mysql/database/reconciler.go
+++ b/pkg/controller/mysql/database/reconciler.go
@@ -77,7 +77,7 @@ func Setup(mgr ctrl.Manager, o xpcontroller.Options) error {
 type connector struct {
 	kube  client.Client
 	usage resource.Tracker
-	newDB func(creds map[string][]byte, tls *string) xsql.DB
+	newDB func(creds map[string][]byte, tls *string, binlog *bool) xsql.DB
 }
 
 func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.ExternalClient, error) {
@@ -110,7 +110,7 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E
 		return nil, errors.Wrap(err, errGetSecret)
 	}
 
-	return &external{db: c.newDB(s.Data, pc.Spec.TLS)}, nil
+	return &external{db: c.newDB(s.Data, pc.Spec.TLS, cr.Spec.ForProvider.BinLog)}, nil
 }
 
 type external struct{ db xsql.DB }
@@ -148,10 +148,9 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 		return managed.ExternalCreation{}, errors.New(errNotDatabase)
 	}
 
-	binlog := cr.Spec.ForProvider.BinLog
 	query := "CREATE DATABASE " + mysql.QuoteIdentifier(meta.GetExternalName(cr))
 
-	if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errCreateDB}, mysql.ExecOptions{Binlog: binlog, Flush: ptr.To(false)}); err != nil {
+	if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errCreateDB}, mysql.ExecOptions{Flush: ptr.To(false)}); err != nil {
 		return managed.ExternalCreation{}, err
 	}
 
@@ -169,10 +168,9 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error {
 		return errors.New(errNotDatabase)
 	}
 
-	binlog := cr.Spec.ForProvider.BinLog
 	query := "DROP DATABASE IF EXISTS " + mysql.QuoteIdentifier(meta.GetExternalName(cr))
 
-	if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errDropDB}, mysql.ExecOptions{Binlog: binlog, Flush: ptr.To(false)}); err != nil {
+	if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errDropDB}, mysql.ExecOptions{Flush: ptr.To(false)}); err != nil {
 		return err
 	}
 
diff --git a/pkg/controller/mysql/grant/reconciler.go b/pkg/controller/mysql/grant/reconciler.go
index a4d071b..5d5b35c 100644
--- a/pkg/controller/mysql/grant/reconciler.go
+++ b/pkg/controller/mysql/grant/reconciler.go
@@ -87,7 +87,7 @@ func Setup(mgr ctrl.Manager, o xpcontroller.Options) error {
 type connector struct {
 	kube  client.Client
 	usage resource.Tracker
-	newDB func(creds map[string][]byte, tls *string) xsql.DB
+	newDB func(creds map[string][]byte, tls *string, binlog *bool) xsql.DB
 }
 
 func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.ExternalClient, error) {
@@ -121,7 +121,7 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E
 	}
 
 	return &external{
-		db:   c.newDB(s.Data, pc.Spec.TLS),
+		db:   c.newDB(s.Data, pc.Spec.TLS, cr.Spec.ForProvider.BinLog),
 		kube: c.kube,
 	}, nil
 }
@@ -259,10 +259,9 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 	table := defaultIdentifier(cr.Spec.ForProvider.Table)
 
 	privileges, grantOption := getPrivilegesString(cr.Spec.ForProvider.Privileges.ToStringSlice())
-	binlog := cr.Spec.ForProvider.BinLog
 	query := createGrantQuery(privileges, dbname, username, table, grantOption)
 
-	if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errCreateGrant}, mysql.ExecOptions{Binlog: binlog}); err != nil {
+	if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errCreateGrant}, mysql.ExecOptions{}); err != nil {
 		return managed.ExternalCreation{}, err
 	}
 	return managed.ExternalCreation{}, nil
@@ -277,7 +276,6 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 	username := *cr.Spec.ForProvider.User
 	dbname := defaultIdentifier(cr.Spec.ForProvider.Database)
 	table := defaultIdentifier(cr.Spec.ForProvider.Table)
-	binlog := cr.Spec.ForProvider.BinLog
 
 	observed := cr.Status.AtProvider.Privileges
 	desired := cr.Spec.ForProvider.Privileges.ToStringSlice()
@@ -287,11 +285,10 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 		sort.Strings(toRevoke)
 		privileges, grantOption := getPrivilegesString(toRevoke)
 		query := createRevokeQuery(privileges, dbname, username, table, grantOption)
-		if err := mysql.ExecWithBinlogAndFlush(ctx, c.db,
+		if err := mysql.ExecWithFlush(ctx, c.db,
 			mysql.ExecQuery{
 				Query: query, ErrorValue: errRevokeGrant,
-			}, mysql.ExecOptions{
-				Binlog: binlog}); err != nil {
+			}, mysql.ExecOptions{}); err != nil {
 			return managed.ExternalUpdate{}, err
 		}
 	}
@@ -300,11 +297,10 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 		sort.Strings(toGrant)
 		privileges, grantOption := getPrivilegesString(toGrant)
 		query := createGrantQuery(privileges, dbname, username, table, grantOption)
-		if err := mysql.ExecWithBinlogAndFlush(ctx, c.db,
+		if err := mysql.ExecWithFlush(ctx, c.db,
 			mysql.ExecQuery{
 				Query: query, ErrorValue: errCreateGrant,
-			}, mysql.ExecOptions{
-				Binlog: binlog}); err != nil {
+			}, mysql.ExecOptions{}); err != nil {
 			return managed.ExternalUpdate{}, err
 		}
 	}
@@ -369,12 +365,11 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error {
 	username := *cr.Spec.ForProvider.User
 	dbname := defaultIdentifier(cr.Spec.ForProvider.Database)
 	table := defaultIdentifier(cr.Spec.ForProvider.Table)
-	binlog := cr.Spec.ForProvider.BinLog
 
 	privileges, grantOption := getPrivilegesString(cr.Spec.ForProvider.Privileges.ToStringSlice())
 	query := createRevokeQuery(privileges, dbname, username, table, grantOption)
 
-	if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errRevokeGrant}, mysql.ExecOptions{Binlog: binlog}); err != nil {
+	if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errRevokeGrant}, mysql.ExecOptions{}); err != nil {
 		var myErr *mysqldriver.MySQLError
 		if errors.As(err, &myErr) && myErr.Number == errCodeNoSuchGrant {
 			// MySQL automatically deletes related grants if the user has been deleted
diff --git a/pkg/controller/mysql/user/reconciler.go b/pkg/controller/mysql/user/reconciler.go
index b446e2b..92e7e0f 100644
--- a/pkg/controller/mysql/user/reconciler.go
+++ b/pkg/controller/mysql/user/reconciler.go
@@ -82,7 +82,7 @@ func Setup(mgr ctrl.Manager, o xpcontroller.Options) error {
 type connector struct {
 	kube  client.Client
 	usage resource.Tracker
-	newDB func(creds map[string][]byte, tls *string) xsql.DB
+	newDB func(creds map[string][]byte, tls *string, binlog *bool) xsql.DB
 }
 
 func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.ExternalClient, error) {
@@ -116,7 +116,7 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E
 	}
 
 	return &external{
-		db:   c.newDB(s.Data, pc.Spec.TLS),
+		db:   c.newDB(s.Data, pc.Spec.TLS, cr.Spec.ForProvider.BinLog),
 		kube: c.kube,
 	}, nil
 }
@@ -251,8 +251,7 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 	}
 
 	ro := resourceOptionsToClauses(cr.Spec.ForProvider.ResourceOptions)
-	binlog := cr.Spec.ForProvider.BinLog
-	if err := c.executeCreateUserQuery(ctx, username, host, ro, pw, binlog); err != nil {
+	if err := c.executeCreateUserQuery(ctx, username, host, ro, pw); err != nil {
 		return managed.ExternalCreation{}, err
 	}
 
@@ -265,7 +264,7 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 	}, nil
 }
 
-func (c *external) executeCreateUserQuery(ctx context.Context, username string, host string, resourceOptionsClauses []string, pw string, binlog *bool) error {
+func (c *external) executeCreateUserQuery(ctx context.Context, username string, host string, resourceOptionsClauses []string, pw string) error {
 	resourceOptions := ""
 	if len(resourceOptionsClauses) != 0 {
 		resourceOptions = fmt.Sprintf(" WITH %s", strings.Join(resourceOptionsClauses, " "))
@@ -279,7 +278,7 @@ func (c *external) executeCreateUserQuery(ctx context.Context, username string,
 		resourceOptions,
 	)
 
-	if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errCreateUser}, mysql.ExecOptions{Binlog: binlog}); err != nil {
+	if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errCreateUser}, mysql.ExecOptions{}); err != nil {
 		return err
 	}
 
@@ -303,14 +302,13 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 	if len(rochanged) > 0 {
 		resourceOptions := fmt.Sprintf("WITH %s", strings.Join(ro, " "))
 
-		binlog := cr.Spec.ForProvider.BinLog
 		query := fmt.Sprintf(
 			"ALTER USER %s@%s %s",
 			mysql.QuoteValue(username),
 			mysql.QuoteValue(host),
 			resourceOptions,
 		)
-		if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errUpdateUser}, mysql.ExecOptions{Binlog: binlog}); err != nil {
+		if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errUpdateUser}, mysql.ExecOptions{}); err != nil {
 			return managed.ExternalUpdate{}, err
 		}
 
@@ -336,9 +334,8 @@ func (c *external) UpdatePassword(ctx context.Context, cr *v1alpha1.User, userna
 	}
 
 	if pwchanged {
-		binlog := cr.Spec.ForProvider.BinLog
 		query := fmt.Sprintf("ALTER USER %s@%s IDENTIFIED BY %s", mysql.QuoteValue(username), mysql.QuoteValue(host), mysql.QuoteValue(pw))
-		if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errUpdateUser}, mysql.ExecOptions{Binlog: binlog}); err != nil {
+		if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errUpdateUser}, mysql.ExecOptions{}); err != nil {
 			return managed.ConnectionDetails{}, err
 		}
 
@@ -358,9 +355,8 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error {
 
 	username, host := mysql.SplitUserHost(meta.GetExternalName(cr))
 
-	binlog := cr.Spec.ForProvider.BinLog
 	query := fmt.Sprintf("DROP USER IF EXISTS %s@%s", mysql.QuoteValue(username), mysql.QuoteValue(host))
-	if err := mysql.ExecWithBinlogAndFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errDropUser}, mysql.ExecOptions{Binlog: binlog}); err != nil {
+	if err := mysql.ExecWithFlush(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errDropUser}, mysql.ExecOptions{}); err != nil {
 		return err
 	}
 


Test patch:
diff --git a/pkg/clients/mysql/mysql_test.go b/pkg/clients/mysql/mysql_test.go
index 60f054d..2a9cc88 100644
--- a/pkg/clients/mysql/mysql_test.go
+++ b/pkg/clients/mysql/mysql_test.go
@@ -2,6 +2,7 @@ package mysql
 
 import (
 	"fmt"
+	"strconv"
 	"testing"
 )
 
@@ -11,13 +12,15 @@ func TestDSNURLEscaping(t *testing.T) {
 	user := "username"
 	rawPass := "password^"
 	tls := "true"
-	dsn := DSN(user, rawPass, endpoint, port, tls)
-	if dsn != fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s",
+	binlog := false
+	dsn := DSN(user, rawPass, endpoint, port, tls, binlog)
+	if dsn != fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s&sql_log_bin=%s",
 		user,
 		rawPass,
 		endpoint,
 		port,
-		tls) {
+		tls,
+		strconv.FormatBool(binlog)) {
 		t.Errorf("DSN string did not match expected output with URL encoded")
 	}
 }
diff --git a/pkg/controller/mysql/database/reconciler_test.go b/pkg/controller/mysql/database/reconciler_test.go
index 6ae33c7..e1a6a59 100644
--- a/pkg/controller/mysql/database/reconciler_test.go
+++ b/pkg/controller/mysql/database/reconciler_test.go
@@ -64,7 +64,7 @@ func TestConnect(t *testing.T) {
 	type fields struct {
 		kube  client.Client
 		usage resource.Tracker
-		newDB func(creds map[string][]byte, tls *string) xsql.DB
+		newDB func(creds map[string][]byte, tls *string, binlog *bool) xsql.DB
 	}
 
 	type args struct {
diff --git a/pkg/controller/mysql/grant/reconciler_test.go b/pkg/controller/mysql/grant/reconciler_test.go
index 2eefa99..1f54281 100644
--- a/pkg/controller/mysql/grant/reconciler_test.go
+++ b/pkg/controller/mysql/grant/reconciler_test.go
@@ -74,7 +74,7 @@ func TestConnect(t *testing.T) {
 	type fields struct {
 		kube  client.Client
 		usage resource.Tracker
-		newDB func(creds map[string][]byte, tls *string) xsql.DB
+		newDB func(creds map[string][]byte, tls *string, binlog *bool) xsql.DB
 	}
 
 	type args struct {
diff --git a/pkg/controller/mysql/user/reconciler_test.go b/pkg/controller/mysql/user/reconciler_test.go
index 17858cc..855fe45 100644
--- a/pkg/controller/mysql/user/reconciler_test.go
+++ b/pkg/controller/mysql/user/reconciler_test.go
@@ -72,7 +72,7 @@ func TestConnect(t *testing.T) {
 	type fields struct {
 		kube  client.Client
 		usage resource.Tracker
-		newDB func(creds map[string][]byte, tls *string) xsql.DB
+		newDB func(creds map[string][]byte, tls *string, binlog *bool) xsql.DB
 	}
 
 	type args struct {