Fix puppet error when setting etc_root_password in mysql.

[ezheidtmann]

Puppet agent fails with duplicate definition errors if I enable etc_root_password.

This patch avoids the error by omitting the standard definition.

[apenney]

As this PR stands I think this is a bad idea. Mostly because I don't want to see any more defined() use as it causes parsing problems.

I have a better suggestion for this if you feel like tackling it - we introduce concat into the module and convert the two file{} resources:

• file { $config_file:
• file{ '/etc/my.cnf':

Into concat::fragments. That way it can happily drop in the additional [client] section fragment to /etc/my.cnf in the case of etc_root_password.

This will need a bunch of changes:

• Modulefile to add in concat.
• Changes in the spec tests (can be tested with rake spec).
• Changes in config.pp and possibly some other places.

If you're willing to make a stab at this, awesome! If not come back to me in the comments and we'll work on it together.

[hunner]

I'm trying to figure out what etc_root_password is even for, because conflicts like this shouldn't happen in the first place.

It looks like root_password, old_root_password, and etc_root_password are the three parameters involved with this.

It appears that old_root_password was so that the old password could be supplied so that a new root_password could be set, and root_password would also be stored in /root/.my.cnf so that the types and providers could function against the mysqld.

etc_root_password appears to add the root_password to /etc/my.cnf also, but why I don't know. Perhaps so anyone with access to /etc/my.cnf could connect to the mysqld without having their own ~/.my.cnf? In that case, why wasn't it just added to the config template itself?

The short-term fix would probably be to add the password fragment from templates/my.cnf.pass.erb to templates/my.cnf.erb because this is the template which is used for /etc/my.cnf.

Long term would be to audit and potentially redraft the authentication workflow of the module.

This looks like organically-grown madness and should be fixed by removing code rather than adding patches.

[ezheidtmann]

My gut feeling is the same as hunner's. I'm not sure what etc_root_password is supposed to be doing; I'm not using it anymore either. I'm controlling access to the mysql root account by requiring users to run sudo mysql if they want to use the root account.

[apenney]

I like this idea, I was having a lot of trouble understanding etc_root_password and the use case for it. I just assumed there was some cases I didn't understand where this made sense.

[ezheidtmann]

Looks like etc_root_password was added in 0963d2e -- would bodepd remember why? Should I contact him? Should I draft a PR removing the parameter?

[hunner]

Looks like etc_root_password was added in 0963d2e -- would bodepd remember why? Should I contact him? Should I draft a PR removing the parameter?

I think the quickest way forward would be to cause etc_root_password to add password = <% @etc_root_password %> to the [client] section of templates/my.cnf.erb and remove the resource that causes the duplicates. This basically allows the (questionable) behavior to continue until it is refactored. Thanks for your input @ezheidtmann !

[hunner]

Actually probably more like:

<% if @etc_root_password -%>
  password='<% @root_password %>'
<% end -%>

and use validate_bool($etc_root_password) because of #166

[apenney]

14:48:13 <+bodepd> ashp: I though it existed to allow to set the password in /etc/my.cnf instead of /root/my.cnf
14:48:24 <+bodepd> I forget the use case. it's been years

So basically bodepd doesn't really remember either, I don't think we need to put any more thought into this one. :)

[ezheidtmann]

Added a commit that does as hunner prescribes. Tested locally, but let's see if Travis likes it.

I'm also happy to change the PR to delete the feature entirely.

[hunner]

Hmm. This appears to be on purpose on RedHat (https://github.com/puppetlabs/puppetlabs-mysql/blob/master/spec/classes/mysql_config_spec.rb#L212) as $config_file on Debian is /etc/mysql/my.cnf instead of /etc/my.cnf... Now I have no idea what the desired behavior is. Would this even work on Debian if /etc/mysql/my.cnf had the password set in it?

I'm working on getting some rspec-system tests added which would probably make it easier to answer this question if I knew why the password was being added to the etc-config's client section in the first place.

[ezheidtmann]

That's quite strange. I set the password = and user = directives in /etc/mysql/my.cnf in Ubuntu and it works (unprivileged user can connect as root). Haven't tried to run puppet on ubuntu.

I'm equally clueless about what etc_root_password is good for, so I'll sit back and let you do your work.

[apenney]

Hi,

I've recently merged in a very large changeset that completely redesigns this module. As a result I'm afraid this PR is no longer mergeable and possibly no longer valid (sorry, mass message for all PRs) due to the changes to binding and the removal of most of the parameters for my.cnf (as it's handed by an override hash currently).

I'm going to close all existing PRs that can't be merged and ask you to take a look at rebasing them against master or redeveloping them against master if the feature is still important to you. I hope that many of the existing PRs won't be needed in the refactoring!

Thanks for your patience everyone, I know this is pain.