From f342ccfe7462c9dfc0df1d0aa1cf037fcb6659ff Mon Sep 17 00:00:00 2001
From: Shim Shtein <sshtein@redhat.com>
Date: Tue, 19 Dec 2023 12:45:48 +0200
Subject: [PATCH] Switch to 'network' directive instead of ifcfg

Use kickstart's `network` directive to create interfaces according to
foreman's DB instead of using ifcfg files.
---
 app/services/foreman/renderer/scope/base.rb   |   1 +
 .../renderer/scope/macros/kickstart.rb        | 118 +++++++
 .../finish/kickstart_default_finish.erb       |   8 -
 .../provision/kickstart_default.erb           |  93 +----
 .../provision/kickstart_ovirt.erb             |  26 +-
 .../renderer/scope/macros/kickstart_test.rb   | 325 ++++++++++++++++++
 6 files changed, 475 insertions(+), 96 deletions(-)
 create mode 100644 app/services/foreman/renderer/scope/macros/kickstart.rb
 create mode 100644 test/unit/foreman/renderer/scope/macros/kickstart_test.rb

diff --git a/app/services/foreman/renderer/scope/base.rb b/app/services/foreman/renderer/scope/base.rb
index 5b83063f2d02..44f12cce4dc3 100644
--- a/app/services/foreman/renderer/scope/base.rb
+++ b/app/services/foreman/renderer/scope/base.rb
@@ -9,6 +9,7 @@ class Base
         include Foreman::Renderer::Scope::Macros::TemplateLogging
         include Foreman::Renderer::Scope::Macros::SnippetRendering
         include Foreman::Renderer::Scope::Macros::Transpilers
+        include Foreman::Renderer::Scope::Macros::Kickstart
 
         def self.inherited(child_class)
           loaders.each { |loader| child_class.register_loader(loader) }
diff --git a/app/services/foreman/renderer/scope/macros/kickstart.rb b/app/services/foreman/renderer/scope/macros/kickstart.rb
new file mode 100644
index 000000000000..79463e29eaa2
--- /dev/null
+++ b/app/services/foreman/renderer/scope/macros/kickstart.rb
@@ -0,0 +1,118 @@
+module Foreman
+  module Renderer
+    module Scope
+      module Macros
+        module Kickstart
+          include Foreman::Renderer::Errors
+          extend ApipieDSL::Module
+
+          apipie :class, desc: 'Macros to use within a kickstart template' do
+            name 'Kickstart'
+            sections only: %w[all]
+          end
+
+          # For more information about KS network directive: https://pykickstart.readthedocs.io/en/latest/kickstart-docs.html#network
+          apipie :method, "Returns a kickstart 'network' directive for a specific interface" do
+            required :iface, Nic::Managed, "Managed interface object to represent"
+            keyword :rhel_compatible, [true, false], "Is the OS rhel-compatible?"
+            keyword :os_major, Integer, "OS major version number"
+            keyword :use_slaac, [true, false], "Is the interface configured using SLAAC"
+            keyword :static, [true, false], "Use static configuration for IPv4"
+            keyword :static6, [true, false], "Use static configuration for IPv6"
+            returns String, "'network' directive with all switches that represent the input NIC object"
+            example 'kickstart_network(iface, rhel_compatible: true, os_major: 10, use_slaac: false) #=> "network --bootproto=dhcp --device=ens3"'
+          end
+          def kickstart_network(iface, host:, rhel_compatible:, os_major:, use_slaac:, static:, static6:)
+            return nil unless iface.is_a?(Nic::Managed)
+
+            network_options = []
+            nameservers = []
+            subnet4 = iface.subnet
+            subnet6 = iface.subnet6
+
+            # device and hostname
+            if iface.bond? && rhel_compatible && os_major >= 6
+              network_options.push("--device=#{iface.identifier}")
+            else
+              network_options.push("--device=#{iface.mac || iface.identifier}")
+            end
+            network_options.push("--hostname #{@host.name}")
+
+            # single stack
+            if subnet4 && !subnet6
+              network_options.push("--noipv6")
+            elsif !subnet4 && subnet6
+              network_options.push("--noipv4")
+            end
+
+            # dual stack MTU check
+            raise("IPv4 and IPv6 subnets have different MTU") if subnet4 && subnet6 && subnet4.mtu.present? && subnet6.mtu.present? && subnet4.mtu != subnet6.mtu
+
+            # mtu method is taking into account both ipv4 and ipv6 stacks
+            network_options.push("--mtu=#{iface.mtu}") if iface.mtu
+
+            # IPv4
+            if (subnet4 && !subnet4.dhcp_boot_mode?) || static
+              network_options.push("--bootproto static")
+              network_options.push("--ip=#{iface.ip}")
+              network_options.push("--netmask=#{subnet4.mask}")
+              network_options.push("--gateway=#{subnet4.gateway}")
+            elsif subnet4&.dhcp_boot_mode?
+              network_options.push("--bootproto dhcp")
+            end
+            if subnet4
+              nameservers.concat(subnet4.dns_servers)
+            end
+
+            # IPv6
+            if rhel_compatible && os_major >= 6
+              if (subnet6 && !subnet6.dhcp_boot_mode?) || static6
+                network_options.push("--ipv6=#{iface.ip6}/#{subnet6.cidr}")
+                network_options.push("--ipv6gateway=#{subnet6.gateway}")
+              elsif subnet6&.dhcp_boot_mode?
+                if use_slaac
+                  network_options.push("--ipv6 auto")
+                else
+                  network_options.push("--ipv6 dhcp")
+                end
+              end
+              if subnet6
+                nameservers.concat(subnet6.dns_servers)
+              end
+            end
+
+            # bond
+            if iface.bond? && rhel_compatible && os_major >= 6
+              bond_slaves = iface.attached_devices_identifiers.join(',')
+              network_options.push("--bondslaves=#{bond_slaves}")
+              network_options.push("--bondopts=mode=#{iface.mode},#{iface.bond_options.tr(' ', ',')}")
+            end
+
+            # VLAN (only on physical is recognized)
+            if iface.virtual? && iface.tag.present? && iface.attached_to.present?
+              if rhel_compatible
+                network_options.push("--vlanid=#{iface.tag}")
+                network_options.push("--interfacename=vlan#{iface.tag}") if os_major > 6
+              end
+            end
+
+            # DNS
+            if !nameservers.empty?
+              network_options.push("--nameserver=#{nameservers.join(',')}")
+            else
+              network_options.push("--nodns")
+            end
+
+            # Search domain - available from Fedora 39 (RHEL 10)
+            if iface.domain && rhel_compatible && os_major >= 10
+              network_options.push("--ipv4-dns-search=#{iface.domain}") if subnet4
+              network_options.push("--ipv6-dns-search=#{iface.domain}") if subnet6
+            end
+
+            "network #{network_options.join(' ')}"
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/app/views/unattended/provisioning_templates/finish/kickstart_default_finish.erb b/app/views/unattended/provisioning_templates/finish/kickstart_default_finish.erb
index 8fe6bcd3a152..f4d5c9835904 100644
--- a/app/views/unattended/provisioning_templates/finish/kickstart_default_finish.erb
+++ b/app/views/unattended/provisioning_templates/finish/kickstart_default_finish.erb
@@ -33,14 +33,6 @@ description: |
 %>
 
 <%= snippet_if_exists(template_name + " custom pre") -%>
-<% if @host.subnet.respond_to?(:dhcp_boot_mode?) -%>
-<%= snippet 'kickstart_networking_setup' %>
-<% if (rhel_compatible && os_major >= 8) -%>
-systemctl restart NetworkManager
-<% else -%>
-service network restart
-<% end -%>
-<% end -%>
 
 <% if @host.provision_method == 'image' && root_pass.present? -%>
 # Install the root password
diff --git a/app/views/unattended/provisioning_templates/provision/kickstart_default.erb b/app/views/unattended/provisioning_templates/provision/kickstart_default.erb
index da7b7b12d79a..d96a30d49b3e 100644
--- a/app/views/unattended/provisioning_templates/provision/kickstart_default.erb
+++ b/app/views/unattended/provisioning_templates/provision/kickstart_default.erb
@@ -111,85 +111,20 @@ selinux --<%= host_param('selinux-mode') || host_param('selinux') || 'enforcing'
 keyboard <%= host_param('keyboard') || 'us' %>
 
 <%
-  network_options = []
-  nameservers = []
-  subnet4 = iface.subnet
-  subnet6 = iface.subnet6
-
-  # device and hostname
-  if iface.bond? && rhel_compatible && os_major >= 6
-    network_options.push("--device=#{iface.identifier}")
-  else
-    network_options.push("--device=#{iface.mac || iface.identifier}")
-  end
-  network_options.push("--hostname #{@host.name}")
-
-  # single stack
-  if subnet4 && !subnet6
-    network_options.push("--noipv6")
-  elsif !subnet4 && subnet6
-    network_options.push("--noipv4")
-  end
-
-  # dual stack MTU check
-  raise("IPv4 and IPv6 subnets have different MTU") if subnet4 && subnet6 && subnet4.mtu.present? && subnet6.mtu.present? && subnet4.mtu != subnet6.mtu
-
-  # IPv4
-  if (subnet4 && !subnet4.dhcp_boot_mode?) || @static
-    network_options.push("--bootproto static")
-    network_options.push("--ip=#{iface.ip}")
-    network_options.push("--netmask=#{subnet4.mask}")
-    network_options.push("--gateway=#{subnet4.gateway}")
-  elsif subnet4 && subnet4.dhcp_boot_mode?
-    network_options.push("--bootproto dhcp")
-  end
-  if subnet4
-    nameservers.concat(subnet4.dns_servers)
-    network_options.push("--mtu=#{subnet4.mtu}") if subnet4.mtu.present?
-  end
-
-  # IPv6
-  if rhel_compatible && os_major >= 6
-    if (subnet6 && !subnet6.dhcp_boot_mode?) || @static6
-      network_options.push("--ipv6=#{iface.ip6}/#{subnet6.cidr}")
-      network_options.push("--ipv6gateway=#{subnet6.gateway}")
-    elsif subnet6 && subnet6.dhcp_boot_mode?
-      if host_param_true?('use-slaac')
-        network_options.push("--ipv6 auto")
-      else
-        network_options.push("--ipv6 dhcp")
-      end
-    end
-    if subnet6
-      nameservers.concat(subnet6.dns_servers)
-      network_options.push("--mtu=#{subnet6.mtu}") if subnet6.mtu.present?
-    end
-  end
-
-  # bond
-  if iface.bond? && rhel_compatible && os_major >= 6
-    bond_slaves = iface.attached_devices_identifiers.join(',')
-    network_options.push("--bondslaves=#{bond_slaves}")
-    network_options.push("--bondopts=mode=#{iface.mode},#{iface.bond_options.tr(' ', ',')}")
-  end
-
-  # VLAN (only on physical is recognized)
-  if iface.virtual? && iface.tag.present? && iface.attached_to.present?
-    if rhel_compatible && os_major == 6
-      network_options.push("--vlanid=#{iface.tag}")
-    else
-      network_options.push("--interfacename=vlan#{iface.tag}")
-    end
-  end
-
-  # DNS
-  if nameservers.size > 0
-    network_options.push("--nameserver=#{nameservers.join(',')}")
-  else
-    network_options.push("--nodns")
-  end
+# start with provisioning interface, then other non-bond interfaces and the bonds at the end
+@host.managed_interfaces.sort_by { |iface| iface.bond? 0 : iface.provision? 20 : 10 }.each |iface| do
+%>
+  <%= kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: rhel_compatible,
+        os_major: os_major,
+        use_slaac: host_param_true?('use-slaac'),
+        static: @static,
+        static6: @static6) %>
+<%
+end
 -%>
-network <%= network_options.join(' ') %>
 
 rootpw --iscrypted <%= root_pass %>
 <% if host_param_true?('disable-firewall') -%>
@@ -297,8 +232,6 @@ exec < /dev/tty3 > /dev/tty3
 chvt 3
 (
 logger "Starting anaconda <%= @host %> postinstall"
-<%= snippet 'kickstart_networking_setup' %>
-
 <%= snippet 'ntp' %>
 
 <%= snippet 'yum_proxy' %>
diff --git a/app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb b/app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb
index f548795c93ba..e99047ac661e 100644
--- a/app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb
+++ b/app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb
@@ -51,13 +51,24 @@ end
 
 liveimg --url=<%= liveimg_url %>
 
-<% subnet = @host.subnet -%>
-<% if subnet.respond_to?(:dhcp_boot_mode?) -%>
-<% dhcp = subnet.dhcp_boot_mode? && !@static -%>
-<% else -%>
-<% dhcp = !@static -%>
-<% end -%>
-network --bootproto <%= dhcp ? 'dhcp' : "static --ip=#{@host.ip} --netmask=#{subnet.mask} --gateway=#{subnet.gateway} --nameserver=#{subnet.dns_servers.join(',')}" %> --hostname <%= @host %> --device=<%= @host.mac -%>
+<%
+rhel_compatible = @host.operatingsystem.family == 'Redhat' && @host.operatingsystem.name != 'Fedora'
+os_major = @host.operatingsystem.major.to_i
+# start with provisioning interface, then other non-bond interfaces and the bonds at the end
+@host.managed_interfaces.sort_by { |iface| iface.bond? 0 : iface.provision? 20 : 10 }.each |iface| do
+%>
+  <%= kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: rhel_compatible,
+        os_major: os_major,
+        use_slaac: false,
+        static: @static,
+        static6: @static6) %>
+<%
+end
+-%>
+
 
 rootpw --iscrypted <%= root_pass %>
 <% if host_param_true?('disable-firewall') -%>
@@ -77,7 +88,6 @@ reboot
 %post --log=/root/ks.post.log --erroronfail
 nodectl init
 <%= snippet 'redhat_register' %>
-<%= snippet 'kickstart_networking_setup' %>
 <%= snippet 'efibootmgr_netboot' %>
 <% if host_param_true?('host_registration_insights') -%>
 <%= snippet 'insights' -%>
diff --git a/test/unit/foreman/renderer/scope/macros/kickstart_test.rb b/test/unit/foreman/renderer/scope/macros/kickstart_test.rb
new file mode 100644
index 000000000000..15d91e13bb64
--- /dev/null
+++ b/test/unit/foreman/renderer/scope/macros/kickstart_test.rb
@@ -0,0 +1,325 @@
+require 'test_helper'
+
+class KickstartTest < ActiveSupport::TestCase
+  setup do
+    os = FactoryBot.create(:for_snapshots_rhel9, :with_provision, :with_associations)
+
+    @host = FactoryBot.create(:host, :managed, :build => true, :operatingsystem => os,
+      :interfaces => [
+        FactoryBot.build(:nic_managed, :primary => true),
+        FactoryBot.build(:nic_managed, :provision => true),
+      ])
+
+    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::Macros::Kickstart
+    end.send(:new, host: @host, source: source)
+    # HostInfo.stubs(:providers).returns([HostDummyEncInfo])
+  end
+
+  describe '#network' do
+    test 'should return a network line for an interface' do
+      actual = @scope.kickstart_network(
+        @host.managed_interfaces.first,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/network/, actual)
+    end
+
+    test 'should skip non-managed interfaces' do
+      iface = FactoryBot.build(:nic_base, :primary => true)
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_nil actual
+    end
+
+    test 'should create bond interface' do
+      iface = FactoryBot.build(
+        :nic_bond,
+        primary: true,
+        identifier: 'test_bond',
+        attached_devices: ['bonded_slave1', 'bonded_slave2'],
+        mode: 'test_mode',
+        bond_options: 'option_a=foo option_b=bar'
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/bondslaves/, actual)
+      assert_match(/bonded_slave1/, actual)
+      assert_match(/bonded_slave2/, actual)
+      assert_match(/mode=test_mode,/, actual)
+      assert_match(/,option_a=foo,option_b=bar/, actual)
+    end
+
+    test 'should set correct noipv6 flag' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet: FactoryBot.build(:subnet_ipv4)
+      )
+
+      iface.subnet6 = nil
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--noipv6/, actual)
+    end
+
+    test 'should set correct noipv4 flag' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet6: FactoryBot.build(:subnet_ipv6)
+      )
+
+      iface.subnet = nil
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--noipv4/, actual)
+    end
+
+    test 'should use static ipv4 configuration' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet: FactoryBot.build(:subnet_ipv4_static_for_snapshots)
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: true,
+        static6: false
+      )
+
+      assert_match(/--ip/, actual)
+      assert_match(/--netmask/, actual)
+      assert_match(/--gateway/, actual)
+      assert_match(/--bootproto/, actual)
+      assert_match(/static/, actual)
+    end
+
+    test 'should use dhcp ipv4 configuration' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet: FactoryBot.build(:subnet_ipv4_with_domains)
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--bootproto/, actual)
+      assert_match(/dhcp/, actual)
+    end
+
+    test 'should use static ipv6 configuration' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet6: FactoryBot.build(:subnet_ipv6_static_for_snapshots)
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: true
+      )
+
+      assert_match(/--ipv6=/, actual)
+      assert_match(/--ipv6gateway=/, actual)
+    end
+
+    test 'should use dhcp ipv6 configuration' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet6: FactoryBot.build(:subnet_ipv6_with_domains)
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--ipv6/, actual)
+      assert_match(/dhcp/, actual)
+    end
+
+    test 'should use auto ipv6 configuration' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet6: FactoryBot.build(:subnet_ipv6_dhcp_for_snapshots)
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: true,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--ipv6/, actual)
+      assert_match(/auto/, actual)
+    end
+
+    test 'should set vlan options' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        virtual: true,
+        tag: '333',
+        attached_to: 'test_iface1'
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--vlanid/, actual)
+      assert_match(/333/, actual)
+      assert_match(/--interfacename/, actual)
+    end
+
+    test 'should set DNS servers' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet: FactoryBot.build(:subnet_ipv4_static_for_snapshots)
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--nameserver/, actual)
+      assert_match(/192.168.42.2/, actual)
+      assert_match(/192.168.42.3/, actual)
+    end
+
+    test 'should set nodns flag' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet: FactoryBot.build(:subnet_ipv4)
+      )
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 9,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--nodns/, actual)
+    end
+
+    test 'should set search domain' do
+      iface = FactoryBot.build(
+        :nic_managed,
+        primary: true,
+        subnet: FactoryBot.build(:subnet_ipv4)
+      )
+
+      iface.domain = FactoryBot.build(:domain, name: 'test.com')
+
+      actual = @scope.kickstart_network(
+        iface,
+        host: @host,
+        rhel_compatible: true,
+        os_major: 10,
+        use_slaac: false,
+        static: false,
+        static6: false
+      )
+
+      assert_match(/--ipv4-dns-search/, actual)
+      assert_match(/test.com/, actual)
+    end
+  end
+end