Merge pull request #212 from mchf/bnc#879617-bridge

Generate proper bridge port configuration
yast · Jun 20, 2014 · f083cce · f083cce
2 parents 69149ef + 55b7e13
commit f083cce
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 147 deletions.
diff --git a/package/yast2-network.changes b/package/yast2-network.changes
@@ -1,3 +1,11 @@
+-------------------------------------------------------------------
+Thu Jun 19 11:53:55 UTC 2014 - mfilka@suse.com
+
+- bnc#879617
+  - fixed generated bridge port configuration
+  - tun device is not allowed as bridge port anymore
+- 3.1.65 
+
 -------------------------------------------------------------------
 Sat Jun 14 20:20:57 UTC 2014 - mfilka@suse.com
 

diff --git a/package/yast2-network.spec b/package/yast2-network.spec
@@ -17,7 +17,7 @@
 
 
 Name:           yast2-network
-Version:        3.1.64
+Version:        3.1.65
 Release:        0
 
 BuildRoot:      %{_tmppath}/%{name}-%{version}-build

diff --git a/src/include/network/lan/bridge.rb b/src/include/network/lan/bridge.rb
@@ -28,6 +28,8 @@
 #
 module Yast
   module NetworkLanBridgeInclude
+    include Logger
+
     def initialize_network_lan_bridge(include_target)
       textdomain "network"
     end
@@ -37,10 +39,8 @@ def initialize_network_lan_bridge(include_target)
     #
     # @param [String] key	id of the widget
     def InitBridge(key)
-      br_ports = Builtins.splitstring(
-        Ops.get_string(NetworkInterfaces.Current, "BRIDGE_PORTS", ""),
-        " "
-      )
+      br_ports = (NetworkInterfaces.Current["BRIDGE_PORTS"] || "").split
+
       items = CreateSlaveItems(
         LanItems.GetBridgeableInterfaces(LanItems.GetCurrentName),
         br_ports
@@ -51,111 +51,48 @@ def InitBridge(key)
       nil
     end
 
+    def configure_as_bridge_port(device)
+      log.info("Adapt device #{device} as bridge port")
+
+      # when using wicked every device which can be bridged
+      # can be set to BOOTPROTO=none. No workaround with
+      # BOOTPROTO=static required anymore
+      NetworkInterfaces.Edit(device)
+      NetworkInterfaces.Current["IPADDR"] = ""
+      NetworkInterfaces.Current["NETMASK"] = ""
+      NetworkInterfaces.Current["BOOTPROTO"] = "none"
+      NetworkInterfaces.Commit
+      NetworkInterfaces.Add
+    end
+
     def ValidateBridge(key, event)
-      event = deep_copy(event)
-      old_name = NetworkInterfaces.Name
-      valid = true
-      confirmed = false
-      items = Convert.convert(
-        UI.QueryWidget(Id(key), :Items),
-        :from => "any",
-        :to   => "list <term>"
-      )
-      sel = Convert.convert(
-        UI.QueryWidget(Id("BRIDGE_PORTS"), :SelectedItems),
-        :from => "any",
-        :to   => "list <string>"
-      )
-      confs = []
+      sel = UI.QueryWidget(Id("BRIDGE_PORTS"), :SelectedItems)
+
       configurations = NetworkInterfaces.FilterDevices("netcard")
-      Builtins.foreach(
-        Builtins.splitstring(
-          Ops.get(NetworkInterfaces.CardRegex, "netcard", ""),
-          "|"
-        )
-      ) do |devtype|
-        confs = Convert.convert(
-          Builtins.union(
-            confs,
-            Map.Keys(Ops.get_map(configurations, devtype, {}))
-          ),
-          :from => "list",
-          :to   => "list <string>"
-        )
+      netcard_types = (NetworkInterfaces.CardRegex["netcard"] || "").split("|")
+
+      confs = netcard_types.reduce([]) do |res, devtype|
+        res.concat((configurations[devtype] || {}).keys)
       end
-      Builtins.foreach(items) do |t|
-        device = Ops.get_string(t, [0, 0], "")
-        if Builtins.contains(sel, device) && IsNotEmpty(device)
-          if Builtins.contains(confs, device)
-            # allow to add bonding device into bridge and also device with mask /32(bnc#405343)
-            if Builtins.contains(
-                ["tun", "tap"],
-                NetworkInterfaces.GetType(device)
+
+      valid = true
+      confirmed = false
+
+      sel.each do |device|
+        next if !confs.include?(device)
+
+        dev_type = NetworkInterfaces.GetType(device)
+        ifcfg_conf = configurations[dev_type][device]
+
+        if ifcfg_conf["BOOTPROTO"] != "none" && !confirmed
+            valid = Popup.ContinueCancel(
+              _(
+                "At least one selected device is already configured.\nAdapt the configuration for bridge?\n"
               )
-              next
-            end
-            if Builtins.contains(["bond"], NetworkInterfaces.GetType(device))
-              if LanItems.operation == :add
-                old_name2 = NetworkInterfaces.Name
-                NetworkInterfaces.Edit(device)
-                Ops.set(NetworkInterfaces.Current, "IPADDR", "0.0.0.0")
-                Ops.set(NetworkInterfaces.Current, "NETMASK", "255.255.255.255")
-                Ops.set(NetworkInterfaces.Current, "BOOTPROTO", "static")
-                NetworkInterfaces.Commit
-                NetworkInterfaces.Add
-              end
-              next
-            end
-            if Ops.get_string(
-                configurations,
-                [NetworkInterfaces.GetType(device), device, "PREFIXLEN"],
-                ""
-              ) != "32" ||
-                Ops.get_string(
-                  configurations,
-                  [NetworkInterfaces.GetType(device), device, "NETMASK"],
-                  ""
-                ) != "255.255.255.255"
-              if Ops.get_string(
-                  configurations,
-                  [NetworkInterfaces.GetType(device), device, "IPADDR"],
-                  ""
-                ) != "0.0.0.0" &&
-                  Ops.get_string(
-                    configurations,
-                    [NetworkInterfaces.GetType(device), device, "BOOTPROTO"],
-                    ""
-                  ) != "none"
-                if !confirmed
-                  valid = Popup.ContinueCancel(
-                    _(
-                      "At least one selected device is already configured.\nAdapt the configuration for bridge (IP address 0.0.0.0/32)?\n"
-                    )
-                  )
-                  confirmed = true
-                end
-                if valid
-                  i = LanItems.current
-                  if LanItems.FindAndSelect(device)
-                    Builtins.y2internal(
-                      "Adapt device %1 for bridge (0.0.0.0/32)",
-                      device
-                    )
-                    NetworkInterfaces.Edit(device)
-                    Ops.set(NetworkInterfaces.Current, "IPADDR", "0.0.0.0")
-                    Ops.set(NetworkInterfaces.Current, "PREFIXLEN", "32")
-                    Ops.set(NetworkInterfaces.Current, "BOOTPROTO", "static")
-                    NetworkInterfaces.Commit
-                    NetworkInterfaces.Add
-                    LanItems.current = i
-                  end
-                end
-              end
-            end
-          end
+            )
+            confirmed = true
         end
       end
-      NetworkInterfaces.Select(old_name)
       valid
     end
   end

diff --git a/src/modules/LanItems.rb b/src/modules/LanItems.rb
@@ -42,6 +42,7 @@ def main
       Yast.include self, "network/routines.rb"
       Yast.include self, "network/lan/s390.rb"
       Yast.include self, "network/lan/udev.rb"
+      Yast.include self, "network/lan/bridge.rb"
 
       # Hardware information
       # @see #ReadHardware
@@ -323,14 +324,14 @@ def IsCurrentConfigured
     #
     # First it looks into the item's netconfig and if it doesn't exist
     # it uses device name from hwinfo if available.
-    def GetDeviceName(itemId)
-      lanItem = GetLanItem(itemId)
+    def GetDeviceName(item_id)
+      lan_item = GetLanItem(item_id)
 
-      Ops.get_string(
-        lanItem,
-        "ifcfg",
-        Ops.get_string(lanItem, ["hwinfo", "dev_name"], "")
-      )
+      return lan_item["ifcfg"] if lan_item["ifcfg"]
+      return lan_item["hwinfo"]["dev_name"] || "" if lan_item["hwinfo"]
+
+      log.error("Item #{item_id} has no dev_name nor configuration associated")
+      return "" # this should never happen
     end
 
     # Returns name which is going to be used in the udev rule
@@ -823,34 +824,31 @@ def IsBridgeable(bridgeMaster, itemId)
       devname = GetDeviceName(itemId)
       bonded = BuildBondIndex()
 
-      if bonded[ devname]
-        Builtins.y2debug(
-          "IsBridgeable: excluding lan item (#{itemId}: #{devname}) - is bonded",
-        )
+      if bonded[devname]
+        log.debug("Excluding lan item (#{itemId}: #{devname}) - is bonded")
         return false
       end
 
       devtype = GetDeviceType(itemId)
 
       # exclude forbidden configurations
-      if devtype == "br"
-        Builtins.y2debug(
-          "IsBridgeable: excluding lan item (#{itemId}: #{devname}) - is bridge",
-        )
-        return false
+      case devtype
+        when "br"
+          log.debug("Excluding lan item (#{itemId}: #{devname}) - is bridge")
+          return false
+
+        when "tun"
+          log.debug("Excluding lan item (#{itemId}: #{devname}) - is tun")
+          return false
       end
 
-      case ifcfg[ "STARTMODE"]
+      case ifcfg["STARTMODE"]
         when "nfsroot"
-          Builtins.y2debug(
-            "IsBridgeable: excluding lan item (#{itemId}: #{devname}) - is nfsroot",
-          )
+          log.debug("Excluding lan item (#{itemId}: #{devname}) - is nfsroot")
           return false
 
         when "ifplugd"
-          Builtins.y2debug(
-            "IsBridgeable: excluding lan item (#{itemId}: #{devname}) - ifplugd",
-          )
+          log.debug("Excluding lan item (#{itemId}: #{devname}) - ifplugd")
           return false
 
         else
@@ -943,21 +941,15 @@ def getNetworkInterfaces
       end
     end
 
+    def find_configured(device)
+      @Items.select { |k,v| v["ifcfg"] == device }.keys.first
+    end
+
     def FindAndSelect(device)
-      found = false
-      Builtins.foreach(
-        Convert.convert(
-          @Items,
-          :from => "map <integer, any>",
-          :to   => "map <integer, map <string, any>>"
-        )
-      ) do |i, a|
-        if Ops.get_string(a, "ifcfg", "") == device
-          found = true
-          @current = i
-        end
-      end
-      found
+      item_id = find_configured(device)
+      @current = item_id if item_id
+
+      return !item_id.nil?
     end
 
     # search all known devices to find it's index in Items array
@@ -2126,6 +2118,11 @@ def Commit
 
       Ops.set(@Items, [@current, "ifcfg"], "") if !NetworkInterfaces.Commit
 
+      # configure bridge ports
+      if @bridge_ports
+        @bridge_ports.split.each { |bp| configure_as_bridge_port(bp) }
+      end
+
       @modified = true
       @operation = nil
       true

diff --git a/test/bridge_test.rb b/test/bridge_test.rb
@@ -60,8 +60,7 @@ module Yast
       "eth4",
       "eth11",
       "eth12",
-      "tap0",
-      "tun0"
+      "tap0"
     ]
 
     before(:each) do

diff --git a/test/netcard_test.rb b/test/netcard_test.rb
@@ -64,7 +64,7 @@
       "name" => "SUSE test card",
       "dev_name" => "enp0s3"
     }
-  },
+  }
 }
 
 require "yast"
@@ -82,7 +82,7 @@
 
     # mocking only neccessary parts of Yast::LanItems so we need not to call
     # and mock inputs for Yast::LanItems.Read here
-    @lan_items.Items = MOCKED_ITEMS
+    @lan_items.Items = deep_copy(MOCKED_ITEMS)
   end
 
   it "returns empty list when querying device name with nil or empty input" do
@@ -140,7 +140,7 @@
   before(:each) do
     @lan_items = Yast::LanItems
     @lan_items.main
-    @lan_items.Items = MOCKED_ITEMS
+    @lan_items.Items = deep_copy(MOCKED_ITEMS)
   end
 
   it "returns description and uses custom name if present" do
@@ -185,7 +185,7 @@
   before(:each) do
     @lan_items = Yast::LanItems
     @lan_items.main
-    @lan_items.Items = MOCKED_ITEMS
+    @lan_items.Items = deep_copy(MOCKED_ITEMS)
   end
 
   it "removes an existing item" do
@@ -215,3 +215,39 @@
     expect(@lan_items.Items.size).to eql before_size
   end
 end
+
+describe "LanItemsClass#GetItemName" do
+  before(:each) do
+    @lan_items = Yast::LanItems
+    @lan_items.main
+    @lan_items.Items = deep_copy(MOCKED_ITEMS)
+  end
+
+  it "returns name provided by hwinfo if not configured" do
+    MOCKED_ITEMS.select { |k,v| !v.has_key?("ifcfg") }.each_pair do |item_id, conf|
+      expect(@lan_items.GetDeviceName(item_id)).to eql conf["hwinfo"]["dev_name"]
+    end
+  end
+
+  it "returns name according configuration if available" do
+    MOCKED_ITEMS.select { |k,v| v.has_key?("ifcfg") }.each_pair do |item_id, conf|
+      expect(@lan_items.GetDeviceName(item_id)).to eql conf["ifcfg"]
+    end
+  end
+end
+
+describe "LanItemsClass#FindAndSelect" do
+  before(:each) do
+    @lan_items = Yast::LanItems
+    @lan_items.main
+    @lan_items.Items = deep_copy(MOCKED_ITEMS)
+  end
+
+  it "finds configured device" do
+    expect(@lan_items.FindAndSelect("enp0s3")).to be_true
+  end
+
+  it "fails to find unconfigured device" do
+    expect(@lan_items.FindAndSelect("nonexistent")).to be_false
+  end
+end