Calculate the client DN for Artemis, ignoring empty values

[ehelms]

Moving the computation of the Artemis client DN into this class
instead of relying on the value provided directly by classes in
puppet-certs aims to solve two problems:

1. Testing at all levels of the value of the client DN
2. Adding a function to calculate the DN based on a set of values and
   ignoring any empty values which do not end up in the actual certificate

Additionally this removes the layers of indirection certs::candlepin::artemis_client_dn which pointed certs::foreman::client_dn and moves the calculation closer to the input as this class serves to bring another of items together to properly configure Candlepin/Artemis/Tomcat.

This aims to solve a reported BZ from Satellite where if a user customizes parts of the certificate creation process, the statically calculated client DN breaks and the Artemis connection breaks. By ignoring empty values the right DN gets provided.

Somewhere in the future, there is a path to eventually calculate this directly from the certificate and avoid even more ambiguity -- my attempts at this previously have proved difficult.

[ehelms]

@ekohl could I ask for a round of review on this idea?

[ekohl]

I like the concept, but I think it breaks some design choices.

The idea is that katello::candlepin could be deployed on one server and katello::application on another (requiring the user/admin to take care of any relationship between the two). Then there is katello for the all-in-one installation we use in our installer. I'll grant you that we may be a long way away from a true split deployment, but this shows how you could build HA setups.

[ekohl]

I don't quite like that this pulls in Foreman certificates, even though they may not be used at all (this class is still intended to be used standalone on a server without Katello).

Instead, I'd suggest to make add $artemis_client_dn as a class parameter here. Then in init.pp you would pass it. For that to work you need to swap the order of katello::candlepin and katello::application (due to compile time dependencies, no actual chaining).

You could set $artemis_client_dn (as you calculate it below) in application.pp and reuse it in init.pp. That would signal the relationship.

[ehelms]

Hrmm, testing that idea out I am running into cyclic dependency issues:

        Evaluation Error: Error while evaluating a Resource Statement, Duplicate declaration: Class[Certs::Candlepin] is already declared; cannot redeclare (file: /home/ehelms/workspace/upstream/installer/puppet-katello/spec/fixtures/modules/katello/manifests/candlepin.pp, line: 37) (file: /home/ehelms/workspace/upstream/installer/puppet-katello/spec/fixtures/modules/katello/manifests/candlepin.pp, line: 37, column: 3) on node wareagle.com

Thoughts on #457 (comment) ?

[ekohl]

Hmm, tricky. We don't have a proper separation in certs::candlepin. Ideally we'd have certs::candlepin::server and certs::candlepin::client where katello::candlepin configures the former while katello::application uses the latter.

Given it actually uses the Foreman certificates as client bits, perhaps we can get away with NOT including certs::candlepin at all in katello::application.

I've pushed some changes that pass unit tests.

[ehelms]

Does the prior existence of this not already imply the behavior you mentioned trying to avoid with this change? This relies on certs::candlepin which in turn relies on certs::foreman -- https://github.com/theforeman/puppet-certs/blob/master/manifests/candlepin.pp#L27

[ekohl]

Yes, I do think that eventually certs::candlepin should no longer include certs::foreman. It'll be a breaking change, but worth it.

[ekohl]

Technically you need to write

Suggested change

	# @return String
	# @return [String] human readable description

However, I prefer to use return_type 'String' in the dispatch, similar to param.

[ehelms]

Currently, acceptance tests are failing due to foreman.service timing out on boot through systemd. I observed this locally and that theforeman/foreman-packaging#8660 seemed to provide a fix.... granted that is not a complete root cause analysis but does close a gap. I could see no other obvious evidence as to why it was suddenly timing out on start yet.

[ehelms]

Testing the same assertion round trip, seems to fix test runs locally as well:

diff --git a/spec/acceptance/katello_spec.rb b/spec/acceptance/katello_spec.rb
index 98abe98..963422d 100644
--- a/spec/acceptance/katello_spec.rb
+++ b/spec/acceptance/katello_spec.rb
@@ -8,6 +8,8 @@ describe 'Scenario: install katello' do
       <<-PUPPET
         include katello
         include foreman::cli
+
+        package { 'rubygem-sd_notify': ensure => installed }
       PUPPET
     end
   end

[ekohl]

I rebased it and included the return type. I'm OK with ignoring the sd_notify bit for now.