Fixes #29715: Add option to expose mongodb on a public interface with… #340

ehelms · 2020-05-11T12:40:47Z

… SSL

Requires:

manifests/pulp.pp

manifests/init.pp

manifests/pulp.pp

spec/classes/pulp_spec.rb

manifests/pulp.pp

wbclark · 2020-06-19T19:50:11Z

Actually I missed something, there is still notify => Service['httpd'] at https://github.com/theforeman/puppet-pulp/blob/master/manifests/apache.pp#L135

wbclark · 2020-06-19T20:00:14Z

So, I think the issue is a bit simpler than I previously thought:

When $expose_mongodb is changed to true in katello::pulp what happens? We don't make any changes to the actual pulp classes -- we're actually touching certs and mongodb.

wbclark · 2020-06-19T20:00:51Z

manifests/pulp.pp

+    include certs::mongodb
+    include certs::mongodb_client
+
+    Class['certs::mongodb_client'] -> Class['pulp']


Suggested change

Class['certs::mongodb_client'] -> Class['pulp']

Class['certs::mongodb_client'] ~> Class['pulp']

I think this will fix your issue, @ehelms

I thin kthat will only cover if the certificates get created or changed? But doesn't cover I feel the whole of the issue which is if /etc/pulp/server.conf gets updated, then httpd should get restarted to pick up the changes.

@ekohl adding you in case you had any thoughts as to why I don't seem to be seeing the refreshing happen

I suspect Pulp doesn't actually manage the vhost and thus doesn't refresh Apache. I'd try Class['certs::mongodb_client'] ~> Class['pulp', 'apache::service']. Both since pulp will still refresh the workers.

ekohl · 2020-06-22T15:46:02Z

spec/classes/pulp_spec.rb

+
+            context 'with mongodb_ipv6 => true' do
+              let :params do
+                super().merge({ 'expose_mongodb_ipv6' => true, })


I've grown accustomed to the shorter syntax:

Suggested change

super().merge({ 'expose_mongodb_ipv6' => true, })

super().merge(expose_mongodb_ipv6: true)

ekohl · 2020-06-22T15:48:30Z

manifests/pulp.pp

+    include certs::mongodb
+    include certs::mongodb_client
+
+    Class['certs::mongodb_client'] -> Class['pulp']


I suspect Pulp doesn't actually manage the vhost and thus doesn't refresh Apache. I'd try Class['certs::mongodb_client'] ~> Class['pulp', 'apache::service']. Both since pulp will still refresh the workers.

ehelms · 2020-06-30T02:51:14Z

This will need -- theforeman/puppet-certs#288

ehelms · 2020-06-30T14:21:03Z

@wbclark @ekohl ready for review now, not the prettiest but nothing with MongoDB ever is >.<

manifests/pulp.pp

ekohl · 2020-06-30T16:01:05Z

manifests/pulp.pp

+
+    Class['certs::mongodb_client'] ~> Class['pulp', 'apache::service']
+
+    $mongodb_seeds_real = "${facts['networking']['fqdn']}:27017"


Edit: Actually it should be using the hostname on the server cert:

Suggested change

$mongodb_seeds_real = "${facts['networking']['fqdn']}:27017"

$mongodb_seeds_real = "${certs::mongodb::hostname}:${mongodb::server::port}"

The definition needs to be moved below mongodb::server

They are 99.9% of the time going to be the same thing because this is a tightly controlled scenario. But I shall may the change.

The use of mongodb::server::port is incorrect here since port is undef -- https://github.com/voxpupuli/puppet-mongodb/blob/master/manifests/server.pp#L27

They are 99.9% of the time going to be the same thing because this is a tightly controlled scenario. But I shall may the change.

The important part is showing that these are linked. That's what variables are for. It gives understanding to the reader of the code.

The use of mongodb::server::port is incorrect here since port is undef -- https://github.com/voxpupuli/puppet-mongodb/blob/master/manifests/server.pp#L27

Ah right. Fair enough.

ekohl · 2020-07-01T09:47:52Z

manifests/pulp.pp

@@ -97,10 +104,16 @@
  Optional[Stdlib::Absolutepath] $https_cert = undef,
  Optional[Stdlib::Absolutepath] $https_key = undef,
  Optional[Stdlib::Absolutepath] $https_ca_cert = undef,
+  Boolean $expose_mongodb = false,
+  Boolean $expose_mongodb_ipv6 = false,


Why would this default to false? In which case are you not able to bind to IPv6? Note that in our install Redis already binds to ::1 so not having IPv6 is already a requirement. And if you go that far, why do we need a parameter for it?

I think you are saying we can default to exposing on IPv4 and IPv6 in all cases? Is there a case where IPv4 only is required? Or IPv6 only is required?

Indeed I am. I think IPv4-only would be required if the kernel has no IPv6 support but we don't support that case anymore. I'm not sure you can actually run the Linux kernel without IPv4 support so binding should always work.

Maybe I'm missing something, but I'd prefer to at least default to true. Another option is:

Variant[Boolean, Array[Stdlib::IP::Address]] $expose_mongodb = false,

Then true would imply dual stack. If the user needs full control over exact binding, you have it and only need a single parameter.

I set the default to true and left it there given this is designed for a specific use case and I think the parameters provide the needed flexibility.

… SSL

ekohl · 2020-07-08T12:10:46Z

manifests/pulp.pp

+    }
+
+    $mongodb_seeds_real = "${certs::mongodb::hostname}:27017"
+    $db_password = extlib::cache_data('foreman_cache_data', 'mongo_db_password', extlib::random_password(32))


We already have $mongodb_password as a parameter and IMH we should at least prefer that if specified.

That password is intended for the use case when a user is providing credentials for an external database. I don't think it should apply to this workflow.

I think it does. You're exposing the user with a certain password. The parameter allows the end user to specify something. That makes it easier on the consuming node to use the same password. Now the end user needs to look in files to see which password was generated.

More concrete: we have server-1.example.com which runs Katello with Pulp and MongoDB. The user runs foreman-installer --katello-pulp-mongodb-password mysecretpassword.

Now the user wants to consume this on server-2.example.com. The user runs foreman-installer --foreman-proxy-content-mongo-db-password mysecretpassword.

The current implementation doesn't support this because it's silently ignored.

My point, unless I am just missing something is, that nobody should be doing this other than our own tooling which is tightly controlled for a very specific upgrade scenario.

So how do you get the password now? Do you parse it out of a config file later on?

That will be the most likely tactic given it's the only reliable way to get the value. I think I get what you are saying now but not form the users perspective, but the if the parameter is used then it can be stored on the answers file. But I don't see how you set the parameter conditionally.

theforeman-bot added Needs testing Not yet reviewed labels May 11, 2020

ekohl requested changes May 11, 2020

View reviewed changes

manifests/pulp.pp Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels May 11, 2020

ehelms force-pushed the fixes-29715 branch from c83af1b to 1e50d7e Compare May 12, 2020 02:11

theforeman-bot added Needs re-review and removed Waiting on contributor labels May 12, 2020

ekohl reviewed May 12, 2020

View reviewed changes

manifests/pulp.pp Outdated Show resolved Hide resolved

manifests/pulp.pp Outdated Show resolved Hide resolved

manifests/pulp.pp Show resolved Hide resolved

manifests/pulp.pp Outdated Show resolved Hide resolved

ehelms force-pushed the fixes-29715 branch from 1e50d7e to 3b18b60 Compare June 3, 2020 21:07

ekohl reviewed Jun 4, 2020

View reviewed changes

manifests/pulp.pp Show resolved Hide resolved

ehelms force-pushed the fixes-29715 branch from 3b18b60 to 60de283 Compare June 5, 2020 12:56

ekohl reviewed Jun 5, 2020

View reviewed changes

manifests/init.pp Outdated Show resolved Hide resolved

manifests/init.pp Outdated Show resolved Hide resolved

ehelms force-pushed the fixes-29715 branch from 60de283 to bd79ad5 Compare June 5, 2020 14:22

ehelms commented Jun 5, 2020

View reviewed changes

manifests/pulp.pp Show resolved Hide resolved

ehelms force-pushed the fixes-29715 branch 2 times, most recently from 14c4929 to 7103bc2 Compare June 8, 2020 14:27

wbclark closed this Jun 8, 2020

wbclark reopened this Jun 8, 2020

wbclark reviewed Jun 8, 2020

View reviewed changes

spec/classes/pulp_spec.rb Outdated Show resolved Hide resolved

wbclark reviewed Jun 8, 2020

View reviewed changes

spec/classes/pulp_spec.rb Outdated Show resolved Hide resolved

wbclark reviewed Jun 8, 2020

View reviewed changes

manifests/pulp.pp Outdated Show resolved Hide resolved

ehelms force-pushed the fixes-29715 branch 2 times, most recently from b91de9d to 68f16cb Compare June 9, 2020 12:43

wbclark reviewed Jun 9, 2020

View reviewed changes

manifests/pulp.pp Show resolved Hide resolved

ehelms force-pushed the fixes-29715 branch from 68f16cb to a0a9d4d Compare June 9, 2020 14:17

wbclark closed this Jun 9, 2020

wbclark reopened this Jun 9, 2020

wbclark closed this Jun 9, 2020

wbclark reopened this Jun 9, 2020

wbclark reviewed Jun 19, 2020

View reviewed changes

ekohl reviewed Jun 22, 2020

View reviewed changes

ehelms force-pushed the fixes-29715 branch 2 times, most recently from 067dda7 to bf4cb42 Compare June 29, 2020 12:12

ehelms force-pushed the fixes-29715 branch 6 times, most recently from 44976df to 78191b7 Compare June 30, 2020 01:00

ehelms force-pushed the fixes-29715 branch 2 times, most recently from 18782b4 to 2654948 Compare June 30, 2020 13:46

ekohl reviewed Jun 30, 2020

View reviewed changes

manifests/pulp.pp Outdated Show resolved Hide resolved

manifests/pulp.pp Outdated Show resolved Hide resolved

manifests/pulp.pp Outdated Show resolved Hide resolved

manifests/pulp.pp Show resolved Hide resolved

ehelms force-pushed the fixes-29715 branch from 2654948 to dead03d Compare June 30, 2020 15:32

ekohl reviewed Jun 30, 2020

View reviewed changes

ehelms force-pushed the fixes-29715 branch 3 times, most recently from efbf688 to ca7e3e5 Compare June 30, 2020 23:49

ekohl reviewed Jul 1, 2020

View reviewed changes

ehelms force-pushed the fixes-29715 branch from ca7e3e5 to 50c26bb Compare July 1, 2020 12:59

Fixes #29715: Add option to expose mongodb on a public interface with…

f448c5d

… SSL

ehelms force-pushed the fixes-29715 branch from 50c26bb to f448c5d Compare July 1, 2020 13:00

ehelms requested review from ekohl and wbclark July 6, 2020 12:35

ekohl reviewed Jul 8, 2020

View reviewed changes

ehelms closed this Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #29715: Add option to expose mongodb on a public interface with… #340

Fixes #29715: Add option to expose mongodb on a public interface with… #340

ehelms commented May 11, 2020

wbclark commented Jun 19, 2020

wbclark commented Jun 19, 2020

wbclark Jun 19, 2020

ehelms Jun 22, 2020

ekohl Jun 22, 2020

ekohl Jun 22, 2020

ekohl Jun 22, 2020

ehelms commented Jun 30, 2020

ehelms commented Jun 30, 2020

ekohl Jun 30, 2020 •

edited

ehelms Jun 30, 2020

ehelms Jun 30, 2020

ekohl Jul 1, 2020

ekohl Jul 1, 2020

ehelms Jul 1, 2020

ekohl Jul 1, 2020

ehelms Jul 2, 2020

ekohl Jul 8, 2020

ehelms Jul 8, 2020

ekohl Jul 8, 2020

ehelms Jul 8, 2020

ekohl Jul 8, 2020

ehelms Jul 8, 2020

	Class['certs::mongodb_client'] -> Class['pulp']
	Class['certs::mongodb_client'] ~> Class['pulp']

	super().merge({ 'expose_mongodb_ipv6' => true, })
	super().merge(expose_mongodb_ipv6: true)


		Class['certs::mongodb_client'] ~> Class['pulp', 'apache::service']

		$mongodb_seeds_real = "${facts['networking']['fqdn']}:27017"

	$mongodb_seeds_real = "${facts['networking']['fqdn']}:27017"
	$mongodb_seeds_real = "${certs::mongodb::hostname}:${mongodb::server::port}"

Fixes #29715: Add option to expose mongodb on a public interface with… #340

Fixes #29715: Add option to expose mongodb on a public interface with… #340

Conversation

ehelms commented May 11, 2020

wbclark commented Jun 19, 2020

wbclark commented Jun 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Jun 30, 2020

ehelms commented Jun 30, 2020

ekohl Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl Jun 30, 2020 •

edited