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

MySQL database plugin: allow no password; default Host, User, and Password to undef #970

Merged
merged 5 commits into from
May 31, 2021

Conversation

ph448
Copy link
Contributor

@ph448 ph448 commented May 29, 2021

Pull Request (PR) description

This started off as a change to allow for not specifying a password, as my setup uses socket authentication, then I noticed #724 and thought to include the changes to fix that as well.
Setting the default host to 'localhost' and not requiring a username or password is better in line with mysqlclient defaults, which the collectd mysql plugin ultimately uses to interact with the server. Also, the plugin supports specifying a socket, for which no username or password is required, so having them optional allows for configuring the plugin in correct ways that were not possible before the change.

This Pull Request (PR) fixes the following issues

Fixes #724

ph448 added 4 commits May 8, 2021 06:55
Both MySQL and MariaDB now support authentication types that do not
require a password to be set, eg. socket authentication, which is
now the default at least on Debian-based distributions on new
installations.

Currently the module hardcodes 'UNSET' for the password, this
trivial change makes supplying the passsword optional.
Using 'localhost' as the default host to connect to better matches
what the clients for these databases default to.
The collectd mysql plugin documentation has examples where a socket
is configured for accessing the database, similar to the default
behaviour of mysqlclient. Along with the previous change to set the
default hostname to localhost, this matches up better with what
users of these tools expect.
adjust to reflect changes introduced by previous commits
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

That looks reasonable. I guess empty strings are not valid for these parameters? If so, String[1] should be preferred.

@ph448
Copy link
Contributor Author

ph448 commented May 29, 2021

@smortex empty strings are probably not valid for any of the string parameters in manifests/plugin/mysql/database.pp, but none of them have a minimum length requirement specified, making it look like a convention rather than an omission. that said, I'm happy to make the change, shall I change all of them?

@smortex
Copy link
Member

smortex commented May 30, 2021

This module has a lot of classes and a rather long history, it has some classes without data types at all, some with "relaxed" data types and some with "strict" data types. Tackling all this at once is not something appealing and fun, and I think that improving bits here and there is a better way to go, progressively improving the module validation.

If MySQL allow empty password the module must allow this (no idea, I am not a user of MySQL), for the two other parameters I would go for a minimum length of 1.

Thanks!

use Stdlib::Host for $hostname and make $username and $password
require a non-empty string
@ph448
Copy link
Contributor Author

ph448 commented May 30, 2021

OK, I've added the length restriction to both $username and $password as it doesn't make sense to set either to an empty string. Seeing that $port was already using a puppetlabs-stdlib type I've chosen to use Stdlib::Host for $hostname.

@bastelfreak bastelfreak added the enhancement New feature or request label May 30, 2021
@kenyon kenyon changed the title Mysql no password MySQL database plugin: allow no password; default Host, User, and Password to undef May 31, 2021
@kenyon kenyon merged commit fb69b9e into voxpupuli:master May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin::mysql::database - remove default 'UNSET' values
5 participants