Use environment instead of pgpass file #6328

kbrock · 2016-01-25T17:29:08Z

blocked on: ManageIQ/awesome_spawn#34

removes potential threading conflicts
no longer writes passwords to text files
no longer tied to root user
no longer blows away customer created .pgpass file.
now can run tasks on password protected databases.
now works on mac, linux, and appliances.
less code

@Fryguy ~~would it be ok to release awesome_spawn? feature was added there in August~~ THANKS

UPDATE: allow passwords to be specified and database dumps to work. Added testing instructions below

jrafanie · 2016-01-25T18:56:43Z

gems/pending/util/postgres_admin.rb

-    File.delete(PGPASS_FILE) if opts[:password]
+    AwesomeSpawn.run!(cmd_str, :params => params, :env => {
+                        "PGUSER"     => opts[:username],
+                        "PGPASSWORD" => opts[:password]}).output


If this works, 👍

I see we aren't using the opts[:dbname], will this work if the dbname for the cmd_str is ambiguous?

Before:
The command line arguments (cmd_str and params) needs to specify the username and dbname.
The dbname in .pgpass helps libpg determine which password to use.

After:
The command line still needs to specify the username and database name.
The environment variables helps libpg determine which password to use.

It looks like PGUSER will not be read because we are specifying the username on the command line.
We could specify PGDATABASE, but it would not be read because we are specifying that too on the command line.

@jrafanie Those 2 fields are redundant. I can:

add PGDATABASE

remove PGUSER

OR leave as is

I'm more than happy with any of those 3 solutions.

I'd suggest we change the callers to do the right thing if we're not... specify everything we need to specific. I believe it's isolated to just a handful of locations. I don't remember if there was a reason I did a pgpass file instead of environment variables.

Either way, if you are able to do a backup and restore from an appliance using your changes, I think we can fix the fallout in the future.

I'm leaning towards option 3.

chessbyte · 2016-01-27T14:24:52Z

@gtanzillo please review

chessbyte · 2016-02-11T03:10:42Z

@kbrock @jrafanie what is the status of this PR?

kbrock · 2016-02-11T04:25:27Z

@jrafanie I'm not sure what you wanted to do here. Remove the PGUSER?

jrafanie · 2016-02-12T16:37:54Z

Either way, if you are able to do a backup and restore from an appliance using your changes, I think we can fix the fallout in the future.

To clarify, if you can run these rake tasks on an appliance, I'm fine with the changes. These should cover the use cases I was concerned about:

rake evm:db:backup:local
rake evm:db:backup:remote
rake evm:db:gc
rake evm:db:restore:local
rake evm:db:restore:remote

The backup/restore args are documented in the rake task here

If you have ideas on how to unit test these, I'm all 👂

gtanzillo · 2016-02-23T22:04:13Z

@kbrock I'm ok with these changes as long as you are able to verify that it does not break the rake tasks that @jrafanie mentioned above. Have you been able to test them?

miq-bot · 2016-03-23T16:37:15Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

chessbyte · 2016-04-27T21:51:59Z

@kbrock any interest in completing this?

kbrock · 2016-04-27T22:30:48Z

Thanks - I'll test the rake tasks here and rebase

chriskacerguis · 2016-06-17T16:34:28Z

@kbrock can you please rebase?

miq-bot · 2016-06-20T17:12:12Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

kbrock · 2016-06-21T00:23:53Z

@jrafanie so I started digging into this

change pg_hba.conf s/trust/md5/g meaning, require password from everyone.
run bundle exec rake evm:db:backup:local

I can't figure out how to even get a password into the system to backup the database.

Since most appliances are setup to trust, this problem is not evident.
I had expected to type --password or for it to parse database.yml

So we have a few options:

delete the password handling all together
allow password to be passed in
just default the values from database.yml

kbrock · 2016-07-11T16:19:49Z

WIPing: this functionality is never used. no password can be passed in

- no potential conflicts - not writing passwords to text files - no longer tied to root user - now works on mac - less code

kbrock · 2016-07-11T21:13:13Z

@jrafanie let me know if your now good with this.

miq-bot · 2016-07-11T21:20:44Z

Checked commits kbrock/manageiq@0dcef4f~...2a1402b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. 🍪

gtanzillo · 2016-07-13T15:50:51Z

@carbonin @jrafanie Are you guys ok with this change?

jrafanie · 2016-07-13T16:00:23Z

@kbrock can you confirm you tested this?

kbrock · 2016-07-13T16:53:20Z

@jrafanie I did the following on a mac:

modified pg_hba.conf to require a password for vmdb_development
bundle exec rake evm:db:backup:local # fails
git checkout pr/6328
bundle exec rake evm:db:backup:local # succeeds

jrafanie · 2016-07-13T22:02:34Z

Thanks @kbrock. I'm ok with this. Since @carbonin is in this neck of the woods with HA/replication... I'll wait for his 👍 before merging.

carbonin · 2016-07-14T12:38:12Z

Looks good 👍

simaishi · 2017-04-18T21:29:50Z

Darga BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1438674

kbrock added enhancement appliance labels Jan 25, 2016

miq-bot added the dependencies label Jan 25, 2016

jrafanie reviewed Jan 25, 2016
View reviewed changes

kbrock changed the title ~~Use environment instead of pgpass file~~ [WIP] Use environment instead of pgpass file Jan 26, 2016

chessbyte added the wip label Jan 26, 2016

kbrock force-pushed the pgpass_be_gone branch from 428b8b1 to 1164ec7 Compare January 26, 2016 21:19

kbrock removed the wip label Jan 27, 2016

kbrock force-pushed the pgpass_be_gone branch from 1164ec7 to bbcb0cf Compare January 27, 2016 13:24

kbrock changed the title ~~[WIP] Use environment instead of pgpass file~~ Use environment instead of pgpass file Jan 27, 2016

chessbyte assigned gtanzillo May 10, 2016

kbrock force-pushed the pgpass_be_gone branch from bbcb0cf to c5d43c5 Compare June 17, 2016 17:12

kbrock force-pushed the pgpass_be_gone branch from c5d43c5 to a0234e9 Compare June 20, 2016 21:09

kbrock changed the title ~~Use environment instead of pgpass file~~ [WIP] Use environment instead of pgpass file Jul 11, 2016

kbrock added the wip label Jul 11, 2016

kbrock and others added 2 commits July 11, 2016 16:48

Use environment instead of pgpass file

0dcef4f

- no potential conflicts - not writing passwords to text files - no longer tied to root user - now works on mac - less code

allow passwords to be passed to tasks

2a1402b

kbrock removed the wip label Jul 11, 2016

kbrock changed the title ~~[WIP] Use environment instead of pgpass file~~ Use environment instead of pgpass file Jul 11, 2016

kbrock force-pushed the pgpass_be_gone branch from a0234e9 to 2a1402b Compare July 11, 2016 21:11

jrafanie added the darga/no label Jul 14, 2016

jrafanie merged commit 9a714af into ManageIQ:master Jul 14, 2016

jrafanie added this to the Sprint 44 Ending Aug 1, 2016 milestone Jul 14, 2016

kbrock deleted the pgpass_be_gone branch July 14, 2016 14:22

carbonin added darga/yes and removed darga/no labels Apr 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use environment instead of pgpass file #6328

Use environment instead of pgpass file #6328

kbrock commented Jan 25, 2016 •

edited

jrafanie Jan 25, 2016

jrafanie Jan 27, 2016

kbrock Jan 27, 2016

jrafanie Jan 27, 2016

kbrock Jun 22, 2016

chessbyte commented Jan 27, 2016

chessbyte commented Feb 11, 2016

kbrock commented Feb 11, 2016

jrafanie commented Feb 12, 2016

gtanzillo commented Feb 23, 2016

miq-bot commented Mar 23, 2016

chessbyte commented Apr 27, 2016

kbrock commented Apr 27, 2016

chriskacerguis commented Jun 17, 2016

miq-bot commented Jun 20, 2016

kbrock commented Jun 21, 2016 •

edited

kbrock commented Jul 11, 2016 •

edited

kbrock commented Jul 11, 2016

miq-bot commented Jul 11, 2016

gtanzillo commented Jul 13, 2016

jrafanie commented Jul 13, 2016

kbrock commented Jul 13, 2016

jrafanie commented Jul 13, 2016 •

edited by chessbyte

carbonin commented Jul 14, 2016

simaishi commented Apr 18, 2017

Use environment instead of pgpass file #6328

Use environment instead of pgpass file #6328

Conversation

kbrock commented Jan 25, 2016 • edited

jrafanie Jan 25, 2016

Choose a reason for hiding this comment

jrafanie Jan 27, 2016

Choose a reason for hiding this comment

kbrock Jan 27, 2016

Choose a reason for hiding this comment

jrafanie Jan 27, 2016

Choose a reason for hiding this comment

kbrock Jun 22, 2016

Choose a reason for hiding this comment

chessbyte commented Jan 27, 2016

chessbyte commented Feb 11, 2016

kbrock commented Feb 11, 2016

jrafanie commented Feb 12, 2016

gtanzillo commented Feb 23, 2016

miq-bot commented Mar 23, 2016

chessbyte commented Apr 27, 2016

kbrock commented Apr 27, 2016

chriskacerguis commented Jun 17, 2016

miq-bot commented Jun 20, 2016

kbrock commented Jun 21, 2016 • edited

kbrock commented Jul 11, 2016 • edited

kbrock commented Jul 11, 2016

miq-bot commented Jul 11, 2016

gtanzillo commented Jul 13, 2016

jrafanie commented Jul 13, 2016

kbrock commented Jul 13, 2016

jrafanie commented Jul 13, 2016 • edited by chessbyte

carbonin commented Jul 14, 2016

simaishi commented Apr 18, 2017

kbrock commented Jan 25, 2016 •

edited

kbrock commented Jun 21, 2016 •

edited

kbrock commented Jul 11, 2016 •

edited

jrafanie commented Jul 13, 2016 •

edited by chessbyte