Fixes #30316: Move bootstrap RPM generation from puppet-certs #272

ehelms · 2020-07-02T19:17:55Z

No description provided.

ehelms · 2020-07-02T19:18:35Z

After this goes in, theforeman/puppet-certs#290 can be merged to clean up but this changed does not require that change.

This is the first step in a large change to drop the dependence on katello-certs-tools tracked here https://projects.theforeman.org/issues/30315. This change is intended to be an exact copy first to allow future PR reviews to better track and test the changes being made rather than attempting a lot of change all at once.

You'll also notice that this chunk of code deals with two things: creation of the RPM and Apache directory management for the RPM to live (along with other things like boostrap.py).

manifests/bootstrap_rpm.pp

ekohl · 2020-07-07T12:28:20Z

manifests/bootstrap_rpm.pp

+# This file is placed in $katello_www_pub_dir.
+# @api private
+class foreman_proxy_content::bootstrap_rpm (
+  $hostname                          = $facts['networking']['fqdn'],


Can we use data types for the parameters to figure out what they are?

Also, I don't think we need to align parameters. I've stopped doing that because it quickly becomes too wide if you also add data types.

manifests/init.pp

manifests/rhsm_reconfigure_script.pp

ekohl · 2020-07-07T12:33:43Z

templates/bootstrap_rpm/rhsm-katello-reconfigure.erb

+KATELLO_SERVER_CA_CERT=<%= scope['certs::server_ca_name'] %>.pem
+KATELLO_DEFAULT_CA_CERT=<%= scope['certs::default_ca_name'] %>.pem
+KATELLO_CERT_DIR=<%= scope['certs::katello::rhsm_ca_dir'] %>


I'm not a fan of this linking because it can easily break if we start to refactor things. Any thoughts on switching to EPP and explicitly passing it as context? It's more verbose, but we'll notice easily if it does break.

ekohl · 2020-07-07T12:58:13Z

manifests/bootstrap_rpm.pp

+# This file is placed in $katello_www_pub_dir.
+# @api private
+class foreman_proxy_content::bootstrap_rpm (
+  String $hostname = $facts['networking']['fqdn'],


Suggested change

String $hostname = $facts['networking']['fqdn'],

Stdlib::Fqdn $hostname = $facts['networking']['fqdn'],

ekohl · 2020-07-07T12:59:23Z

manifests/bootstrap_rpm.pp

+  include trusted_ca
+  trusted_ca::ca { 'katello_server-host-cert':
+    source  => $katello_server_ca_cert,
+    require => File[$katello_server_ca_cert],
+  }


This is also a part I always wondered about: why does the server need to globally trust the server certificate? Don't we set the CA explicitly everywhere?

Looks like, based on https://projects.theforeman.org/issues/13634, we could drop it for the reasons stated it was added. However, there is currently known code that does rely on this we would need to fix. I was discussing that last week with @jlsherrill

Perhaps we can start with adding a class parameter Boolean $add_ca_as_trusted_ca = true so we can toggle it to false in nightly to see what breaks? Since it's a private class, users shouldn't see anything.

I know at least Pulp communication will likely break due to how that is currently coded. Let me compromise with creating a dedicated issue to explore this (https://projects.theforeman.org/issues/30338) so that this code is still as clean as can be a copy from puppet-certs. Then it can be a targeted follow up along with the bootstrap RPM being re-factored to drop katello-cert-tools as a stand alone fix.

I do agree our code should be robust against the CA certificate being in the system wide truststore but are there other reasons that we shouldn't do this?

The less invasive we are to the system, the easier. We can also drop the trusted_ca module from the installer. Right now I'm maintaining that in the Voxpupuli organization because we need it. That overhead might not be directly visible, but it's there.

I also don't know if there may be security policies on customer systems that would disallow this. Perhaps now they don't know so don't care, but might if they did.

Let's keep this and make it part of the "kill pulp 2"-PR

manifests/rhsm_reconfigure_script.pp

ekohl

Some bikeshedding and thought dump in the bigger picture.

Should we have a foreman_proxy_content::rhsm class? The script (currently define) could be foreman_proxy_content::rhsm::script or foreman_proxy_content::rhsm::reconfigure_script.

We're not completely there, but my thoughts are very much around what is the content host. There's https://github.com/theforeman/smart_proxy_pulp and I started https://github.com/ekohl/smart_proxy_rhsm but now I'm wondering if we shouldn't have smart_proxy_content that does both. Is there a reason you would ever deploy a host with Pulp but without the RHSM part? The other way around: RHSM but not Pulp? If we get this thinking right, we can set the goals and design things in a future proof way.

ekohl · 2020-07-07T15:59:46Z

manifests/bootstrap_rpm.pp

+    mode    => '0644',
+    require => File[$katello_server_ca_cert],
+  } ~>
+  foreman_proxy_content::rhsm_reconfigure_script { "${katello_www_pub_dir}/${katello_rhsm_setup_script}":


Does it make sense to inline the script instead of having a separate define? If you keep it separate, it might as well be a class instead of a define.

This define also should have a dependency on the certs class

Maybe? But I am trying not to over think redesigning this due to how much this is going to change when it gets re-factored away from katello-certs-tools under the hood and towards both a simpler puppet layer and using RPM directly. I know that's hard, but instead of big bang, I'm trying to find the baby steps I can take to allow easier review while maintaining functionality

ehelms · 2020-07-07T16:34:35Z

The most realistic deployment is that the reverse proxy + RHSM endpoint will always be collocated with Pulp deployment. Now, if we talk possible deployments, subscription-manager does support configuring its RHSM API endpoint differently than where the content lives. In other words, you can point at two different hostnames.

There is also the evolving of this general concept into being able to do simple registration (through a simple bash script most likely fetched from the Foreman itself) that should proxy through the external Foreman node (whether through smart proxy an Apache reverse proxy remains to be seen) back to the Foreman to isolate traffic similar to how we do RHSM today. If using a Red Hat based system, then RHSM end point should be deployed with local content.

@sbernard had a use case for pulling content through the reverse proxy from the main Foreman server which would imply RHSM endpoint but no Pulp -- theforeman/puppet-pulp#372

So I'd say there are use cases for having pieces that we compose into the 90% case and could build into these use cases but that we don't over engineer it and risk missing the release deadlines so that can we make good, reasonable changes for most folks and set ourselves up to iterate quickly going forward.

ekohl · 2020-07-08T10:04:15Z

It sounds to me like we could go with a single content plugin but have explicit capabilities that Katello should check for.

The first one is Pulp, which will bring in its own capabilities for each content type. If the Pulp capability is present, it should expose the setting pulpcore_api_url and pulpcore_content_url as valid URLs. Without the capability, the setting may be null and no assumptions about it should be made.

The other capability is RHSM, but I don't have a good grasp on what it'd need. It feels like today it's only used by Katello to tell the client where to get the registration RPM. Within this RPM the actual RHSM endpoint lives and the client uses this. It feels like we should expose both these URLs as settings. While Katello doesn't use the real RHSM endpoint today, it can be useful. Registration can be implemented with templates inside Foreman and a REX job can update the RHSM endpoints if it changed. All without having to guess. It'll also be possible to run RHSM on a different hostname than foreman-proxy.

Big question I have is whether it makes sense to combine both in a single Smart Proxy plugin or rather keep them as separate plugins and separate Smart Proxy Features.

Thinking about this, it feels like separate plugins is easier though.

I'm wondering what @jlsherrill thinks about this.

ehelms · 2020-07-09T20:29:24Z

I think separate plugins makes sense and I would not invest in the RHSM one too much just yet as there is work on going to re-think the registration process and mechanisms (an RFC will be coming that goes into more detail from my understanding) that might influence the direction that plugin should go.

Do you consider the, really good, design discussions here relevant to the specific change in this PR? Once this change is in, I would like to start re-factoring the underlying mechanisms and cleaning this up.

ekohl

No, I think we don't need to do it as a final version. We can merge it and iterate further. Having it in this module with tests does allow easier iteration.

ehelms · 2020-07-10T13:49:13Z

Now with acceptance tests

theforeman-bot added Needs testing Not yet reviewed labels Jul 2, 2020

ehelms mentioned this pull request Jul 2, 2020

Refs #30316: Drop bootstrap RPM code theforeman/puppet-certs#290

Merged

ehelms force-pushed the fixes-30316 branch from ef5acd8 to 53225e8 Compare July 2, 2020 19:27

ekohl mentioned this pull request Jul 7, 2020

Remove Pulp 2 functionality #271

Closed

ekohl reviewed Jul 7, 2020

View reviewed changes

ehelms force-pushed the fixes-30316 branch 2 times, most recently from 27379eb to 2a5cbf8 Compare July 7, 2020 12:54

ekohl reviewed Jul 7, 2020

View reviewed changes

ehelms mentioned this pull request Jul 7, 2020

Fixes #30312: Drop docker, atomic and goferd support from consumer RPM theforeman/puppet-certs#289

Merged

ehelms force-pushed the fixes-30316 branch from 2a5cbf8 to 55f5016 Compare July 7, 2020 15:48

ekohl reviewed Jul 7, 2020

View reviewed changes

manifests/rhsm_reconfigure_script.pp Outdated Show resolved Hide resolved

ehelms force-pushed the fixes-30316 branch from 55f5016 to 672af22 Compare July 7, 2020 16:00

ekohl reviewed Jul 7, 2020

View reviewed changes

ehelms force-pushed the fixes-30316 branch 2 times, most recently from 3c2b28d to c649cc4 Compare July 10, 2020 12:14

ekohl approved these changes Jul 10, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jul 10, 2020

Fixes #30316: Move bootstrap RPM generation from puppet-certs

dcf1a95

ehelms force-pushed the fixes-30316 branch from c649cc4 to dcf1a95 Compare July 10, 2020 13:47

ehelms merged commit 7bf101d into theforeman:master Jul 10, 2020

wbclark added Backwards incompatible and removed Needs testing labels Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30316: Move bootstrap RPM generation from puppet-certs #272

Fixes #30316: Move bootstrap RPM generation from puppet-certs #272

ehelms commented Jul 2, 2020

ehelms commented Jul 2, 2020 •

edited

Loading

ekohl Jul 7, 2020

ehelms Jul 7, 2020

ekohl Jul 7, 2020

ekohl Jul 7, 2020

ekohl Jul 7, 2020

ehelms Jul 7, 2020

ekohl Jul 7, 2020

ehelms Jul 7, 2020

ekohl Jul 7, 2020

ekohl left a comment

ekohl Jul 7, 2020

ehelms Jul 7, 2020

ehelms commented Jul 7, 2020

ekohl commented Jul 8, 2020

ehelms commented Jul 9, 2020

ekohl left a comment

ehelms commented Jul 10, 2020

	String $hostname = $facts['networking']['fqdn'],
	Stdlib::Fqdn $hostname = $facts['networking']['fqdn'],

Fixes #30316: Move bootstrap RPM generation from puppet-certs #272

Fixes #30316: Move bootstrap RPM generation from puppet-certs #272

Conversation

ehelms commented Jul 2, 2020

ehelms commented Jul 2, 2020 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Jul 7, 2020

ekohl commented Jul 8, 2020

ehelms commented Jul 9, 2020

ekohl left a comment

Choose a reason for hiding this comment

ehelms commented Jul 10, 2020

ehelms commented Jul 2, 2020 •

edited

Loading