Fixes #31642 - Add container gateway support #643

ianballou · 2021-01-29T16:48:12Z

Adds support for the Container Gateway smart proxy plugin.

I'm pretty new to the foreman-installer so let me know any places where I'm breaking standards.

TODO:

Add tests
~~Add foreman-proxy-content answers file migration (Fixes #31642 - Add container gateway support foreman-installer#643)~~

ianballou · 2021-01-29T16:49:39Z

Related PR: theforeman/puppet-foreman_proxy_content#319

ekohl

It would be nice to have at least a minimal test. in spec/classes there are plenty of examples.

ekohl · 2021-01-29T16:55:17Z

manifests/plugin/container_gateway.pp

+  Stdlib::Absolutepath $pulp_client_ssl_key = $foreman_proxy::plugin::container_gateway::params::pulp_ssl_client_key,
+  Stdlib::Absolutepath $sqlite_db_path = $foreman_proxy::plugin::container_gateway::params::sqlite_db_path,
+) inherits foreman_proxy::plugin::container_gateway::params {
+


Nit: this empty line is not needed and a lint plugin I intend to add will complain about it

Suggested change

Okay, it's removed.

ekohl · 2021-01-29T16:56:35Z

manifests/plugin/container_gateway.pp

+class foreman_proxy::plugin::container_gateway (
+  Optional[String] $version = $foreman_proxy::plugin::container_gateway::params::version,
+  Boolean $enabled = $foreman_proxy::plugin::container_gateway::params::enabled,
+  Foreman_proxy::ListenOn $listen_on = $foreman_proxy::plugin::container_gateway::params::listen_on,
+  Stdlib::HTTPUrl $pulp_endpoint = $foreman_proxy::plugin::container_gateway::params::pulp_endpoint,
+  Stdlib::Absolutepath $pulp_client_ssl_cert = $foreman_proxy::plugin::container_gateway::params::pulp_ssl_client_cert,
+  Stdlib::Absolutepath $pulp_client_ssl_key = $foreman_proxy::plugin::container_gateway::params::pulp_ssl_client_key,
+  Stdlib::Absolutepath $sqlite_db_path = $foreman_proxy::plugin::container_gateway::params::sqlite_db_path,


We try to add static values inline nowadays because it's easier to follow. Ideally we would do everything but Kafo has some issues parsing some defaults. So all but $pulp_endpoint can be declared here.

I'm a bit confused, so if $pulp_endpoint isn't declared here, where would it go? It looks like I'm doing pretty much the same thing as$pulp_url in the Pulp plugin for example.

So a bit more insight from my point. Now that I've seen the various PRs I changed my opinion a bit. If we're not going to expose it via Kafo, there really isn't any point in keeping params.pp.

There are 2 reasons to have params.pp:

You need logic (if/else/...) to determine the value, like OS-dependent defaults

You want to expose the class via Kafo and need something Kafo can't parse

Let's start with the last part. We expose the defaults via puppet-strings. This gives the raw Puppet code as a default. Then kafo_parsers has some very naive "parsing". This naive parsing can't deal with a lot of things, like "https://${facts['networking']['fqdn']}" but it can deal with $variable. Putting the default value in a params.pp is thus a workaround, but it works.

For the second part, you can also use data in modules which utilizes hiera, but we haven't really used that and Kafo doesn't understand it either so let's leave that out of scope now.

Alright, thanks for the explanation. I assumed at first that Kafo was something that was being used in the background rather than something I to purposefully use. I'll remove params.pp and move the defaults into container_gateway.pp.

ekohl · 2021-01-29T16:59:52Z

manifests/plugin/container_gateway/params.pp

+  $pulp_ssl_client_cert = '/etc/foreman-proxy/foreman_ssl_cert.pem'
+  $pulp_ssl_client_key  = '/etc/foreman-proxy/foreman_ssl_key.pem'


Isn't this the same as $foreman_proxy::foreman_ssl_cert and key? That also makes me think you should have a CA. It's a katelloism to rely on the system store to trust certificates while Foreman is more explicit.

Isn't this the same as $foreman_proxy::foreman_ssl_cert and key?

It is, I'll use $foreman_proxy::foreman_ssl_cert & key then.

We're using that cert and key to avoid handing out the Pulp 3 certs to every smart proxy.

That also makes me think you should have a CA.

I'll have to take a look at see what that entails installer-wise.

Is /etc/foreman-proxy/foreman_ssl_ca.pem the CA that you were talking about?

I'll make an issue on the container gateway to take a CA as a configurable parameter (https://projects.theforeman.org/issues/31759)

@ekohl is this something you'd consider crucial to get in for Katello 4.0?

ekohl · 2021-01-29T17:00:44Z

manifests/plugin/container_gateway.pp

+
+  foreman_proxy::plugin::module { 'container_gateway':
+    version       => $version,
+    template_path => 'foreman_proxy/plugin/container_gateway.yml.erb',


Isn't this the same as the default?

Yep, I removed it.

ianballou · 2021-01-29T17:13:37Z

It would be nice to have at least a minimal test. in spec/classes there are plenty of examples.

Will do, it's on my TODO list for this and the other related PR. Figured I'd get these in first to see if I'm on the right track standards-wise (it does work at least :) )

ekohl

Another thing to consider is to modify the plugin to support not passing SSL certs at all. For reference, you can read the core settings in code:
https://github.com/theforeman/smart-proxy/blob/758f197d4b452424d59149abb11429fd6ab50465/lib/proxy/request.rb#L71-L73

That means if the value changes, you don't need configuration management to keep them in sync. Less code is better.

ianballou · 2021-02-01T16:39:26Z

spec/classes/foreman_proxy__plugin__container_gateway_spec.rb

+            '---',
+            ':enabled: https',
+            ":pulp_endpoint: https://#{facts[:fqdn]}",
+            ':pulp_client_ssl_cert: /etc/foreman-proxy/foreman_ssl_cert.pem',


@ekohl is there a way to "mock" a variable from another class? It seems like $foreman_proxy::foreman_ssl_cert and key is undef for the spec tests.

I'm hitting:

Puppet::PreformattedError: Evaluation Error: Error while evaluating a Function Call, Class[Foreman_proxy::Plugin::Container_gateway]: parameter 'pulp_client_ssl_cert' expects a Stdlib::Absolutepath = Variant[Stdlib::Windowspath = Pattern[/\A(([a-zA-Z]:[\\\/])|([\\\/][\\\/][^\\\/]+[\\\/][^\\\/]+)|([\\\/][\\\/]\?[\\\/][^\\\/]+)).*\z/], Stdlib::Unixpath = Pattern[/\A\/([^\n\/\0]+\/*)*\z/]] value, got Undef parameter 'pulp_client_ssl_key' expects a Stdlib::Absolutepath = Variant[Stdlib::Windowspath = Pattern[/\A(([a-zA-Z]:[\\\/])|([\\\/][\\\/][^\\\/]+[\\\/][^\\\/]+)|([\\\/][\\\/]\?[\\\/][^\\\/]+)).*\z/], Stdlib::Unixpath = Pattern[/\A\/([^\n\/\0]+\/*)*\z/]] value, got Undef (line: 3, column: 1) on node cannolo.usersys.redhat.com

There is

let(:pre_condition) { 'include foreman_proxy' }

I do have that. I think it's reporting undef because of this:

puppet-foreman_proxy/manifests/params.pp

Line 172 in e4b7763

$foreman_ssl_cert = undef

Perhaps I can override the parameter itself?

Okay, looks like I figured it out. I updated the test in my latest push.

ianballou · 2021-02-01T17:40:34Z

Another thing to consider is to modify the plugin to support not passing SSL certs at all. For reference, you can read the core settings in code:
https://github.com/theforeman/smart-proxy/blob/758f197d4b452424d59149abb11429fd6ab50465/lib/proxy/request.rb#L71-L73

That means if the value changes, you don't need configuration management to keep them in sync. Less code is better.

This sounds like a good idea, I didn't think to use the Proxy::HttpRequest.

@ekohl I'm curious where you'd rank this new cert strategy on your priority list. Do we need this for the initial 4.0 release? Just want to make sure it's essential before I bump fixing the Container Gateway to the top of my priority list. I'm assuming the Container Gateway installer support should get in quite soon due to Katello 4.0 branching soon.

ianballou · 2021-02-01T22:32:58Z

I realized the installer is failing with my PRs because /usr/share/foreman-proxy/bundler.d/container_gateway.rb isn't being created. Is the installer supposed to be in charge of that?

ekohl · 2021-02-02T10:28:36Z

That's packaging's responsibility.

ianballou · 2021-02-02T17:33:27Z

I'm starting to fix the container gateway to enable default certs & ca cert configuration. It's a quick change, should be merged very soon. Just need to update these PRs after and make a quick packaging one.

ianballou · 2021-02-02T22:07:30Z

@ekohl I think I've addressed all of your concerns. The plugin has shrunk quite nicely.

ekohl · 2021-02-03T10:30:28Z

manifests/plugin/container_gateway.pp

+  foreman_proxy::plugin::module { 'container_gateway':
+    version   => $version,
+    enabled   => $enabled,
+    feature   => 'Container_Gateway',


Just to verify: the Katello plugin does recognize this feature? Setting it here means the installer checks if Foreman recognized the feature after installation and otherwise fails.

Katello does, yeah. We've already introduced the functionality of sending over the unauthenticated repo list at proxy sync time if the feature exists.

ianballou · 2021-02-03T15:33:38Z

I don't have merge access, so feel free to merge whenever.

ianballou · 2021-02-04T14:43:28Z

@ekohl is this all good to merge?

ekohl · 2021-02-04T14:56:56Z

Yes, I approved and then went to check something else but it looks like we don't really have acceptance tests for plugins in this module so I'll just merge it now.

theforeman-bot added Needs testing Not yet reviewed labels Jan 29, 2021

ianballou mentioned this pull request Jan 29, 2021

Fixes #31642 - Add container gateway support theforeman/puppet-foreman_proxy_content#319

Merged

1 task

ekohl requested changes Jan 29, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jan 29, 2021

ianballou force-pushed the 31642-container-gateway-support branch from 41e8d02 to fb30601 Compare January 29, 2021 19:59

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jan 29, 2021

ianballou mentioned this pull request Jan 29, 2021

Fixes #31642 - Add container gateway support theforeman/foreman-installer#643

Closed

ianballou force-pushed the 31642-container-gateway-support branch 2 times, most recently from 43f611f to 35bff0c Compare January 29, 2021 22:42

ekohl reviewed Jan 30, 2021

View reviewed changes

ianballou force-pushed the 31642-container-gateway-support branch 2 times, most recently from 81bc48b to b436677 Compare February 1, 2021 16:16

ianballou commented Feb 1, 2021

View reviewed changes

ianballou force-pushed the 31642-container-gateway-support branch 2 times, most recently from 57a62d2 to 7fb9b30 Compare February 1, 2021 17:27

ianballou marked this pull request as ready for review February 1, 2021 18:11

Fixes #31642 - Add container gateway support

f36cb6a

ianballou force-pushed the 31642-container-gateway-support branch from 7fb9b30 to f36cb6a Compare February 2, 2021 22:02

ianballou requested a review from ekohl February 2, 2021 22:29

ekohl approved these changes Feb 3, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Feb 3, 2021

ekohl added the Enhancement label Feb 3, 2021

ekohl reviewed Feb 3, 2021

View reviewed changes

ekohl merged commit 0891089 into theforeman:master Feb 4, 2021

ianballou deleted the 31642-container-gateway-support branch February 4, 2021 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #31642 - Add container gateway support #643

Fixes #31642 - Add container gateway support #643

ianballou commented Jan 29, 2021 •

edited

Loading

ianballou commented Jan 29, 2021

ekohl left a comment

ekohl Jan 29, 2021

ianballou Jan 29, 2021

ekohl Jan 29, 2021

ianballou Jan 29, 2021

ekohl Jan 30, 2021

ianballou Feb 1, 2021 •

edited

Loading

ekohl Jan 29, 2021

ianballou Jan 29, 2021

ianballou Jan 29, 2021

ianballou Jan 29, 2021 •

edited

Loading

ekohl Jan 29, 2021

ianballou Jan 29, 2021

ianballou commented Jan 29, 2021

ekohl left a comment

ianballou Feb 1, 2021

ekohl Feb 1, 2021

ianballou Feb 1, 2021

ianballou Feb 1, 2021

ianballou commented Feb 1, 2021 •

edited

Loading

ianballou commented Feb 1, 2021

ekohl commented Feb 2, 2021

ianballou commented Feb 2, 2021

ianballou commented Feb 2, 2021

ekohl Feb 3, 2021

ianballou Feb 3, 2021

ianballou commented Feb 3, 2021

ianballou commented Feb 4, 2021

ekohl commented Feb 4, 2021

		$pulp_ssl_client_cert = '/etc/foreman-proxy/foreman_ssl_cert.pem'
		$pulp_ssl_client_key = '/etc/foreman-proxy/foreman_ssl_key.pem'

Fixes #31642 - Add container gateway support #643

Fixes #31642 - Add container gateway support #643

Conversation

ianballou commented Jan 29, 2021 • edited Loading

ianballou commented Jan 29, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou Feb 1, 2021 • 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

ianballou Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Jan 29, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Feb 1, 2021 • edited Loading

ianballou commented Feb 1, 2021

ekohl commented Feb 2, 2021

ianballou commented Feb 2, 2021

ianballou commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Feb 3, 2021

ianballou commented Feb 4, 2021

ekohl commented Feb 4, 2021

ianballou commented Jan 29, 2021 •

edited

Loading

ianballou Feb 1, 2021 •

edited

Loading

ianballou Jan 29, 2021 •

edited

Loading

ianballou commented Feb 1, 2021 •

edited

Loading