Merge pull request #1199 from yast/cleanup_write

Write bonding slaves config properly

package/yast2-network.changes

@@ -1,3 +1,12 @@
+-------------------------------------------------------------------
+Fri Apr  9 11:22:42 UTC 2021 - Knut Anderssen <kanderssen@suse.com>
+
+- bsc#1181956
+  - Omit empty values or the '0.0.0.0' IPADDR when writing ifcfg
+    files.
+  - Do not write empty ifroute files.
+- 4.4.2
+
 -------------------------------------------------------------------
 Thu Apr  8 09:27:47 UTC 2021 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>
 

package/yast2-network.spec

@@ -17,7 +17,7 @@
 
 
 Name:           yast2-network
-Version:        4.4.1
+Version:        4.4.2
 Release:        0
 Summary:        YaST2 - Network Configuration
 License:        GPL-2.0-only

src/lib/y2network/boot_protocol.rb

@@ -43,6 +43,7 @@ def from_name(name)
 
     # @return [String] Returns protocol name
     attr_reader :name
+    alias_method :to_s, :name
 
     # Constructor
     #

src/lib/y2network/interface_config_builders/bonding.rb

@@ -60,20 +60,21 @@ def require_adaptation?(devices)
         devices.any? do |device|
           next false unless yast_config.configured_interface?(device)
 
-          yast_config.connections.by_name(device).bootproto.name != "none"
+          !valid_slave_config?(device)
         end
       end
 
       # additionally it adapt slaves if needed
       def save
         slaves.each do |slave|
           interface = yast_config.interfaces.by_name(slave)
-          connection = yast_config.connections.by_name(slave)
-          next if connection && connection.bootproto.name == "none"
+          next if valid_slave_config?(slave)
 
+          connection = yast_config.connections.by_name(slave)
           builder = InterfaceConfigBuilder.for(interface.type, config: connection)
           builder.name = interface.name
           builder.configure_as_slave
+          builder.startmode = "hotplug"
           builder.save
         end
 
@@ -118,6 +119,16 @@ def bondable?(iface)
 
         true
       end
+
+      # Convenience method to check whether the config of an enslaved interface
+      # is valid or not
+      #
+      # @param iface [String] slave name to be validated
+      def valid_slave_config?(iface)
+        conn = yast_config.connections.by_name(iface)
+
+        conn&.bootproto&.name == "none" && conn&.startmode&.name == "hotplug"
+      end
     end
   end
 end

src/lib/y2network/wicked/config_writer.rb

@@ -64,10 +64,12 @@ def write_interface_routes(config, old_config)
           file = routes_file_for(dev)
 
           # Remove ifroutes-* if empty or interface is not configured
-          file.remove if routes.empty? || !config.configured_interface?(dev.name)
-
-          file.routes = routes
-          file.save
+          if routes.empty? || !config.configured_interface?(dev.name)
+            file.remove
+          else
+            file.routes = routes
+            file.save
+          end
         end
 
         # Actions needed for removed interfaces

src/lib/y2network/wicked/connection_config_writers/base.rb

@@ -26,9 +26,12 @@ module ConnectionConfigWriters
       #
       # The derived classes should implement {#update_file} method.
       class Base
+        extend Forwardable
         # @return [CFA::InterfaceFile] Interface's configuration file
         attr_reader :file
 
+        def_delegators :@file, :value_as_string
+
         # Constructor
         #
         # @param file [CFA::InterfaceFile] Interface's configuration file
@@ -40,17 +43,15 @@ def initialize(file)
         #
         # @param conn [Y2Network::ConnectionConfig::Base] Connection to take settings from
         def write(conn)
-          file.bootproto = conn.bootproto&.name
-          file.name = conn.description
-          file.lladdr = conn.lladdress
-          file.startmode = conn.startmode.to_s
-          file.dhclient_set_hostname = dhclient_set_hostname(conn)
-          file.ifplugd_priority = conn.startmode.priority if conn.startmode&.name == "ifplugd"
-          if conn.ethtool_options && !conn.ethtool_options.empty?
-            file.ethtool_options = conn.ethtool_options
-          end
-          file.zone = conn.firewall_zone
-          file.mtu = conn.mtu
+          file.bootproto = value_as_string(conn.bootproto.to_s)
+          file.name = value_as_string(conn.description)
+          file.lladdr = value_as_string(conn.lladdress)
+          file.startmode = value_as_string(conn.startmode.to_s)
+          file.dhclient_set_hostname = value_as_string(dhclient_set_hostname(conn))
+          file.ifplugd_priority = conn.startmode.priority if conn.startmode.to_s == "ifplugd"
+          file.ethtool_options = value_as_string(conn.ethtool_options)
+          file.zone = value_as_string(conn.firewall_zone)
+          file.mtu = value_as_string(conn.mtu)
           add_ips(conn)
 
           update_file(conn)
@@ -82,7 +83,7 @@ def dhclient_set_hostname(conn)
         def add_ips(conn)
           file.ipaddrs.clear
           ips_to_add = conn.ip_aliases.clone
-          ips_to_add << conn.ip if conn.ip && !conn.bootproto.dhcp?
+          ips_to_add << conn.ip if static_valid_ip?(conn)
           ips_to_add.each { |i| add_ip(i) }
         end
 
@@ -91,7 +92,7 @@ def add_ips(conn)
         # @param ip [Y2Network::IPAddress] IP address to add
         def add_ip(ip)
           file.ipaddrs[ip.id] = ip.address
-          file.labels[ip.id] = ip.label
+          file.labels[ip.id] = value_as_string(ip.label)
           file.remote_ipaddrs[ip.id] = ip.remote_address
           file.broadcasts[ip.id] = ip.broadcast
         end
@@ -105,6 +106,18 @@ def add_hostname(conn)
 
           Yast::Host.Update("", conn.hostname, conn.ip.address.address.to_s)
         end
+
+        # Convenience method to check whether a connection is configured using
+        # the static bootproto and a valid IP address.
+        #
+        # @param conn [Y2Network::ConnectionConfig::Base] Connection to take settings from
+        # @return [Boolean] whether the connection is configured with a valid
+        #   static IP address or not
+        def static_valid_ip?(conn)
+          return false unless conn.bootproto.static?
+
+          conn.ip && conn.ip.address.address.to_s != "0.0.0.0"
+        end
       end
     end
   end

test/y2network/interface_config_builders/bonding_test.rb

@@ -26,6 +26,10 @@
 
 describe Y2Network::InterfaceConfigBuilders::Bonding do
   let(:config) { Y2Network::Config.new(source: :test) }
+  let(:dhcp) { Y2Network::BootProtocol::DHCP }
+  let(:none) { Y2Network::BootProtocol::NONE }
+  let(:hotplug) { Y2Network::Startmode.create("hotplug") }
+  let(:auto) { Y2Network::Startmode.create("auto") }
 
   before do
     allow(config).to receive(:interfaces).and_return(interfaces_collection)
@@ -154,20 +158,20 @@
     end
 
     context "when all the slaves are properly configure do" do
-      it "return false" do
+      it "returns false" do
         subject.save
         expect(subject.require_adaptation?(connection_config.slaves)).to eql(false)
       end
     end
+
     context "when at least one configured slave need to be adapted" do
-      it "return true" do
+      it "returns true" do
         subject.save
         iface1_conn = connection_configs_collection.by_name("iface1")
-        iface1_conn.bootproto = Y2Network::BootProtocol::DHCP
+        iface1_conn.bootproto = dhcp
         expect(subject.require_adaptation?(connection_config.slaves)).to eql(true)
       end
     end
-
   end
 
   describe "#save" do
@@ -181,12 +185,22 @@
       subject.save
       iface1_conn = connection_configs_collection.by_name("iface1")
       iface2_conn = connection_configs_collection.by_name("iface2")
-      iface2_conn.bootproto = Y2Network::BootProtocol::DHCP
+      iface2_conn.bootproto = dhcp
       expect(Y2Network::InterfaceConfigBuilder)
         .to receive(:for).with(anything, config: iface2_conn).once.and_call_original
       expect(Y2Network::InterfaceConfigBuilder)
         .to_not receive(:for).with(anything, config: iface1_conn)
       subject.save
     end
+
+    it "sets the BOOTPROTO to 'none' and STARTMODE to 'hotplug' in the adapted configs" do
+      connection_config.slaves = ["iface1", "iface2"]
+      subject.save
+      iface2_conn = connection_configs_collection.by_name("iface2")
+      iface2_conn.bootproto = dhcp
+      expect { subject.save }.to change { iface2_conn.bootproto }.from(dhcp).to(none)
+      iface2_conn.startmode = auto
+      expect { subject.save }.to change { iface2_conn.startmode }.from(auto).to(hotplug)
+    end
   end
 end

test/y2network/wicked/connection_config_writers/bridge_test.rb

@@ -47,7 +47,7 @@
     it "writes common properties" do
       handler.write(conn)
       expect(file).to have_attributes(
-        name:      conn.description,
+        name:      nil,
         startmode: "auto",
         bootproto: "dhcp"
       )