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

mod/ssl: Add option to configure SSL mutex #1371

Merged
merged 1 commit into from Feb 16, 2016
Merged

mod/ssl: Add option to configure SSL mutex #1371

merged 1 commit into from Feb 16, 2016

Conversation

daenney
Copy link
Contributor

@daenney daenney commented Feb 16, 2016

This allows the end user to explicitly define or override what the SSLMutex or Mutex configuration for Apache will be as the platform default might not always be desirable.

Supersedes #1346
Closes #1346

@daenney
Copy link
Contributor Author

daenney commented Feb 16, 2016

@bmjen @igalic This change introduces a huge problem. Because we need to do a versioncmp for Debian all tests now fail as apache_version isn't set by any of the tests. What to do?

@@ -212,6 +213,13 @@
$default_ssl_cert = '/etc/ssl/certs/ssl-cert-snakeoil.pem'
$default_ssl_key = '/etc/ssl/private/ssl-cert-snakeoil.key'
$ssl_certs_dir = '/etc/ssl/certs'
if versioncmp($apache_version, '2.4') >= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is causing the lint failure:

metadata-json-lint metadata.json manifests/params.pp - WARNING: top-scope variable being used without an explicit namespace on line 216

@bmjen
Copy link
Contributor

bmjen commented Feb 16, 2016

Thanks @daenney ! This looks good. Just need to fix the linting error.

@daenney
Copy link
Contributor Author

daenney commented Feb 16, 2016

@bmjen But that's a problem. Because apache_version comes from version.pp which gets loaded and exposed by init.pp as $apache_version = $::apache::version::default in the parameter declaration. But since params.pp gets loaded as part of init.pp, loading it in params.pp from version.pp might cause us to use a different version for the comparison there than anything the user might explicitly provide by passing in apache_version, breaking the configuration for their platform.

We get a circular dependency of sorts.

@bmjen
Copy link
Contributor

bmjen commented Feb 16, 2016

Yep. I agree. I'm thinking you're right @daenney, in going back to the implementation of #1346... but with the correct parameter name, docs, and tests.

This allows the end user to explicitly define or override what the
`SSLMutex` or `Mutex` configuration for Apache will be as the
platform default might not always be desirable.

Supersedes #1346
Closes #1346
@daenney
Copy link
Contributor Author

daenney commented Feb 16, 2016

@bmjen Alright, updated. I used _ssl_mutex instead of the ssl_mutex_real pattern so we get the benefit of it being a private variable on Puppet 4.

@bmjen
Copy link
Contributor

bmjen commented Feb 16, 2016

👍

bmjen added a commit that referenced this pull request Feb 16, 2016
mod/ssl: Add option to configure SSL mutex
@bmjen bmjen merged commit ffbc072 into puppetlabs:master Feb 16, 2016
@daenney daenney deleted the ssl-mutex-config branch February 17, 2016 08:04
@daenney
Copy link
Contributor Author

daenney commented Feb 17, 2016

🍻 💃 👯 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants