Skip to content

Conversation

Heidistein
Copy link
Contributor

@Heidistein Heidistein commented Oct 27, 2021

In some very special (read: stupid) configuration I need to supply a
special configuration file so that puppet can log in.

The problem is: Some other package (plesk) will not allow to have /root/my.cnf in existence. Having the credentials in /etc/my.cnd.d/client.cfg break other stuff in the application. It will authenticate as a (non root) user, using a password from the ENV. The password in the client.cnf [mysql] section overrides this.

On top of this, I imagine someone would like to use a specific user for puppet to configure stuff, and/or disable/remove the root user

I really need help with creating a spec...

In some very special (read: stupid) configuration I need to supply a
special configuration file so that puppet can log in.
@Heidistein Heidistein requested a review from a team as a code owner October 27, 2021 12:02
@CLAassistant
Copy link

CLAassistant commented Oct 27, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Heidistein
❌ LukasAud
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -48,6 +48,12 @@ def self.defaults_file
"--defaults-extra-file=#{Facter.value(:root_home)}/.my.cnf" if File.file?("#{Facter.value(:root_home)}/.my.cnf")
end

# Optional puppet-defaults file
def self.puppet_defaults_file
"--defaults-file=#{Facter.value(:root_home)}/.my.puppet.cnf" if File.file?("#{Facter.value(:root_home)}/.my.puppet.cnf")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason for the my.puppet.cnf filename? is that something that should be configureable / why can't you use the .mylogin.cnf? do you create the .my.puppet.cnf via puppet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no this cant be configurable, because it is used in a type, which is before hiera. I really which it would be possible to configure.
The .mylogin.cnf is only available on mysql, and not mariadb.

If you want, another possibility could be to not use a different file, but the --login-path command line option. I would need to test this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

answering all questions: the filename is just chosen by looking at the stars, correlating it with average windspeed during my dogwalk. I personally create the file using puppet, most would. Should i hack this into the module to do it for us? There is a chance that users of this file would want to add mysterious and unforseen options to it.

@Heidistein Heidistein requested review from bastelfreak and removed request for a team October 29, 2021 10:28
@Heidistein
Copy link
Contributor Author

Hi!
I would really like some feedback on this...
should I continue this path or should I change to --defaults-group-suffix= option. Is there even a remote chance this will ever get merged?

@binford2k
Copy link
Contributor

Adding another method and more conditionals is more complexity than needed. I'd much prefer to simply extend the original method to return the most specific options file that exists. Something like

def self.defaults_file
  options = [
    "#{Facter.value(:root_home)}/.my.puppet.cnf",
    "#{Facter.value(:root_home)}/.my.cnf",
  ]
  found = options.find {|path| File.file? path}

  "--defaults-extra-file=#{found}" if found
end

I kind of hate doing this, but I do see the utility. (I think.)

The best I can tell, it's not that Plesk disallows /root/my.cnf but that for whatever reason, its installer fails if /root/my.cnf provides authentication options. There is precedent for similar config files, eg /etc/mysql/debian.cnf.

@Heidistein
Copy link
Contributor Author

Adding another method and more conditionals is more complexity than needed. I'd much prefer to simply extend the original method to return the most specific options file that exists. Something like

def self.defaults_file
  options = [
    "#{Facter.value(:root_home)}/.my.puppet.cnf",
    "#{Facter.value(:root_home)}/.my.cnf",
  ]
  found = options.find {|path| File.file? path}

  "--defaults-extra-file=#{found}" if found
end

I kind of hate doing this, but I do see the utility. (I think.)

The best I can tell, it's not that Plesk disallows /root/my.cnf but that for whatever reason, its installer fails if /root/my.cnf provides authentication options. There is precedent for similar config files, eg /etc/mysql/debian.cnf.

Yes, your option is nicer, however. .my.cnf uses --defaults-extra-file= whereas .my.puppet.cnf uses --defaults-file=. This is on purpose. .my.cnf is just to leave backwards compatability (leave 'as is'). The .my.puppet.cnf user the "use only this file, and none other" option

@github-actions
Copy link

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the stale label May 21, 2022
@Heidistein
Copy link
Contributor Author

This still is relevant!

@github-actions github-actions bot removed the stale label May 23, 2022
@github-actions
Copy link

Hello! 👋

This pull request has been open for a while and has had no recent activity. We've labelled it with attention-needed so that we can get a clear view of which PRs need our attention.

If you are waiting on a response from us we will try and address your comments on a future Community Day.

Alternatively, if it is no longer relevant to you please close the PR with a comment.

Please note that if a pull request receives no update for 7 after it has been labelled, it will be closed. We are always happy to re-open pull request if they have been closed in error.

@LukasAud
Copy link
Contributor

Hey @Heidistein, just wanted to follow up on this PR. We are investigating your PR as we currently are not sure whether this feature can be justified in terms of functionality/reachability/maintenance-cost.

We understand this PR has been sitting around for close to a year already, but it may take a bit longer still until we can assign the resources to properly look at it. Sorry for the inconvenience.

@github-actions github-actions bot closed this Aug 2, 2022
@LukasAud
Copy link
Contributor

LukasAud commented Aug 2, 2022

Apologies for that. It seems like our bot might be malfunctioning.

@LukasAud LukasAud reopened this Aug 2, 2022
@github-actions github-actions bot closed this Aug 10, 2022
@LukasAud LukasAud reopened this Aug 10, 2022
@LukasAud
Copy link
Contributor

Hey @Heidistein, our team has finally been able to evaluate your proposed change. Unfortunately, after some discussions, we have decided that we will not merge this PR. While the change itself is valid and seems to work, this is not something that we are looking to implement right now.

We will be closing this PR now. However, we are willing to re-open this PR in the future if more people from the community share their interest in it. As such, I recommend you create a 'feature request' in our 'Issues' section and link this PR there so that the changes and discussions are not lost.

@LukasAud LukasAud closed this Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants