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

Fixes #23306 - pass database connection configs to cpdb command #196

Merged

Conversation

kgaikwad
Copy link
Member

No description provided.

@theforeman-bot
Copy link
Member

Issues: #23306

parent_cmd = '/usr/share/candlepin/cpdb' if parent_cmd.empty?
help_cmd = "#{parent_cmd} --help | grep -c '\\-\\-#{option_name} '"
option_name += ' ' if followed_by_whitespace
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this? Wouldn't matching for whole word via grep -c '\\-\\-#{option_name}\b'" solve it or it's something else?

Copy link
Member Author

@kgaikwad kgaikwad Jun 27, 2018

Choose a reason for hiding this comment

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

@iNecas,
It is matching whole word.
I did mistake while testing command on CLI so this space was added by me in previous code. Extra-space not needed so removed.

@kgaikwad kgaikwad force-pushed the 23306_candlepin_external_db_config branch from 49ccfa3 to 12e25b3 Compare June 27, 2018 13:15
@iNecas
Copy link
Member

iNecas commented Jun 28, 2018

@alda519 mind giving this a try, as you've filed the original BZ?

@iNecas
Copy link
Member

iNecas commented Jun 28, 2018

No other comments code-wise.

@alda519
Copy link
Contributor

alda519 commented Jun 28, 2018

sure, it works, but not over SSL
dbhost, port, dbname is passed to cpdb, but not other options like ssl and sslfactory

ssl=true would fix

Error running command: liquibase --driver=org.postgresql.Driver --classpath=/usr/share/java/postgresql-jdbc.jar:/var/lib/tomcat/webapps/candlepin/WEB-INF/classes/ --changeLogFile=db/changelog/changelog-validate.xml --url=jdbc:postgresql://dbhost:5432/candlepin1db --username=candlepin1 --password=candlepin1pw --logLevel=debug migrate -Dcommunity=False
Status code: 65280
Command output: Liquibase update Failed: liquibase.exception.DatabaseException: org.postgresql.util.PSQLException: FATAL: no pg_hba.conf entry for host "...", user "candlepin1", database "candlepin1db", SSL off

sslfactory=org.postgresql.ssl.NonValidatingFactory would fix

Command output: Liquibase update Failed: liquibase.exception.DatabaseException: org.postgresql.util.PSQLException: SSL error: sun.security.validator.ValidatorException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

candlepin.conf (after satellite-installer --katello-candlepin-db-ssl-verify false --katello-candlepin-db-ssl true)

org.quartz.dataSource.myDS.URL=jdbc:postgresql://hostname:5432/candlepin1db?ssl=true&sslfactory=org.postgresql.ssl.NonValidatingFactory

@kgaikwad
Copy link
Member Author

kgaikwad commented Jun 28, 2018

@alda519,
I am looking into candlepin-2.4.1-1.el7.noarch. I can see that there is no options to pass sslandsslfactorytocpdb` command so creating an issue to expose those options in cpdb.

For now, I will add changes and skip this check when there is ssl=true set in candlepin.conf file.

@alda519
Copy link
Contributor

alda519 commented Jun 29, 2018

it can be appended to db name
but it is quite a hack

@iNecas
Copy link
Member

iNecas commented Jun 29, 2018

@alda519 could you give us an example on how this was supposed to work?

@alda519
Copy link
Contributor

alda519 commented Jun 29, 2018

I meant something like '-d' => configuration['database'] + '?ssl=true&sslfactory=...'

@iNecas
Copy link
Member

iNecas commented Jun 29, 2018

if the cpdb accepts it, seems like a good workaround - better then skipping the check for remote dbs

@kgaikwad kgaikwad force-pushed the 23306_candlepin_external_db_config branch from 12e25b3 to aacb65e Compare July 2, 2018 06:10
@kgaikwad kgaikwad changed the title Fixes #23306 - pass db configs i.e dbport, dbhost to cpdb command Fixes #23306 - pass database connection configs to cpdb command Jul 2, 2018
@kgaikwad
Copy link
Member Author

kgaikwad commented Jul 2, 2018

@iNecas, @alda519,
Updated PR as per above discussion!

@alda519, thank you for providing a workaround. Please give a try with updated changes.

@@ -48,7 +49,7 @@ def env_content_ids_with_null_content
def load_configuration
raw_config = File.read(CANDLEPIN_DB_CONFIG)
full_config = Hash[raw_config.scan(/(^[^#\n][^=]*)=(.*)/)]
uri_regexp = %r{://(([^/:]*):?([^/]*))/([^?]*)\??(ssl=([^&]*))?}
uri_regexp = %r{://(([^/:]*):?([^/]*))/([^?]*)\??(ssl=([^&]*))?\&?(sslfactory=([^&]*))?}
Copy link
Member

Choose a reason for hiding this comment

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

This depens on the ordering of the params (the ssl and sslfactory). It would be probably better not trying to extract them both in the same regexp, but having separate regexps for those two.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@kgaikwad kgaikwad force-pushed the 23306_candlepin_external_db_config branch from aacb65e to 80a4b30 Compare July 2, 2018 10:51
query_string = url.split('?')[1]
return nil unless query_string
output = /#{key_name}=([^&]*)?/.match(query_string)
return nil unless output
Copy link
Member

Choose a reason for hiding this comment

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

The last two lines can be simplified by this, unless there is a reason I can't think of for checking on output[0].include(key_name):

output[1] if output

Copy link
Member Author

@kgaikwad kgaikwad Jul 2, 2018

Choose a reason for hiding this comment

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

Thank you @iNecas!
Yeah, no reason.
I think, I need to take a cup of coffee :-).

@iNecas
Copy link
Member

iNecas commented Jul 2, 2018

On nitpick, but ready for testing in production @alda519

@kgaikwad kgaikwad force-pushed the 23306_candlepin_external_db_config branch from 80a4b30 to ab5227d Compare July 2, 2018 11:55
@alda519
Copy link
Contributor

alda519 commented Jul 2, 2018

Well, now ssl and sslfactory works!

D, [2018-07-02 09:04:50+0200 #17936] DEBUG -- : Running command /usr/share/candlepin/cpdb --validate --verbose -u 'candlepin1' -p '[FILTERED]' -d 'candlepin1db\?ssl\=true' --dbhost '...com' --dbport '5432' with stdin nil
D, [2018-07-02 09:12:26+0200 #20558] DEBUG -- : Running command /usr/share/candlepin/cpdb --validate --verbose -u 'candlepin1' -p '[FILTERED]' -d 'candlepin1db\?ssl\=true\&sslfactory\=org.postgresql.ssl.NonValid
atingFactory' --dbhost '...com' --dbport '5432' with stdin nil

Check to validate candlepin database:                                 [OK]

Perfect!

@iNecas iNecas merged commit 9c41454 into theforeman:master Jul 2, 2018
@iNecas
Copy link
Member

iNecas commented Jul 2, 2018

Thanks @kgaikwad and @alda519

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.

None yet

4 participants