Merge pull request #11791 from AlexSCorey/11713-PreventDisassociateHybridNodeFromControlplan

Prevents disassociate hybrid node on controlplane instance group

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:

awx/api/views/__init__.py

@@ -409,7 +409,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
 
 
@@ -511,7 +519,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
 
 

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):

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)

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 =
+    isDisabled = itemsToDisassociate.some(cannotDisassociate);
-      itemsToDisassociate.length === 0 ||
-      itemsToDisassociate.some(cannotDisassociate);
-  } else {
-    isDisabled = itemsToDisassociate.length === 0;
   }
 
   // NOTE: Once PF supports tooltips on disabled elements,
@@ -89,7 +89,7 @@ function DisassociateButton({
         <DropdownItem
           key="add"
           aria-label={t`disassociate`}
-          isDisabled={isDisabled}
+          isDisabled={isDisabled || !itemsToDisassociate.length}
           component="button"
           ouiaId="disassociate-tooltip"
           onClick={() => 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`}
             </Button>

awx/ui/src/components/DisassociateButton/DisassociateButton.test.js

@@ -124,5 +124,21 @@ describe('<DisassociateButton />', () => {
       );
       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(
+        <DisassociateButton
+          onDisassociate={() => {}}
+          isProectedInstanceGroup
+          itemsToDelete={[
+            {
+              id: 1,
+              hostname: 'awx',
+              node_type: 'control',
+            },
+          ]}
+        />
+      );
+      expect(wrapper.find('button[disabled]')).toHaveLength(1);
+    });
   });
 });

awx/ui/src/screens/InstanceGroup/InstanceDetails/InstanceDetails.js

@@ -275,10 +275,11 @@ function InstanceDetails({ setBreadcrumb, instanceGroup }) {
           </Tooltip>
           {me.is_superuser && instance.node_type !== 'control' && (
             <DisassociateButton
-              verifyCannotDisassociate={!me.is_superuser}
+              verifyCannotDisassociate={instanceGroup.name === 'controlplane'}
               key="disassociate"
               onDisassociate={disassociateInstance}
               itemsToDisassociate={[instance]}
+              isProtectedInstanceGroup={instanceGroup.name === 'controlplane'}
               modalTitle={t`Disassociate instance from instance group?`}
             />
           )}

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() {
                   ]
                 : []),
               <DisassociateButton
-                verifyCannotDisassociate={selected.some(
+                verifyCannotDisassociate={
-                  (s) => s.node_type === 'control'
+                  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'}
               />,
               <HealthCheckButton
                 isDisabled={!canAdd}

awx/ui/src/screens/InstanceGroup/Instances/InstanceList.test.js

@@ -123,7 +123,7 @@ describe('<InstanceList/>', () => {
     await act(async () => {
       wrapper = mountWithContexts(
         <Route path="/instance_groups/:id/instances">
-          <InstanceList />
+          <InstanceList instanceGroup={{ name: 'Alex' }} />
         </Route>,
         {
           context: {

awx/ui/src/screens/InstanceGroup/Instances/Instances.js

@@ -21,7 +21,7 @@ function Instances({ setBreadcrumb, instanceGroup }) {
         />
       </Route>
       <Route key="instanceList" path="/instance_groups/:id/instances">
-        <InstanceList />
+        <InstanceList instanceGroup={instanceGroup} />
       </Route>
     </Switch>
   );

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;
-}

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');
-  });
 });