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.

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

Enable native authentication in MySQL #6628

Merged

Conversation

Mrod1598
Copy link
Contributor

@Mrod1598 Mrod1598 commented Dec 8, 2021

Description:
Add support for native authentication in mysql.

@Mrod1598 Mrod1598 requested a review from djaglowski as a code owner December 8, 2021 04:31
@Mrod1598 Mrod1598 requested a review from a team December 8, 2021 04:31
Net: conf.Transport,
Addr: conf.Endpoint,
DBName: conf.Database,
AllowNativePasswords: true,
Copy link
Member

Choose a reason for hiding this comment

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

It seems wrong to just add this without giving users the chance to customize it. Besides, the default value for this seems to indeed be true: what would this change bring?

https://pkg.go.dev/github.com/go-sql-driver/mysql#Config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that the default was incorrect in testing as it was building the DSN by setting this parameter to false. I'll add it as a config option but default to true.

@Mrod1598 Mrod1598 requested a review from jpkrohling December 8, 2021 13:48
@@ -19,6 +19,7 @@ The following settings are optional:
- `endpoint`: (default = `localhost:3306`)
- `username`: (default = `root`)
- `password`: The password to the username.
- `AllowNativePasswords`: (default = `true`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `AllowNativePasswords`: (default = `true`)
- `allow_native_passwords`: (default = `true`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@@ -24,5 +24,6 @@ type Config struct {
Username string `mapstructure:"username,omitempty"`
Password string `mapstructure:"password,omitempty"`
Database string `mapstructure:"database,omitempty"`
AllowNativePasswords bool `mapstructure:"database,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
AllowNativePasswords bool `mapstructure:"database,omitempty"`
AllowNativePasswords bool `mapstructure:"allow_native_passwords,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@Mrod1598 Mrod1598 requested a review from jpkrohling December 8, 2021 14:35
@jpkrohling jpkrohling merged commit a4ff3f9 into open-telemetry:main Dec 8, 2021
@djaglowski djaglowski deleted the Support-native-authentication branch December 8, 2021 14:58
povilasv referenced this pull request in coralogix/opentelemetry-collector-contrib Dec 19, 2022
Bit of cleanup left over from #6322.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants