Fixes #34026 - authorize puppet reports via Proxy by lzap · Pull Request #8958 · theforeman/foreman

lzap · 2021-11-29T12:34:19Z

The patch #5010 changed how fact importer features are registered, the change was made in the host controller but not in the API controller. Therefore uploading reports via host controller works fine, however, using the API endpoint does not authorize automatically as the feature is not found.

theforeman-bot · 2021-11-29T12:34:29Z

Issues: #34026

lzap · 2021-11-30T12:02:16Z

The reporter says it did NOT help: https://community.theforeman.org/t/no-smart-proxy-server-found-on-foreman-puppet-example-com-and-is-not-in-trusted-hosts/26215

Any clue? @ShimShtein and @tbrisker ?

tbrisker · 2021-12-02T09:47:32Z

app/controllers/api/v2/config_reports_controller.rb

      before_action :compatibility, :only => :create

-      add_smart_proxy_filters :create, :features => proc { ConfigReportImporter.authorized_smart_proxy_features }
+      add_smart_proxy_filters :facts, :features => proc { Foreman::Plugin.fact_importer_registry.fact_features }


the endpoint here is called :create, not :facts. Also i'm not sure if there are plugins that import reports but don't have a fact importer, especially now that all fact importers are in core?

Oh copy and paste error. Good find.

I do not understand why this endpoint is used, normally /v2/host/xxx/facts should be used. But it should save the same purpose I guess and the change I refer to should should change both.

lzap · 2021-12-03T11:50:10Z

I asked gvde to try again, it could not work since I had the typo there.

gvde · 2021-12-03T13:34:30Z

I asked gvde to try again, it could not work since I had the typo there.

Yes. Works for me now. Applied the patch, then restarted

# foreman-maintain service stop
# foreman-maintain service stop

Config reports still coming in from my puppet only smart proxy...

ekohl · 2021-12-06T11:15:15Z

app/controllers/api/v2/config_reports_controller.rb

      before_action :compatibility, :only => :create

-      add_smart_proxy_filters :create, :features => proc { ConfigReportImporter.authorized_smart_proxy_features }
+      add_smart_proxy_filters :create, :features => proc { Foreman::Plugin.fact_importer_registry.fact_features }


Why do we use fact features to allow config reports?

Right, I accidentally mis-read the #5010 patch I thought it was about reports while it was about facts. This is wrong, need to dig deeper.

lzap · 2021-12-06T11:45:16Z

DO NOT MERGE

lzap · 2021-12-06T11:46:09Z

This appears like some issue with class overriding after we pulled all report importers into core. Without Ansible:

[1] pry(main)> ConfigReportImporter.authorized_smart_proxy_features
=> ["Puppet"]

With Ansible plugin:

[4] pry(main)> ConfigReportImporter.authorized_smart_proxy_features
=> ["Ansible"]

ekohl · 2021-12-06T11:57:53Z

DO NOT MERGE

I've marked it as a draft to make that clear. You can do the same thing: there's a well hidden link in the Reviewers block on the top right.

lzap · 2021-12-06T12:29:54Z

Thanks, I figured it out. The problem was that Ansible plugin simply includes module into ConfigReportImporter and overrides the authorized_smart_proxy_features in a way that it would never return what was previously defined there. A registry similar to facts could be a good solution, however, since we are currently rewriting reports completely and they will all reside in a new Foreman plugin, implementing registry is overkill since we are throwing away the code soon.

So the fix is to remove overriding of the authorized_smart_proxy_features method and start relying on the ReportImporter.register_smart_proxy_feature. It looks like we have introduced this kind of registry, the problem is only chef plugin is using that:

foreman_chef/lib/foreman_chef/engine.rb
92:      ::ConfigReportImporter.register_smart_proxy_feature('Chef')

Puppet and Ansible are both overriding the authorized_smart_proxy_features instead. There is actually another problem, if plugins use ConfigReportImporter.register_smart_proxy_feature instead of ReportImporter.register_smart_proxy_feature it won't work as ReportImporter and ConfigReportImporter are two separate classes with two separate class variables for the array holding the registry.

This have never worked, the moment you installed either Chef or Ansible (Salt does appear to implement reports differently), Puppet feature was always removed from the list. Weird that we only got this reported recently, looks like not many people are using more than one configuration management. Or I dunno why we see it now.

So with this patch in core, the following changes needs to be done in order to fully fix this:

Ansible: remove overriding of authorized_smart_proxy_features and use ReportImporter.register_smart_proxy_feature("Ansible") instead
Chef: use ReportImporter.register_smart_proxy_feature("Chef") instead of ConfigReportImporter.register_smart_proxy_feature("Chef")
Salt: should work as the whole importer is reimplemented from scratch there

ekohl · 2021-12-06T12:33:02Z

It sounds like we have a regression and shortest term fix is to use ConfigReportImporter.register_smart_proxy_feature in Ansible now. Then the follow up patch would move things over to ReportImporter, correct?

lzap · 2021-12-06T12:34:48Z

It sounds like we have a regression and shortest term fix is to use ConfigReportImporter.register_smart_proxy_feature in Ansible now. Then the follow up patch would move things over to ReportImporter, correct?

Yes but I prefer to change Chef to use ReportImporter instead since the registry (the class variable) is actually defined there and not in the child class. Here is the patch for Ansible: theforeman/foreman_ansible#491

Going to do Chef one as well.

lzap · 2021-12-06T12:39:37Z

Here is the chef PR: theforeman/foreman_chef#98

I am going to deprecate the ConfigReportImporter register method just in case there is something still utilizing it.

ekohl · 2021-12-06T12:45:43Z

I was mostly concerned with cherry picks and compatibility. I haven't looked at the exact details since I just came back from PTO.

lzap · 2021-12-06T12:49:05Z

Hmm I could actually call class methods from one of the classes so it is always stored just once:

  def self.authorized_smart_proxy_features
    ReportImporter.authorized_smart_proxy_features
  end

  def self.register_smart_proxy_feature(feature)
    ReportImporter.register_smart_proxy_feature(feature)
  end

  def self.unregister_smart_proxy_feature(feature)
    ReportImporter.unregister_smart_proxy_feature(feature)
  end

This is backport-friendly, however, Ansible still needs a patch because currently it overrides the self.authorized_smart_proxy_features so it returns irrelevant result.

ekohl · 2021-12-06T12:54:59Z

app/services/config_report_importer.rb

  end
 end
+
+ReportImporter.register_smart_proxy_feature("Puppet")


Should this be in an initializer? I wonder how it would deal with code reloads.

It should behave fine, but it is ugly

I've fixed that manually

ekohl · 2021-12-06T12:56:02Z

app/services/config_report_importer.rb

+  def self.register_smart_proxy_feature(feature)
+    ReportImporter.register_smart_proxy_feature(feature)
+  end
+
+  def self.unregister_smart_proxy_feature(feature)
+    ReportImporter.unregister_smart_proxy_feature(feature)
  end


Perhaps I'm missing something, but ConfigReportImporter inherits from ReportImporter so isn't this redundant?

It is not actually, because the definition of the parent method means it's defined on class, so you'd define another array on ConfigReportImporter tho this issue would appear in any of the descendants. That is one of the reasons to not have registries in a parent classes.

@lzap could we deprecate these then and disable it's usage through ConfigReportImporter?

Ah right, that's why I dislike static methods with state.

ezr-ondrej · 2021-12-10T11:14:56Z

app/services/report_importer.rb


  def self.register_smart_proxy_feature(feature)
-    @authorized_smart_proxy_features = (authorized_smart_proxy_features + [feature]).uniq
+    @authorized_smart_proxy_features = (authorized_smart_proxy_features + [feature.freeze]).uniq


didn't you want to freeze the array instead? You're creating new one here, so it would be fine and it would not allow touching the array anywhere else, then through this method. But freezing the strings sounds weird, are we worried someone is changing the actual strings?

Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>

ezr-ondrej

I do not totally agree with this solution, but as a temporary workaround untill we have the new sollution for Reports, I'm ok 👍

ezr-ondrej · 2021-12-11T21:53:38Z

[test foreman][test katello]

ezr-ondrej · 2021-12-15T11:29:58Z

Thanks @lzap 👍

ezr-ondrej · 2021-12-15T16:12:36Z

Shall this be cherry-picked into 3.1?

loopway · 2022-01-10T15:39:12Z

Just run into the issue myself with 3.1. Would be nice if it could be included in the next release. Just a friendly reminder. :)

ezr-ondrej · 2022-01-10T20:58:14Z

CP 3.1: 24d9df7

I've checked and old Ansible code should work just fine with this (so hopefully this won't break 3.1 🤷 )
Thanks for the reminder @loopway! 👍

lzap · 2022-01-12T08:48:58Z

Thanks, sorry late on my GH inbox.

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

ezr-ondrej self-assigned this Dec 1, 2021

tbrisker reviewed Dec 2, 2021

View reviewed changes

lzap force-pushed the config-report-import-feat-34026 branch from f70778d to 310d21e Compare December 2, 2021 13:25

ekohl reviewed Dec 6, 2021

View reviewed changes

ekohl marked this pull request as draft December 6, 2021 11:56

lzap force-pushed the config-report-import-feat-34026 branch from 310d21e to 5b1e10e Compare December 6, 2021 12:19

lzap marked this pull request as ready for review December 6, 2021 12:19

lzap mentioned this pull request Dec 6, 2021

Fixes #34094 - use report importer registry theforeman/foreman_ansible#491

Merged

lzap mentioned this pull request Dec 6, 2021

Fixes #34095 - use report importer registry theforeman/foreman_chef#98

Open

lzap force-pushed the config-report-import-feat-34026 branch from 5b1e10e to 3ac7e06 Compare December 6, 2021 12:42

lzap force-pushed the config-report-import-feat-34026 branch from 3ac7e06 to 1712b92 Compare December 6, 2021 12:49

ekohl reviewed Dec 6, 2021

View reviewed changes

ezr-ondrej reviewed Dec 10, 2021

View reviewed changes

Fixes #34026 - authorize puppet reports via Proxy

67fbecc

Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>

ezr-ondrej force-pushed the config-report-import-feat-34026 branch from 1712b92 to 67fbecc Compare December 10, 2021 11:28

ezr-ondrej approved these changes Dec 10, 2021

View reviewed changes

theforeman-bot removed the Not yet reviewed label Dec 10, 2021

ezr-ondrej merged commit 9c94e1a into theforeman:develop Dec 15, 2021

lzap deleted the config-report-import-feat-34026 branch January 12, 2022 08:48

Conversation

lzap commented Nov 29, 2021

Uh oh!

theforeman-bot commented Nov 29, 2021

Uh oh!

lzap commented Nov 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzap commented Dec 3, 2021

Uh oh!

gvde commented Dec 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzap commented Dec 6, 2021

Uh oh!

lzap commented Dec 6, 2021

Uh oh!

ekohl commented Dec 6, 2021

Uh oh!

lzap commented Dec 6, 2021

Uh oh!

ekohl commented Dec 6, 2021

Uh oh!

lzap commented Dec 6, 2021

Uh oh!

lzap commented Dec 6, 2021

Uh oh!

ekohl commented Dec 6, 2021

Uh oh!

lzap commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej left a comment

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej commented Dec 11, 2021

Uh oh!

ezr-ondrej commented Dec 15, 2021

Uh oh!

ezr-ondrej commented Dec 15, 2021

Uh oh!

loopway commented Jan 10, 2022

Uh oh!

ezr-ondrej commented Jan 10, 2022

Uh oh!

lzap commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants