Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue where security group selection is shown regardless of zone settings in EditVM #10224

Open
wants to merge 1 commit into
base: 4.20
Choose a base branch
from

Conversation

toolmanwyj
Copy link

Description

In CloudStack, the security group selection box is displayed when editing a VM, regardless of whether the zone supports security groups or not. This causes the securitygroupids parameter to be sent with the request even if no security group is selected. If the zone is of type NetworkType.Advanced and does not support security groups, this triggers a NullPointerException in the UserVmManagerImpl.updateVirtualMachine method when calling isSecurityGroupSupportedInNetwork.

Upon investigation, it was found that the listZones API call, which is used to fetch the zone details, incorrectly uses zoneid instead of id for the zone identifier. This prevents the correct zone information from being retrieved. Additionally, since the listZones API does not properly filter zones, it fetches all zones and always takes the first one in the list (index 0) to determine whether security groups are enabled. Consequently, if that first zone supports security groups, the security group selection box is displayed in the UI—even if the zone actually used by the VM does not support them.

Moreover, the logic for checking whether the security group selection box is empty is not strict enough, which causes the securitygroupids parameter to be included in the request even when the selection box is empty.

Proposed Solution

  1. Correct the parameter name for the zone identifier when calling the listZones API by using id instead of zoneid.
  2. Filter the zones correctly in the listZones API call to ensure that only the relevant zone is considered when checking for security group support.
  3. Improve the logic for checking if the security group selection box is empty before attaching the securitygroupids parameter to the request.

Modified Files

  • EditVM.vue

PR Changes

  • Fix incorrect usage of zoneid in listZones calls to properly retrieve the VM’s zone details.
  • Only consider the matching zone (instead of just taking the first item) to determine whether security groups are enabled.
  • Enhance the empty-check logic for the security group selection, preventing securitygroupids from being included if no security group is actually selected.
  • Prevent a NullPointerException when editing VMs in NetworkType.Advanced zones that do not support security groups.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

  1. Local Dev Environment

    • Configured multiple zones: one supporting security groups and one not supporting security groups (VPC).
    • Edited a VM in each zone to confirm that:
      1. When the zone supports SG, the security group selection is displayed and functions correctly.
      2. When the zone does not support SG, no security group selection is displayed or an empty list is properly handled, and no securitygroupids is sent to the backend.
    • Verified no NullPointerException is thrown even if securitygroupids is null or empty.
  2. UI Validation

    • Checked that the modified logic in EditVM.vue properly filters the correct zone from listZones.
    • Ensured that if the user does not select a security group, the request does not include securitygroupids.

How did you try to break this feature and the system with this change?

  • Attempted to edit a VM in a zone that does not support SG and artificially forced the frontend to pass securitygroupids, verifying that no NPE occurs.
  • Made sure any new changes do not affect zones where security groups are legitimately supported.

Copy link

boring-cyborg bot commented Jan 21, 2025

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@DaanHoogland
Copy link
Contributor

@blueorangutan ui

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.02%. Comparing base (a163831) to head (9d4128b).

❗ There is a different number of reports uploaded between BASE (a163831) and HEAD (9d4128b). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (a163831) HEAD (9d4128b)
unittests 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##               4.20   #10224       +/-   ##
=============================================
- Coverage     16.13%    4.02%   -12.12%     
=============================================
  Files          5639      394     -5245     
  Lines        494264    32345   -461919     
  Branches      59899     5713    -54186     
=============================================
- Hits          79760     1301    -78459     
+ Misses       405684    30896   -374788     
+ Partials       8820      148     -8672     
Flag Coverage Δ
uitests 4.02% <ø> (+<0.01%) ⬆️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/10224 (QA-JID-517)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants