Skip to content

Add support for custom tls.Configs #101

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

Merged
merged 4 commits into from
Jul 1, 2013
Merged

Add support for custom tls.Configs #101

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
481dc97
Add support for customizing tls.Config
lukescott Jun 19, 2013
4974720
Change SetTLSConfig to exported RegisterTLSConfig and add documentation
lukescott Jun 22, 2013
b18f20a
Revise documentation for RegisterTLSConfig
lukescott Jun 29, 2013
17ee918
Add test for RegisterTLSConfig
lukescott Jul 1, 2013
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
15 changes: 15 additions & 0 deletions tlsconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package mysql
Copy link
Member

Choose a reason for hiding this comment

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

The total number of lines in this file is really small. I don't think this is worth its own file. Please move it to a section in utils.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.


import (
"crypto/tls"
)

type TLSConfig interface {
Copy link
Member

Choose a reason for hiding this comment

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

What is this interface for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed, as you commented on later.

SetTLSConfig(key string, config *tls.Config)
}

var tlsConfigMap = make(map[string]*tls.Config)

func (d *mysqlDriver) SetTLSConfig(key string, config *tls.Config) {
Copy link
Member

Choose a reason for hiding this comment

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

Four things here:
1.) Don't make it a method on *mysqlDriver. To you this, you must import the mysql-package non-anonymously anyways. It makes no sense to indirect access to this over the sql package. Just make it a exported function.
2.) Please name it consistently with the infile functions, so RegisterTLSConfig here.
3.) A deregister function is missing.
4.) Please add a short documentation to the function. A short example how to use this (like https://github.com/go-sql-driver/mysql/blob/master/utils.go#L30 ) would be extra nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Will do.

tlsConfigMap[key] = config
}
3 changes: 3 additions & 0 deletions utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ func parseDSN(dsn string) (cfg *config, err error) {
cfg.tls = &tls.Config{}
} else if strings.ToLower(value) == "skip-verify" {
cfg.tls = &tls.Config{InsecureSkipVerify: true}
} else if tlsConfig, ok := tlsConfigMap[value]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

Please add TODO: Check for Boolean false here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you want this done in a different way?

Copy link
Member

Choose a reason for hiding this comment

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

My plan is to edit the readBool function / introduce another function which reports if the input was a valid bool representation (so also checks if the input was 0 / false)

cfg.tls = &tls.Config{}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this line is necessary (but I haven't tested 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.

How else would you be able to do "tls=customkey" in the DSN?

Copy link
Member

Choose a reason for hiding this comment

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

I meant L156, the "zeroing".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, got ya. Yeah, I'll change that as well.

*cfg.tls = *tlsConfig
}

default:
Expand Down