Add unit test for listener_port validation

certain combinations of listener_port and
peers_from_control_nodes are invalid at the
serializer level.

namely, if peers_from_control_nodes is True,
a listener_port must be set.

Further, payload may not include one of these
fields at all. Validation needs to handle this case
as well by checking the value on the existing
canonical address.

The parametrized unit test was createdy from itertools
product of the following:

  initial_port       [None, 27199]
  payload_port       [None, 27199, 27200, 'missing']
  initial_peers_from [False, True]
  payload_peers_from [False, True, 'missing']

where initial_port is the starting listener_port
and initial_peers_from is the starting peers_from_control_nodes
value

An instance is first patched with those initial values.

Then a 2nd patch uses the payload_* values.

The test then asserts HTTP codes and in the case of 400,
that the validation error matches the expected error message.

Signed-off-by: Seth Foster <[email protected]>

awx/api/serializers.py

@@ -5709,9 +5709,21 @@ def check_peers_changed():
             raise serializers.ValidationError(_("Cannot peer to the same instance more than once."))
 
         # cannot enable peers_from_control_nodes if listener_port is not set
-        if attrs.get('peers_from_control_nodes'):
-            if not attrs.get('listener_port') and self.instance and self.instance.canonical_address_port is None:
-                raise serializers.ValidationError(_("Cannot enable peers_from_control_nodes if listener_port is not set."))
+        listener_port = None
+        peers_from_control_nodes = None
+
+        if 'listener_port' in attrs:
+            listener_port = attrs.get('listener_port')
+        elif self.instance:
+            listener_port = self.instance.canonical_address_port
+
+        if 'peers_from_control_nodes' in attrs:
+            peers_from_control_nodes = attrs.get('peers_from_control_nodes')
+        elif self.instance:
+            peers_from_control_nodes = self.instance.canonical_address_peers_from_control_nodes
+
+        if peers_from_control_nodes and not listener_port:
+            raise serializers.ValidationError(_("Cannot enable peers_from_control_nodes if listener_port is not set."))
 
         return super().validate(attrs)
 

awx/main/tests/functional/api/test_instance_peers.py

@@ -63,6 +63,102 @@ def test_changing_node_type(self, admin_user, patch):
         )
         assert 'Cannot change node type.' in str(resp.data)
 
+    @pytest.mark.parametrize(
+        'initial_port, payload_port, initial_peers_from, payload_peers_from, initial_expect, payload_expect',
+        [
+            (None, None, False, False, 200, 200),
+            (None, None, False, True, 200, 400),
+            (None, None, False, 'missing', 200, 200),
+            (None, None, True, False, 400, None),
+            (None, None, True, True, 400, None),
+            (None, None, True, 'missing', 400, None),
+            (None, 27199, False, False, 200, 200),
+            (None, 27199, False, True, 200, 200),
+            (None, 27199, False, 'missing', 200, 200),
+            (None, 27199, True, False, 400, None),
+            (None, 27199, True, True, 400, None),
+            (None, 27199, True, 'missing', 400, None),
+            (None, 27200, False, False, 200, 200),
+            (None, 27200, False, True, 200, 200),
+            (None, 27200, False, 'missing', 200, 200),
+            (None, 27200, True, False, 400, None),
+            (None, 27200, True, True, 400, None),
+            (None, 27200, True, 'missing', 400, None),
+            (None, 'missing', False, False, 200, 200),
+            (None, 'missing', False, True, 200, 400),
+            (None, 'missing', False, 'missing', 200, 200),
+            (None, 'missing', True, False, 400, None),
+            (None, 'missing', True, True, 400, None),
+            (None, 'missing', True, 'missing', 400, None),
+            (27199, None, False, False, 200, 200),
+            (27199, None, False, True, 200, 400),
+            (27199, None, False, 'missing', 200, 200),
+            (27199, None, True, False, 200, 200),
+            (27199, None, True, True, 200, 400),
+            (27199, None, True, 'missing', 200, 400),
+            (27199, 27199, False, False, 200, 200),
+            (27199, 27199, False, True, 200, 200),
+            (27199, 27199, False, 'missing', 200, 200),
+            (27199, 27199, True, False, 200, 200),
+            (27199, 27199, True, True, 200, 200),
+            (27199, 27199, True, 'missing', 200, 200),
+            (27199, 27200, False, False, 200, 400),
+            (27199, 27200, False, True, 200, 400),
+            (27199, 27200, False, 'missing', 200, 400),
+            (27199, 27200, True, False, 200, 400),
+            (27199, 27200, True, True, 200, 400),
+            (27199, 27200, True, 'missing', 200, 400),
+            (27199, 'missing', False, False, 200, 200),
+            (27199, 'missing', False, True, 200, 200),
+            (27199, 'missing', False, 'missing', 200, 200),
+            (27199, 'missing', True, False, 200, 200),
+            (27199, 'missing', True, True, 200, 200),
+            (27199, 'missing', True, 'missing', 200, 200),
+        ],
+    )
+    def test_listener_port_and_peers_from_control_node(
+        self, initial_port, payload_port, initial_peers_from, payload_peers_from, initial_expect, payload_expect, admin_user, patch
+    ):
+        """
+        ensure listener_port and peers_from_control_nodes are validated correctly
+        parameterized to test all combinations of initial data, and payload data
+        'missing' means the field is not included in the payload
+        """
+        data = {}
+        data['listener_port'] = initial_port
+        data['peers_from_control_nodes'] = initial_peers_from
+
+        hop = Instance.objects.create(hostname='abc', node_type="hop")
+
+        patch(
+            url=reverse('api:instance_detail', kwargs={'pk': hop.pk}),
+            data=data,
+            user=admin_user,
+            expect=initial_expect,
+        )
+
+        if initial_expect == 400:
+            return
+
+        data = {}
+        if payload_port is not 'missing':
+            data['listener_port'] = payload_port
+        if payload_peers_from is not 'missing':
+            data['peers_from_control_nodes'] = payload_peers_from
+
+        resp = patch(
+            url=reverse('api:instance_detail', kwargs={'pk': hop.pk}),
+            data=data,
+            user=admin_user,
+            expect=payload_expect,
+        )
+
+        if payload_expect == 400:
+            if payload_port == 27200:
+                assert 'Cannot change listener port.' in str(resp.data)
+            else:
+                assert "Cannot enable peers_from_control_nodes if listener_port is not set." in str(resp.data)
+
     def test_listener_port(self, admin_user, patch):
         """
         setting listener_port should create a receptor address