-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add support for non-validated SSL on database connections. #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks OK
manifests/init.pp
Outdated
@@ -23,6 +23,10 @@ | |||
# from standard port of the :db_type. | |||
# ex. mysql will default to 3306 and postgresql will default to 5432. | |||
# | |||
# $db_ssl:: Boolean indicating if the connection to the database should be over |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a validate_bool for this, like $amq_enable
on Line 168
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
I addressed tso, not good enough yet. I forgot to take mysql into account and so not sure what this would do on a mysql system. |
Is there also validated SSL connections as a possible configuration? Should the sslfactory aspect be parameterized or is that the only factory useful for SSL? |
@stbenjam Looks like you also need to re-review this since oyu requested changes. |
So, i believe that by default it would verify the cert, the factory is only needed to disable the CA verification (so trust non-ca chain signed certs). Would you want that as |
I think a boolean would be good. |
Looks like this will need a rebase and a few updates |
okay... that should be better... or not |
Okay.. now it should be better. i missed a few things last comment. |
manifests/database/postgresql.pp
Outdated
@@ -10,6 +10,8 @@ | |||
$db_type = $::candlepin::db_type, | |||
$db_host = $::candlepin::db_host, | |||
$db_port = pick($::candlepin::db_port, 5432), | |||
$db_ssl = $::candlepin::db_ssl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing comma here and the line below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah.. so its in the init.pp. should it be in both or just here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All class parameters are separated by comma's. init.pp isn't special in terms of syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that comment was for the validation comment. not sure how it ended up on the comma one.
…d this to the liquibase since the default local install doesnt use SSL, and it appears there might be an issue with liquibase connecting to postgresql over SSL. https://liquibase.jira.com/browse/CORE-1281
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Puppet code looks correct.
@stbenjam could you have a look again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
We are working on various changes to the default katello install, such as splitting the database off to an external host. For that, we use an SSL'ed connection. I'd actually rather have an options of no, yes, verified but am not sure the best way to expose that.
I did not add this to the liquibase since the default local install doesnt use SSL, and it appears there might be an issue with [liquibase connecting to postgresql over SSL|https://liquibase.jira.com/browse/CORE-1281].
I'm open to other validation factories, but this seemed like the easy start.
Need to add testing. sorry.