From df35ef73ef99db3b049d732809d2f9619fd1b92d Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Tue, 29 Apr 2025 11:46:47 -0400 Subject: [PATCH 1/8] Allow updating of Load Balancer source CIDR list * Should fix https://github.com/apache/cloudstack/issues/9313 --- .../UpdateLoadBalancerRuleCmd.java | 7 ++++++ .../com/cloud/network/dao/LoadBalancerVO.java | 4 ++++ .../lb/LoadBalancingRulesManagerImpl.java | 23 +++++++++++++++---- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java index 25254ba9eb75..7b13dde1d357 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java @@ -33,6 +33,7 @@ import com.cloud.network.rules.FirewallRule; import com.cloud.network.rules.LoadBalancer; import com.cloud.user.Account; +import java.util.List; @APICommand(name = "updateLoadBalancerRule", description = "Updates load balancer", responseObject = LoadBalancerResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) @@ -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") + private List cidrList; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -92,6 +96,9 @@ public String getLbProtocol() { return lbProtocol; } + public List getCidrList() { + return cidrList; + } ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java b/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java index ad0338b98497..5ef98da2d86a 100644 --- a/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java +++ b/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java @@ -144,4 +144,8 @@ public String toString() { ReflectionToStringBuilderUtils.reflectOnlySelectedFields( this, "id", "uuid", "name", "purpose", "state")); } + + public void setCidrList(String cidrList) { + this.cidrList = cidrList; + } } diff --git a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index f786626ee317..69aafa3e442c 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -2257,6 +2257,17 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { } validateInputsForExternalNetworkProvider(lb, algorithm, lbProtocol); + + List cidrList = cmd.getCidrList(); + + if (cidrList != null) { + String cidrListStr = StringUtils.join(cidrList, ","); + if (!cidrListStr.isEmpty() && !NetUtils.isValidCidrList(cidrListStr)) { + throw new InvalidParameterValueException("Invalid CIDR list: " + cidrListStr); + } + lb.setCidrList(cidrListStr); + } + // Validate rule in LB provider LoadBalancingRule rule = getLoadBalancerRuleToApply(lb); if (!validateLbRule(rule)) { @@ -2266,10 +2277,12 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { LoadBalancerVO tmplbVo = _lbDao.findById(lbRuleId); boolean success = _lbDao.update(lbRuleId, lb); - // If algorithm or lb protocol is changed, have to reapply the lb config - boolean needToReApplyRule = (algorithm != null && !algorithm.equals(tmplbVo.getAlgorithm())) - || (lbProtocol != null && !lbProtocol.equals(tmplbVo.getLbProtocol())); - if (needToReApplyRule) { + // Check if algorithm, lb protocol, or cidrlist has changed, and reapply the lb config if needed + boolean algorithmChanged = !Objects.equals(algorithm, tmplbVo.getAlgorithm()); + boolean protocolChanged = !Objects.equals(lbProtocol, tmplbVo.getLbProtocol()); + boolean cidrListChanged = !Objects.equals(tmplbVo.getCidrList(), lb.getCidrList()); + + if (algorithmChanged || protocolChanged || cidrListChanged) { try { lb.setState(FirewallRule.State.Add); _lbDao.persist(lb); @@ -2296,6 +2309,8 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { if (lbBackup.getLbProtocol() != null) { lb.setLbProtocol(lbBackup.getLbProtocol()); } + + lb.setCidrList(lbBackup.getCidrList()); lb.setState(lbBackup.getState()); _lbDao.update(lb.getId(), lb); _lbDao.persist(lb); From 1a92bf3ffcb309a0d1d2eb8e98522de022a7b39a Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:01:10 -0400 Subject: [PATCH 2/8] Added test coverage for LoadBalancerVO --- .../cloud/network/dao/LoadBalancerVOTest.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java diff --git a/engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java b/engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java new file mode 100644 index 000000000000..ef348af4c782 --- /dev/null +++ b/engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java @@ -0,0 +1,29 @@ +package com.cloud.network.dao; + +import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +public class LoadBalancerVOTest { + @Test + public void testSetCidrList() { + LoadBalancerVO loadBalancer = new LoadBalancerVO(); + String cidrList = "192.168.1.0/24,10.0.0.0/16"; + loadBalancer.setCidrList(cidrList); + assertEquals(cidrList, loadBalancer.getCidrList()); + } + + @Test + public void testSetCidrListEmpty() { + LoadBalancerVO loadBalancer = new LoadBalancerVO(); + loadBalancer.setCidrList(""); + assertEquals("", loadBalancer.getCidrList()); + } + + @Test + public void testSetCidrListNull() { + LoadBalancerVO loadBalancer = new LoadBalancerVO(); + loadBalancer.setCidrList(null); + assertNull(loadBalancer.getCidrList()); + } +} From dd590445572c88c20c403a49e90dfb24be84b77f Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:13:35 -0400 Subject: [PATCH 3/8] Add License text to LoadBalancerVOTest.java file --- .../cloud/network/dao/LoadBalancerVOTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java b/engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java index ef348af4c782..8f84ecac3dda 100644 --- a/engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java +++ b/engine/schema/src/test/java/com/cloud/network/dao/LoadBalancerVOTest.java @@ -1,3 +1,19 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. package com.cloud.network.dao; import org.junit.Test; From 3b342dab5dcd37900210516b20d7a6932805d2be Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Fri, 29 Aug 2025 10:10:50 -0400 Subject: [PATCH 4/8] Refactor LB CIDR list comparison & rollback code - 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 --- .../src/main/java/com/cloud/network/dao/LoadBalancerVO.java | 5 +++++ .../com/cloud/network/lb/LoadBalancingRulesManagerImpl.java | 3 --- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java b/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java index 5ef98da2d86a..5a4ded78a255 100644 --- a/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java +++ b/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java @@ -145,6 +145,11 @@ public String toString() { this, "id", "uuid", "name", "purpose", "state")); } + /** + * Sets the CIDR list associated with this load balancer rule. + * + * @param cidrList a comma-separated list of CIDR strings, e.g. "1.2.3.4/24,1.2.3.5/24" or and empty string e.g. "" to clear the restrictions + */ public void setCidrList(String cidrList) { this.cidrList = cidrList; } diff --git a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index 69aafa3e442c..d8ea9b550716 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -2306,9 +2306,6 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { if (lbBackup.getAlgorithm() != null) { lb.setAlgorithm(lbBackup.getAlgorithm()); } - if (lbBackup.getLbProtocol() != null) { - lb.setLbProtocol(lbBackup.getLbProtocol()); - } lb.setCidrList(lbBackup.getCidrList()); lb.setState(lbBackup.getState()); From e359ed868be724f6088b868cd64c64f92f46b926 Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Fri, 29 Aug 2025 10:33:50 -0400 Subject: [PATCH 5/8] Added missing Import for java.util.Objects --- .../java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index d8ea9b550716..e39aaf6752bf 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Objects; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; From b901cc9f8400fbd961ab7192d6c2fb2a332faaf0 Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Wed, 3 Sep 2025 09:37:39 -0400 Subject: [PATCH 6/8] Removed duplicate import --- .../java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index e39aaf6752bf..d8ea9b550716 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -18,7 +18,6 @@ import java.util.ArrayList; import java.util.Arrays; -import java.util.Objects; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; From 316036b0814b1160e9d3ae7b1572936c80e8b452 Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Wed, 3 Sep 2025 10:21:03 -0400 Subject: [PATCH 7/8] Added `since` to api parameter --- .../command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java index 7b13dde1d357..8593cad82c65 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UpdateLoadBalancerRuleCmd.java @@ -65,7 +65,7 @@ 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") + @Parameter(name = ApiConstants.CIDR_LIST, type = CommandType.LIST, collectionType = CommandType.STRING, description = "the cidr list to forward traffic from", since = "4.22") private List cidrList; ///////////////////////////////////////////////////// From 1d5a7f62228f460664b7680654e6ac6f5251d303 Mon Sep 17 00:00:00 2001 From: Jason Hollis <400979+CodeBleu@users.noreply.github.com> Date: Wed, 3 Sep 2025 12:30:39 -0400 Subject: [PATCH 8/8] Fix typo --- .../src/main/java/com/cloud/network/dao/LoadBalancerVO.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java b/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java index 5a4ded78a255..3886529322e3 100644 --- a/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java +++ b/engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java @@ -148,7 +148,7 @@ public String toString() { /** * Sets the CIDR list associated with this load balancer rule. * - * @param cidrList a comma-separated list of CIDR strings, e.g. "1.2.3.4/24,1.2.3.5/24" or and empty string e.g. "" to clear the restrictions + * @param cidrList a comma-separated list of CIDR strings, e.g. "1.2.3.4/24,1.2.3.5/24" or an empty string e.g. "" to clear the restrictions */ public void setCidrList(String cidrList) { this.cidrList = cidrList;