From 6a7adbfaef25db08f38d052fb44f9203c48c4097 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Tue, 8 Aug 2023 22:56:36 -0400 Subject: [PATCH] Change PeersSerializer to SlugRelatedField Get rid of PeersSerializer and just use SlugRelatedField, which should be more a straightforward approach. Other changes: - cleanup code related to the already-removed api/v2/peers endpoint - add "hybrid" node type into more instance_peers test cases --- awx/api/serializers.py | 9 +---- awx/api/views/__init__.py | 14 ------- awx/main/access.py | 17 --------- .../functional/api/test_instance_peers.py | 37 ++++++++++++------- awx_collection/plugins/modules/instance.py | 2 - 5 files changed, 26 insertions(+), 53 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 61cff4838f99..580549f1efbc 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -5374,11 +5374,6 @@ class Meta: fields = ('id', 'hostname', 'node_type', 'node_state', 'enabled') -class PeersSerializer(serializers.StringRelatedField): - def to_internal_value(self, value): - return Instance.objects.get(hostname=value) - - class InstanceSerializer(BaseSerializer): show_capabilities = ['edit'] @@ -5387,7 +5382,7 @@ class InstanceSerializer(BaseSerializer): jobs_running = serializers.IntegerField(help_text=_('Count of jobs in the running or waiting state that are targeted for this instance'), read_only=True) jobs_total = serializers.IntegerField(help_text=_('Count of all jobs that target this instance'), read_only=True) health_check_pending = serializers.SerializerMethodField() - peers = PeersSerializer(many=True, required=False) + peers = serializers.SlugRelatedField(many=True, required=False, slug_field="hostname", queryset=Instance.objects.all()) class Meta: model = Instance @@ -5493,7 +5488,7 @@ def get_field_from_model_or_attrs(fd): if peers_from_control_nodes and node_type not in (Instance.Types.EXECUTION, Instance.Types.HOP): raise serializers.ValidationError(_("peers_from_control_nodes can only be enabled for execution or hop nodes.")) - if node_type == Instance.Types.CONTROL: + if node_type in [Instance.Types.CONTROL, Instance.Types.HYBRID]: if self.instance and 'peers' in attrs and set(self.instance.peers.all()) != set(attrs['peers']): raise serializers.ValidationError( _("Setting peers manually for control nodes is not allowed. Enable peers_from_control_nodes on the hop and execution nodes instead.") diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index b62be5ed395b..3189bde1e3f5 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -4349,17 +4349,3 @@ def post(self, request, *args, **kwargs): return Response({"error": _("This workflow step has already been approved or denied.")}, status=status.HTTP_400_BAD_REQUEST) obj.deny(request) return Response(status=status.HTTP_204_NO_CONTENT) - - -class PeersList(ListAPIView): - name = _("Peers") - model = models.InstanceLink - serializer_class = serializers.InstanceLinkSerializer - search_fields = ('source', 'target', 'link_state') - - -class PeersDetail(RetrieveAPIView): - name = _("Peers Detail") - always_allow_superuser = True - model = models.InstanceLink - serializer_class = serializers.InstanceLinkSerializer diff --git a/awx/main/access.py b/awx/main/access.py index f3878a57a3eb..277076d74149 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -39,7 +39,6 @@ Host, Instance, InstanceGroup, - InstanceLink, Inventory, InventorySource, InventoryUpdate, @@ -2950,22 +2949,6 @@ def filtered_queryset(self): return self.model.objects.filter(workflowjobtemplatenodes__workflow_job_template__in=WorkflowJobTemplate.accessible_pk_qs(self.user, 'read_role')) -class PeersAccess(BaseAccess): - model = InstanceLink - - def can_add(self, data): - return False - - def can_change(self, obj, data): - return False - - def can_delete(self, obj): - return True - - def can_copy(self, obj): - return False - - for cls in BaseAccess.__subclasses__(): access_registry[cls.model] = cls access_registry[UnpartitionedJobEvent] = UnpartitionedJobEventAccess diff --git a/awx/main/tests/functional/api/test_instance_peers.py b/awx/main/tests/functional/api/test_instance_peers.py index 37601ee9e4f3..ce15d9f2d967 100644 --- a/awx/main/tests/functional/api/test_instance_peers.py +++ b/awx/main/tests/functional/api/test_instance_peers.py @@ -23,15 +23,16 @@ class TestPeers: def configure_settings(self, settings): settings.IS_K8S = True - def test_prevent_peering_to_self(self): + @pytest.mark.parametrize('node_type', ['control', 'hybrid']) + def test_prevent_peering_to_self(self, node_type): ''' cannot peer to self ''' - control_instance = Instance.objects.create(hostname='abc', node_type="control") + control_instance = Instance.objects.create(hostname='abc', node_type=node_type) with pytest.raises(IntegrityError): control_instance.peers.add(control_instance) - @pytest.mark.parametrize('node_type', ['control', 'hop', 'execution']) + @pytest.mark.parametrize('node_type', ['control', 'hybrid', 'hop', 'execution']) def test_creating_node(self, node_type, admin_user, post): ''' can only add hop and execution nodes via API @@ -40,7 +41,7 @@ def test_creating_node(self, node_type, admin_user, post): url=reverse('api:instance_list'), data={"hostname": "abc", "node_type": node_type}, user=admin_user, - expect=400 if node_type == 'control' else 201, + expect=400 if node_type in ['control', 'hybrid'] else 201, ) def test_changing_node_type(self, admin_user, patch): @@ -67,7 +68,7 @@ def test_listener_port_null(self, node_type, admin_user, post): expect=201, ) - @pytest.mark.parametrize('node_type, allowed', [('control', False), ('hop', True), ('execution', True)]) + @pytest.mark.parametrize('node_type, allowed', [('control', False), ('hybrid', False), ('hop', True), ('execution', True)]) def test_peers_from_control_nodes_allowed(self, node_type, allowed, post, admin_user): ''' only hop and execution nodes can have peers_from_control_nodes set to True @@ -96,7 +97,6 @@ def test_peers_from_control_nodes_listener_port_enabled(self, admin_user, post): if peers_from_control_nodes is True, listener_port must an integer Assert that all other combinations are allowed ''' - Instance.objects.create(hostname='abc', node_type="control") i = 0 for node_type in ['hop', 'execution']: for peers_from in [True, False]: @@ -111,12 +111,13 @@ def test_peers_from_control_nodes_listener_port_enabled(self, admin_user, post): ) i += 1 - def test_disallow_modify_peers_control_nodes(self, admin_user, patch): + @pytest.mark.parametrize('node_type', ['control', 'hybrid']) + def test_disallow_modifying_peers_control_nodes(self, node_type, admin_user, patch): ''' for control nodes, peers field should not be modified directly via patch. ''' - control = Instance.objects.create(hostname='abc', node_type='control') + control = Instance.objects.create(hostname='abc', node_type=node_type) hop1 = Instance.objects.create(hostname='hop1', node_type='hop', peers_from_control_nodes=True, listener_port=6789) hop2 = Instance.objects.create(hostname='hop2', node_type='hop', peers_from_control_nodes=False, listener_port=6789) assert [hop1] == list(control.peers.all()) # only hop1 should be peered @@ -151,7 +152,6 @@ def test_disallow_modify_peers_control_nodes(self, admin_user, patch): user=admin_user, expect=200, # patching without data should be fine too ) - control.refresh_from_db() assert {hop1, hop2} == set(control.peers.all()) # hop1 and hop2 should now be peered from control node def test_disallow_changing_hostname(self, admin_user, patch): @@ -166,6 +166,15 @@ def test_disallow_changing_hostname(self, admin_user, patch): expect=400, ) + def test_disallow_changing_ip_address(self, admin_user, patch): + hop = Instance.objects.create(hostname='hop', ip_address='10.10.10.10', node_type='hop') + patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"ip_address": "12.12.12.12"}, + user=admin_user, + expect=400, + ) + def test_disallow_changing_node_state(self, admin_user, patch): ''' only allow setting to deprovisioning @@ -184,7 +193,8 @@ def test_disallow_changing_node_state(self, admin_user, patch): expect=400, ) - def test_control_node_automatically_peers(self): + @pytest.mark.parametrize('node_type', ['control', 'hybrid']) + def test_control_node_automatically_peers(self, node_type): ''' a new control node should automatically peer to hop @@ -193,12 +203,13 @@ def test_control_node_automatically_peers(self): ''' hop = Instance.objects.create(hostname='hop', node_type='hop', peers_from_control_nodes=True, listener_port=6789) - control = Instance.objects.create(hostname='abc', node_type='control') + control = Instance.objects.create(hostname='abc', node_type=node_type) assert hop in control.peers.all() hop.delete() assert not control.peers.exists() - def test_control_node_retains_other_peers(self): + @pytest.mark.parametrize('node_type', ['control', 'hybrid']) + def test_control_node_retains_other_peers(self, node_type): ''' if a new node comes online, other peer relationships should remain intact @@ -207,7 +218,7 @@ def test_control_node_retains_other_peers(self): hop2 = Instance.objects.create(hostname='hop2', node_type='hop', listener_port=6789, peers_from_control_nodes=False) hop1.peers.add(hop2) - control = Instance.objects.create(hostname='control', node_type='control', listener_port=None) + control = Instance.objects.create(hostname='control', node_type=node_type, listener_port=None) assert hop1.peers.exists() diff --git a/awx_collection/plugins/modules/instance.py b/awx_collection/plugins/modules/instance.py index 432dca4a3914..54866fb731ea 100644 --- a/awx_collection/plugins/modules/instance.py +++ b/awx_collection/plugins/modules/instance.py @@ -140,8 +140,6 @@ def main(): new_fields['listener_port'] = listener_port if peers is not None: new_fields['peers'] = peers - if peers is None: - peers = [''] if peers_from_control_nodes is not None: new_fields['peers_from_control_nodes'] = peers_from_control_nodes