Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mysql: pass TLS config directly to MySQL's config #3348

Merged
merged 2 commits into from
Dec 28, 2023
Merged

mysql: pass TLS config directly to MySQL's config #3348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00165ed
mysql: pass TLS config directly to MySQL's config
giautm Nov 25, 2023
30f5dc5
mysql/gcpmysql: fix ensure the dialer is unique
giautm Nov 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions mysql/awsmysql/awsmysql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
"database/sql/driver"
"fmt"
"net/url"
"sync/atomic"

"contrib.go.opencensus.io/integrations/ocsql"
"github.com/go-sql-driver/mysql"
Expand Down Expand Up @@ -112,20 +111,14 @@
c.sem <- struct{}{} // release
return nil, fmt.Errorf("connect RDS: %v", err)
}
// TODO(light): Avoid global registry once https://github.com/go-sql-driver/mysql/issues/771 is fixed.
tlsConfigName := fmt.Sprintf(
"gocloud.dev/mysql/awsmysql/%d",
atomic.AddUint32(&tlsConfigCounter, 1),
)
err = mysql.RegisterTLSConfig(tlsConfigName, &tls.Config{
RootCAs: certPool,
})
cfg, err := mysql.ParseDSN(c.dsn)

Check warning on line 114 in mysql/awsmysql/awsmysql.go

View check run for this annotation

Codecov / codecov/patch

mysql/awsmysql/awsmysql.go#L114 

Added line #L114 was not covered by tests
if err != nil {
c.sem <- struct{}{} // release
return nil, fmt.Errorf("connect RDS: register TLS: %v", err)
return nil, fmt.Errorf("connect RDS: parse DSN: %v", err)

Check warning on line 117 in mysql/awsmysql/awsmysql.go

View check run for this annotation

Codecov / codecov/patch

mysql/awsmysql/awsmysql.go#L117 

Added line #L117 was not covered by tests
}
cfg.TLS = &tls.Config{
RootCAs: certPool,

Check warning on line 120 in mysql/awsmysql/awsmysql.go

View check run for this annotation

Codecov / codecov/patch

mysql/awsmysql/awsmysql.go#L119-L120 

Added lines #L119 - L120 were not covered by tests
}
cfg, _ := mysql.ParseDSN(c.dsn)
cfg.TLSConfig = tlsConfigName
c.dsn = cfg.FormatDSN()
close(c.ready)
// Don't release sem: make it block forever, so this case won't be run again.
Expand All @@ -141,8 +134,6 @@
return ocsql.Wrap(mysql.MySQLDriver{}, c.traceOpts...)
}

var tlsConfigCounter uint32

// A CertPoolProvider obtains a certificate pool that contains the RDS CA certificate.
type CertPoolProvider = rds.CertPoolProvider

Expand Down
22 changes: 1 addition & 21 deletions mysql/azuremysql/azuremysql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
"fmt"
"net/url"
"strings"
"sync"

"contrib.go.opencensus.io/integrations/ocsql"
"github.com/go-sql-driver/mysql"
Expand Down Expand Up @@ -106,26 +105,12 @@
c.sem <- struct{}{} // release
return nil, fmt.Errorf("connect Azure MySql: %v", err)
}

// TODO(light): Avoid global registry once https://github.com/go-sql-driver/mysql/issues/771 is fixed.
tlsConfigCounter.mu.Lock()
tlsConfigNum := tlsConfigCounter.n
tlsConfigCounter.n++
tlsConfigCounter.mu.Unlock()
tlsConfigName := fmt.Sprintf("gocloud.dev/mysql/azuremysql/%d", tlsConfigNum)
err = mysql.RegisterTLSConfig(tlsConfigName, &tls.Config{
RootCAs: certPool,
})
if err != nil {
c.sem <- struct{}{} // release
return nil, fmt.Errorf("connect Azure MySql: register TLS: %v", err)
}
cfg := &mysql.Config{
Net: "tcp",
Addr: c.addr,
User: c.user,
Passwd: c.password,
TLSConfig: tlsConfigName,
TLS: &tls.Config{RootCAs: certPool},

Check warning on line 113 in mysql/azuremysql/azuremysql.go

View check run for this annotation

Codecov / codecov/patch

mysql/azuremysql/azuremysql.go#L113 

Added line #L113 was not covered by tests
AllowCleartextPasswords: true,
AllowNativePasswords: true,
DBName: c.dbName,
Expand All @@ -145,11 +130,6 @@
return ocsql.Wrap(mysql.MySQLDriver{}, c.traceOpts...)
}

var tlsConfigCounter struct {
mu sync.Mutex
n int
}

// A CertPoolProvider obtains a certificate pool that contains the Azure CA certificate.
type CertPoolProvider = azuredb.CertPoolProvider

Expand Down
16 changes: 5 additions & 11 deletions mysql/gcpmysql/gcpmysql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"net/url"
"strings"
"sync"
"sync/atomic"

"contrib.go.opencensus.io/integrations/ocsql"
"github.com/GoogleCloudPlatform/cloudsql-proxy/proxy/proxy"
Expand Down Expand Up @@ -97,12 +98,8 @@
if uo.CertSource == nil {
return nil, fmt.Errorf("gcpmysql: URLOpener CertSource is nil")
}
// TODO(light): Avoid global registry once https://github.com/go-sql-driver/mysql/issues/771 is fixed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this TODO can be addressed while the issue referenced is still open?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this part, it really doesnt have another solution, and the package itself will not support to change this to a custom dialer function. So, I think the current implement is correct and no need to change in future. The package sql-cloud-proxy also used this way without comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help if you would explain what problem you are trying to solve. Does the current code not work? Or are you trying to simplify it?

Our testing for this part of the codebase is not good, and I am hesitant to make changes that I don't fully understand without any rationale, especially when there's a TODO in the code that hasn't been addressed.

Copy link
Contributor Author

@giautm giautm Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, hello again.

I was checked the source code of awsmysql and azuremysql and notice about TLSConfig's todo. I checked mysql driver on the upstream repository and it was supported to replace with TLS field

https://github.com/go-sql-driver/mysql/blob/master/dsn.go#L125-L142

for the gcpmysql, the todo comment wasn't correct, because this package use Dial function to make the connection, which no replacement in the feature.

This is another example that use the same approach for Dial:
https://github.com/GoogleCloudPlatform/cloud-sql-go-connector/blob/main/mysql/mysql/mysql.go#L40-L49

And the package gcpmysql also have a bug for generate driver name, it used same name for all connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help if you would explain what problem you are trying to solve. Does the current code not work? Or are you trying to simplify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify, and fix one bug.

dialerCounter.mu.Lock()
dialerNum := dialerCounter.n
dialerCounter.mu.Unlock()
Comment on lines -101 to -103
Copy link
Contributor Author

@giautm giautm Nov 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No increment, it always zero

dialerName := fmt.Sprintf("gocloud.dev/mysql/gcpmysql/%d", dialerNum)

dialerName := fmt.Sprintf("gocloud.dev/mysql/gcpmysql/%d",
atomic.AddUint32(&dialerCounter, 1))

Check warning on line 102 in mysql/gcpmysql/gcpmysql.go

View check run for this annotation

Codecov / codecov/patch

mysql/gcpmysql/gcpmysql.go#L101-L102 

Added lines #L101 - L102 were not covered by tests
cfg, err := configFromURL(u, dialerName)
if err != nil {
return nil, fmt.Errorf("gcpmysql: open config %v", err)
Expand All @@ -112,7 +109,7 @@
Port: 3307,
Certs: uo.CertSource,
}
mysql.RegisterDial(dialerName, client.Dial)
mysql.RegisterDialContext(dialerName, client.DialContext)

Check warning on line 112 in mysql/gcpmysql/gcpmysql.go

View check run for this annotation

Codecov / codecov/patch

mysql/gcpmysql/gcpmysql.go#L112 

Added line #L112 was not covered by tests

db := sql.OpenDB(connector{cfg.FormatDSN(), uo.TraceOpts})
return db, nil
Expand Down Expand Up @@ -161,10 +158,7 @@
return parts[0] + ":" + parts[1] + ":" + parts[2], parts[3], nil
}

var dialerCounter struct {
mu sync.Mutex
n int
}
var dialerCounter uint32

type connector struct {
dsn string
Expand Down
Loading