diff --git a/networking_generic_switch/devices/netmiko_devices/__init__.py b/networking_generic_switch/devices/netmiko_devices/__init__.py index 5db17fca..c1e246b7 100644 --- a/networking_generic_switch/devices/netmiko_devices/__init__.py +++ b/networking_generic_switch/devices/netmiko_devices/__init__.py @@ -24,6 +24,7 @@ from tooz import coordination from networking_generic_switch import devices +from networking_generic_switch.devices import utils as device_utils from networking_generic_switch import exceptions as exc from networking_generic_switch import locking as ngs_lock @@ -117,11 +118,11 @@ def _create_connection(): except tenacity.RetryError as e: LOG.error("Reached maximum SSH connection attempts, not retrying") raise exc.GenericSwitchNetmikoConnectError( - config=self.config, error=e) + config=device_utils.sanitise_config(self.config), error=e) except Exception as e: LOG.error("Unexpected exception during SSH connection") raise exc.GenericSwitchNetmikoConnectError( - config=self.config, error=e) + config=device_utils.sanitise_config(self.config), error=e) # Now yield the connection to the caller. with net_connect: @@ -135,15 +136,17 @@ def send_commands_to_device(self, cmd_set): try: with ngs_lock.PoolLock(self.locker, **self.lock_kwargs): with self._get_connection() as net_connect: - net_connect.enable() - output = net_connect.send_config_set( - config_commands=cmd_set) + output = self.send_config_set(net_connect, cmd_set) # NOTE (vsaienko) always save configuration # when configuration is applied successfully. self.save_configuration(net_connect) + except exc.GenericSwitchException as e: + # Reraise without modification exceptions originating from this + # module. + raise except Exception as e: - raise exc.GenericSwitchNetmikoConnectError(config=self.config, - error=e) + raise exc.GenericSwitchNetmikoConnectError( + config=device_utils.sanitise_config(self.config), error=e) LOG.debug(output) return output @@ -187,6 +190,16 @@ def delete_port(self, port, segmentation_id): port=port, segmentation_id=segmentation_id)) + def send_config_set(self, net_connect, cmd_set): + """Send a set of configuration lines to the device. + + :param net_connect: a netmiko connection object. + :param cmd_set: a list of configuration lines to send. + :returns: The output of the configuration commands. + """ + net_connect.enable() + return net_connect.send_config_set(config_commands=cmd_set) + def save_configuration(self, net_connect): """Save the device's configuration. diff --git a/networking_generic_switch/devices/netmiko_devices/juniper.py b/networking_generic_switch/devices/netmiko_devices/juniper.py index a64e46ad..9c3dde14 100644 --- a/networking_generic_switch/devices/netmiko_devices/juniper.py +++ b/networking_generic_switch/devices/netmiko_devices/juniper.py @@ -12,7 +12,23 @@ # License for the specific language governing permissions and limitations # under the License. +import logging + +import tenacity + from networking_generic_switch.devices import netmiko_devices +from networking_generic_switch.devices import utils as device_utils +from networking_generic_switch import exceptions as exc + +LOG = logging.getLogger(__name__) + +# Internal ngs options will not be passed to driver. +JUNIPER_INTERNAL_OPTS = [ + # Timeout (seconds) for committing configuration changes. + {'name': 'ngs_commit_timeout', 'default': 60}, + # Interval (seconds) between attempts to commit configuration changes. + {'name': 'ngs_commit_interval', 'default': 5}, +] class Juniper(netmiko_devices.NetmikoSwitch): @@ -49,11 +65,89 @@ class Juniper(netmiko_devices.NetmikoSwitch): 'vlan members {segmentation_id}', ) + def __init__(self, device_cfg): + super(Juniper, self).__init__(device_cfg) + + # Do not expose Juniper internal options to device config. + for opt in JUNIPER_INTERNAL_OPTS: + opt_name = opt['name'] + if opt_name in self.config: + self.ngs_config[opt_name] = self.config.pop(opt_name) + elif 'default' in opt: + self.ngs_config[opt_name] = opt['default'] + + def send_config_set(self, net_connect, cmd_set): + """Send a set of configuration lines to the device. + + :param net_connect: a netmiko connection object. + :param cmd_set: a list of configuration lines to send. + :returns: The output of the configuration commands. + """ + # We use the private configuration mode, which hides the configuration + # changes of concurrent sessions from us, and discards uncommitted + # changes on termination of the session. + net_connect.config_mode(config_command='configure private') + + # Don't exit configuration mode, as we still need to commit the changes + # in save_configuration(). + return net_connect.send_config_set(config_commands=cmd_set, + exit_config_mode=False) + def save_configuration(self, net_connect): """Save the device's configuration. :param net_connect: a netmiko connection object. + :raises GenericSwitchNetmikoConfigError if saving the configuration + fails. """ # Junos configuration is transactional, and requires an explicit commit - # of changes in order for them to be applied. - net_connect.commit() + # of changes in order for them to be applied. Since committing requires + # an exclusive lock on the configuration database, it can fail if + # another session has a lock. We use a retry mechanism to work around + # this. + + class DBLocked(Exception): + """Switch configuration DB is locked by another user.""" + + def __init__(self, err): + self.err = err + + @tenacity.retry( + # Log a message after each failed attempt. + after=tenacity.after_log(LOG, logging.DEBUG), + # Reraise exceptions if our final attempt fails. + reraise=True, + # Retry on failure to commit the configuration due to the DB + # being locked by another session. + retry=(tenacity.retry_if_exception_type(DBLocked)), + # Stop after the configured timeout. + stop=tenacity.stop_after_delay( + int(self.ngs_config['ngs_commit_timeout'])), + # Wait for the configured interval between attempts. + wait=tenacity.wait_fixed( + int(self.ngs_config['ngs_commit_interval'])), + ) + def commit(): + try: + net_connect.commit() + except ValueError as e: + # Netmiko raises ValueError on commit failure, and appends the + # CLI output to the exception message. Raise a more specific + # exception for a locked DB, on which tenacity will retry. + if "error: configuration database locked" in str(e): + raise DBLocked(e) + raise + + try: + commit() + except DBLocked as e: + msg = ("Reached timeout waiting for switch configuration DB lock: " + "%s" % e.err) + LOG.error(msg) + raise exc.GenericSwitchNetmikoConfigError( + config=device_utils.sanitise_config(self.config), error=msg) + except ValueError as e: + msg = "Failed to commit configuration: %s" % e + LOG.error(msg) + raise exc.GenericSwitchNetmikoConfigError( + config=device_utils.sanitise_config(self.config), error=msg) diff --git a/networking_generic_switch/devices/utils.py b/networking_generic_switch/devices/utils.py index f05dd2c0..7fdd2c15 100644 --- a/networking_generic_switch/devices/utils.py +++ b/networking_generic_switch/devices/utils.py @@ -33,3 +33,16 @@ def get_switch_device(switches, switch_info=None, return switch if switch_info: return switches.get(switch_info) + + +def sanitise_config(config): + """Return a sanitised the configuration of a switch device. + + :param config: a configuration dict to sanitise. + :returns: a copy of the configuration, with sensitive fields removed. + """ + sanitised_fields = {"password"} + return { + key: "******" if key in sanitised_fields else value + for key, value in config.items() + } diff --git a/networking_generic_switch/exceptions.py b/networking_generic_switch/exceptions.py index e8a2d7bd..29fc0743 100644 --- a/networking_generic_switch/exceptions.py +++ b/networking_generic_switch/exceptions.py @@ -35,3 +35,7 @@ class GenericSwitchNetmikoNotSupported(GenericSwitchException): class GenericSwitchNetmikoConnectError(GenericSwitchException): message = _("Netmiko connection error: %(config)s, error: %(error)s") + + +class GenericSwitchNetmikoConfigError(GenericSwitchException): + message = _("Netmiko configuration error: %(config)s, error: %(error)s") diff --git a/networking_generic_switch/tests/unit/devices/test_utils.py b/networking_generic_switch/tests/unit/devices/test_utils.py index 8b97a495..cfc5176d 100644 --- a/networking_generic_switch/tests/unit/devices/test_utils.py +++ b/networking_generic_switch/tests/unit/devices/test_utils.py @@ -52,3 +52,9 @@ def test_get_switch_device_fallback_to_switch_info(self): self.assertEqual(self.devices['A'], device_utils.get_switch_device( self.devices, switch_info='A', ngs_mac_address='11:22:33:44:55:77')) + + def test_sanitise_config(self): + config = {'username': 'fake-user', 'password': 'fake-password'} + result = device_utils.sanitise_config(config) + expected = {'username': 'fake-user', 'password': '******'} + self.assertEqual(expected, result) diff --git a/networking_generic_switch/tests/unit/netmiko/test_juniper.py b/networking_generic_switch/tests/unit/netmiko/test_juniper.py index 3426e411..2d599861 100644 --- a/networking_generic_switch/tests/unit/netmiko/test_juniper.py +++ b/networking_generic_switch/tests/unit/netmiko/test_juniper.py @@ -13,8 +13,12 @@ # under the License. import mock +import netmiko +import tenacity +from networking_generic_switch.devices import netmiko_devices from networking_generic_switch.devices.netmiko_devices import juniper +from networking_generic_switch import exceptions as exc from networking_generic_switch.tests.unit.netmiko import test_netmiko_base @@ -51,7 +55,8 @@ def test_add_network_with_trunk_ports(self, mock_exec): 'NetmikoSwitch.send_commands_to_device') def test_del_network(self, mock_exec): self.switch.del_network(33, '0ae071f55be943e480eae41fefe85b21') - mock_exec.assert_called_with(['delete vlans 0ae071f55be943e480eae41fefe85b21']) + mock_exec.assert_called_with( + ['delete vlans 0ae071f55be943e480eae41fefe85b21']) @mock.patch('networking_generic_switch.devices.netmiko_devices.' 'NetmikoSwitch.send_commands_to_device') @@ -83,11 +88,74 @@ def test_delete_port(self, mock_exec): ['delete interface 3333 unit 0 family ethernet-switching ' 'vlan members']) + def test_send_config_set(self): + connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection) + connect_mock.send_config_set.return_value = 'fake output' + result = self.switch.send_config_set(connect_mock, ['spam ham aaaa']) + self.assertFalse(connect_mock.enable.called) + connect_mock.send_config_set.assert_called_once_with( + config_commands=['spam ham aaaa'], exit_config_mode=False) + self.assertEqual('fake output', result) + def test_save_configuration(self): mock_connection = mock.Mock() self.switch.save_configuration(mock_connection) mock_connection.commit.assert_called_once_with() + @mock.patch.object(netmiko_devices.tenacity, 'wait_fixed', + return_value=tenacity.wait_fixed(0.01)) + @mock.patch.object(netmiko_devices.tenacity, 'stop_after_delay', + return_value=tenacity.stop_after_delay(0.1)) + def test_save_configuration_timeout(self, m_stop, m_wait): + mock_connection = mock.Mock() + output = """ +error: configuration database locked by: + user terminal p0 (pid 1234) on since 2017-1-1 00:00:00 UTC + exclusive private [edit] + +{master:0}[edit]""" + mock_connection.commit.side_effect = ValueError( + "Commit failed with the following errors:\n\n{0}".format(output)) + + self.assertRaisesRegexp(exc.GenericSwitchNetmikoConfigError, + "Reached timeout waiting for", + self.switch.save_configuration, + mock_connection) + self.assertGreater(mock_connection.commit.call_count, 1) + m_stop.assert_called_once_with(60) + m_wait.assert_called_once_with(5) + + def test_save_configuration_error(self): + mock_connection = mock.Mock() + output = """ +[edit vlans] + 'duplicate-vlan' + l2ald: Duplicate vlan-id exists for vlan duplicate-vlan +[edit vlans] + Failed to parse vlan hierarchy completely +error: configuration check-out failed + +{master:0}[edit]""" + mock_connection.commit.side_effect = ValueError( + "Commit failed with the following errors:\n\n{0}".format(output)) + + self.assertRaisesRegexp(exc.GenericSwitchNetmikoConfigError, + "Failed to commit configuration", + self.switch.save_configuration, + mock_connection) + mock_connection.commit.assert_called_once_with() + + @mock.patch.object(netmiko_devices.tenacity, 'wait_fixed') + @mock.patch.object(netmiko_devices.tenacity, 'stop_after_delay') + def test_save_configuration_non_default_timing(self, m_stop, m_wait): + self.switch = self._make_switch_device({'ngs_commit_timeout': 42, + 'ngs_commit_interval': 43}) + mock_connection = mock.Mock() + self.switch.save_configuration(mock_connection) + mock_connection.commit.assert_called_once_with() + m_stop.assert_called_once_with(42) + m_wait.assert_called_once_with(43) + def test__format_commands(self): cmd_set = self.switch._format_commands( juniper.Juniper.ADD_NETWORK, diff --git a/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py b/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py index fb688439..b7e68274 100644 --- a/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py +++ b/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py @@ -144,14 +144,93 @@ def test_send_commands_to_device_empty(self, gc_mock): self.assertFalse(connect_mock.send_command.called) @mock.patch.object(netmiko_devices.NetmikoSwitch, '_get_connection') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'send_config_set') @mock.patch.object(netmiko_devices.NetmikoSwitch, 'save_configuration') - def test_send_commands_to_device(self, save_mock, gc_mock): + def test_send_commands_to_device(self, save_mock, send_mock, gc_mock): connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection) gc_mock.return_value.__enter__.return_value = connect_mock - self.switch.send_commands_to_device(['spam ham aaaa']) + send_mock.return_value = 'fake output' + result = self.switch.send_commands_to_device(['spam ham aaaa']) + send_mock.assert_called_once_with(connect_mock, ['spam ham aaaa']) + self.assertEqual('fake output', result) + save_mock.assert_called_once_with(connect_mock) + + @mock.patch.object(netmiko_devices.NetmikoSwitch, '_get_connection') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'send_config_set') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'save_configuration') + def test_send_commands_to_device_send_failure(self, save_mock, send_mock, + gc_mock): + connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection) + gc_mock.return_value.__enter__.return_value = connect_mock + + class FakeError(Exception): + pass + send_mock.side_effect = FakeError + self.assertRaises(exc.GenericSwitchNetmikoConnectError, + self.switch.send_commands_to_device, + ['spam ham aaaa']) + send_mock.assert_called_once_with(connect_mock, ['spam ham aaaa']) + self.assertFalse(save_mock.called) + + @mock.patch.object(netmiko_devices.NetmikoSwitch, '_get_connection') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'send_config_set') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'save_configuration') + def test_send_commands_to_device_send_ngs_failure(self, save_mock, + send_mock, gc_mock): + connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection) + gc_mock.return_value.__enter__.return_value = connect_mock + + class NGSFakeError(exc.GenericSwitchException): + pass + send_mock.side_effect = NGSFakeError + self.assertRaises(NGSFakeError, self.switch.send_commands_to_device, + ['spam ham aaaa']) + send_mock.assert_called_once_with(connect_mock, ['spam ham aaaa']) + self.assertFalse(save_mock.called) + + @mock.patch.object(netmiko_devices.NetmikoSwitch, '_get_connection') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'send_config_set') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'save_configuration') + def test_send_commands_to_device_save_failure(self, save_mock, send_mock, + gc_mock): + connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection) + gc_mock.return_value.__enter__.return_value = connect_mock + + class FakeError(Exception): + pass + send_mock.return_value = 'fake output' + save_mock.side_effect = FakeError + self.assertRaises(exc.GenericSwitchNetmikoConnectError, + self.switch.send_commands_to_device, + ['spam ham aaaa']) + send_mock.assert_called_once_with(connect_mock, ['spam ham aaaa']) + save_mock.assert_called_once_with(connect_mock) + + @mock.patch.object(netmiko_devices.NetmikoSwitch, '_get_connection') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'send_config_set') + @mock.patch.object(netmiko_devices.NetmikoSwitch, 'save_configuration') + def test_send_commands_to_device_save_ngs_failure(self, save_mock, + send_mock, gc_mock): + connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection) + gc_mock.return_value.__enter__.return_value = connect_mock + + class NGSFakeError(exc.GenericSwitchException): + pass + send_mock.return_value = 'fake output' + save_mock.side_effect = NGSFakeError + self.assertRaises(NGSFakeError, self.switch.send_commands_to_device, + ['spam ham aaaa']) + send_mock.assert_called_once_with(connect_mock, ['spam ham aaaa']) + save_mock.assert_called_once_with(connect_mock) + + def test_send_config_set(self): + connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection) + connect_mock.send_config_set.return_value = 'fake output' + result = self.switch.send_config_set(connect_mock, ['spam ham aaaa']) + connect_mock.enable.assert_called_once_with() connect_mock.send_config_set.assert_called_once_with( config_commands=['spam ham aaaa']) - save_mock.assert_called_once_with(connect_mock) + self.assertEqual('fake output', result) def test_save_configuration(self): connect_mock = mock.MagicMock(netmiko.base_connection.BaseConnection)