From dc64168ed40bdf0d59a715ef82b2a6b46c2ab58e Mon Sep 17 00:00:00 2001 From: Alex Corey Date: Mon, 21 Mar 2022 13:12:44 -0400 Subject: [PATCH] Disallows disassociate of hubrid type instances from controlplane instance group Introduce new pattern for is_valid_removal Makes disassociate error message a bit more dynamic --- awx/api/generics.py | 10 ++++++++ awx/api/views/__init__.py | 20 ++++++++++++++-- awx/api/views/mixin.py | 24 +------------------ .../functional/api/test_instance_group.py | 14 +++++++++++ .../DisassociateButton/DisassociateButton.js | 16 ++++++------- .../DisassociateButton.test.js | 16 +++++++++++++ .../InstanceDetails/InstanceDetails.js | 3 ++- .../InstanceGroup/Instances/InstanceList.js | 10 ++++---- .../Instances/InstanceList.test.js | 2 +- .../InstanceGroup/Instances/Instances.js | 2 +- awx/ui/src/util/validators.js | 5 ---- awx/ui/src/util/validators.test.js | 18 -------------- 12 files changed, 77 insertions(+), 63 deletions(-) diff --git a/awx/api/generics.py b/awx/api/generics.py index f2faec5c47..dddd9d9e6f 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -638,6 +638,11 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): # attaching/detaching them from the parent. def is_valid_relation(self, parent, sub, created=False): + "Override in subclasses to do efficient validation of attaching" + return None + + def is_valid_removal(self, parent, sub): + "Same as is_valid_relation but called on disassociation" return None def get_description_context(self): @@ -722,6 +727,11 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): if not request.user.can_access(self.parent_model, 'unattach', parent, sub, self.relationship, request.data): raise PermissionDenied() + # Verify that removing the relationship is valid. + unattach_errors = self.is_valid_removal(parent, sub) + if unattach_errors is not None: + return Response(unattach_errors, status=status.HTTP_400_BAD_REQUEST) + if parent_key: sub.delete() else: diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 37bf8cfab7..b09d23da3c 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -408,7 +408,15 @@ class InstanceInstanceGroupsList(InstanceGroupMembershipMixin, SubListCreateAtta if parent.node_type == 'control': return {'msg': _(f"Cannot change instance group membership of control-only node: {parent.hostname}.")} if parent.node_type == 'hop': - return {'msg': _(f"Cannot change instance group membership of hop node: {parent.hostname}.")} + return {'msg': _(f"Cannot change instance group membership of hop node : {parent.hostname}.")} + return None + + def is_valid_removal(self, parent, sub): + res = self.is_valid_relation(parent, sub) + if res: + return res + if sub.name == settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME and parent.node_type == 'hybrid': + return {'msg': _(f"Cannot disassociate hybrid instance {parent.hostname} from {sub.name}.")} return None @@ -510,7 +518,15 @@ class InstanceGroupInstanceList(InstanceGroupMembershipMixin, SubListAttachDetac if sub.node_type == 'control': return {'msg': _(f"Cannot change instance group membership of control-only node: {sub.hostname}.")} if sub.node_type == 'hop': - return {'msg': _(f"Cannot change instance group membership of hop node: {sub.hostname}.")} + return {'msg': _(f"Cannot change instance group membership of hop node : {sub.hostname}.")} + return None + + def is_valid_removal(self, parent, sub): + res = self.is_valid_relation(parent, sub) + if res: + return res + if sub.node_type == 'hybrid' and parent.name == settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME: + return {'msg': _(f"Cannot disassociate hybrid node {sub.hostname} from {parent.name}.")} return None diff --git a/awx/api/views/mixin.py b/awx/api/views/mixin.py index 2ba254d3b3..9ad757c406 100644 --- a/awx/api/views/mixin.py +++ b/awx/api/views/mixin.py @@ -68,21 +68,10 @@ class InstanceGroupMembershipMixin(object): membership. """ - def attach_validate(self, request): - parent = self.get_parent_object() - sub_id, res = super().attach_validate(request) - if res: # handle an error - return sub_id, res - sub = get_object_or_400(self.model, pk=sub_id) - attach_errors = self.is_valid_relation(parent, sub) - if attach_errors: - return sub_id, Response(attach_errors, status=status.HTTP_400_BAD_REQUEST) - return sub_id, res - def attach(self, request, *args, **kwargs): response = super(InstanceGroupMembershipMixin, self).attach(request, *args, **kwargs) - sub_id, res = self.attach_validate(request) if status.is_success(response.status_code): + sub_id = request.data.get('id', None) if self.parent_model is Instance: inst_name = self.get_parent_object().hostname else: @@ -100,17 +89,6 @@ class InstanceGroupMembershipMixin(object): ig_obj.save(update_fields=['policy_instance_list']) return response - def unattach_validate(self, request): - parent = self.get_parent_object() - (sub_id, res) = super(InstanceGroupMembershipMixin, self).unattach_validate(request) - if res: - return (sub_id, res) - sub = get_object_or_400(self.model, pk=sub_id) - attach_errors = self.is_valid_relation(parent, sub) - if attach_errors: - return (sub_id, Response(attach_errors, status=status.HTTP_400_BAD_REQUEST)) - return (sub_id, res) - def unattach(self, request, *args, **kwargs): response = super(InstanceGroupMembershipMixin, self).unattach(request, *args, **kwargs) if status.is_success(response.status_code): diff --git a/awx/main/tests/functional/api/test_instance_group.py b/awx/main/tests/functional/api/test_instance_group.py index 97b8abcff2..530fea2c65 100644 --- a/awx/main/tests/functional/api/test_instance_group.py +++ b/awx/main/tests/functional/api/test_instance_group.py @@ -301,3 +301,17 @@ def test_instance_group_unattach_from_instance(post, instance_group, node_type_i assert new_activity.instance_group.first() == instance_group else: assert not new_activity + + +@pytest.mark.django_db +def test_cannot_remove_controlplane_hybrid_instances(post, controlplane_instance_group, node_type_instance, admin_user): + instance = node_type_instance(hostname='hybrid_node', node_type='hybrid') + controlplane_instance_group.instances.add(instance) + + url = reverse('api:instance_group_instance_list', kwargs={'pk': controlplane_instance_group.pk}) + r = post(url, {'disassociate': True, 'id': instance.id}, admin_user, expect=400) + assert 'Cannot disassociate hybrid node' in str(r.data) + + url = reverse('api:instance_instance_groups_list', kwargs={'pk': instance.pk}) + r = post(url, {'disassociate': True, 'id': controlplane_instance_group.id}, admin_user, expect=400) + assert f'Cannot disassociate hybrid instance' in str(r.data) diff --git a/awx/ui/src/components/DisassociateButton/DisassociateButton.js b/awx/ui/src/components/DisassociateButton/DisassociateButton.js index d3924882a1..9b0c8278b1 100644 --- a/awx/ui/src/components/DisassociateButton/DisassociateButton.js +++ b/awx/ui/src/components/DisassociateButton/DisassociateButton.js @@ -18,6 +18,7 @@ function DisassociateButton({ modalTitle = t`Disassociate?`, onDisassociate, verifyCannotDisassociate = true, + isProtectedInstanceGroup = false, }) { const [isOpen, setIsOpen] = useState(false); const { isKebabified, onKebabModalChange } = useContext(KebabifiedContext); @@ -37,7 +38,10 @@ function DisassociateButton({ return !item.summary_fields?.user_capabilities?.delete; } function cannotDisassociateInstances(item) { - return item.node_type === 'control'; + return ( + item.node_type === 'control' || + (isProtectedInstanceGroup && item.node_type === 'hybrid') + ); } const cannotDisassociate = itemsToDisassociate.some( @@ -73,11 +77,7 @@ function DisassociateButton({ let isDisabled = false; if (verifyCannotDisassociate) { - isDisabled = - itemsToDisassociate.length === 0 || - itemsToDisassociate.some(cannotDisassociate); - } else { - isDisabled = itemsToDisassociate.length === 0; + isDisabled = itemsToDisassociate.some(cannotDisassociate); } // NOTE: Once PF supports tooltips on disabled elements, @@ -89,7 +89,7 @@ function DisassociateButton({ setIsOpen(true)} @@ -108,7 +108,7 @@ function DisassociateButton({ variant="secondary" aria-label={t`Disassociate`} onClick={() => setIsOpen(true)} - isDisabled={isDisabled} + isDisabled={isDisabled || !itemsToDisassociate.length} > {t`Disassociate`} diff --git a/awx/ui/src/components/DisassociateButton/DisassociateButton.test.js b/awx/ui/src/components/DisassociateButton/DisassociateButton.test.js index 49528bc1e2..6a1cf5b040 100644 --- a/awx/ui/src/components/DisassociateButton/DisassociateButton.test.js +++ b/awx/ui/src/components/DisassociateButton/DisassociateButton.test.js @@ -124,5 +124,21 @@ describe('', () => { ); expect(wrapper.find('button[disabled]')).toHaveLength(1); }); + test('should disable button when selected items contain instances thaat are hybrid and are inside a protected instances', () => { + const wrapper = mountWithContexts( + {}} + isProectedInstanceGroup + itemsToDelete={[ + { + id: 1, + hostname: 'awx', + node_type: 'control', + }, + ]} + /> + ); + expect(wrapper.find('button[disabled]')).toHaveLength(1); + }); }); }); diff --git a/awx/ui/src/screens/InstanceGroup/InstanceDetails/InstanceDetails.js b/awx/ui/src/screens/InstanceGroup/InstanceDetails/InstanceDetails.js index f99a008f33..5232e6de2d 100644 --- a/awx/ui/src/screens/InstanceGroup/InstanceDetails/InstanceDetails.js +++ b/awx/ui/src/screens/InstanceGroup/InstanceDetails/InstanceDetails.js @@ -275,10 +275,11 @@ function InstanceDetails({ setBreadcrumb, instanceGroup }) { {me.is_superuser && instance.node_type !== 'control' && ( )} diff --git a/awx/ui/src/screens/InstanceGroup/Instances/InstanceList.js b/awx/ui/src/screens/InstanceGroup/Instances/InstanceList.js index 219fceb4dc..332c4319e1 100644 --- a/awx/ui/src/screens/InstanceGroup/Instances/InstanceList.js +++ b/awx/ui/src/screens/InstanceGroup/Instances/InstanceList.js @@ -31,7 +31,7 @@ const QS_CONFIG = getQSConfig('instance', { order_by: 'hostname', }); -function InstanceList() { +function InstanceList({ instanceGroup }) { const [isModalOpen, setIsModalOpen] = useState(false); const location = useLocation(); const { id: instanceGroupId } = useParams(); @@ -224,13 +224,15 @@ function InstanceList() { ] : []), s.node_type === 'control' - )} + verifyCannotDisassociate={ + selected.some((s) => s.node_type === 'control') || + instanceGroup.name === 'controlplane' + } key="disassociate" onDisassociate={handleDisassociate} itemsToDisassociate={selected} modalTitle={t`Disassociate instance from instance group?`} + isProtectedInstanceGroup={instanceGroup.name === 'controlplane'} />, ', () => { await act(async () => { wrapper = mountWithContexts( - + , { context: { diff --git a/awx/ui/src/screens/InstanceGroup/Instances/Instances.js b/awx/ui/src/screens/InstanceGroup/Instances/Instances.js index f77bad9d6d..8a7d9132f7 100644 --- a/awx/ui/src/screens/InstanceGroup/Instances/Instances.js +++ b/awx/ui/src/screens/InstanceGroup/Instances/Instances.js @@ -21,7 +21,7 @@ function Instances({ setBreadcrumb, instanceGroup }) { /> - + ); diff --git a/awx/ui/src/util/validators.js b/awx/ui/src/util/validators.js index c8ef9d7563..51a68126ac 100644 --- a/awx/ui/src/util/validators.js +++ b/awx/ui/src/util/validators.js @@ -186,8 +186,3 @@ export function regExp() { return undefined; }; } - -export function protectedResourceName(message, names = []) { - return (value) => - names.some((name) => value.trim() === `${name}`) ? message : undefined; -} diff --git a/awx/ui/src/util/validators.test.js b/awx/ui/src/util/validators.test.js index a8631893c6..0bbd4d1f19 100644 --- a/awx/ui/src/util/validators.test.js +++ b/awx/ui/src/util/validators.test.js @@ -12,7 +12,6 @@ import { regExp, requiredEmail, validateTime, - protectedResourceName, } from './validators'; describe('validators', () => { @@ -188,21 +187,4 @@ describe('validators', () => { expect(validateTime()('12.15 PM')).toEqual('Invalid time format'); expect(validateTime()('12;15 PM')).toEqual('Invalid time format'); }); - test('protectedResourceName should validate properly', () => { - expect( - protectedResourceName('failed validation', ['Alex'])('Apollo') - ).toBeUndefined(); - expect( - protectedResourceName('failed validation', ['Alex', 'Athena'])('alex') - ).toBeUndefined(); - expect( - protectedResourceName('failed validation', ['Alex', 'Athena'])('Alex') - ).toEqual('failed validation'); - expect( - protectedResourceName('failed validation', ['Alex'])('Alex') - ).toEqual('failed validation'); - expect( - protectedResourceName('failed validation', ['Alex'])('Alex ') - ).toEqual('failed validation'); - }); });