ICMP redirects default is now adapter specific

netman/adapters/switches/cached.py

@@ -341,7 +341,4 @@ def enable_lldp(self, interface_id, enabled):
 
     def set_vlan_icmp_redirects_state(self, vlan_number, state):
         self.real_switch.set_vlan_icmp_redirects_state(vlan_number, state)
-        try:
-            self.vlans_cache[vlan_number].icmp_redirects = state
-        except ValueError:
-            pass
+        self.vlans_cache[vlan_number].icmp_redirects = state

netman/adapters/switches/cisco.py

@@ -81,7 +81,7 @@ def rollback_transaction(self):
         pass
 
     def get_vlan(self, number):
-        vlan = Vlan(int(number))
+        vlan = Vlan(int(number), icmp_redirects=True)
         apply_vlan_running_config_data(vlan, self._get_vlan_run_conf(number))
         apply_interface_running_config_data(
             vlan,
@@ -100,7 +100,7 @@ def get_vlans(self):
                 if name == ("VLAN{}".format(number)):
                     name = None
 
-                vlans[number] = Vlan(int(number), name)
+                vlans[number] = Vlan(int(number), name, icmp_redirects=True)
 
         for ip_interface_data in split_on_dedent(self.ssh.do("show ip interface")):
             if regex.match("^Vlan(\d+)\s.*", ip_interface_data[0]):
@@ -403,6 +403,8 @@ def remove_vrrp_group(self, vlan_number, group_id):
                 raise VrrpDoesNotExistForVlan(vlan=vlan_number, vrrp_group_id=group_id)
 
     def set_vlan_icmp_redirects_state(self, vlan_number, state):
+        self.get_vlan_interface_data(vlan_number)
+
         with self.config(), self.interface_vlan(vlan_number):
             if state:
                 self.ssh.do('ip redirects')

netman/adapters/switches/remote.py

@@ -257,14 +257,11 @@ def remove_dhcp_relay_server(self, vlan_number, ip_address):
 
     def enable_lldp(self, interface_id, enabled):
         self.put("/interfaces/{}/lldp".format(interface_id),
-                 raw_data=self.get_json_boolean(enabled))
+                 raw_data=_get_json_boolean(enabled))
 
     def set_vlan_icmp_redirects_state(self, vlan_number, state):
         self.put('/vlans/{}/icmp-redirects'.format(vlan_number),
-                 raw_data=self.get_json_boolean(state))
-
-    def get_json_boolean(self, state):
-        return {True: "true", False: "false"}[state]
+                 raw_data=_get_json_boolean(state))
 
     def get(self, relative_url):
         return self.validated(self.requests.get(**self.request(relative_url)))
@@ -333,3 +330,7 @@ def validated(self, req):
 
             raise exception
         return req
+
+
+def _get_json_boolean(state):
+    return {True: "true", False: "false"}[state]

netman/core/objects/switch_base.py

@@ -57,7 +57,6 @@ def get_interfaces(self):
     def set_access_vlan(self, interface_id, vlan):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called unset_access_vlan
     def remove_access_vlan(self, interface_id):
         raise NotImplementedError()
 
@@ -73,19 +72,15 @@ def add_trunk_vlan(self, interface_id, vlan):
     def remove_trunk_vlan(self, interface_id, vlan):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called set_interface_state
     def shutdown_interface(self, interface_id):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be part of set_interface_state
     def openup_interface(self, interface_id):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called set_native_vlan
     def configure_native_vlan(self, interface_id, vlan):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called unset_native_vlan
     def remove_native_vlan(self, interface_id):
         raise NotImplementedError()
 
@@ -98,25 +93,21 @@ def remove_ip_from_vlan(self, vlan_number, ip_network):
     def set_vlan_access_group(self, vlan_number, direction, name):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called unset_vlan_access_group
     def remove_vlan_access_group(self, vlan_number, direction):
         raise NotImplementedError()
 
     def set_vlan_vrf(self, vlan_number, vrf_name):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called unset_vlan_vrf
     def remove_vlan_vrf(self, vlan_number):
         raise NotImplementedError()
 
     def set_interface_description(self, interface_id, description):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called unset_interface_description
     def remove_interface_description(self, interface_id):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called set_interface_spanning_tree_state
     def edit_interface_spanning_tree(self, interface_id, edge=None):
         raise NotImplementedError()
 
@@ -144,7 +135,6 @@ def set_bond_link_speed(self, number, speed):
     def set_bond_description(self, number, description):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called unset_bond_description
     def remove_bond_description(self, number):
         raise NotImplementedError()
 
@@ -160,15 +150,12 @@ def add_bond_trunk_vlan(self, number, vlan):
     def remove_bond_trunk_vlan(self, number, vlan):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called set_bond_native_vlan
     def configure_bond_native_vlan(self, number, vlan):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called unset_bond_native_vlan
     def remove_bond_native_vlan(self, number):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called set_bond_interface_spanning_tree_state
     def edit_bond_spanning_tree(self, number, edge=None):
         raise NotImplementedError()
 
@@ -185,7 +172,6 @@ def add_dhcp_relay_server(self, vlan_number, ip_address):
     def remove_dhcp_relay_server(self, vlan_number, ip_address):
         raise NotImplementedError()
 
-    # TODO(mmitchell): Should be called set_interface_lldp_state
     def enable_lldp(self, interface_id, enabled):
         raise NotImplementedError()
 

netman/core/objects/vlan.py

@@ -26,7 +26,4 @@ def __init__(self, number=None, name=None, ips=None, vrrp_groups=None, vrf_forwa
         self.ips = ips or []
         self.vrrp_groups = vrrp_groups or []
         self.dhcp_relay_servers = dhcp_relay_servers or []
-        if icmp_redirects is not None:
-            self.icmp_redirects = icmp_redirects
-        else:
-            self.icmp_redirects = True
+        self.icmp_redirects = icmp_redirects

tests/adapters/compliance_tests/get_vlan_test.py

@@ -34,6 +34,7 @@ def test_returns_a_vlan_with_all_available_data(self):
                                    ips=[IPAddress("10.10.10.1")], priority=110, hello_interval=10,
                                    dead_interval=11, track_id=self.test_vrrp_track_id, track_decrement=50)
         self.try_to.add_dhcp_relay_server(1000, IPAddress("11.11.11.1"))
+        self.try_to.set_vlan_icmp_redirects_state(1000, False)
 
         assert_that(self.client.get_vlan(1000), is_(self.get_vlan_from_list(1000)))
 

tests/adapters/compliance_tests/set_vlan_icmp_redirects_state_test.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 from hamcrest import assert_that, is_
+from netman.core.objects.exceptions import UnknownVlan
 
 from tests.adapters.configured_test_case import ConfiguredTestCase
 
@@ -23,7 +24,6 @@ class SetVlanIcmpRedirectsStateTest(ConfiguredTestCase):
     def setUp(self):
         super(SetVlanIcmpRedirectsStateTest, self).setUp()
         self.client.add_vlan(1000)
-        self.client.add_vlan(1001)
 
     def test_enables_icmp_redirects_when_given_true(self):
         self.client.set_vlan_icmp_redirects_state(1000, True)
@@ -32,12 +32,15 @@ def test_enables_icmp_redirects_when_given_true(self):
         assert_that(vlan.icmp_redirects, is_(True))
 
     def test_disables_icmp_redirects_when_given_false(self):
-        self.client.set_vlan_icmp_redirects_state(1001, False)
-        vlan = self.get_vlan_from_list(1001)
-        assert_that(vlan.number, is_(1001))
+        self.client.set_vlan_icmp_redirects_state(1000, False)
+        vlan = self.get_vlan_from_list(1000)
+        assert_that(vlan.number, is_(1000))
         assert_that(vlan.icmp_redirects, is_(False))
 
+    def test_raises_UnknownVlan_when_operating_on_a_vlan_that_does_not_exist(self):
+        with self.assertRaises(UnknownVlan):
+            self.client.set_vlan_icmp_redirects_state(2000, False)
+
     def tearDown(self):
         self.janitor.remove_vlan(1000)
-        self.janitor.remove_vlan(1001)
         super(SetVlanIcmpRedirectsStateTest, self).tearDown()

tests/adapters/switches/cisco_test.py

@@ -162,6 +162,7 @@ def test_get_vlan(self):
         assert_that(v3.access_groups[IN], equal_to(None))
         assert_that(v3.access_groups[OUT], equal_to("GAGA"))
         assert_that(len(v3.ips), equal_to(3))
+        assert_that(v3.icmp_redirects, equal_to(True))
 
         v3.ips = sorted(v3.ips, key=lambda ip: (ip.value, ip.prefixlen))
         assert_that(str(v3.ips[0].ip), equal_to('1.1.1.1'))
@@ -218,6 +219,7 @@ def test_get_vlan_with_no_interface(self):
         assert_that(vlan.ips, is_(empty()))
         assert_that(vlan.vrrp_groups, is_(empty()))
         assert_that(vlan.dhcp_relay_servers, is_(empty()))
+        assert_that(vlan.icmp_redirects, is_(True))
 
     def test_get_vlan_with_an_empty_interface(self):
         self.command_setup()
@@ -274,6 +276,7 @@ def test_get_vlan_with_a_full_interface(self):
             " standby 1 track 101 decrement 50",
             " ip helper-address 10.10.10.1",
             " ip helper-address 10.10.10.2",
+            " no ip redirects"
             "end"
         ])
 
@@ -285,6 +288,7 @@ def test_get_vlan_with_a_full_interface(self):
         assert_that(vlan.access_groups[OUT], is_("ACL-OUT"))
         assert_that(vlan.vrf_forwarding, is_("SHIZZLE"))
         assert_that(vlan.ips, has_length(3))
+        assert_that(vlan.icmp_redirects, is_(False))
 
         vrrp_group = vlan.vrrp_groups[0]
         assert_that(len(vrrp_group.ips), equal_to(3))
@@ -2454,6 +2458,15 @@ def test_remove_non_existent_dhcp_relay_server_fails(self):
     def test_set_vlan_icmp_redirects_state_enable(self):
         self.command_setup()
 
+        self.mocked_ssh_client.should_receive("do").with_args("show running-config interface vlan 1234").once().ordered().and_return([
+            "Building configuration...",
+            "Current configuration : 41 bytes",
+            "!",
+            "interface Vlan1234",
+            " no ip address",
+            "end"
+        ])
+
         self.mocked_ssh_client.should_receive("do").with_args("configure terminal").once().ordered().and_return([
             "Enter configuration commands, one per line.  End with CNTL/Z."
         ])
@@ -2468,6 +2481,15 @@ def test_set_vlan_icmp_redirects_state_enable(self):
     def test_set_vlan_icmp_redirects_state_disable(self):
         self.command_setup()
 
+        self.mocked_ssh_client.should_receive("do").with_args("show running-config interface vlan 1234").once().ordered().and_return([
+            "Building configuration...",
+            "Current configuration : 41 bytes",
+            "!",
+            "interface Vlan1234",
+            " no ip address",
+            "end"
+        ])
+
         self.mocked_ssh_client.should_receive("do").with_args("configure terminal").once().ordered().and_return([
             "Enter configuration commands, one per line.  End with CNTL/Z."
         ])
@@ -2479,4 +2501,42 @@ def test_set_vlan_icmp_redirects_state_disable(self):
 
         self.switch.set_vlan_icmp_redirects_state(1234, False)
 
+    def test_set_vlan_icmp_redirects_state_without_interface_creates_it(self):
+        self.command_setup()
 
+        self.mocked_ssh_client.should_receive("do").with_args("show running-config interface vlan 1234").once().ordered().and_return([
+            "                                  ^",
+            "% Invalid input detected at '^' marker.",
+        ])
+
+        self.mocked_ssh_client.should_receive("do").with_args("show running-config vlan 1234 | begin vlan").once().ordered().and_return([
+            "vlan 1234",
+            "end",
+        ])
+
+        self.mocked_ssh_client.should_receive("do").with_args("configure terminal").once().ordered().and_return([
+            "Enter configuration commands, one per line.  End with CNTL/Z."
+        ])
+        self.mocked_ssh_client.should_receive("do").with_args("interface vlan 1234").and_return([]).once().ordered()
+        self.mocked_ssh_client.should_receive("do").with_args("no shutdown").and_return([]).once().ordered()
+        self.mocked_ssh_client.should_receive("do").with_args("no ip redirects").and_return([]).once().ordered()
+        self.mocked_ssh_client.should_receive("do").with_args("exit").and_return([]).twice().ordered().ordered()
+        self.mocked_ssh_client.should_receive("do").with_args("write memory").and_return([]).once().ordered()
+
+        self.switch.set_vlan_icmp_redirects_state(1234, False)
+
+    def test_set_vlan_icmp_redirects_state_unknown_vlan(self):
+        self.command_setup()
+
+        self.mocked_ssh_client.should_receive("do").with_args("show running-config interface vlan 1234").once().ordered().and_return([
+            "                                  ^",
+            "% Invalid input detected at '^' marker.",
+        ])
+
+        self.mocked_ssh_client.should_receive("do").with_args("show running-config vlan 1234 | begin vlan").once().ordered().and_return([
+        ])
+
+        with self.assertRaises(UnknownVlan) as expect:
+            self.switch.set_vlan_icmp_redirects_state(1234, False)
+
+        assert_that(str(expect.exception), equal_to("Vlan 1234 not found"))