From db689f7221a18e33504fd1fe2bac3d3a5e190266 Mon Sep 17 00:00:00 2001 From: Daniel Mellado Area Date: Wed, 26 Jun 2019 16:48:46 +0200 Subject: [PATCH] Fix ovsdb module not being idempotent (#57735) openvswitch_db was not parsing properly some arguments, which caused some commands to be executed when they shouldn't. This commit fixesit and adds unit testing for the usecase. Closes-Bug: #55432 Closes-bug: #43858 --- .../modules/network/ovs/openvswitch_db.py | 4 +- .../targets/openvswitch_db/tests/basic.yaml | 65 +++++++++++++++---- .../openvswitch_db_disable_in_band_true.cfg | 2 +- .../network/ovs/test_openvswitch_db.py | 50 ++++++++++---- 4 files changed, 96 insertions(+), 25 deletions(-) diff --git a/lib/ansible/modules/network/ovs/openvswitch_db.py b/lib/ansible/modules/network/ovs/openvswitch_db.py index 5a4c794d526..347fab1f452 100644 --- a/lib/ansible/modules/network/ovs/openvswitch_db.py +++ b/lib/ansible/modules/network/ovs/openvswitch_db.py @@ -157,7 +157,7 @@ def map_config_to_obj(module): if NON_EMPTY_MAP_RE.match(col_value): for kv in col_value[1:-1].split(', '): k, v = kv.split('=') - col_value_to_dict[k.strip()] = v.strip() + col_value_to_dict[k.strip()] = v.strip('\"') obj = { 'table': module.params['table'], @@ -176,6 +176,8 @@ def map_config_to_obj(module): def map_params_to_obj(module): + if module.params['value'] in ['True', 'False']: + module.params['value'] = module.params['value'].lower() obj = { 'table': module.params['table'], 'record': module.params['record'], diff --git a/test/integration/targets/openvswitch_db/tests/basic.yaml b/test/integration/targets/openvswitch_db/tests/basic.yaml index 5bfae852229..4ad9ec0b097 100644 --- a/test/integration/targets/openvswitch_db/tests/basic.yaml +++ b/test/integration/targets/openvswitch_db/tests/basic.yaml @@ -21,7 +21,7 @@ - assert: that: - - "result.changed == true" + - result is changed - name: Create bridge again (idempotent) openvswitch_db: @@ -35,7 +35,50 @@ - assert: that: - - "result.changed == false" + - result is not changed + +- name: Change key value with quotes + openvswitch_db: + table: open_vswitch + record: . + col: other_config + key: pmd-cpu-mask + value: "0xaaa00000000" + become: yes + register: result + +- assert: + that: + - result is changed + +- name: Change keyvalue with quotes(idempotent) + openvswitch_db: + table: open_vswitch + record: . + col: other_config + key: pmd-cpu-mask + value: "0xaaa00000000" + become: yes + register: result + +- assert: + that: + - result is not changed + +- name: Remove key value with quotes + openvswitch_db: + table: open_vswitch + record: . + col: other_config + key: pmd-cpu-mask + value: "0xaaa00000000" + state: absent + become: yes + register: result + +- assert: + that: + - result is changed - name: Change column value in a map openvswitch_db: @@ -49,7 +92,7 @@ - assert: that: - - "result.changed == true" + - result is changed - name: Change column value in a map again (idempotent) openvswitch_db: @@ -63,33 +106,33 @@ - assert: that: - - "result.changed == false" + - result is not changed - name: Change column value openvswitch_db: table: Bridge record: br-test col: stp_enable - value: 'true' + value: true become: yes register: result - assert: that: - - "result.changed == true" + - result is changed - name: Change column value again (idempotent) openvswitch_db: table: Bridge record: br-test col: stp_enable - value: 'true' + value: true become: yes register: result - assert: that: - - "result.changed == false" + - result is not changed - name: Try to set value on a map type without a key (negative) ignore_errors: true @@ -103,7 +146,7 @@ - assert: that: - - "result.failed == true" + - result is failed - name: Remove bridge openvswitch_db: @@ -118,7 +161,7 @@ - assert: that: - - "result.changed == true" + - result is changed - name: Remove bridge again (idempotent) openvswitch_db: @@ -133,7 +176,7 @@ - assert: that: - - "result.changed == false" + - result is not changed - name: Tear down test bridge command: ovs-vsctl del-br br-test diff --git a/test/units/modules/network/ovs/fixtures/openvswitch_db_disable_in_band_true.cfg b/test/units/modules/network/ovs/fixtures/openvswitch_db_disable_in_band_true.cfg index 53e1a4e4554..4b9f3a799f1 100644 --- a/test/units/modules/network/ovs/fixtures/openvswitch_db_disable_in_band_true.cfg +++ b/test/units/modules/network/ovs/fixtures/openvswitch_db_disable_in_band_true.cfg @@ -13,7 +13,7 @@ mcast_snooping_enable: false mirrors : [] name : test-br netflow : [] -other_config : {disable-in-band=True} +other_config : {disable-in-band=true} ports : [2c9c1b35-a304-4dee-bb7a-092d656543c7] protocols : [] rstp_enable : false diff --git a/test/units/modules/network/ovs/test_openvswitch_db.py b/test/units/modules/network/ovs/test_openvswitch_db.py index 99262b4ffb4..0b0b154c0ec 100644 --- a/test/units/modules/network/ovs/test_openvswitch_db.py +++ b/test/units/modules/network/ovs/test_openvswitch_db.py @@ -20,11 +20,26 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type -from units.compat.mock import patch from ansible.modules.network.ovs import openvswitch_db +from units.compat.mock import patch, MagicMock from units.modules.utils import set_module_args from .ovs_module import TestOpenVSwitchModule, load_fixture +import pytest + + +@pytest.fixture +def patched_openvswitch_db(monkeypatch): + mocked_ovs_db = MagicMock() + mocked_ovs_db.return_value = {'table': 'open_vswitch', + 'record': '.', + 'col': 'other_config', + 'key': 'pmd-cpu-mask', + 'value': '0xaaa00000000'} + monkeypatch.setattr(openvswitch_db, 'map_config_to_obj', mocked_ovs_db) + return openvswitch_db + + test_name_side_effect_matrix = { 'test_openvswitch_db_absent_idempotent': [ (0, 'openvswitch_db_disable_in_band_missing.cfg', None), @@ -35,6 +50,8 @@ test_name_side_effect_matrix = { 'test_openvswitch_db_present_idempotent': [ (0, 'openvswitch_db_disable_in_band_true.cfg', None), (0, None, None)], + 'test_openvswitch_db_present_idempotent_value': [ + (0, None, None)], 'test_openvswitch_db_present_adds_key': [ (0, 'openvswitch_db_disable_in_band_missing.cfg', None), (0, None, None)], @@ -86,62 +103,71 @@ class TestOpenVSwitchDBModule(TestOpenVSwitchModule): set_module_args(dict(state='absent', table='Bridge', record='test-br', col='other_config', key='disable-in-band', - value='True')) + value='true')) self.execute_module(test_name='test_openvswitch_db_absent_idempotent') def test_openvswitch_db_absent_removes_key(self): set_module_args(dict(state='absent', table='Bridge', record='test-br', col='other_config', key='disable-in-band', - value='True')) + value='true')) self.execute_module( changed=True, commands=['/usr/bin/ovs-vsctl -t 5 remove Bridge test-br other_config' - ' disable-in-band=True'], + ' disable-in-band=true'], test_name='test_openvswitch_db_absent_removes_key') def test_openvswitch_db_present_idempotent(self): set_module_args(dict(state='present', table='Bridge', record='test-br', col='other_config', key='disable-in-band', - value='True')) + value='true')) self.execute_module(test_name='test_openvswitch_db_present_idempotent') + @pytest.mark.usefixtures('patched_openvswitch_db') + def test_openvswitch_db_present_idempotent_value(self): + set_module_args({"col": "other_config", + "key": "pmd-cpu-mask", + "record": ".", + "table": "open_vswitch", + "value": "0xaaa00000000"}) + self.execute_module(test_name='test_openvswitch_db_present_idempotent_value') + def test_openvswitch_db_present_adds_key(self): set_module_args(dict(state='present', table='Bridge', record='test-br', col='other_config', key='disable-in-band', - value='True')) + value='true')) self.execute_module( changed=True, commands=['/usr/bin/ovs-vsctl -t 5 set Bridge test-br other_config' - ':disable-in-band=True'], + ':disable-in-band=true'], test_name='test_openvswitch_db_present_adds_key') def test_openvswitch_db_present_updates_key(self): set_module_args(dict(state='present', table='Bridge', record='test-br', col='other_config', key='disable-in-band', - value='False')) + value='false')) self.execute_module( changed=True, commands=['/usr/bin/ovs-vsctl -t 5 set Bridge test-br other_config' - ':disable-in-band=False'], + ':disable-in-band=false'], test_name='test_openvswitch_db_present_updates_key') def test_openvswitch_db_present_missing_key_on_map(self): set_module_args(dict(state='present', table='Bridge', record='test-br', col='other_config', - value='False')) + value='false')) self.execute_module( failed=True, - test_name='test_openvswitch_db_present_idempotent') + test_name='test_openvswitch_db_present_missing_key_on_map') def test_openvswitch_db_present_stp_enable(self): set_module_args(dict(state='present', table='Bridge', record='test-br', col='stp_enable', - value='False')) + value='true')) self.execute_module(changed=True, test_name='test_openvswitch_db_present_stp_enable')