openvswitch_db: Make 'key' parameter optional (#42110) (#43552)

* openvswitch_db: Split key-value pairs correctly (#33335)

Map values can contain commas, e.g.
    - name: Configure OVN bridge mapping
      openvswitch_db:
        state: present                                                                                                                                                                                                                                table: open_vswitch                                                                                                                                                                                                                           record: .                                                                                                                                                                                                                                     col: external_ids                                                                                                                                                                                                                             key: ovn-bridge-mappings
        value: '"vmnet-static:br-vmnet-st,vmnet-dynamic:br-vmnet-dyn"'

Previous behaviour was splitting the value and raised an exception.
(cherry picked from commit 3c53e2f8ea)

* openvswitch_db: Make 'key' parameter optional (#42110)

The OVSDB schema consists of typed columns. The 'key' parameter is
required only for columns with type of a 'map'. This patch makes 'key'
an optional parameter to allow setting values for other column types
like int.

Fixes #42108

(cherry picked from commit 26b0908270)
(cherry picked from commit 01097715fd)

changelogs/fragments/openvswitch_db_make_key_optional.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - openvswitch_db - make 'key' argument optional https://github.com/ansible/ansible/issues/42108

lib/ansible/modules/network/ovs/openvswitch_db.py

@@ -48,9 +48,10 @@ options:
         description:
             - Identifies the column in the record.
     key:
-        required: true
+        required: false
         description:
-            - Identifies the key in the record column
+            - Identifies the key in the record column, when the column is a map
+              type.
     value:
         required: true
         description:
@@ -87,11 +88,23 @@ EXAMPLES = '''
     record: br-int
     col: other_config
     key: disable-in-band
+
+# Mark port with tag 10
+- openvswitch_db:
+    table: Port
+    record: port0
+    col: tag
+    value: 10
 '''
 import re
 
 from ansible.module_utils.basic import AnsibleModule
 
+# Regular expression for map type, must not be empty
+NON_EMPTY_MAP_RE = re.compile(r'{.+}')
+# Regular expression for a map column type
+MAP_RE = re.compile(r'{.*}')
+
 
 def map_obj_to_commands(want, have, module):
     """ Define ovs-vsctl command to meet desired state """
@@ -102,8 +115,16 @@ def map_obj_to_commands(want, have, module):
             templatized_command = "%(ovs-vsctl)s -t %(timeout)s remove %(table)s %(record)s " \
                                   "%(col)s %(key)s=%(value)s"
             commands.append(templatized_command % module.params)
+        elif module.params['key'] is None:
+            templatized_command = "%(ovs-vsctl)s -t %(timeout)s remove %(table)s %(record)s " \
+                                  "%(col)s"
+            commands.append(templatized_command % module.params)
     else:
-        if 'key' not in have.keys():
+        if module.params['key'] is None:
+            templatized_command = "%(ovs-vsctl)s -t %(timeout)s set %(table)s %(record)s " \
+                                  "%(col)s=%(value)s"
+            commands.append(templatized_command % module.params)
+        elif 'key' not in have.keys():
             templatized_command = "%(ovs-vsctl)s -t %(timeout)s add %(table)s %(record)s " \
                                   "%(col)s %(key)s=%(value)s"
             commands.append(templatized_command % module.params)
@@ -125,9 +146,17 @@ def map_config_to_obj(module):
     match = re.search(r'^' + module.params['col'] + r'(\s+):(\s+)(.*)$', out, re.M)
 
     col_value = match.group(3)
+
+    # Map types require key argument
+    has_key = module.params['key'] is not None
+    is_map = MAP_RE.match(col_value)
+    if is_map and not has_key:
+        module.fail_json(
+            msg="missing required arguments: key for map type of column")
+
     col_value_to_dict = {}
-    if col_value and col_value != '{}':
-        for kv in col_value[1:-1].split(','):
+    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()
 
@@ -137,9 +166,12 @@ def map_config_to_obj(module):
         'col': module.params['col'],
     }
 
-    if module.params['key'] in col_value_to_dict:
-        obj['key'] = module.params['key']
-        obj['value'] = col_value_to_dict[module.params['key']]
+    if has_key and is_map:
+        if module.params['key'] in col_value_to_dict:
+            obj['key'] = module.params['key']
+            obj['value'] = col_value_to_dict[module.params['key']]
+    else:
+            obj['value'] = col_value.strip()
 
     return obj
 
@@ -149,10 +181,13 @@ def map_params_to_obj(module):
         'table': module.params['table'],
         'record': module.params['record'],
         'col': module.params['col'],
-        'key': module.params['key'],
         'value': module.params['value']
     }
 
+    key = module.params['key']
+    if key is not None:
+        obj['key'] = key
+
     return obj
 
 
@@ -163,7 +198,7 @@ def main():
         'table': {'required': True},
         'record': {'required': True},
         'col': {'required': True},
-        'key': {'required': True},
+        'key': {'required': False},
         'value': {'required': True},
         'timeout': {'default': 5, 'type': 'int'},
     }

test/integration/targets/openvswitch_db/tests/basic.yaml

@@ -33,7 +33,7 @@
     that:
       - "result.changed == false"
 
-- name: Change column value
+- name: Change column value in a map
   openvswitch_db:
     table: Bridge
     record: br-test
@@ -46,7 +46,7 @@
     that:
       - "result.changed == true"
 
-- name: Change column value again (idempotent)
+- name: Change column value in a map again (idempotent)
   openvswitch_db:
     table: Bridge
     record: br-test
@@ -59,6 +59,43 @@
     that:
       - "result.changed == false"
 
+- name: Change column value
+  openvswitch_db:
+    table: Bridge
+    record: br-test
+    col: stp_enable
+    value: true
+  register: result
+
+- assert:
+    that:
+      - "result.changed == true"
+
+- name: Change column value again (idempotent)
+  openvswitch_db:
+    table: Bridge
+    record: br-test
+    col: stp_enable
+    value: true
+  register: result
+
+- assert:
+    that:
+      - "result.changed == false"
+
+- name: Try to set value on a map type without a key (negative)
+  ignore_errors: true
+  openvswitch_db:
+    table: Bridge
+    record: br-test
+    col: other_config
+    value: true
+  register: result
+
+- assert:
+    that:
+      - "result.failed == true"
+
 - name: Remove bridge
   openvswitch_db:
     table: Bridge

test/units/modules/network/ovs/test_openvswitch_db.py

@@ -41,6 +41,12 @@ test_name_side_effect_matrix = {
     'test_openvswitch_db_present_updates_key': [
         (0, 'openvswitch_db_disable_in_band_true.cfg', None),
         (0, None, None)],
+    'test_openvswitch_db_present_missing_key_on_map': [
+        (0, 'openvswitch_db_disable_in_band_true.cfg', None),
+        (0, None, None)],
+    'test_openvswitch_db_present_stp_enable': [
+        (0, 'openvswitch_db_disable_in_band_true.cfg', None),
+        (0, None, None)],
 }
 
 
@@ -122,3 +128,20 @@ class TestOpenVSwitchDBModule(TestOpenVSwitchModule):
             commands=['/usr/bin/ovs-vsctl -t 5 set Bridge test-br other_config'
                       ':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'))
+        self.execute_module(
+            failed=True,
+            test_name='test_openvswitch_db_present_idempotent')
+
+    def test_openvswitch_db_present_stp_enable(self):
+        set_module_args(dict(state='present',
+                             table='Bridge', record='test-br',
+                             col='stp_enable',
+                             value='False'))
+        self.execute_module(changed=True,
+                            test_name='test_openvswitch_db_present_stp_enable')