Skip to content

Conversation

CodeBleu
Copy link
Contributor

@CodeBleu CodeBleu commented Sep 30, 2025

Description

This PR allows editing the source CIDR of a load balancer rule.

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

Screenshots (if appropriate):

image image image

How Has This Been Tested?

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

Followed steps in ui/README.md

Ran on Cloudstack environment using the update cidr API fix (#11568) and confirmed access based on IP's added to source CIDR.

Copy link

codecov bot commented Sep 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.50%. Comparing base (d60f455) to head (0c962bf).
⚠️ Report is 31 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #11766      +/-   ##
============================================
- Coverage     17.50%   17.50%   -0.01%     
  Complexity    15427    15427              
============================================
  Files          5894     5894              
  Lines        526847   526851       +4     
  Branches      64335    64337       +2     
============================================
- Hits          92234    92233       -1     
- Misses       424236   424241       +5     
  Partials      10377    10377              
Flag Coverage Δ
uitests 3.61% <ø> (-0.01%) ⬇️
unittests 18.56% <ø> (-0.01%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@CodeBleu CodeBleu added this to the 4.22.0 milestone Sep 30, 2025
@CodeBleu
Copy link
Contributor Author

@blueorangutan ui

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't test this, but UI changes LGTM. I'm assuming this editing CIDR is available via API

@DaanHoogland
Copy link
Contributor

@CodeBleu , I tested this in QA, but it silently fails, i.e. it reports success but does not change the cidr. Have a look at https://qa.cloudstack.cloud/client/pr/11766/#/publicip/e7314c68-c570-42c5-939e-e61f5b30cb4b

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 7, 2025

@CodeBleu , I tested this in QA, but it silently fails, i.e. it reports success but does not change the cidr. Have a look at https://qa.cloudstack.cloud/client/pr/11766/#/publicip/e7314c68-c570-42c5-939e-e61f5b30cb4b

@DaanHoogland I believe this is because the QA build is not running the version needed to actually make the changes via the API.

#11766 (comment)

@DaanHoogland
Copy link
Contributor

@CodeBleu , I tested this in QA, but it silently fails, i.e. it reports success but does not change the cidr. Have a look at https://qa.cloudstack.cloud/client/pr/11766/#/publicip/e7314c68-c570-42c5-939e-e61f5b30cb4b

@DaanHoogland I believe this is because the QA build is not running the version needed to actually make the changes via the API.

#11766 (comment)

ah, you might have a point, I’ll create a lab env.

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 7, 2025

Didn't test this, but UI changes LGTM. I'm assuming this editing CIDR is available via API

Yes, this is the API code that was already merged #11568

@borisstoyanov
Copy link
Contributor

@rosi-shapeblue can you help testing this, we'd like to include it in 4.22

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 15325

@harikrishna-patnala
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15333

@DaanHoogland
Copy link
Contributor

@CodeBleu , I tested this in QA, but it silently fails, i.e. it reports success but does not change the cidr. Have a look at https://qa.cloudstack.cloud/client/pr/11766/#/publicip/e7314c68-c570-42c5-939e-e61f5b30cb4b

@DaanHoogland I believe this is because the QA build is not running the version needed to actually make the changes via the API.
#11766 (comment)

ah, you might have a point, I’ll create a lab env.

@CodeBleu , I see the same behaviour in a lab env build with the code in this PR.

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 8, 2025

@CodeBleu , I tested this in QA, but it silently fails, i.e. it reports success but does not change the cidr. Have a look at https://qa.cloudstack.cloud/client/pr/11766/#/publicip/e7314c68-c570-42c5-939e-e61f5b30cb4b

@DaanHoogland I believe this is because the QA build is not running the version needed to actually make the changes via the API.
#11766 (comment)

ah, you might have a point, I’ll create a lab env.

@CodeBleu , I see the same behaviour in a lab env build with the code in this PR.

@DaanHoogland are you sure you are using a lab that has this code in it? - #11568 ? It works for me.

Here is it working for me:

image image
lb-test-2025-10-08_09.32.48.mp4

@DaanHoogland
Copy link
Contributor

I created packages #11766 (comment), which is based on this PR merged in main. It should contain all the code.

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 8, 2025

I created packages #11766 (comment), which is based on this PR merged in main. It should contain all the code.

Could it not be including the code because it's in the code to use 4.22, and the packaging is not including it? I'm not sure exactly how that works. I just manually built it from the main branch locally and used that to test with.

I'm just not able to reproduce your issue, so it's hard for me to do anything differently if it's working for me.

If you let me know the steps you are taking, I can try to reproduce. How does one get the package from the package build ? #11766 (comment)

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Oct 9, 2025

I created packages #11766 (comment), which is based on this PR merged in main. It should contain all the code.

Could it not be including the code because it's in the code to use 4.22, and the packaging is not including it? I'm not sure exactly how that works. I just manually built it from the main branch locally and used that to test with.

I'm just not able to reproduce your issue, so it's hard for me to do anything differently if it's working for me.

I understand, but the packeging job creates a merge commit and builds from that.

If you let me know the steps you are taking, I can try to reproduce. How does one get the package from the package build ? #11766 (comment)

So I build a clean set of packages from this PR merged into main (the standard at blueorangutan package) and then I build a virtualised environment from that. I tried several formats. As the last one exactly the one you show in your clip; I first made it empty (that was successful) and then I added two cidrs back in. I tried with “1.1.1.1/32,2.2.2.0/24” and with “1.2.3.4/32,5.6.7.8/32”. Both failed to add. When finally I add a single cidr it succeeds (e.g. “1.2.3.0/24”)

Are you sure you don’t have an extra fix (in comparison to main) in your environment?

Screenshot 2025-10-09 at 09 05 49 Screenshot 2025-10-09 at 09 06 26 Screenshot 2025-10-09 at 09 07 57

@apache apache deleted a comment from blueorangutan Oct 9, 2025
@rosi-shapeblue
Copy link
Collaborator

I’ll double-check @DaanHoogland 's findings on my test environment and update

@vladimirpetrov
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@vladimirpetrov a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 15354

@vladimirpetrov
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@vladimirpetrov a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15355

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15356

@rosi-shapeblue
Copy link
Collaborator

rosi-shapeblue commented Oct 9, 2025

@DaanHoogland , @CodeBleu - I’ve completed testing this on my test environment.

  1. Creating a Load Balancer rule with single/multiple CIDRs (e.g. 1.1.1.1/32,2.2.2.0/24,3.3.3.3/32) works as expected.

  2. Editing an existing rule and appending additional CIDRs fails with:

Failed to edit rule
Invalid CIDR list: 1.1.1.1/32 2.2.2.0/24,3.3.3.3/32

image

The input was entered correctly (no spaces), but during the edit flow the UI alters the CIDR string - a space replaces the first comma between the CIDRs, resulting in a malformed value being sent to the backend.

Browser console shows:

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'push')
at LoadBalancing.vue:1188

➝ This confirms the issue is UI-related, not API-related.

image

Backend log showing malformed CIDR string (space between 1st and 2nd CIDR) received during UpdateLoadBalancerRuleCmd.

Screencast.from.2025-10-09.15-02-15.webm
Screencast.from.2025-10-09.15-19-14.webm

Summary:

  • Multiple CIDRs work on rule creation.
  • Single CIDRs work on rule creation.
  • Multiple CIDRs break when editing due to the UI transforming the CIDR list incorrectly.
  • API itself accepts the correct format.

Happy to retest after a fix.

Copy link
Collaborator

@rosi-shapeblue rosi-shapeblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For more information, please check: #11766 (comment)

@DaanHoogland
Copy link
Contributor

ok @CodeBleu so basically @rosi-shapeblue confirms my testing. Are you sure you have added all your code to the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

10 participants