-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Allow updating of Load Balancer source CIDR list #11568
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
- 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
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
Outdated
Show resolved
Hide resolved
@@ -64,6 +65,9 @@ public class UpdateLoadBalancerRuleCmd extends BaseAsyncCustomIdCmd { | |||
@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") |
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.
add since
to this api parameter ?
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.
Should this be:
@Parameter(name = ApiConstants.CIDR_LIST, type = CommandType.LIST, collectionType = CommandType.STRING, description = "the cidr list to forward traffic from") | |
@Parameter(name = ApiConstants.CIDR_LIST, type = CommandType.LIST, collectionType = CommandType.STRING, description = "the cidr list to forward traffic from", since = "4.22") |
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.
yes @CodeBleu , normally 4.22.0
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.
@weizhouapache After thinking about this more, what needs to be done to get this fix in the next patch release of 4.20, and 4.21 so I don't have to wait until 4.22 is released. Can I change the since
to 4.20.1.1
and 4.21.0.1
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.
@weizhouapache After thinking about this more, what needs to be done to get this fix in the next patch release of 4.20, and 4.21 so I don't have to wait until 4.22 is released. Can I change the
since
to4.20.1.1
and4.21.0.1
@CodeBleu
There is no 4.21.x release.
4.22 is planned to be released in Oct/Nov, you do not need to wait long time
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.
@weizhouapache Yeh, I saw the tag for 4.21.0.0
, but no branch. The issue for me personally, is we don't immediately jump to the latest released and it would make it in to our environment sooner if it were included in a patch release of 4.20
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11568 +/- ##
============================================
- Coverage 17.36% 17.35% -0.01%
Complexity 15237 15237
============================================
Files 5888 5888
Lines 525741 525756 +15
Branches 64164 64168 +4
============================================
- Hits 91274 91271 -3
- Misses 424167 424180 +13
- Partials 10300 10305 +5
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:
|
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.
code lgtm
@blueorangutan package |
@weizhouapache 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 ✔️ debian ✖️ suse15. SL-JID 14834 |
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.
code lgtm.
engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java
Outdated
Show resolved
Hide resolved
@blueorangutan package |
@Pearl1594 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. |
@Pearl1594 @weizhouapache Can I get one of you to manually test this and post results? Once that is complete, I believe I can merge it 😄 |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 14835 |
@blueorangutan test |
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
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.
code LGTM
[SF] Trillian test result (tid-14205)
|
@harikrishna-patnala Would you be able to test this and post results? I believe I'm just needed that from someone and then I can merge. Unless @weizhouapache the following Trillian test counts?
|
Switching base to
main
from4.19
, SEE #10968 for Comment historyDescription
This PR will allow the updating of a loadbalancer rules
CIDR list
via the API.* Should fix #9313
I have tested this code in
4.19
,4.20
, andmain
branches via simulator and all worksTypes of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
Before, the Source CIDR list was blank (Simulator env)

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
With correct source CIDR or 0.0.0.0/0
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