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

candlepin: use own certs for qpid #215

Merged
merged 1 commit into from
Oct 5, 2017

Conversation

timogoebel
Copy link
Member

The qpid_ssl_cert and qpid_ssl_key parameters for candlepin are just to set up an exchange.

This can be done with candlepin's qpid certificates and removed the dependency for qpid's certificates.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

It looks good and I think you're right, but the tests are failing.

@timogoebel
Copy link
Member Author

Tests currently fail, because ::katello::qpid_client isn't in the catalog anymore. I'm curious, if ::katello::qpid_client is just needed by something else or it's just unneeded clutter.

@timogoebel
Copy link
Member Author

I have a feeling, that actually the katello rails application needs to have include ::katello::qpid_client. I'll continue looking into this and update the PR accordingly.

@timogoebel
Copy link
Member Author

Yeah, my expectation was correct: The Katello rails app needs katello::qpid_client to work properly. You can see this in /var/log/foreman/dynflow_executor.output:

Starting dynflow with the following options: {:foreman_root=>"/usr/share/foreman", :process_name=>"dynflow_executor", :pid_dir=>"/usr/share/foreman/tmp/pids", :log_dir=>"/usr/share/foreman/log", :wait_attempts=>300, :wait_sleep=>1, :executors_count=>1, :memory_limit=>0, :memory_init_delay=>7200, :memory_polling_interval=>60}
Everything ready for world: 42678733-a5ff-4ee1-a860-f2dca3bec4bd
terminate called after throwing an instance of 'qpid::types::Exception'
  what():  Failed to initialise SSL: Failed: NSS error [-8015] (/builddir/build/BUILD/qpid-cpp-1.36.0/src/qpid/sys/ssl/util.cpp:100) (/builddir/build/BUILD/qpid-cpp-1.36.0/src/qpid/client/SslConnector.cpp:149)

@timogoebel
Copy link
Member Author

This is still missing a notify from Class['certs::qpid'] ~> Service['foreman-tasks'].

@@ -22,6 +22,8 @@
include ::certs::apache
include ::certs::foreman
include ::certs::pulp_client
include ::certs::qpid
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't qpid_client be sufficient? Or can we make it sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, certs::qpid_client just rolls out certificates for the qpid-config tool. certs::qpid rolls out the whole nss certstore. We need the latter for the app.
::katello::qpid_client does include include ::certs::qpid, so this is here just for completeness' sake.

Copy link
Member

Choose a reason for hiding this comment

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

If we're not using it in this class then I'd consider it an implementation detail of qpid_client, but it might even be better to just include qpid::client in this class rather than split it off if there's just 1 use of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left this in now, as it's required for the notify of foreman-tasks.

@ekohl
Copy link
Member

ekohl commented Oct 4, 2017

This is still missing a notify from Class['certs::qpid'] ~> Service['foreman-tasks'].

With theforeman/puppet-foreman#530 that'll be nicely wrapped in foreman::service

@timogoebel
Copy link
Member Author

@ekohl: Added the notify and a test for it so it works with the current implementation of puppet-foreman.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'll likely revisit the notifications when the foreman PR is merged

@ekohl ekohl merged commit ae249c6 into theforeman:master Oct 5, 2017
@ekohl
Copy link
Member

ekohl commented Oct 5, 2017

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants