The Road to Making Puppet Optional

ekohl · April 8, 2020, 5:19pm

Correct, but I’m not sure if it’s implemented today and must be verified.

TimoGoebel · April 9, 2020, 7:08am

Not with our current implementation. This could be fixed (if it needs fixing) by proxying the requests via smart-proxy as we’ve already discussed.

It works as I’ve used it for foreman_omaha. If you want to verify it yourself:

github.com

theforeman/foreman/blob/bad5f878b6fb275dbc7267930bcd0cb589d9aa66/app/controllers/api/v2/hosts_controller.rb#L18


      
          include ParameterAttributes
          
          wrap_parameters :host, :include => host_params_filter.accessible_attributes(parameter_filter_context) + ['compute_attributes']
          include HostsControllerExtension
          
          before_action :find_optional_nested_object, :except => [:facts]
          before_action :find_resource, :except => [:index, :create, :facts]
          check_permissions_for %w{power boot}
          before_action :process_parameter_attributes, :only => %w{update}
          
          add_smart_proxy_filters :facts, :features => proc { Foreman::Plugin.fact_importer_registry.fact_features }
          
          add_scope_for(:index) do |base_scope|
            base_scope.preload([:host_statuses, :compute_resource, :hostgroup, :operatingsystem,
                                :interfaces, :token, :owner, :model, :environment, :location,
                                :organization, :image, :compute_profile, :realm, :architecture,
                                :ptable, :medium, :puppet_proxy, :puppet_ca_proxy])
          end
          
          api :GET, "/hosts/", N_("List all hosts")
          api :GET, "/hostgroups/:hostgroup_id/hosts", N_("List all hosts for a host group")

github.com

theforeman/foreman/blob/bad5f878b6fb275dbc7267930bcd0cb589d9aa66/app/registries/foreman/plugin/fact_importer_registry.rb#L20


      
                def register(key, klass, default = false)
                  @importers.default = klass.to_s if default
                  @importers[key.to_sym] = klass.to_s
                end
          
                def get(key)
                  @importers[key].constantize
                end
          
                def fact_features
                  @importers.values.map { |importer| importer.constantize.authorized_smart_proxy_features }.compact.flatten.uniq
                end
              end
            end
          end

github.com

theforeman/foreman_ansible/blob/19d755b9404f8b44c903a3d75cee2f7acf30c070/app/services/foreman_ansible/structured_fact_importer.rb#L11-L13


      
          def self.authorized_smart_proxy_features
            'Ansible'
          end

nadjaheitmann · May 28, 2021, 1:09pm

Hi everyone,
I have been working on Hammer and the V2 API of the new Foreman Puppet plugin, and I am looking for a more concrete direction to go:

Users of Puppet should not see a deterioration in their workflows - ideally, some workflows would even see an improvement.

Taking this quote from @tbrisker, I would assume that the main objective is to keep the new Puppet Plugin API as similar to the old one as possible. Ideally, it has exactly the same interface and the user will never notice a difference.

Advantages:

No changes for Hammer-related interface
No changes for FAM
No changes for Foreman users that use the API directly.

Disadvantages:

Implementation is a little bit messy for Foreman 2.X as functionality from Foreman core needs to be overwritten by the plugin.
Changes to the API might bring advantages for the new hammer-cli-foreman-puppet plugin.
We might miss a chance to clean up the API (in case this is necessary).

As of now, the implementation aims to go somewhere in between by prepending the current API commands with foreman_puppet, e.g. foreman_puppet/api/.... This is done in other Foreman plugins as well. As consequence, people using the API directly are the ones eventually “suffering”.

Please correct me if anything I wrote is not correct.

So the remaining question is:

Continue with the current implementation?
Move towards a complete copy of the current Foreman core API?
Add any other changes to the new API?

Thank you for your thoughts!

tbrisker · June 1, 2021, 11:56am

Thanks for raising the concern @nadjaheitmann !
Currently, the new plugin still supports the old API endpoints as well, only logging a deprecation warning when users use them instead of the namespaced ones:

github.com

theforeman/foreman_puppet/blob/master/app/controllers/foreman_puppet/api/v2/base_controller.rb#L10-L23


  before_action :show_deprecation_for_core_routes
  protected
  def show_deprecation_for_core_routes
    return if request.path.starts_with?('/foreman_puppet')
    Foreman::Deprecation.api_deprecation_warning(
      format(
        '/api/v2/%{controller} API endpoints are deprecated, please use /foreman_puppet/api/v2/%{controller} instead',
        controller: controller_name
      )
    )
  end
end

This means that users will have time to update any scripts or tools to use the new endpoints instead of the old ones and will not have a breaking change until we actually drop support for the non-namespaced endpoints.
We can certainly consider making changes to the API in the future if we feel it is needed, but I would suggest to avoid any breaking changes right now as the move to the plugin is a big enough change already.

cuky · August 27, 2021, 1:01pm

I was already hitting issues with these smart varibles and them over riding anything in hiera. Is the hostgroup way of selecting classes staying?

We have started to move away from setting hostgroup varibles via foreman and redesigned the Hiera a little have an extra lyer for hostgroups

extract here , this is getting hostgroup fact, will this still be around in the newer versions.

" - name: “Hostgroup level settings/overrides YAML”
data_hash: yaml_data
paths:
- “environment/%{::environment}/roles/%{::hostgroup}.yaml”

We put this just before any node over ride and after env , default

I test and build foreman a lot with my dev platform, we also use it in prod rather extensivily.

steve