mysql: Refactor out usage of servenv #14732

dbussink · 2023-12-08T14:47:21Z

The servenv package is something that is use for server environments like vtgate, vttablet etc. But the go/mysql package should really be independent code for things like the MySQL protocol bits.

This change refactors things so that go/mysql doesn't depend anymore on servenv. This makes it easier to be used as a library for example.

The remaining bits here are in collations, which is something to tackle separately as it's very invasive.

Related Issue(s)

Part of #14717

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

vitess-bot · 2023-12-08T14:47:24Z

dbussink · 2023-12-08T14:47:57Z

go/cmd/vtgate/cli/plugin_auth_clientcert.go

-	vtgate.RegisterPluginInitializer(func() { mysql.InitAuthServerClientCert() })
+	servenv.OnParseFor("vtgate", func(fs *pflag.FlagSet) {
+		fs.StringVar(&clientcertAuthMethod, "mysql_clientcert_auth_method", string(mysql.MysqlClearPassword), "client-side authentication method to use. Supported values: mysql_clear_password, dialog.")
+	})


This one and others still have the same flags, but now moved into vtgate which I think is a better place anyway to have them set up.

dbussink · 2023-12-08T14:48:51Z

go/mysql/auth_server_static.go

-		fs.StringVar(&mysqlAuthServerStaticFile, "mysql_auth_server_static_file", "", "JSON File to read the users/passwords from.")
-		fs.StringVar(&mysqlAuthServerStaticString, "mysql_auth_server_static_string", "", "JSON representation of the users/passwords config.")
-		fs.DurationVar(&mysqlAuthServerStaticReloadInterval, "mysql_auth_static_reload_interval", 0, "Ticker to reload credentials")
-		fs.DurationVar(&mysqlServerFlushDelay, "mysql_server_flush_delay", mysqlServerFlushDelay, "Delay after which buffered response will be flushed to the client.")


This flag was in a very wrong place. It was in the auth plugin, but it's a MySQL protocol config flag! It was moved to the vtgate mysql plugin server instead which seems much more appropriate.

dbussink · 2023-12-08T14:49:40Z

go/mysql/server_flaky_test.go

@@ -263,6 +263,8 @@ func getHostPort(t *testing.T, a net.Addr) (string, int) {
 	return host, port
 }

+const mysqlVersion = "8.0.30-Vitess"


Only used for testing here, so better to just copy a static value than depending on the whole servenv.

there are a few places i saw where you missed using this

@ajm188 I'll fix up the ones I missed, but it can't be used in all cases, since this is in the test file only. That's deliberate so it doesn't get used accidentally.

dbussink · 2023-12-08T14:50:09Z

go/vt/vtgate/plugin_mysql_server.go

@@ -97,6 +99,7 @@ func registerPluginFlags(fs *pflag.FlagSet) {
 	fs.DurationVar(&mysqlQueryTimeout, "mysql_server_query_timeout", mysqlQueryTimeout, "mysql query timeout")
 	fs.BoolVar(&mysqlConnBufferPooling, "mysql-server-pool-conn-read-buffers", mysqlConnBufferPooling, "If set, the server will pool incoming connection read buffers")
 	fs.DurationVar(&mysqlKeepAlivePeriod, "mysql-server-keepalive-period", mysqlKeepAlivePeriod, "TCP period between keep-alives")
+	fs.DurationVar(&mysqlServerFlushDelay, "mysql_server_flush_delay", mysqlServerFlushDelay, "Delay after which buffered response will be flushed to the client.")


It has underscores since this moves the flag to its appropriate location.

we can also define a new flag and fs.MarkDeprecated this one to support both

I'd like to keep it this way. Dunno if we have a generic plan to do this? Since I wouldn't want to do it just for a one off flag then which only happens due to a refactor that for most end users is not really relevant anyway.

Yeah let's not do a one-off flag rename/deprecation. We should tackle all of them together.

dbussink · 2023-12-08T14:50:50Z

go/mysql/server_flaky_test.go

@@ -1470,12 +1472,10 @@ func TestParseConnAttrs(t *testing.T) {
 }

 func TestServerFlush(t *testing.T) {
-	defer func(saved time.Duration) { mysqlServerFlushDelay = saved }(mysqlServerFlushDelay)
-	mysqlServerFlushDelay = 10 * time.Millisecond


This removes global state modification which is a great win imho.

The servenv package is something that is use for server environments like vtgate, vttablet etc. But the go/mysql package should really be independent code for things like the MySQL protocol bits. This change refactors things so that go/mysql doesn't depend anymore on servenv. This makes it easier to be used as a library for example. The remaining bits here are in collations, which is something to tackle separately as it's very invasive. Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink · 2023-12-08T15:23:11Z

go/cmd/vtgate/cli/plugin_auth_ldap.go

-	vtgate.RegisterPluginInitializer(func() { ldapauthserver.Init() })
+	Main.Flags().StringVar(&ldapAuthConfigFile, "mysql_ldap_auth_config_file", "", "JSON File from which to read LDAP server config.")
+	Main.Flags().StringVar(&ldapAuthConfigString, "mysql_ldap_auth_config_string", "", "JSON representation of LDAP server config.")
+	Main.Flags().StringVar(&ldapAuthMethod, "mysql_ldap_auth_method", string(mysql.MysqlClearPassword), "client-side authentication method to use. Supported values: mysql_clear_password, dialog.")


@ajm188 Had to use Main.Flags() here since the on parse callbacks don't work as it's too late since cli.go inits first, but dunno if this is the right fix then? Or if there's another way?

yeah, that's what we're currently doing? https://github.com/vitessio/vitess/blob/main/go/cmd/vtgate/cli/cli.go#L187-L189 i'm not sure if that answers your question

ajm188

looking good! few small things but lgtm after those

ajm188 · 2023-12-08T15:41:28Z

go/cmd/vtgate/cli/plugin_auth_ldap.go

-	vtgate.RegisterPluginInitializer(func() { ldapauthserver.Init() })
+	Main.Flags().StringVar(&ldapAuthConfigFile, "mysql_ldap_auth_config_file", "", "JSON File from which to read LDAP server config.")
+	Main.Flags().StringVar(&ldapAuthConfigString, "mysql_ldap_auth_config_string", "", "JSON representation of LDAP server config.")
+	Main.Flags().StringVar(&ldapAuthMethod, "mysql_ldap_auth_method", string(mysql.MysqlClearPassword), "client-side authentication method to use. Supported values: mysql_clear_password, dialog.")


yeah, that's what we're currently doing? https://github.com/vitessio/vitess/blob/main/go/cmd/vtgate/cli/cli.go#L187-L189 i'm not sure if that answers your question

go/mysql/conn.go

go/mysql/fakesqldb/server.go

ajm188 · 2023-12-08T15:43:53Z

go/mysql/mysql_fuzzer.go

@@ -327,7 +327,7 @@ func FuzzTLSServer(data []byte) int {
 		Password: "password1",
 	}}
 	defer authServer.close()
-	l, err := NewListener("tcp", "127.0.0.1:", authServer, th, 0, 0, false)
+	l, err := NewListener("tcp", "127.0.0.1:", authServer, th, 0, 0, false, 0, "8.0.30-Vitess")


Same here, this one can't use it as it's not in the test file(s).

ajm188 · 2023-12-08T15:44:15Z

go/mysql/server_flaky_test.go

@@ -263,6 +263,8 @@ func getHostPort(t *testing.T, a net.Addr) (string, int) {
 	return host, port
 }

+const mysqlVersion = "8.0.30-Vitess"


there are a few places i saw where you missed using this

ajm188 · 2023-12-08T15:45:10Z

go/vt/vtgate/plugin_mysql_server.go

@@ -97,6 +99,7 @@ func registerPluginFlags(fs *pflag.FlagSet) {
 	fs.DurationVar(&mysqlQueryTimeout, "mysql_server_query_timeout", mysqlQueryTimeout, "mysql query timeout")
 	fs.BoolVar(&mysqlConnBufferPooling, "mysql-server-pool-conn-read-buffers", mysqlConnBufferPooling, "If set, the server will pool incoming connection read buffers")
 	fs.DurationVar(&mysqlKeepAlivePeriod, "mysql-server-keepalive-period", mysqlKeepAlivePeriod, "TCP period between keep-alives")
+	fs.DurationVar(&mysqlServerFlushDelay, "mysql_server_flush_delay", mysqlServerFlushDelay, "Delay after which buffered response will be flushed to the client.")


we can also define a new flag and fs.MarkDeprecated this one to support both

ajm188 · 2023-12-08T15:45:18Z

go/vt/vtgate/plugin_mysql_server_test.go

@@ -348,7 +348,7 @@ func TestGracefulShutdown(t *testing.T) {

 	vh := newVtgateHandler(&VTGate{executor: executor, timings: timings, rowsReturned: rowsReturned, rowsAffected: rowsAffected})
 	th := &testHandler{}
-	listener, err := mysql.NewListener("tcp", "127.0.0.1:", mysql.NewAuthServerNone(), th, 0, 0, false, false, 0)
+	listener, err := mysql.NewListener("tcp", "127.0.0.1:", mysql.NewAuthServerNone(), th, 0, 0, false, false, 0, 0, "8.0.30-Vitess")


missed one here

It's not an exposed constant, so it can't be used here. I can turn these two into their own constant? I thought about it and didn't know if it was worth it for 2 or not then.

yeah tests-only seems fine this way

ajm188 · 2023-12-08T15:45:23Z

go/vt/vtgate/plugin_mysql_server_test.go

@@ -378,7 +378,7 @@ func TestGracefulShutdownWithTransaction(t *testing.T) {

 	vh := newVtgateHandler(&VTGate{executor: executor, timings: timings, rowsReturned: rowsReturned, rowsAffected: rowsAffected})
 	th := &testHandler{}
-	listener, err := mysql.NewListener("tcp", "127.0.0.1:", mysql.NewAuthServerNone(), th, 0, 0, false, false, 0)
+	listener, err := mysql.NewListener("tcp", "127.0.0.1:", mysql.NewAuthServerNone(), th, 0, 0, false, false, 0, 0, "8.0.30-Vitess")


vmg

Looks good overall. Not looking forward to fixing collations.

vmg · 2023-12-08T16:10:00Z

go/mysql/vault/auth_server_vault.go

@@ -80,7 +51,7 @@ type AuthServerVault struct {
 }

 // InitAuthServerVault - entrypoint for initialization of Vault AuthServer implementation
-func InitAuthServerVault() {
+func InitAuthServerVault(vaultAddr string, vaultTimeout time.Duration, vaultCACert, vaultPath string, vaultCacheTTL time.Duration, vaultTokenFile, vaultRoleID, vaultRoleSecretIDFile, vaultRoleMountPoint string) {


These are quite a few args. Maybe enough that we should construct a struct out of them.

I thought about that, but we already use the whole list one call site down as well. So I don't consider it to really be anything significantly worse than before.

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink added Type: Internal Cleanup Component: General Changes throughout the code base labels Dec 8, 2023

dbussink requested review from harshit-gangal, systay, frouioui, GuptaManan100, mattlord, ajm188 and deepthi as code owners December 8, 2023 14:47

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Dec 8, 2023

github-actions bot added this to the v19.0.0 milestone Dec 8, 2023

dbussink commented Dec 8, 2023

View reviewed changes

dbussink removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels Dec 8, 2023

dbussink force-pushed the dbussink/refactor-mysql-servenv-usage branch from a84ee7a to 1e8b3b3 Compare December 8, 2023 15:20

dbussink commented Dec 8, 2023

View reviewed changes

ajm188 approved these changes Dec 8, 2023

View reviewed changes

vmg approved these changes Dec 8, 2023

View reviewed changes

dbussink merged commit ab1ba2e into vitessio:main Dec 8, 2023
116 checks passed

dbussink deleted the dbussink/refactor-mysql-servenv-usage branch December 8, 2023 20:50

ejortegau pushed a commit to slackhq/vitess that referenced this pull request Dec 13, 2023

mysql: Refactor out usage of servenv (vitessio#14732)

a2fe7ae

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink mentioned this pull request Jan 12, 2024

Refactor out more usage of servenv for mysql version #14938

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mysql: Refactor out usage of servenv #14732

mysql: Refactor out usage of servenv #14732

dbussink commented Dec 8, 2023

vitess-bot bot commented Dec 8, 2023

dbussink Dec 8, 2023

dbussink Dec 8, 2023

dbussink Dec 8, 2023

ajm188 Dec 8, 2023

dbussink Dec 8, 2023

dbussink Dec 8, 2023

ajm188 Dec 8, 2023

dbussink Dec 8, 2023

deepthi Dec 8, 2023

dbussink Dec 8, 2023

dbussink Dec 8, 2023

ajm188 Dec 8, 2023

ajm188 left a comment

ajm188 Dec 8, 2023

ajm188 Dec 8, 2023

dbussink Dec 8, 2023

ajm188 Dec 8, 2023

ajm188 Dec 8, 2023

ajm188 Dec 8, 2023

dbussink Dec 8, 2023

ajm188 Dec 8, 2023

ajm188 Dec 8, 2023

vmg left a comment

vmg Dec 8, 2023

dbussink Dec 8, 2023

mysql: Refactor out usage of servenv #14732

mysql: Refactor out usage of servenv #14732

Conversation

dbussink commented Dec 8, 2023

Related Issue(s)

Checklist

vitess-bot bot commented Dec 8, 2023

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment