[CP 1.20] #25285 - proposed cherry-picks for additional media support

app/models/operatingsystem.rb

@@ -79,6 +79,10 @@ def self.title_name
     "title".freeze
   end
 
+  def additional_media(medium_provider)
+    medium_provider.additional_media.map(&:with_indifferent_access)
+  end
+
   def self.inherited(child)
     child.instance_eval do
       # Ensure all subclasses behave in the same way as the parent, and remain
@@ -115,6 +119,11 @@ def self.families_as_collection
     end
   end
 
+  # DEPRECATED - This will be removed in 1.22.
+  #
+  # This method is deprecated in favor of the additional media features of
+  # medium providers.
+  #
   # Operating system family can override this method to provide an array of
   # hashes, each describing a repository. For example, to describe a yum repo,
   # the following structure can be returned by the method:

app/services/medium_providers/provider.rb

@@ -6,6 +6,8 @@ module MediumProviders
   # provider.medium_uri
   # => #<URI::HTTP http://mirror.centos.org/centos/7/os/x86_64>
   class Provider
+    delegate :logger, :to => :Rails
+
     # Provides a friendly name of the provider in case of provider error.
     def self.friendly_name
       self.name
@@ -24,6 +26,21 @@ def medium_uri
       throw "medium_uri is not implemented for #{self.class.name}"
     end
 
+    # A medium provider can optionally return an array of hashes for additional
+    # software repos to enable during installation, if the template supports
+    # it.  The default implemenation looks at host parameters. The hash keys:
+    #
+    #   name:    Repo name, no spaces
+    #   comment: Repo comment
+    #   url:     Repo URL
+    #   gpgkey:  GPG key URL
+    #   install: Install repo on system for after boot
+    #
+    def additional_media
+      return [] unless entity.respond_to?(:host_param) && (media = entity.host_param('additional_media'))
+      parse_media(media) || []
+    end
+
     # Returns unique string representing current installation medium.
     def unique_id
       @unique_id ||= Base64.urlsafe_encode64(Digest::SHA1.digest(medium_uri.to_s), padding: false)
@@ -49,6 +66,26 @@ def validate
 
     private
 
+    def parse_media(media)
+      media = JSON.parse(media)
+      if media.is_a?(Array)
+        media.reject { |medium| is_invalid_hash(medium) }
+      else
+        logger.error("Expected #{entity.name} additional_media parameter to be an array.")
+        []
+      end
+    rescue JSON::ParserError => e
+      Foreman::Logging.exception("JSON parsing error on #{entity.name}'s additional_media parameter.", e)
+      []
+    end
+
+    def is_invalid_hash(medium)
+      return false unless medium['name'].blank? || medium['url'].blank?
+      logger.error("Medium #{medium} missing name.") if medium['name'].blank?
+      logger.error("Medium #{medium} missing URL.") if medium['url'].blank?
+      true
+    end
+
     attr_reader :entity
   end
 end

config/as_deprecation_whitelist.yaml

@@ -18,3 +18,7 @@
 # https://projects.theforeman.org/issues/23300
 - message: 'Dangerous query method (method whose arguments are used as raw SQL) called
     with non-attribute argument(s):'
+
+# https://projects.theforeman.org/issues/25285
+- message: 'You are using a deprecated behavior, it will be removed in version 1.22,
+    `repos` is deprecated in favor of providing additional media through a Medium Provider.'

lib/foreman/renderer/configuration.rb

@@ -39,6 +39,7 @@ class Configuration
       ]
 
       DEFAULT_ALLOWED_VARIABLES = [
+        :additional_media,
         :arch,
         :dynamic,
         :host,

lib/foreman/renderer/scope/variables/base.rb

@@ -11,7 +11,7 @@ def self.included(base)
 
           delegate :diskLayout, :disk_layout_source, :medium, :architecture, :ptable, :use_image, :arch,
                    :image_file, :default_image_file, to: :host, allow_nil: true
-          delegate :mediumpath, :supports_image, :major, :repos, :preseed_path, :preseed_server,
+          delegate :mediumpath, :additional_media, :supports_image, :major, :repos, :preseed_path, :preseed_server,
                    :xen, :kernel, :initrd, to: :operatingsystem, allow_nil: true
           delegate :name, to: :architecture, allow_nil: true, prefix: true
           delegate :content, to: :disk_layout_source, allow_nil: true, prefix: true
@@ -30,6 +30,7 @@ def load_variables_base
               send "#{operatingsystem.pxe_type}_attributes"
               pxe_config
             end
+            @additional_media = @medium_provider.nil? ? [] : additional_media(@medium_provider)
             @provisioning_type = host.is_a?(Hostgroup) ? 'hostgroup' : 'host'
             @static = !params[:static].empty?
             @template_url = params['url']

test/factories/host_related.rb

@@ -262,6 +262,18 @@ def set_environment_taxonomies(host_or_hostgroup, environment = host_or_hostgrou
       association :operatingsystem, :with_associations
     end
 
+    trait :debian do
+      operatingsystem { FactoryBot.build(:debian7_0, :with_associations) }
+    end
+
+    trait :suse do
+      operatingsystem { FactoryBot.build(:suse, :with_associations) }
+    end
+
+    trait :redhat do
+      operatingsystem { FactoryBot.build(:rhel7_5, :with_associations) }
+    end
+
     trait :with_ipv6 do
       subnet6 do
         overrides = {:dns => FactoryBot.create(:dns_smart_proxy)}

test/factories/operatingsystem.rb

@@ -98,6 +98,14 @@
       title { 'OpenSuse 11.4' }
     end
 
+    factory :rhel7_5, class: Redhat do
+      sequence(:name) { 'Red Hat Enterprise Linux' }
+      major { '7' }
+      minor { '5' }
+      type { 'Redhat' }
+      title { 'Red Hat Enterprise Linux 7.5' }
+    end
+
     factory :solaris, class: Solaris do
       sequence(:name) { 'Solaris' }
       major { '10' }

test/models/operatingsystem_test.rb

@@ -276,6 +276,17 @@ class OperatingsystemTest < ActiveSupport::TestCase
     assert_equal "Grub UEFI", os.preferred_loader
   end
 
+  test "additional_media returns media from medium provider" do
+    os = FactoryBot.create(:operatingsystem, :with_associations, :with_pxelinux)
+    additional_media = [{name: 'EPEL', url: 'http://yum.example.com/epel'}]
+    MediumProviders::Default.any_instance.stubs(:additional_media).returns(additional_media)
+    provider = MediumProviders::Default.new(FactoryBot.build(:host))
+
+    os_media = os.additional_media(provider)
+    assert_instance_of HashWithIndifferentAccess, os_media.first
+    assert_equal os_media, additional_media.map(&:with_indifferent_access)
+  end
+
   context 'os default templates' do
     setup do
       @template_kind = FactoryBot.create(:template_kind)

test/unit/foreman/renderer/scope/variables/base_test.rb

@@ -2,13 +2,8 @@
 
 class BaseVariablesTest < ActiveSupport::TestCase
   setup do
-    template = OpenStruct.new(
-      name: 'Test',
-      template: 'Test'
-    )
-    @source = Foreman::Renderer::Source::Database.new(
-      template
-    )
+    template = OpenStruct.new(name: 'Test', template: 'Test')
+    @source = Foreman::Renderer::Source::Database.new(template)
     @scope = Class.new(Foreman::Renderer::Scope::Base) do
       include Foreman::Renderer::Scope::Variables::Base
     end
@@ -25,18 +20,22 @@ class BaseVariablesTest < ActiveSupport::TestCase
     end
 
     test "set @preseed_server and @preseed_path if @host has medium and os" do
-      host = FactoryBot.build_stubbed(:host, :managed)
-      architecture = FactoryBot.build_stubbed(:architecture)
-      medium = FactoryBot.build_stubbed(:medium, :path => 'http://my-example.com/my_path')
-      os = FactoryBot.build_stubbed(:debian7_0, :media => [ medium ], :architectures => [architecture])
-      host.architecture = architecture
-      host.operatingsystem = os
-      host.medium = medium
-
+      host = FactoryBot.build_stubbed(:host, :managed, :debian)
+      uri = URI(host.operatingsystem.media.first.path)
       scope = @scope.new(host: host, source: @source)
 
-      assert_equal scope.instance_variable_get('@preseed_path'), '/my_path'
-      assert_equal scope.instance_variable_get('@preseed_server'), 'my-example.com:80'
+      assert_equal scope.instance_variable_get('@preseed_path'), uri.path
+      assert_equal scope.instance_variable_get('@preseed_server'), uri.select(:host, :port).join(':')
+    end
+
+    test "sets @additional_media from medium provider" do
+      additional_media = [{name: 'SaltStack', url: 'http://ppa.launchpad.net/saltstack/salt/ubuntu', gpgkey: 'http://keyserver.ubuntu.com/pks/lookup?op=get&search=0x4759FA960E27C0A6'}]
+
+      host = FactoryBot.build_stubbed(:host, :managed, :debian)
+      MediumProviders::Default.any_instance.stubs(:additional_media).returns(additional_media)
+
+      scope = @scope.new(host: host, source: @source)
+      assert_equal additional_media.map(&:with_indifferent_access), scope.instance_variable_get('@additional_media')
     end
   end
 
@@ -49,6 +48,72 @@ class BaseVariablesTest < ActiveSupport::TestCase
       assert_nil scope.instance_variable_get('@mediapath')
     end
 
+    describe "@dynamic" do
+      before do
+        @ptable_template = "%#\n" \
+                           "kind: ptable\n" \
+                           "name: test template\n" \
+                           "oses:\n" \
+                           "- SLES 12\n" \
+                           "%>\n" \
+                           "#Dynamic\n"
+
+        ptable = FactoryBot.create(:ptable, template: @ptable_template, operatingsystem_ids: [operatingsystems(:redhat).id])
+        @host = FactoryBot.create(:host, :managed, ptable: ptable, operatingsystem: operatingsystems(:redhat))
+      end
+
+      test "is true when '#dynamic' is present in the template" do
+        scope = @scope.new(host: @host, source: @source)
+
+        assert_equal true, scope.instance_variable_get('@dynamic')
+      end
+
+      test "is false when '#dynamic' is not present in the template" do
+        @host.ptable.update(template: @ptable_template.gsub('#Dynamic', ''))
+        scope = @scope.new(host: @host, source: @source)
+
+        assert_equal false, scope.instance_variable_get('@dynamic')
+      end
+    end
+  end
+
+  describe "kickstart_attributes" do
+    test "does not fail if @host does not have medium" do
+      host = FactoryBot.build_stubbed(:host)
+      scope = @scope.new(host: host, source: @source)
+      assert_nil scope.instance_variable_get('@mediapath')
+    end
+
+    test "sets @arch" do
+      host = FactoryBot.build_stubbed(:host, :managed, :redhat)
+      scope = @scope.new(host: host, source: @source)
+      assert_equal scope.instance_variable_get('@arch'), host.architecture_name
+    end
+
+    test "sets @osver" do
+      host = FactoryBot.build_stubbed(:host, :managed, :redhat)
+      os = host.operatingsystem
+      scope = @scope.new(host: host, source: @source)
+      assert_equal scope.instance_variable_get('@osver'), os.major.to_i
+    end
+
+    test "sets @mediapath" do
+      host = FactoryBot.build_stubbed(:host, :managed, :redhat)
+      media = host.operatingsystem.media.first.path
+      scope = @scope.new(host: host, source: @source)
+      assert_equal scope.instance_variable_get('@mediapath'), "url --url #{media}"
+    end
+
+    test "sets @additional_media from medium provider" do
+      additional_media = [{name: 'EPEL', url: 'http://yum.example.com/epel'}]
+
+      host = FactoryBot.build_stubbed(:host, :managed, :redhat)
+      MediumProviders::Default.any_instance.stubs(:additional_media).returns(additional_media)
+
+      scope = @scope.new(host: host, source: @source)
+      assert_equal additional_media.map(&:with_indifferent_access), scope.instance_variable_get('@additional_media')
+    end
+
     describe "@dynamic" do
       before do
         @ptable_template = "%#\n" \

test/unit/medium_providers/default_test.rb

@@ -17,4 +17,15 @@ class DefaultMediumProviderTest < ActiveSupport::TestCase
     result_path = provider.interpolate_vars('http://foo.org/$version')
     assert result_path, 'http://foo.org/4'
   end
+
+  test 'returns additional_media from host params' do
+    additional_media = [{'name' => 'EPEL', 'url' => 'http://yum.example.com/epel'}]
+    host = FactoryBot.build_stubbed(:host, :managed, :redhat)
+    FactoryBot.create(:host_parameter,
+                      host: host,
+                      name: 'additional_media',
+                      value: additional_media.to_json)
+
+    assert_equal MediumProviders::Default.new(host).additional_media, additional_media
+  end
 end