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

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.56%. Comparing base (b99a030) to head (592bc42).

Files with missing lines Patch % Lines
...a/com/cloud/network/router/CommandSetupHelper.java 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #11766      +/-   ##
============================================
- Coverage     17.56%   17.56%   -0.01%     
+ Complexity    15500    15497       -3     
============================================
  Files          5899     5899              
  Lines        527793   527801       +8     
  Branches      64479    64482       +3     
============================================
- Hits          92714    92704      -10     
- Misses       424653   424674      +21     
+ Partials      10426    10423       -3     
Flag Coverage Δ
uitests 3.59% <ø> (-0.01%) ⬇️
unittests 18.62% <0.00%> (-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

@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?

@CodeBleu
Copy link
Contributor Author

CodeBleu 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

That particular issue in your screenshots is the space.
image

@CodeBleu
Copy link
Contributor Author

CodeBleu 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.

@rosi-shapeblue

Your first video has spaces in the list of IP's, even though you added the new one with a ','.
image

The second video I'm not sure what happened there, and am unable to reproduce on my end. I'm able to add and update multiple times with using ',' between CIDRs. The only thing I see in that video is the name of the rule is the same as the one that is bad and has spaces in the list.

Do you mind testing again and making sure there are no spaces?

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 9, 2025

@rosi-shapeblue @DaanHoogland In both examples above, the issue is there is a space in the CIDR list. As far as the code I have locally, there is no different than what is in this PR.

@rosi-shapeblue I re-read what you said and see now that the issue is only from the create of the rule with multiple CIDR as the output it shows when editing has spaces in it.

@DaanHoogland @rosi-shapeblue I'm looking into a fix....

@DaanHoogland
Copy link
Contributor

@CodeBleu this is why I added the last image to #11766 (comment). It has the correct format, and no spaces.

When I create a new rule with "1.1.1.1/32,2.2.2.0/24,3.3.0.0/16”, all is fine. When I want to edit it it fails. I do not think this is last bit is a UI issue.

2025-10-09 14:15:42,509 DEBUG [c.c.a.t.Request] (API-Job-Executor-68:[ctx-80f235f2, job-79, ctx-d0c6c84b]) (logid:207b8cee) Seq 2-4503599627377228: Sending  { Cmd , MgmtId: 32986590347802, via: 2(pr11766-t14586-kvm-
ol8-kvm2), Ver: v1, Flags: 100001, [{"com.cloud.agent.api.routing.LoadBalancerConfigCommand":{"loadBalancers":[{"uuid":"81e9da8d-2cbd-49ad-aa6a-fdfe3ca95b06","srcIp":"10.0.61.84","srcPort":"80","protocol":"tcp","lbP
rotocol":"tcp","algorithm":"roundrobin","revoked":"false","alreadyAdded":"false","inline":"false","destinations":[],"cidrList":"1.2.3.4/32"},{"uuid":"f52aebc4-5957-4285-83e2-97c5037165ca","srcIp":"10.0.61.100","srcP
ort":"80","protocol":"tcp","lbProtocol":"tcp","algorithm":"roundrobin","revoked":"false","alreadyAdded":"false","inline":"false","destinations":[{"destIp":"10.1.1.125","destPort":"80","revoked":"false","alreadyAdded
":"false"},{"destIp":"10.1.1.177","destPort":"80","revoked":"false","alreadyAdded":"false"}],"cidrList":"1.1.1.1/32,2.2.2.0/24,3.3.0.0/16"}],"lbStatsVisibility":"global","lbStatsPublicIP":"10.0.61.84","lbStatsPrivat
eIP":"169.254.238.242","lbStatsGuestIP":"10.1.1.1","lbStatsPort":"8081","lbStatsSrcCidrs":"0/0","lbStatsAuth":"admin1:AdMiN123","lbStatsUri":"/admin?stats","maxconn":"4096","keepAliveEnabled":"false","nic":{"deviceI
d":"0","networkRateMbps":"200","defaultNic":"false","pxeDisable":"true","nicUuid":"e4587ab1-c0a7-42ff-b13d-1743ae38a37a","details":{"MacAddressChanges":"true","ForgedTransmits":"true","PromiscuousMode":"false","MacL
earning":"false"},"dpdkEnabled":"false","networkId":"204","networkSegmentName":"D1-A2-Z1-S204","uuid":"43d8a429-9f3f-4575-a4c5-facd0004a920","ip":"10.1.1.1","netmask":"255.255.255.0","mac":"02:01:00:cc:00:02","broad
castType":"Vlan","type":"Guest","broadcastUri":"vlan://3272","isolationUri":"vlan://3272","securityGroupEnabled":"false","name":"cloudbr1"},"accessDetails":{"router.name":"r-5-VM","router.guest.ip":"10.1.1.1","route
r.ip":"169.254.238.242","zone.network.type":"Advanced"},"wait":"0","bypassHostMaintenance":"false"}}] }

and then later

2025-10-09 14:15:43,299 DEBUG [c.c.a.t.Request] (AgentManager-Handler-19:[]) (logid:) Seq 2-4503599627377228: Processing:  { Ans: , MgmtId: 32986590347802, via: 2, Ver: v1, Flags: 0, [{"com.cloud.agent.api.routing.G
roupAnswer":{"results":["null - success: Creating file in VR, with ip: 169.254.238.242, file: load_balancer.json.4a6afb57-0f81-45e7-9bf5-aff59523da88","null - failed: java.io.IOException: Stream closed
        at java.base/java.io.BufferedInputStream.getBufIfOpen(BufferedInputStream.java:168)
        at java.base/java.io.BufferedInputStream.read(BufferedInputStream.java:334)
        at java.base/sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:287)
        at java.base/sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:330)
        at java.base/sun.nio.cs.StreamDecoder.read(StreamDecoder.java:190)
        at java.base/java.io.InputStreamReader.read(InputStreamReader.java:177)
        at java.base/java.io.BufferedReader.fill(BufferedReader.java:162)
        at java.base/java.io.BufferedReader.readLine(BufferedReader.java:329)
        at java.base/java.io.BufferedReader.readLine(BufferedReader.java:396)
        at com.cloud.utils.script.OutputInterpreter.processError(OutputInterpreter.java:41)
        at com.cloud.utils.script.Script.execute(Script.java:305)
        at com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.executeInVR(LibvirtComputingResource.java:632)
        at com.cloud.agent.resource.virtualnetwork.VirtualRoutingResource.applyConfigToVR(VirtualRoutingResource.java:315)
        at com.cloud.agent.resource.virtualnetwork.VirtualRoutingResource.applyConfig(VirtualRoutingResource.java:330)
        at com.cloud.agent.resource.virtualnetwork.VirtualRoutingResource.executeRequest(VirtualRoutingResource.java:170)
        at com.cloud.hypervisor.kvm.resource.wrapper.LibvirtNetworkElementCommandWrapper.execute(LibvirtNetworkElementCommandWrapper.java:35)
        at com.cloud.hypervisor.kvm.resource.wrapper.LibvirtNetworkElementCommandWrapper.execute(LibvirtNetworkElementCommandWrapper.java:29)
        at com.cloud.hypervisor.kvm.resource.wrapper.LibvirtRequestWrapper.execute(LibvirtRequestWrapper.java:78)
        at com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.executeRequest(LibvirtComputingResource.java:2254)
        at com.cloud.agent.Agent.processRequest(Agent.java:813)
        at com.cloud.agent.Agent$AgentRequestHandler.doTask(Agent.java:1295)
        at com.cloud.utils.nio.Task.call(Task.java:83)
        at com.cloud.utils.nio.Task.call(Task.java:29)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:840)
"],"result":"false","wait":"0","bypassHostMaintenance":"false"}}] }

while after that I can still create a rule on this VR, just fine. I think we hit an different but related bug on the VR.

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 9, 2025

@DaanHoogland after further digging it appears that the backend for listing the CIDR is returning it with spaces.

image

I have submitted a fix to work around this.

I have updated my branch, but not sure why this is not being updated and reflected in the PR 😕

https://github.com/American-Cloud/cloudstack/commits/LB-source-cidr-edit-UI/

**** UPDATE ****
Must have been issue with Github. I see it finally updated 😄

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 9, 2025

@blueorangutan package

@blueorangutan
Copy link

@CodeBleu 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 15372

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 9, 2025

@DaanHoogland @rosi-shapeblue Whenever you get a chance, I believe it should be ready to test again

@vishesh92
Copy link
Member

image

@CodeBleu The rules which are being sent to the VR seem to be in the wrong format. Because of which I am seeing the below error.

image

Also, minor issue in UI where source CIDR list is coming twice.
image

@CodeBleu
Copy link
Contributor Author

CodeBleu commented Oct 10, 2025

@vishesh92 Thanks for your input. It appears that the real issue here is where some things are listing the CIDR list space formated and some comma.

When I went back and tested multipe CIDRs with cloudmonkey and then checked the haproxy config on the VR, I see the issue you are talking about.

image image

@CodeBleu CodeBleu marked this pull request as draft October 10, 2025 15:25
    * The backend is sending the list with spaces
    * This fix formats the form properly no matter if backend sends with
      spaces or commas.
@CodeBleu
Copy link
Contributor Author

@DaanHoogland @vishesh92 @rosi-shapeblue Fixes have been added. Here is my output now
image

image image image

@CodeBleu
Copy link
Contributor Author

@blueorangutan package

@CodeBleu CodeBleu marked this pull request as ready for review October 10, 2025 18:11
@blueorangutan
Copy link

@CodeBleu 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 15411

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.