Add parameter to enable iop-advisor-engine to rh_cloud plugin

[ehelms]

No description provided.

[ekohl]

Can we add an acceptance test?

[ekohl]

theforeman/foreman_rh_cloud#932 introduces settings. IMHO this should create a /etc/foreman/settings.plugins.d/${plugin}.yaml file with that which can be done easily with the existing config parameter. The following keeps it simple and avoids a template by using stdlib::to_yaml:

Suggested change

	foreman::plugin { 'rh_cloud':
	$config = {
	':foreman_rh_cloud' => {
	':use_local_advisor_engine' => $enable_iop_advisor_engine,
	},
	}
	foreman::plugin { 'rh_cloud':
	config => stdlib::to_yaml($config),

I'm not 100% sure on the use of symbols vs strings though. That could break.

[evgeni]

# cat /etc/foreman/plugins/foreman_rh_cloud.yaml 
---
":foreman_rh_cloud":
  ":use_local_advisor_engine": true

# foreman-rake console
Loading production environment (Rails 7.0.8.7)
irb(main):001:0> SETTINGS.dig(:foreman_rh_cloud, :use_local_advisor_engine)
=> nil

😡

Why are we using symbols again?

[ekohl]

Because it makes life interesting? I wonder if we should enhance https://github.com/theforeman/foreman/blob/9e5da95e3194f37b71f013794835fd1d0f9a4a30/config/settings.rb#L40 to symbolize keys.

[evgeni]

maybe… but I am not shooting at that can of worms today.

[evgeni]

Can we add an acceptance test?

rh_cloud needs Katello, so an acceptance test seems complicated to me?

[ekohl]

rh_cloud needs Katello, so an acceptance test seems complicated to me?

If it's only about installing the package, it may be OK?+ I'd think we can still do a DB migration and start the service without candlepin present since that sounds similar to what we do in CI.

We already have some code to set up the repo:

puppet-foreman/spec/acceptance/foreman_cli_plugins_spec.rb

Lines 66 to 69 in dbfdc94

	yumrepo { 'katello':
	baseurl => "http://yum.theforeman.org/katello/nightly/katello/el${facts['os']['release']['major']}/x86_64/",
	gpgcheck => 0,
	}

[ekohl]

Why do we need a foreman-proxy in puppet-foreman? That feels like a horrible violation of principles where Foreman should never read files from foreman-proxy. Is this because of certs?

[evgeni]

Yepp, advisor-engine uses the proxy cert to auth against Foreman, and that lives in /etc/foreman-proxy (and gets read from there into a podman secret).
We probably can make puppet-certs deploy a second copy into /etc/iop-advisor-engine to avoid that, but today it's not happening.

[ekohl]

I already mentioned before that I feel advisor engine belongs closer to a Smart Proxy because that's where all our external integration lives. We really need to figure out external services in a consistent way, especially if we expect more services to be added. Perhaps our Smart Proxy model isn't scalable, but I've made a mental note we need to discuss this. For now a note above it in the acceptance test is a "good enough" to allow us to proceed?

[evgeni]

Yeah, we're abusing a bit of the SP architecture, even tho I still think it's not actually a SP thing (e.g. you can't have multiple of it, which breaks the SP paradigm).

I'll add a note to the test, yes.

[evgeni]

Not beautiful, but works :)

[ekohl]

I don't kind merging this. IMHO the 2 issues are structural and not limited to this PR.

I'm hopeful that if we fix the service discovery issue properly we don't even need the config file and the symbols in YAML issue disappears