-
Notifications
You must be signed in to change notification settings - Fork 1.2k
ui: Allow edit source CIDR on load balancer rule #11766
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@CodeBleu a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
UI build: ✔️ |
There was a problem hiding this 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
@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. |
ah, you might have a point, I’ll create a lab env. |
@blueorangutan package |
@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. |
Yes, this is the API code that was already merged #11568 |
@rosi-shapeblue can you help testing this, we'd like to include it in 4.22 |
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 15325 |
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15333 |
@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: ![]() ![]() lb-test-2025-10-08_09.32.48.mp4 |
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) |
I understand, but the packeging job creates a merge commit and builds from that.
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? ![]() ![]() ![]() |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15356 |
@DaanHoogland , @CodeBleu - I’ve completed testing this on my test environment.
Failed to edit rule ![]() 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') ➝ This confirms the issue is UI-related, not API-related. ![]() Backend log showing malformed CIDR string (space between 1st and 2nd CIDR) received during UpdateLoadBalancerRuleCmd. Screencast.from.2025-10-09.15-02-15.webmScreencast.from.2025-10-09.15-19-14.webmSummary:
Happy to retest after a fix. |
There was a problem hiding this 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)
ok @CodeBleu so basically @rosi-shapeblue confirms my testing. Are you sure you have added all your code to the PR? |
|
Your first video has spaces in the list of IP's, even though you added the new one with a ','. 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? |
@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 @DaanHoogland @rosi-shapeblue I'm looking into a fix.... |
@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.
and then later
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. |
@DaanHoogland after further digging it appears that the backend for listing the CIDR is returning it with spaces. ![]() 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 **** |
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15372 |
@DaanHoogland @rosi-shapeblue Whenever you get a chance, I believe it should be ready to test again |
![]() @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. ![]() Also, minor issue in UI where source CIDR list is coming twice. |
@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. ![]() ![]() |
* The backend is sending the list with spaces * This fix formats the form properly no matter if backend sends with spaces or commas.
a91de0d
to
592bc42
Compare
@DaanHoogland @vishesh92 @rosi-shapeblue Fixes have been added. Here is my output now ![]() ![]() ![]() |
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15411 |
Description
This PR allows editing the source CIDR of a load balancer rule.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
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.