Allow updating of Load Balancer source CIDR list #10968

CodeBleu · 2025-06-05T16:42:14Z

Description

This PR will allow the updating of a loadbalancer rules CIDR list via the API.
* Should fix #9313

Not 100% sure this is the correct place to base and create PR for, but this issue does exist in 4.19, 4.20, and main. I figure I'd start here and see what is needed to get this into the code base so >= 4.19 will have the fixes.

I have tested this code in 4.19, 4.20, and main branches via simulator and all works, just not sure of the process to get this fix into those branches.

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

Screenshots (if appropriate):

Before, the Source CIDR list was blank (Simulator env)

(localcloud) :penguin: > list loadbalancerrules
{
  "count": 1,
  "loadbalancerrule": [
    {
      "account": "admin",
      "algorithm": "roundrobin",
      "cidrlist": "",
      "domain": "ROOT",
      "domainid": "5b6a2947-416b-11f0-9a39-564a8191c23c",
      "domainpath": "/",
      "fordisplay": true,
      "id": "0d8e3cb9-767b-45f3-834d-3f9ce85df901",
      "name": "newlbtest",
      "networkid": "5bb3ecfb-cde3-4821-8822-0c4c8a8e9559",
      "privateport": "3306",
      "protocol": "tcp",
      "publicip": "192.168.2.10",
      "publicipid": "9b2638d7-7330-49c7-9d6e-5b5777f6ca16",
      "publicport": "3306",
      "state": "Add",
      "tags": [],
      "zoneid": "7cb86a09-676e-4f00-ad39-012f0eb2e69d",
      "zonename": "Sandbox-simulator"
    }
  ]
}
(localcloud) :penguin: > update loadbalancerrule id="0d8e3cb9-767b-45f3-834d-3f9ce85df901" cidrlist="1.2.3.4/32"
{
  "loadbalancer": {
    "account": "admin",
    "algorithm": "roundrobin",
    "cidrlist": "1.2.3.4/32",
    "domain": "ROOT",
    "domainid": "5b6a2947-416b-11f0-9a39-564a8191c23c",
    "domainpath": "/",
    "fordisplay": true,
    "id": "0d8e3cb9-767b-45f3-834d-3f9ce85df901",
    "name": "newlbtest",
    "networkid": "5bb3ecfb-cde3-4821-8822-0c4c8a8e9559",
    "privateport": "3306",
    "protocol": "tcp",
    "publicip": "192.168.2.10",
    "publicipid": "9b2638d7-7330-49c7-9d6e-5b5777f6ca16",
    "publicport": "3306",
    "state": "Add",
    "tags": [],
    "zoneid": "7cb86a09-676e-4f00-ad39-012f0eb2e69d",
    "zonename": "Sandbox-simulator"
  }
}
(localcloud) :penguin: > update loadbalancerrule id="0d8e3cb9-767b-45f3-834d-3f9ce85df901" cidrlist=
{
  "loadbalancer": {
    "account": "admin",
    "algorithm": "roundrobin",
    "cidrlist": "",
    "domain": "ROOT",
    "domainid": "5b6a2947-416b-11f0-9a39-564a8191c23c",
    "domainpath": "/",
    "fordisplay": true,
    "id": "0d8e3cb9-767b-45f3-834d-3f9ce85df901",
    "name": "newlbtest",
    "networkid": "5bb3ecfb-cde3-4821-8822-0c4c8a8e9559",
    "privateport": "3306",
    "protocol": "tcp",
    "publicip": "192.168.2.10",
    "publicipid": "9b2638d7-7330-49c7-9d6e-5b5777f6ca16",
    "publicport": "3306",
    "state": "Add",
    "tags": [],
    "zoneid": "7cb86a09-676e-4f00-ad39-012f0eb2e69d",
    "zonename": "Sandbox-simulator"
  }
}

Tested on actual test environment and below is where you can see when it was restricted it didn't connect to mysql, but when opened up and CIDR set to 0.0.0.0/0 it worked. I tested with specific Public IP in CIDR as well (x.x.0.118/32) and this works and you can see in the virtual router for haproxy it set the ACL.

Restricted with wrong IP in source CIDR of LB

⮡ $ mysql -h x.x.x.110 -u root -p
Enter password:
ERROR 2013 (HY000): Lost connection to MySQL server at 'reading initial communication packet', system error: 104

With correct source CIDR or 0.0.0.0/0

$ mysql -h x.x.x.110 -u root -p
Enter password:
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 4730216
Server version: 8.0.33 MySQL Community Server - GPL

Copyright (c) 2000, 2024, Oracle and/or its affiliates.

Oracle is a registered trademark of Oracle Corporation and/or its
affiliates. Other names may be trademarks of their respective
owners.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> quit
Bye

How Has This Been Tested?

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

Ran multiple tests with Cloudmonkey against simulator and actual test environment. See above for testing info

boring-cyborg · 2025-06-05T16:42:18Z

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:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected] (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

codecov · 2025-06-05T16:50:29Z

Codecov Report

❌ Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 15.18%. Comparing base (d161dc7) to head (182209f).
⚠️ Report is 3 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
...loud/network/lb/LoadBalancingRulesManagerImpl.java	10.00%	5 Missing and 4 partials ⚠️
...d/user/loadbalancer/UpdateLoadBalancerRuleCmd.java	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.19   #10968   +/-   ##
=========================================
  Coverage     15.18%   15.18%           
- Complexity    11368    11375    +7     
=========================================
  Files          5415     5415           
  Lines        476073   476088   +15     
  Branches      58125    58129    +4     
=========================================
+ Hits          72279    72303   +24     
+ Misses       395706   395689   -17     
- Partials       8088     8096    +8

Flag	Coverage Δ
uitests	`4.28% <ø> (ø)`
unittests	`15.91% <31.25%> (+<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.

DaanHoogland · 2025-06-09T14:35:49Z

@CodeBleu 4.19 is perfectly alright for this PR.

DaanHoogland

clgtm

CodeBleu · 2025-06-09T18:31:16Z

@CodeBleu 4.19 is perfectly alright for this PR.

@DaanHoogland Great, thanks!
Since I'm new to this process, I had a couple questions.

Do I need another review/approval before I or someone else does the "Squash and merge"?
How and when will this change make it to the next version of 4.19.x, 4.20.x and 4.21.x?

DaanHoogland · 2025-06-09T19:55:07Z

@CodeBleu 4.19 is perfectly alright for this PR.

@DaanHoogland Great, thanks!
Since I'm new to this process, I had a couple questions.

Do I need another review/approval before I or someone else does the "Squash and merge"?

Yes, we should encode this better. We require least two reviews and minimal 1 "external" tester.

How and when will this change make it to the next version of 4.19.x, 4.20.x and 4.21.x?

When merged by a committer, we will merge the target branch forward to all release branches and main.

DaanHoogland · 2025-06-11T17:24:58Z

@blueorangutan package

blueorangutan · 2025-06-11T17:26:04Z

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

blueorangutan · 2025-06-11T18:24:56Z

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

DaanHoogland · 2025-06-12T06:57:52Z

@blueorangutan test

blueorangutan · 2025-06-12T07:00:05Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2025-06-12T07:50:31Z

[SF] Trillian Build Failed (tid-13506)

blueorangutan · 2025-06-12T22:25:23Z

[SF] Trillian test result (tid-13507)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 47902 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10968-t13507-kvm-ol8.zip
Smoke tests completed. 133 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

CodeBleu · 2025-08-29T13:12:49Z

@vishesh92 @rohityadavcloud Can I get one or both of you to review this, so we can have 2 reviews and hopefully get this merged in please?

Copilot

Pull Request Overview

This PR adds support for updating Load Balancer source CIDR lists via the API, addressing issue #9313. Previously, the CIDR list could only be set during creation but not modified afterwards.

Adds CIDR list parameter to the UpdateLoadBalancerRuleCmd API command
Implements CIDR list validation and update logic in LoadBalancingRulesManagerImpl
Includes proper rollback handling when CIDR list updates fail

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
UpdateLoadBalancerRuleCmd.java	Adds cidrList parameter to enable CIDR list updates via API
LoadBalancingRulesManagerImpl.java	Implements CIDR validation, update logic, and rollback handling
LoadBalancerVO.java	Adds setCidrList method to enable CIDR list updates
LoadBalancerVOTest.java	Adds unit tests for CIDR list setter functionality

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java

server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java

weizhouapache · 2025-08-29T15:00:16Z

@CodeBleu
4.19 will not be supported soon
since this is an improvement, I suggest to change the target branch to main

CodeBleu · 2025-08-29T15:03:10Z

@CodeBleu 4.19 will not be supported soon since this is an improvement, I suggest to change the target branch to main

When I first asked about this, I was told this branch was fine and that it can be merged up?
#10968 (comment)

weizhouapache · 2025-08-29T15:12:21Z

@CodeBleu 4.19 will not be supported soon since this is an improvement, I suggest to change the target branch to main

When I first asked about this, I was told this branch was fine and that it can be merged up? #10968 (comment)

4.19 will be EOL on 1st September, It will not be updated after that.
https://cwiki.apache.org/confluence/display/CLOUDSTACK/LTS

CodeBleu · 2025-08-29T15:13:04Z

@vishesh92 I feel like the failures for checks now is just a glitch and maybe a re-run will resolve? Not sure what is needed at this point.

weizhouapache · 2025-08-29T15:22:53Z

@vishesh92 I feel like the failures for checks now is just a glitch and maybe a re-run will resolve? Not sure what is needed at this point.

it has been addressed by #11537

CodeBleu · 2025-08-29T15:31:11Z

@weizhouapache What is the best way to switch this to main branch? I can edit this PR and select main, but it gives a warning and just want to make sure I don't make things worse. At the same time, I kinda hate to re-base and main and push up and create a new PR, because of all the review/comments in this one.

weizhouapache · 2025-08-29T15:49:39Z

@weizhouapache What is the best way to switch this to main branch? I can edit this PR and select main, but it gives a warning and just want to make sure I don't make things worse. At the same time, I kinda hate to re-base and main and push up and create a new PR, because of all the review/comments in this one.

from my experience, the best way is, merging remote main branch into local branch and fixing the conflicts.
if there are multiple commits in the PR, we only need to fix the conflicts once.
when conflicts are fixed, build the source code locally and then force-push to github.

just bare in mind that, log4j 2.x is used since 4.20, so you need to update the code (to use logger/LOGGER/logging, please check other logging messages in the same file)
(normally we fix the logger issues during merge forward)

CodeBleu · 2025-08-29T15:53:17Z

@DaanHoogland Is this something you think can just be merged before the Sept 1st? I feel like this should be good. The recent copilot changes were minimal and all checks had passed before, but appears that is an issue with checks now, but a fix is in place.

It would be nice to not have to make branch changes at this point, if we can get this merged and then merged forward.

Thoughts?

DaanHoogland · 2025-08-29T16:08:10Z

We can @CodeBleu , but we'll have to deal with the conflicts that @weizhouapache mentioned at some time; either before merge or after... while merging 4.19 forward. Btw, you are committer now, right? So if you have two reviews and a test report you can merge.

CodeBleu · 2025-08-29T16:20:06Z

@weizhouapache can we merge #11537 so the test here can be re-ran? Also, if you approve this review then it will meet the 2 reviewers and checks test and should be able to go ahead and merge

weizhouapache · 2025-08-29T16:37:34Z

@weizhouapache can we merge #11537 so the test here can be re-ran? Also, if you approve this review then it will meet the 2 reviewers and checks test and should be able to go ahead and merge

@CodeBleu
it requires 2 approvals including 1 manual testing.
have someone tested it ?

* Should fix apache#9313

- Replace manual null-check comparison with Objects.equals for clarity and null safety - Simplify CIDR list rollback to always restore backup value unconditionally - Add JavaDoc for setCidrList method for improved documentation

CodeBleu · 2025-08-29T16:40:05Z

@weizhouapache can we merge #11537 so the test here can be re-ran? Also, if you approve this review then it will meet the 2 reviewers and checks test and should be able to go ahead and merge

@CodeBleu it requires 2 approvals including 1 manual testing. have someone tested it ?

@weizhouapache I tested it and have my results listed at the top of this PR. If you can test it also, that would be much appreciated 😄

CodeBleu · 2025-08-29T18:45:06Z

@rohityadavcloud @weizhouapache @vishesh92 If I can get 1 more review approval and a manual test, I can merge this and get it across the line before Sept 1st . That would be much appreciated.

weizhouapache · 2025-08-29T19:38:32Z

@rohityadavcloud @weizhouapache @vishesh92 If I can get 1 more review approval and a manual test, I can merge this and get it across the line before Sept 1st . That would be much appreciated.

@CodeBleu
In my opinion, it should be targeted to main, not 4.19/4.20, since this is an improvement.

DaanHoogland · 2025-08-31T09:26:33Z

@weizhouapache as this introduces no backwards incompatibility and @CodeBleu wants it in an older version I think we can merge it in 4.19. We’ll have to deal with the fallout of merging forwards though.

weizhouapache · 2025-08-31T09:30:00Z

@weizhouapache as this introduces no backwards incompatibility and @CodeBleu wants it in an older version I think we can merge it in 4.19. We’ll have to deal with the fallout of merging forwards though.

ok, no objection

DaanHoogland · 2025-08-31T09:35:32Z

and with “we” need to deal I mainly mean @CodeBleu , of course ;)

CodeBleu · 2025-08-31T11:53:36Z

@weizhouapache as this introduces no backwards incompatibility and @CodeBleu wants it in an older version I think we can merge it in 4.19. We’ll have to deal with the fallout of merging forwards though.

Pending another review approval and a manual test from someone else? Also, not sure what time zone the Sept 1st deadline is either, so this could already be an issue to merge it before Sept 1st ?

DaanHoogland · 2025-09-01T09:51:57Z

@weizhouapache as this introduces no backwards incompatibility and @CodeBleu wants it in an older version I think we can merge it in 4.19. We’ll have to deal with the fallout of merging forwards though.

Pending another review approval and a manual test from someone else? Also, not sure what time zone the Sept 1st deadline is either, so this could already be an issue to merge it before Sept 1st ?

It is not forbidden to merge into 4.19 after today @CodeBleu . We won’t support the branch with fixes perse, but you can if you. want. The more pressing issue remains the conflicts.

And yes, we generally expect that a tester (not being the author) has given their review (and minimal test description).

Tell us what you want to do/how you want to go about it @CodeBleu and we can work towards it.

weizhouapache · 2025-09-01T13:13:48Z

...main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java

    @Parameter(name = ApiConstants.PROTOCOL, type = CommandType.STRING, description = "The protocol for the LB")
    private String lbProtocol;

+    @Parameter(name = ApiConstants.CIDR_LIST, type = CommandType.LIST, collectionType = CommandType.STRING, description = "the cidr list to forward traffic from")


since needs to be added

if since is 4.19.4, but it is actually unsupported in 4.20.0 and 4.20.1 (which have been released), it might cause misunderstanding

@CodeBleu

btw: I have tested the changes. the main problem is, which version should be targeted.
For PRs with api changes or db changes, I suggest to target to latest branch (main)

CodeBleu · 2025-09-03T13:31:34Z

Closing - Created new base off of main and new PR #11568

CodeBleu requested a review from DaanHoogland June 5, 2025 16:42

boring-cyborg bot added component:api component:networking labels Jun 5, 2025

CodeBleu requested a review from weizhouapache June 5, 2025 16:56

CodeBleu mentioned this pull request Jun 6, 2025

Support firewall rules on Public IP in VPC #8863

Open

DaanHoogland approved these changes Jun 9, 2025

View reviewed changes

CodeBleu requested review from rohityadavcloud and vishesh92 August 29, 2025 13:11

vishesh92 requested a review from Copilot August 29, 2025 13:19

Copilot AI reviewed Aug 29, 2025

View reviewed changes

CodeBleu mentioned this pull request Aug 29, 2025

feat: add cidrlist parameter to loadbalancer rule apache/cloudstack-terraform-provider#147

Merged

CodeBleu force-pushed the 419_lb_update_cidr branch from 421ddd9 to d52180e Compare August 29, 2025 15:37

CodeBleu requested a review from DaanHoogland August 29, 2025 15:53

CodeBleu added 5 commits August 29, 2025 12:37

Allow updating of Load Balancer source CIDR list

b9f1c5b

* Should fix apache#9313

Added test coverage for LoadBalancerVO

1f96c10

Add License text to LoadBalancerVOTest.java file

0078b93

Refactor LB CIDR list comparison & rollback code

0d9b2a9

- Replace manual null-check comparison with Objects.equals for clarity and null safety - Simplify CIDR list rollback to always restore backup value unconditionally - Add JavaDoc for setCidrList method for improved documentation

Added missing Import for java.util.Objects

182209f

CodeBleu force-pushed the 419_lb_update_cidr branch from d52180e to 182209f Compare August 29, 2025 16:37

weizhouapache reviewed Sep 1, 2025

View reviewed changes

CodeBleu mentioned this pull request Sep 3, 2025

Allow updating of Load Balancer source CIDR list #11568

Merged

14 tasks

CodeBleu closed this Sep 3, 2025

Allow updating of Load Balancer source CIDR list #10968

Allow updating of Load Balancer source CIDR list #10968

Uh oh!

Conversation

CodeBleu commented Jun 5, 2025

Description

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?

Uh oh!

boring-cyborg bot commented Jun 5, 2025

Uh oh!

codecov bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Jun 9, 2025

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

CodeBleu commented Jun 9, 2025

Uh oh!

DaanHoogland commented Jun 9, 2025

Uh oh!

DaanHoogland commented Jun 11, 2025

Uh oh!

blueorangutan commented Jun 11, 2025

Uh oh!

blueorangutan commented Jun 11, 2025

Uh oh!

DaanHoogland commented Jun 12, 2025

Uh oh!

blueorangutan commented Jun 12, 2025

Uh oh!

blueorangutan commented Jun 12, 2025

Uh oh!

blueorangutan commented Jun 12, 2025

Uh oh!

CodeBleu commented Aug 29, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weizhouapache commented Aug 29, 2025

Uh oh!

CodeBleu commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weizhouapache commented Aug 29, 2025

Uh oh!

CodeBleu commented Aug 29, 2025

Uh oh!

weizhouapache commented Aug 29, 2025

Uh oh!

CodeBleu commented Aug 29, 2025

Uh oh!

weizhouapache commented Aug 29, 2025

Uh oh!

CodeBleu commented Aug 29, 2025

Uh oh!

DaanHoogland commented Aug 29, 2025

Uh oh!

CodeBleu commented Aug 29, 2025

Uh oh!

weizhouapache commented Aug 29, 2025

Uh oh!

CodeBleu commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodeBleu commented Aug 29, 2025

codecov bot commented Jun 5, 2025 •

edited

Loading

CodeBleu commented Aug 29, 2025 •

edited

Loading

CodeBleu commented Aug 29, 2025 •

edited

Loading