From 7a1ed406da946aeeda2cb04ab47a5992bb80c8bd Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Mon, 15 Jan 2024 17:48:07 -0500 Subject: [PATCH] Remove CRUD for Receptor Addresses Removes ability to directly create and delete receptor addresses for a given node. Instead, receptor addresses are created automatically if listener_port is set on the Instance. For example patching "hop" instance with {"listener_port": 6667} will create a canonical receptor address with port 6667. Likewise, peers_from_control_nodes on the instance sets the peers_from_control_nodes on the canonical address (if listener port is also set). protocol is a read-only field that simply reflects the canonical address protocol. Other Changes: - rename k8s_routable to is_internal - add protocol to ReceptorAddress - remove peers_from_control_nodes and listener_port from Instance model Signed-off-by: Seth Foster --- awx/api/serializers.py | 125 ++++++++---------- awx/api/views/__init__.py | 22 +-- awx/api/views/instance_install_bundle.py | 7 +- .../commands/add_receptor_address.py | 11 +- .../management/commands/provision_instance.py | 16 +-- awx/main/managers.py | 19 +-- awx/main/migrations/0189_inbound_hop_nodes.py | 45 +++---- awx/main/models/ha.py | 35 +++-- awx/main/models/receptor_address.py | 22 ++- .../functional/api/test_instance_peers.py | 118 ++++++++++++++--- 10 files changed, 225 insertions(+), 195 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 6cede6f326..c832d598d4 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -637,7 +637,7 @@ class BaseSerializer(serializers.ModelSerializer, metaclass=BaseSerializerMetacl exclusions = self.get_validation_exclusions(self.instance) obj = self.instance or self.Meta.model() for k, v in attrs.items(): - if k not in exclusions: + if k not in exclusions and k != 'canonical_address_port': setattr(obj, k, v) obj.full_clean(exclude=exclusions) # full_clean may modify values on the instance; copy those changes @@ -5496,63 +5496,16 @@ class ReceptorAddressSerializer(BaseSerializer): 'address', 'port', 'websocket_path', - 'k8s_routable', + 'is_internal', 'canonical', 'instance', - 'managed', 'peers_from_control_nodes', 'full_address', ) - read_only_fields = ('full_address', 'managed', 'canonical', 'k8s_routable') def get_full_address(self, obj): return obj.get_full_address() - def validate(self, attrs): - def get_field_from_model_or_attrs(fd): - return attrs.get(fd, self.instance and getattr(self.instance, fd) or None) - - managed = get_field_from_model_or_attrs('managed') - canonical = get_field_from_model_or_attrs('canonical') - - if managed: - raise serializers.ValidationError(_("Cannot modify a managed address.")) - - # cannot modify address field if canonical is True - if canonical and attrs.get('address') and self.instance and self.instance.address != attrs.get('address'): - raise serializers.ValidationError(_("Cannot modify address field if it is canonical.")) - - peers_from_control_nodes = get_field_from_model_or_attrs('peers_from_control_nodes') - instance = get_field_from_model_or_attrs('instance') - address = get_field_from_model_or_attrs('address') - - if not instance.listener_port: - raise serializers.ValidationError(_("Instance must have a listener port set.")) - - # only allow websocket_path to be set if instance protocol is ws - if attrs.get('websocket_path') and instance and instance.protocol != 'ws': - raise serializers.ValidationError(_("Can only set websocket path if protocol is ws.")) - - # an instance can only have one address with peers_from_control_nodes set to True - if peers_from_control_nodes: - for other_address in ReceptorAddress.objects.filter(instance=instance.id): - if other_address.address != address and other_address.peers_from_control_nodes: - raise serializers.ValidationError(_("Only one address can set peers_from_control_nodes to True.")) - - # k8s_routable should be False - if attrs.get('k8s_routable') == True: - raise serializers.ValidationError(_("Only external addresses can be created.")) - - return super().validate(attrs) - - def update(self, obj, validated_data): - addr = super(ReceptorAddressSerializer, self).update(obj, validated_data) - if addr.port != addr.instance.listener_port: - addr.instance.listener_port = addr.port - addr.instance.save(update_fields=['listener_port']) - - return addr - class InstanceSerializer(BaseSerializer): show_capabilities = ['edit'] @@ -5566,6 +5519,9 @@ class InstanceSerializer(BaseSerializer): help_text=_('Primary keys of receptor addresses to peer to.'), many=True, required=False, queryset=ReceptorAddress.objects.all() ) reverse_peers = serializers.SerializerMethodField() + listener_port = serializers.IntegerField(source='canonical_address_port', required=False, allow_null=True) + peers_from_control_nodes = serializers.BooleanField(source='canonical_address_peers_from_control_nodes', required=False) + protocol = serializers.SerializerMethodField() class Meta: model = Instance @@ -5605,6 +5561,7 @@ class InstanceSerializer(BaseSerializer): 'peers', 'reverse_peers', 'listener_port', + 'peers_from_control_nodes', 'protocol', ) extra_kwargs = { @@ -5638,36 +5595,32 @@ class InstanceSerializer(BaseSerializer): res['health_check'] = self.reverse('api:instance_health_check', kwargs={'pk': obj.pk}) return res - def create(self, validated_data): + def create_or_update(self, validated_data, obj=None, create=True): # create a managed receptor address if listener port is defined - kwargs = { - 'port': validated_data.get('listener_port', None), - 'canonical': True, - } - kwargs = {k: v for k, v in kwargs.items() if v is not None} - instance = super(InstanceSerializer, self).create(validated_data) - if kwargs.get('port'): - instance.receptor_addresses.update_or_create(address=instance.hostname, defaults=kwargs) + kwargs = dict() + if 'listener_port' in validated_data: + kwargs['port'] = validated_data.pop('listener_port') + if 'peers_from_control_nodes' in validated_data: + kwargs['peers_from_control_nodes'] = validated_data.pop('peers_from_control_nodes') + + if create: + instance = super(InstanceSerializer, self).create(validated_data) else: - # delete the receptor address if the listener port is not defined + instance = super(InstanceSerializer, self).update(obj, validated_data) + + if 'port' in kwargs and kwargs['port'] is None: + # delete the receptor address if the port is None instance.receptor_addresses.filter(address=instance.hostname).delete() + elif kwargs: + instance.receptor_addresses.update_or_create(address=instance.hostname, defaults=kwargs) + return instance + def create(self, validated_data): + return self.create_or_update(validated_data, create=True) + def update(self, obj, validated_data): - # update the managed receptor address if listener port is defined - kwargs = { - 'port': validated_data.get('listener_port', None), - 'canonical': True, - } - kwargs = {k: v for k, v in kwargs.items() if v is not None} - instance = super(InstanceSerializer, self).update(obj, validated_data) - if kwargs.get('port'): - instance.receptor_addresses.update_or_create(address=instance.hostname, defaults=kwargs) - else: - # delete the receptor address if the listener port is not defined - instance.receptor_addresses.filter(address=instance.hostname).delete() - - return instance + return self.create_or_update(validated_data, obj, create=False) def get_summary_fields(self, obj): summary = super().get_summary_fields(obj) @@ -5681,6 +5634,13 @@ class InstanceSerializer(BaseSerializer): def get_reverse_peers(self, obj): return Instance.objects.prefetch_related('peers').filter(peers__in=obj.receptor_addresses.all()).values_list('id', flat=True) + def get_protocol(self, obj): + # note: don't create a different query for receptor addresses, as this is prefetched on the View for optimization + for addr in obj.receptor_addresses.all(): + if addr.canonical: + return addr.protocol + return 'tcp' + def get_consumed_capacity(self, obj): return obj.consumed_capacity @@ -5694,6 +5654,12 @@ class InstanceSerializer(BaseSerializer): return obj.health_check_pending def validate(self, attrs): + # Oddly, using 'source' on a DRF field populates attrs with the source name, so we should rename it back + if 'canonical_address_port' in attrs: + attrs['listener_port'] = attrs.pop('canonical_address_port') + if 'canonical_address_peers_from_control_nodes' in attrs: + attrs['peers_from_control_nodes'] = attrs.pop('canonical_address_peers_from_control_nodes') + def get_field_from_model_or_attrs(fd): return attrs.get(fd, self.instance and getattr(self.instance, fd) or None) @@ -5773,6 +5739,19 @@ class InstanceSerializer(BaseSerializer): return value + def validate_listener_port(self, value): + """ + Cannot change listener port, unless going from none to integer, and vice versa + If instance is managed, cannot change listener port at all + """ + if self.instance: + canonical_address_port = self.instance.canonical_address_port + if value and canonical_address_port and canonical_address_port != value: + raise serializers.ValidationError(_("Cannot change listener port.")) + if self.instance.managed and value != canonical_address_port: + raise serializers.ValidationError(_("Cannot change listener port for managed nodes.")) + return value + class InstanceHealthCheckSerializer(BaseSerializer): class Meta: diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 0e429d0190..7437c85c66 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -337,12 +337,20 @@ class InstanceList(ListCreateAPIView): search_fields = ('hostname',) ordering = ('id',) + def get_queryset(self): + qs = super().get_queryset().prefetch_related('receptor_addresses') + return qs + class InstanceDetail(RetrieveUpdateAPIView): name = _("Instance Detail") model = models.Instance serializer_class = serializers.InstanceSerializer + def get_queryset(self): + qs = super().get_queryset().prefetch_related('receptor_addresses') + return qs + def update_raw_data(self, data): # these fields are only valid on creation of an instance, so they unwanted on detail view data.pop('node_type', None) @@ -384,7 +392,7 @@ class InstancePeersList(SubListAPIView): search_fields = ('address',) -class InstanceReceptorAddressesList(SubListCreateAPIView): +class InstanceReceptorAddressesList(SubListAPIView): name = _("Receptor Addresses") model = models.ReceptorAddress parent_key = 'instance' @@ -393,23 +401,19 @@ class InstanceReceptorAddressesList(SubListCreateAPIView): search_fields = ('address',) -class ReceptorAddressesList(ListCreateAPIView): +class ReceptorAddressesList(ListAPIView): name = _("Receptor Addresses") model = models.ReceptorAddress serializer_class = serializers.ReceptorAddressSerializer search_fields = ('address',) -class ReceptorAddressDetail(RetrieveUpdateDestroyAPIView): +class ReceptorAddressDetail(RetrieveAPIView): name = _("Receptor Address Detail") model = models.ReceptorAddress serializer_class = serializers.ReceptorAddressSerializer - - def delete(self, request, *args, **kwargs): - obj = self.get_object() - if obj.canonical or obj.managed: - return Response({'detail': _('Cannot delete canonical or managed address.')}, status=status.HTTP_400_BAD_REQUEST) - return super(ReceptorAddressDetail, self).delete(request, *args, **kwargs) + parent_model = models.Instance + relationship = 'receptor_addresses' class InstanceInstanceGroupsList(InstanceGroupMembershipMixin, SubListCreateAttachDetachAPIView): diff --git a/awx/api/views/instance_install_bundle.py b/awx/api/views/instance_install_bundle.py index 4d0fa437d7..521eae1e23 100644 --- a/awx/api/views/instance_install_bundle.py +++ b/awx/api/views/instance_install_bundle.py @@ -127,11 +127,12 @@ def generate_group_vars_all_yml(instance_obj): # get peers peers = [] for addr in instance_obj.peers.all().prefetch_related('instance'): - peers.append(dict(address=addr.get_full_address(), protocol=addr.instance.protocol)) + peers.append(dict(address=addr.get_full_address(), protocol=addr.protocol)) context = dict(instance=instance_obj, peers=peers) - if instance_obj.listener_port: - context['listener_port'] = instance_obj.listener_port + listener_port = instance_obj.canonical_address_port + if listener_port: + context['listener_port'] = listener_port all_yaml = render_to_string("instance_install_bundle/group_vars/all.yml", context=context) # convert consecutive newlines with a single newline diff --git a/awx/main/management/commands/add_receptor_address.py b/awx/main/management/commands/add_receptor_address.py index 032feb4fc5..ef37e5e89a 100644 --- a/awx/main/management/commands/add_receptor_address.py +++ b/awx/main/management/commands/add_receptor_address.py @@ -13,11 +13,6 @@ def add_address(**kwargs): # if ReceptorAddress already exists with address, just update # otherwise, create new ReceptorAddress addr, _ = ReceptorAddress.objects.update_or_create(address=kwargs.pop('address'), defaults=kwargs) - - # update listener_port on instance if address is canonical - if addr.canonical: - addr.instance.listener_port = addr.port - addr.instance.save(update_fields=['listener_port']) print(f"Successfully added receptor address {addr.get_full_address()}") changed = True except Exception as e: @@ -39,16 +34,16 @@ class Command(BaseCommand): parser.add_argument('--address', dest='address', type=str, help="Receptor address") parser.add_argument('--port', dest='port', type=int, help="Receptor listener port") parser.add_argument('--websocket_path', dest='websocket_path', type=str, default="", help="Path for websockets") - parser.add_argument('--k8s_routable', action='store_true', help="If true, address only resolvable within the Kubernetes cluster") + parser.add_argument('--is_internal', action='store_true', help="If true, address only resolvable within the Kubernetes cluster") + parser.add_argument('--protocol', type=str, default='tcp', choices=['tcp', 'ws', 'wss'], help="Protocol to use for the Receptor listener") parser.add_argument('--canonical', action='store_true', help="If true, address is the canonical address for the instance") parser.add_argument('--peers_from_control_nodes', action='store_true', help="If true, control nodes will peer to this address") - parser.add_argument('--managed', action='store_true', help="If True, this address should be managed by the control plane.") def handle(self, **options): self.changed = False address_options = { k: options[k] - for k in ('instance', 'address', 'port', 'websocket_path', 'k8s_routable', 'peers_from_control_nodes', 'canonical', 'managed') + for k in ('instance', 'address', 'port', 'websocket_path', 'is_internal', 'protocol', 'peers_from_control_nodes', 'canonical') if options[k] } self.changed = add_address(**address_options) diff --git a/awx/main/management/commands/provision_instance.py b/awx/main/management/commands/provision_instance.py index 5d17e59ad1..5a60328d96 100644 --- a/awx/main/management/commands/provision_instance.py +++ b/awx/main/management/commands/provision_instance.py @@ -25,23 +25,17 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument('--hostname', dest='hostname', type=str, help="Hostname used during provisioning") - parser.add_argument('--listener_port', dest='listener_port', type=int, help="Receptor listener port") - parser.add_argument( - '--protocol', dest='protocol', type=str, default='tcp', choices=['tcp', 'ws', 'wss'], help="Protocol to use for the Receptor listener" - ) parser.add_argument('--node_type', type=str, default='hybrid', choices=['control', 'execution', 'hop', 'hybrid'], help="Instance Node type") parser.add_argument('--uuid', type=str, help="Instance UUID") - def _register_hostname(self, hostname, node_type, uuid, listener_port, protocol): + def _register_hostname(self, hostname, node_type, uuid): if not hostname: if not settings.AWX_AUTO_DEPROVISION_INSTANCES: raise CommandError('Registering with values from settings only intended for use in K8s installs') from awx.main.management.commands.register_queue import RegisterQueue - (changed, instance) = Instance.objects.register( - ip_address=os.environ.get('MY_POD_IP'), listener_port=listener_port, node_type='control', node_uuid=settings.SYSTEM_UUID, protocol=protocol - ) + (changed, instance) = Instance.objects.register(ip_address=os.environ.get('MY_POD_IP'), node_type='control', node_uuid=settings.SYSTEM_UUID) RegisterQueue(settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME, 100, 0, [], is_container_group=False).register() RegisterQueue( settings.DEFAULT_EXECUTION_QUEUE_NAME, @@ -54,9 +48,7 @@ class Command(BaseCommand): max_concurrent_jobs=settings.DEFAULT_EXECUTION_QUEUE_MAX_CONCURRENT_JOBS, ).register() else: - (changed, instance) = Instance.objects.register( - hostname=hostname, node_type=node_type, node_uuid=uuid, listener_port=listener_port, protocol=protocol - ) + (changed, instance) = Instance.objects.register(hostname=hostname, node_type=node_type, node_uuid=uuid) if changed: print("Successfully registered instance {}".format(hostname)) else: @@ -67,6 +59,6 @@ class Command(BaseCommand): @transaction.atomic def handle(self, **options): self.changed = False - self._register_hostname(options.get('hostname'), options.get('node_type'), options.get('uuid'), options.get('listener_port'), options.get('protocol')) + self._register_hostname(options.get('hostname'), options.get('node_type'), options.get('uuid')) if self.changed: print("(changed: True)") diff --git a/awx/main/managers.py b/awx/main/managers.py index d87de46d19..c501d7b0d3 100644 --- a/awx/main/managers.py +++ b/awx/main/managers.py @@ -120,10 +120,7 @@ class InstanceManager(models.Manager): node_uuid=None, hostname=None, ip_address="", - listener_port=None, - protocol='tcp', node_type='hybrid', - peers_from_control_nodes=False, defaults=None, ): if not hostname: @@ -171,12 +168,6 @@ class InstanceManager(models.Manager): if instance.node_type != node_type: instance.node_type = node_type update_fields.append('node_type') - if instance.protocol != protocol: - instance.protocol = protocol - update_fields.append('protocol') - if instance.listener_port != listener_port: - instance.listener_port = listener_port - update_fields.append('listener_port') if update_fields: instance.save(update_fields=update_fields) return (True, instance) @@ -194,14 +185,6 @@ class InstanceManager(models.Manager): uuid_option = {'uuid': node_uuid if node_uuid is not None else uuid.uuid4()} if node_type == 'execution' and 'version' not in create_defaults: create_defaults['version'] = RECEPTOR_PENDING - instance = self.create( - hostname=hostname, - ip_address=ip_address, - node_type=node_type, - peers_from_control_nodes=peers_from_control_nodes, - protocol=protocol, - **create_defaults, - **uuid_option - ) + instance = self.create(hostname=hostname, ip_address=ip_address, node_type=node_type, **create_defaults, **uuid_option) return (True, instance) diff --git a/awx/main/migrations/0189_inbound_hop_nodes.py b/awx/main/migrations/0189_inbound_hop_nodes.py index d1cf76c3b0..7a7ed05b62 100644 --- a/awx/main/migrations/0189_inbound_hop_nodes.py +++ b/awx/main/migrations/0189_inbound_hop_nodes.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.6 on 2023-12-14 19:14 +# Generated by Django 4.2.6 on 2024-01-12 20:01 import django.core.validators from django.db import migrations, models @@ -25,9 +25,17 @@ class Migration(migrations.Migration): ), ), ('websocket_path', models.CharField(blank=True, default='', help_text='Websocket path.', max_length=255)), - ('k8s_routable', models.BooleanField(default=False, help_text='If True, only routable inside of the Kubernetes cluster.')), - ('canonical', models.BooleanField(default=False, help_text='If True, this address is the canonical address for the instance.')), - ('managed', models.BooleanField(default=False, editable=False, help_text='If True, this address is managed by the control plane.')), + ( + 'protocol', + models.CharField( + choices=[('tcp', 'TCP'), ('ws', 'WS'), ('wss', 'WSS')], + default='tcp', + help_text="Protocol to use for the Receptor listener, 'tcp', 'wss', or 'ws'.", + max_length=10, + ), + ), + ('is_internal', models.BooleanField(default=False, help_text='If True, only routable within the Kubernetes cluster.')), + ('canonical', models.BooleanField(default=True, help_text='If True, this address is the canonical address for the instance.')), ( 'peers_from_control_nodes', models.BooleanField(default=False, help_text='If True, control plane cluster nodes should automatically peer to it.'), @@ -42,32 +50,19 @@ class Migration(migrations.Migration): name='instancelink', unique_together=set(), ), + migrations.RemoveField( + model_name='instance', + name='listener_port', + ), + migrations.RemoveField( + model_name='instance', + name='peers_from_control_nodes', + ), migrations.AddField( model_name='instance', name='managed', field=models.BooleanField(default=False, editable=False, help_text='If True, this instance is managed by the control plane.'), ), - migrations.AddField( - model_name='instance', - name='protocol', - field=models.CharField( - choices=[('tcp', 'TCP'), ('ws', 'WS'), ('wss', 'WSS')], - default='tcp', - help_text="Protocol to use for the Receptor listener, 'tcp', 'wss', or 'ws'.", - max_length=10, - ), - ), - migrations.AlterField( - model_name='instance', - name='listener_port', - field=models.PositiveIntegerField( - blank=True, - default=None, - help_text='Port that Receptor will listen for incoming connections on.', - null=True, - validators=[django.core.validators.MinValueValidator(0), django.core.validators.MaxValueValidator(65535)], - ), - ), migrations.AlterField( model_name='instancelink', name='source', diff --git a/awx/main/models/ha.py b/awx/main/models/ha.py index 6ced0bb1a9..b31286e9c1 100644 --- a/awx/main/models/ha.py +++ b/awx/main/models/ha.py @@ -64,12 +64,6 @@ class HasPolicyEditsMixin(HasEditsMixin): return self._values_have_edits(new_values) -class Protocols(models.TextChoices): - TCP = 'tcp', 'TCP' - WS = 'ws', 'WS' - WSS = 'wss', 'WSS' - - class InstanceLink(BaseModel): class Meta: ordering = ("id",) @@ -171,16 +165,6 @@ class Instance(HasPolicyEditsMixin, BaseModel): default=0, editable=False, ) - listener_port = models.PositiveIntegerField( - blank=True, - null=True, - default=None, - validators=[MinValueValidator(0), MaxValueValidator(65535)], - help_text=_("Port that Receptor will listen for incoming connections on."), - ) - protocol = models.CharField( - help_text=_("Protocol to use for the Receptor listener, 'tcp', 'wss', or 'ws'."), max_length=10, default=Protocols.TCP, choices=Protocols.choices - ) class Types(models.TextChoices): CONTROL = 'control', _("Control plane node") @@ -205,7 +189,6 @@ class Instance(HasPolicyEditsMixin, BaseModel): managed = models.BooleanField(help_text=_("If True, this instance is managed by the control plane."), default=False, editable=False) peers = models.ManyToManyField('ReceptorAddress', through=InstanceLink, through_fields=('source', 'target'), related_name='peers_from') - peers_from_control_nodes = models.BooleanField(default=False, help_text=_("If True, control plane cluster nodes should automatically peer to it.")) POLICY_FIELDS = frozenset(('managed_by_policy', 'hostname', 'capacity_adjustment')) @@ -252,6 +235,22 @@ class Instance(HasPolicyEditsMixin, BaseModel): return True return self.health_check_started > self.last_health_check + @property + def canonical_address_port(self): + # note: don't create a different query for receptor addresses, as this is prefetched on the View for optimization + for addr in self.receptor_addresses.all(): + if addr.canonical: + return addr.port + return None + + @property + def canonical_address_peers_from_control_nodes(self): + # note: don't create a different query for receptor addresses, as this is prefetched on the View for optimization + for addr in self.receptor_addresses.all(): + if addr.canonical: + return addr.peers_from_control_nodes + return False + def get_cleanup_task_kwargs(self, **kwargs): """ Produce options to use for the command: ansible-runner worker cleanup @@ -578,8 +577,6 @@ def on_instance_group_deleted(sender, instance, using, **kwargs): @receiver(post_delete, sender=Instance) def on_instance_deleted(sender, instance, using, **kwargs): schedule_policy_task() - if settings.IS_K8S and instance.node_type in (Instance.Types.EXECUTION, Instance.Types.HOP) and instance.peers_from_control_nodes: - schedule_write_receptor_config() class UnifiedJobTemplateInstanceGroupMembership(models.Model): diff --git a/awx/main/models/receptor_address.py b/awx/main/models/receptor_address.py index 68140c3647..fea1619249 100644 --- a/awx/main/models/receptor_address.py +++ b/awx/main/models/receptor_address.py @@ -4,6 +4,12 @@ from django.utils.translation import gettext_lazy as _ from awx.api.versioning import reverse +class Protocols(models.TextChoices): + TCP = 'tcp', 'TCP' + WS = 'ws', 'WS' + WSS = 'wss', 'WSS' + + class ReceptorAddress(models.Model): class Meta: app_label = 'main' @@ -18,9 +24,11 @@ class ReceptorAddress(models.Model): address = models.CharField(help_text=_("Routable address for this instance."), max_length=255) port = models.IntegerField(help_text=_("Port for the address."), default=27199, validators=[MinValueValidator(0), MaxValueValidator(65535)]) websocket_path = models.CharField(help_text=_("Websocket path."), max_length=255, default="", blank=True) - k8s_routable = models.BooleanField(help_text=_("If True, only routable inside of the Kubernetes cluster."), default=False) - canonical = models.BooleanField(help_text=_("If True, this address is the canonical address for the instance."), default=False) - managed = models.BooleanField(help_text=_("If True, this address is managed by the control plane."), default=False, editable=False) + protocol = models.CharField( + help_text=_("Protocol to use for the Receptor listener, 'tcp', 'wss', or 'ws'."), max_length=10, default=Protocols.TCP, choices=Protocols.choices + ) + is_internal = models.BooleanField(help_text=_("If True, only routable within the Kubernetes cluster."), default=False) + canonical = models.BooleanField(help_text=_("If True, this address is the canonical address for the instance."), default=True) peers_from_control_nodes = models.BooleanField(help_text=_("If True, control plane cluster nodes should automatically peer to it."), default=False) instance = models.ForeignKey( 'Instance', @@ -36,10 +44,10 @@ class ReceptorAddress(models.Model): scheme = "" path = "" port = "" - if self.instance.protocol == "ws": + if self.protocol == "ws": scheme = "wss://" - if self.instance.protocol == "ws" and self.websocket_path: + if self.protocol == "ws" and self.websocket_path: path = f"/{self.websocket_path}" if self.port: @@ -48,9 +56,9 @@ class ReceptorAddress(models.Model): return f"{scheme}{self.address}{port}{path}" def get_peer_type(self): - if self.instance.protocol == 'tcp': + if self.protocol == 'tcp': return 'tcp-peer' - elif self.instance.protocol in ['ws', 'wss']: + elif self.protocol in ['ws', 'wss']: return 'ws-peer' else: return None diff --git a/awx/main/tests/functional/api/test_instance_peers.py b/awx/main/tests/functional/api/test_instance_peers.py index a40189d483..bfb6905e1a 100644 --- a/awx/main/tests/functional/api/test_instance_peers.py +++ b/awx/main/tests/functional/api/test_instance_peers.py @@ -63,32 +63,76 @@ class TestPeers: ) assert 'Cannot change node type.' in str(resp.data) - def test_k8s_routable(self, admin_user, post): + def test_listener_port(self, admin_user, patch): """ - cannot set k8s_routable to True + setting listener_port should create a receptor address + cannot change listener_port to new value + unsetting listener_port should remove that address """ hop = Instance.objects.create(hostname='abc', node_type="hop") - resp = post( - url=reverse('api:instance_receptor_addresses_list', kwargs={'pk': hop.pk}), - data={"address": "hopaddr", "k8s_routable": True}, + patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"listener_port": 27199}, user=admin_user, - expect=400, + expect=200, # can set a port ) - assert 'Only external addresses can be created.' in str(resp.data) + assert ReceptorAddress.objects.filter(instance=hop, port=27199).exists() + resp = patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"listener_port": 5678}, + user=admin_user, + expect=400, # cannot change port + ) + assert 'Cannot change listener port.' in str(resp.data) + patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"listener_port": None}, + user=admin_user, + expect=200, # can unset a port + ) + assert not ReceptorAddress.objects.filter(instance=hop, port=27199).exists() - def test_multiple_peers_from_control_nodes(self, admin_user, post): + def test_changing_managed_listener_port(self, admin_user, patch): """ - only one address can have peers_from_control_nodes set to True for a given instance + if instance is managed, cannot change listener port at all """ - hop = Instance.objects.create(hostname='hop', node_type='hop') - ReceptorAddress.objects.create(instance=hop, address='hopaddr1', peers_from_control_nodes=True) - resp = post( - url=reverse('api:instance_receptor_addresses_list', kwargs={'pk': hop.pk}), - data={"address": "hopaddr2", "peers_from_control_nodes": True}, + hop = Instance.objects.create(hostname='abc', node_type="hop", managed=True) + resp = patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"listener_port": 5678}, user=admin_user, - expect=400, + expect=400, # cannot set port ) - assert 'Only one address can set peers_from_control_nodes to True.' in str(resp.data) + assert 'Cannot change listener port for managed nodes.' in str(resp.data) + ReceptorAddress.objects.create(instance=hop, address='addr', port=27199) + resp = patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"listener_port": None}, + user=admin_user, + expect=400, # cannot unset port + ) + assert 'Cannot change listener port for managed nodes.' in str(resp.data) + + def test_peers_from_control_nodes(self, admin_user, patch): + """ + setting and unsetting peers_from_control_nodes on instance should change the + peers_from_control_nodes on the receptor address + """ + hop = Instance.objects.create(hostname='abc', node_type="hop") + patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"listener_port": 27199, "peers_from_control_nodes": True}, + user=admin_user, + expect=200, + ) + assert ReceptorAddress.objects.filter(instance=hop, port=27199, peers_from_control_nodes=True).exists() + patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"peers_from_control_nodes": False}, + user=admin_user, + expect=200, + ) + assert ReceptorAddress.objects.filter(instance=hop, port=27199, peers_from_control_nodes=False).exists() def test_bidirectional_peering(self, admin_user, patch): """ @@ -125,16 +169,16 @@ class TestPeers: assert 'Cannot peer to the same instance more than once.' in str(resp.data) @pytest.mark.parametrize('node_type', ['control', 'hybrid']) - def test_modifying_peers_control_nodes(self, node_type, admin_user, patch): + def test_changing_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=node_type) hop1 = Instance.objects.create(hostname='hop1', node_type='hop') - hop1addr = ReceptorAddress.objects.create(instance=hop1, address='hop1addr', peers_from_control_nodes=True) + hop1addr = ReceptorAddress.objects.create(instance=hop1, address='hop1', peers_from_control_nodes=True) hop2 = Instance.objects.create(hostname='hop2', node_type='hop') - hop2addr = ReceptorAddress.objects.create(instance=hop2, address='hop2addr') + hop2addr = ReceptorAddress.objects.create(instance=hop2, address='hop2') assert [hop1addr] == list(control.peers.all()) # only hop1addr should be peered resp = patch( url=reverse('api:instance_detail', kwargs={'pk': control.pk}), @@ -166,10 +210,10 @@ class TestPeers: ) # patch hop2 patch( - url=reverse('api:receptor_address_detail', kwargs={'pk': hop2addr.pk}), + url=reverse('api:instance_detail', kwargs={'pk': hop2.pk}), data={"peers_from_control_nodes": True}, user=admin_user, - expect=200, # patching without data should be fine too + expect=200, ) assert {hop1addr, hop2addr} == set(control.peers.all()) # hop1 and hop2 should now be peered from control node @@ -206,6 +250,20 @@ class TestPeers: ) assert "Can only change instances to the 'deprovisioning' state." in str(resp.data) + def test_changing_managed_node_state(self, admin_user, patch): + """ + cannot change node state of managed node + """ + hop = Instance.objects.create(hostname='hop', node_type='hop', managed=True) + resp = patch( + url=reverse('api:instance_detail', kwargs={'pk': hop.pk}), + data={"node_state": "deprovisioning"}, + user=admin_user, + expect=400, + ) + + assert 'Cannot deprovision managed nodes.' in str(resp.data) + @pytest.mark.parametrize('node_type', ['control', 'hybrid']) def test_control_node_automatically_peers(self, node_type): """ @@ -238,6 +296,24 @@ class TestPeers: assert hop1.peers.exists() + def test_reverse_peers(self, admin_user, get): + """ + if hop1 peers to hop2, hop1 should + be in hop2's reverse_peers list + """ + hop1 = Instance.objects.create(hostname='hop1', node_type='hop') + hop2 = Instance.objects.create(hostname='hop2', node_type='hop') + hop2addr = ReceptorAddress.objects.create(instance=hop2, address='hop2addr') + hop1.peers.add(hop2addr) + + resp = get( + url=reverse('api:instance_detail', kwargs={'pk': hop2.pk}), + user=admin_user, + expect=200, + ) + + assert hop1.pk in resp.data['reverse_peers'] + def test_group_vars(self): """ control > hop1 > hop2 < execution