Add support for MySQL Connection Attributes by andygrunwald · Pull Request #1241 · go-sql-driver/mysql

andygrunwald · 2021-07-31T19:28:49Z

Description

This Pull Request adds support for sending connection attributes, which are used for identifying individual connections in MySQL.
See MySQL docs: Performance Schema Connection Attribute Tables.

This feature is supported since MySQL v 5.6.6.

The main work of this pull request was done by @Vasfed in #737.

This Pull Request reintegrates #737 with the current master and gets all unit tests running.

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
-> Not applicable
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file
-> Skipped, because my work was minimal

…chema in readme

* master: (93 commits) return unsigned in database type name when necessary (#1238) add an invalid DSN test case (#1235) refactoring (*textRows).readRow in a more clear way (#1230) use utf8mb4 instead of utf8 in TestCharset (#1228) improve readability follows go-staticcheck (#1227) support Is comparison on MySQLError (#1210) Wording correction in README (#1218) noCopy implements sync.Locker (#1216) Fix readme: MaxIdle is same or less than MaxOpen (#1215) Drop support of Go 1.12 (#1211) Release v1.6.0 (#1197) add Go 1.16 to the build matrix (#1198) Implement driver.Validator interface (#1174) handling decoding pem error (#1192) stop rounding times (#1172) improve GitHub Actions workflows (#1190) Move tests from Travis to Actions (#1183) Fix go vet error (#1173) README: Make usage code more friendly (#1170) Fix a broken link to cleartext client side plugin (#1165) ...

andygrunwald · 2021-07-31T19:34:36Z

I added a small example to showcase / use this feature: https://github.com/andygrunwald/your-connection-deserves-a-name/blob/mysql-go/mysql/go/main.go

This code is using my fork, and I can confirm that this feature is working as expected.

andygrunwald · 2021-10-17T15:06:05Z

@methane @shogo82148 Just checking if there is any update on this or if you found time to have a look at this pull request?

packets.go

…m/go-sql-driver/mysql"

This behaviour is very similar to the native C# MySQL driver. See https://github.com/mysql/mysql-connector-net/blob/502d718bed8ca9cf81a3a0397574f24ec41b25ba/MySQL.Data/src/X/Sessions/XInternalSession.cs#L450-L469

andygrunwald · 2021-12-21T13:17:15Z

I found out that the test TestRawBytesAreNotModified doesn't seem to be stable / a bit flaky.
In the test run 1606512033 it failed:

[...]
=== RUN   TestRawBytesAreNotModified
    driver_test.go:3075: context canceled
--- FAIL: TestRawBytesAreNotModified (0.31s)
[...]

Re-running the workflow (without any additional changes) succeeded.

If you wish that I create a dedicated ticket for this, please let me know.

packets.go

…e it is always set

Andy Co-Authored the PR for MySQL Connection Attributes, based on the work from Vasily Fedoseyev

* master: Go 1.17 and MariaDB 10.6 are released (#1253) Fix sha256_password auth plugin on unix transport (#1246)

andygrunwald · 2021-12-31T10:20:11Z

@dolmen @methane Thanks for your reviews so far. I incorporated your feedback and re-integrated this branch with master.
I kindly ask for a re-review. Let me know if there is anything else

driver_test.go

methane · 2022-01-01T04:48:41Z

packets.go


 	// To specify a db name
-	if n := len(mc.cfg.DBName); n > 0 {
+	if n := len(mc.cfg.DBName); mc.flags&clientConnectWithDB != 0 && n > 0 {


revert this.

methane · 2022-01-01T04:54:38Z

packets.go

 	pktLen := 4 + 4 + 1 + 23 + len(mc.cfg.User) + 1 + len(authRespLEI) + len(authResp) + 21 + 1
+	if clientFlags&clientSecureConn == 0 || clientFlags&clientPluginAuthLenEncClientData == 0 {
+		pktLen++
+	}


Why is this if block needed?

@andygrunwald – any context on this one? Tests pass correctly without this block. I'll dig in deeper if you don't remember why this was needed.

methane · 2022-01-01T04:55:02Z

packets.go

+	} else if clientFlags&clientSecureConn != 0 {
+		data[pos] = uint8(len(authResp))
+		pos++
+	}


Why is this needed?

@andygrunwald – do you remember any context on this one? I tried removing this block and confirmed many tests fail. I'll dig in deeper tomorrow, but wanted to start off seeing if you remembered this code.

fulghum · 2022-06-21T20:41:36Z

@andygrunwald – Any updates on support for connection attributes? We'd love to see this feature land and would be happy to help out if needed.

I've reintegrated master with this branch locally – no conflicts and I can confirm all tests are still passing.

@methane and @dolmen – any advice for moving this forward? I'm happy to take over in another PR if needed. Seems like this is really close and would be a helpful feature to land.

methane · 2022-06-22T01:17:45Z

This pull request contains many changes unrelating to the feature requested.
That made both of author and me tired.
Please make PR minimum next time. But I can't say when I can review and merge it.

andygrunwald · 2022-06-23T07:24:15Z

@fulghum @methane Your comments are fair and I should have done a better job here.
This PR is not (fully) my work. I grabbed the work from @Vasfed in #737 and try to adjust it to the latest main.

It might be a good thing to re-do this again and compare the changes here to the MySQL spec and see what is really needed.

Right now, I am not able to do this, due to limited time available. Just a heads up to set expectations. If someone is reading this and eager to jump in it: Go for it.
Otherwise, I will take this on once I have a bit of spare time.

Summary: In order to be able track Go client usage we enable sending connection attributes as specified in https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_connection_phase_packets_protocol_handshake_response.html (CLIENT_CONNECT_ATTRS). This commit is based on PR go-sql-driver#1241 which is based on PR go-sql-driver#737 (which are still open as of 11.04.2023) Test Plan: manual integration test will be added to the main repo Reviewers: amakarovych-ua, okramarenko-ua Reviewed By: okramarenko-ua Subscribers: engineering-list Differential Revision: https://grizzly.internal.memcompute.com/D61981

Vasfed and others added 10 commits June 18, 2018 17:52

Support for sending connection attributes

71151ad

Rename connectionAttributes to connectAttrs and mention performance_s…

9057689

…chema in readme

Fix formatting

71a1948

Fix excessive null-termination for auth data in handshake

7038d92

Use performance_schema.session_connect_attrs in test

151e4dc

Skip connectAttrs test if performance_schema is disabled

26b97ba

Fix ./packets.go:308: undefined: insecureAuth

e8f4d0d

Fix unit test TestDSNParser for connection attributes

aa0e7a0

Make SQL keywords uppercase

2f9b253

andygrunwald mentioned this pull request Jul 31, 2021

Support for sending connection attributes #737

Closed

andygrunwald mentioned this pull request Aug 13, 2021

Go code with MySQL Custom Attributes based on a fork andygrunwald/your-connection-deserves-a-name#1

Closed

dolmen suggested changes Dec 15, 2021

View reviewed changes

packets.go Outdated Show resolved Hide resolved

packets.go Outdated Show resolved Hide resolved

andygrunwald added 4 commits December 17, 2021 18:29

Changed "_client_name" attribute from "go-mysql-driver" to "github.co…

3ab2571

…m/go-sql-driver/mysql"

Allow overrwriting connection attribute "_client_name"

b5117fc

Error when connection attributes that start with "_" are used.

9042bbd

This behaviour is very similar to the native C# MySQL driver. See https://github.com/mysql/mysql-connector-net/blob/502d718bed8ca9cf81a3a0397574f24ec41b25ba/MySQL.Data/src/X/Sessions/XInternalSession.cs#L450-L469

Set additional (internal) connection attributes: _os, _platform, _pid

7c87a56

methane reviewed Dec 23, 2021

View reviewed changes

packets.go Outdated Show resolved Hide resolved

Removed check if clientSecureConn is set inside clientFlags, becaus…

1058830

…e it is always set

andygrunwald requested a review from methane December 31, 2021 09:29

andygrunwald added 2 commits December 31, 2021 10:36

Add Andy Grunwald to AUTHORS file

00a09d8

Andy Co-Authored the PR for MySQL Connection Attributes, based on the work from Vasily Fedoseyev

Merge branch 'master' into connection-attributes

f6f3d84

* master: Go 1.17 and MariaDB 10.6 are released (#1253) Fix sha256_password auth plugin on unix transport (#1246)

methane reviewed Jan 1, 2022

View reviewed changes

driver_test.go Show resolved Hide resolved

methane reviewed Jan 1, 2022

View reviewed changes

driver_test.go Show resolved Hide resolved

methane reviewed Jan 1, 2022

View reviewed changes

Close rows after execution

406e2a1

Daemonxiao mentioned this pull request Jan 31, 2023

Add connection attributes #1389

Merged

5 tasks

methane closed this May 5, 2023

Conversation

andygrunwald commented Jul 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

andygrunwald commented Jul 31, 2021

Uh oh!

andygrunwald commented Oct 17, 2021

Uh oh!

Uh oh!

Uh oh!

andygrunwald commented Dec 21, 2021

Uh oh!

Uh oh!

andygrunwald commented Dec 31, 2021

Uh oh!

Uh oh!

Uh oh!

methane Jan 1, 2022

Choose a reason for hiding this comment

Uh oh!

methane Jan 1, 2022

Choose a reason for hiding this comment

Uh oh!

fulghum Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

methane Jan 1, 2022

Choose a reason for hiding this comment

Uh oh!

fulghum Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

fulghum commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane commented Jun 22, 2022

Uh oh!

andygrunwald commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andygrunwald commented Jul 31, 2021 •

edited

Loading

fulghum commented Jun 21, 2022 •

edited

Loading