From fe1039034ffecba73b7d4e49cf0fd43dc7ecc7a4 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 14:35:42 +0000 Subject: [PATCH 01/10] Use a keepalive option HAProxy still accepts With keepalive enabled, an LB rule that also uses HTTP stickiness or SSL offload emitted "no option forceclose". HAProxy has rejected that keyword since 2.0, and the system VM has shipped 2.x for several releases: [ALERT] config : parsing [haproxy.cfg:22]: option 'forceclose' is not supported any more since HAProxy 2.0, please just remove it, or use 'option httpclose' if absolutely needed. It is a fatal parse error, so haproxy keeps running the previous config and the rule silently never takes effect. Replaced with "option http-keep-alive", which says the same thing and is valid on every version the system VM has shipped. Signed-off-by: Brad House --- .../java/com/cloud/network/HAProxyConfigurator.java | 2 +- .../com/cloud/network/HAProxyConfiguratorTest.java | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java index b3f7da1c6d82..2f5f5fb9d729 100644 --- a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java +++ b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java @@ -573,7 +573,7 @@ private List getRulesForPool(final LoadBalancerTO lbTO, final LoadBalanc boolean http = (publicPort == NetUtils.HTTP_PORT && !keepAliveEnabled); if (http || httpbasedStickiness || sslOffloading) { frontendConfigs.add("\tmode http"); - String keepAliveLine = keepAliveEnabled ? "\tno option forceclose" : "\toption httpclose"; + String keepAliveLine = keepAliveEnabled ? "\toption http-keep-alive" : "\toption httpclose"; frontendConfigs.add(keepAliveLine); } diff --git a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java index 073f976719b5..211e85d1fe2d 100644 --- a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java +++ b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java @@ -158,6 +158,19 @@ public void generateConfigurationTestWithSslCert() { Assert.assertTrue(result.contains("bind 10.2.0.1:443 ssl crt /etc/cloudstack/ssl/10_2_0_1-443.pem")); } + @Test + public void generateConfigurationTestKeepAliveWithSslOffloading() { + LoadBalancerTO lb = new LoadBalancerTO("1", "10.2.0.1", 443, "ssl", "roundrobin", false, false, false, null); + lb.setLbSslCert(new LbSslCert("cert", "key", "password", "chain", "fingerprint", false)); + LoadBalancerTO[] lba = new LoadBalancerTO[1]; + lba[0] = lb; + HAProxyConfigurator hpg = new HAProxyConfigurator(); + LoadBalancerConfigCommand cmd = new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", true, 0L); + String result = genConfig(hpg, cmd); + Assert.assertFalse("'forceclose' is rejected by HAProxy 2.0 and later", result.contains("forceclose")); + Assert.assertTrue("keepalive should be requested explicitly", result.contains("\toption http-keep-alive")); + } + private String genConfig(HAProxyConfigurator hpg, LoadBalancerConfigCommand cmd) { String[] sa = hpg.generateConfiguration(cmd); StringBuilder sb = new StringBuilder(); From a68d33e1e1a70b2587ae1a76ebf722928f928ebb Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 14:35:42 +0000 Subject: [PATCH 02/10] Stop one load balancer's config leaking into the next The global and defaults sections are static arrays. Wrapping them in Arrays.asList() and calling set() writes through to the array, so a value from one LB rule stays there for every config generated afterwards: - keepalive on any rule leaves "no option httpclose" in the defaults for every later rule, in any network, in any account - an idle timeout from one rule becomes the default for the next - idleTimeout 0 blanks the timeouts permanently Copy the arrays instead. Same output, no shared state. Signed-off-by: Brad House --- .../cloud/network/HAProxyConfigurator.java | 29 +++++++++---------- .../network/HAProxyConfiguratorTest.java | 28 ++++++++++++++++++ 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java index 2f5f5fb9d729..230db113bbe8 100644 --- a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java +++ b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java @@ -20,7 +20,6 @@ package com.cloud.network; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -46,13 +45,15 @@ public class HAProxyConfigurator implements LoadBalancerConfigurator { protected Logger logger = LogManager.getLogger(getClass()); private static final String blankLine = "\t "; - private static String[] globalSection = {"global", "\tlog 127.0.0.1:3914 local0 warning", "\tmaxconn 4096", "\tmaxpipes 1024", "\tchroot /var/lib/haproxy", - "\tuser haproxy", "\tgroup haproxy", "\tstats socket /run/haproxy/admin.sock", "\tdaemon"}; + // Immutable so a config cannot be built by writing into the shared copy. + private static final List globalSection = List.of("global", "\tlog 127.0.0.1:3914 local0 warning", "\tmaxconn 4096", "\tmaxpipes 1024", + "\tchroot /var/lib/haproxy", "\tuser haproxy", "\tgroup haproxy", "\tstats socket /run/haproxy/admin.sock", "\tdaemon"); - private static String[] defaultsSection = {"defaults", "\tlog global", "\tmode tcp", "\toption dontlognull", "\tretries 3", "\toption redispatch", - "\toption forwardfor", "\toption httpclose", "\ttimeout connect 5000", "\ttimeout client 50000", "\ttimeout server 50000"}; + private static final List defaultsSection = List.of("defaults", "\tlog global", "\tmode tcp", "\toption dontlognull", "\tretries 3", + "\toption redispatch", "\toption forwardfor", "\toption httpclose", "\ttimeout connect 5000", "\ttimeout client 50000", + "\ttimeout server 50000"); - private static String[] defaultListen = {"listen vmops", "\tbind 0.0.0.0:9", "\toption transparent"}; + private static final List defaultListen = List.of("listen vmops", "\tbind 0.0.0.0:9", "\toption transparent"); private static final String SSL_CERTS_DIR = "/etc/cloudstack/ssl/"; @@ -80,16 +81,16 @@ public String[] generateConfiguration(final List fwRules) final List result = new ArrayList(); - result.addAll(Arrays.asList(globalSection)); + result.addAll(globalSection); result.add(blankLine); - result.addAll(Arrays.asList(defaultsSection)); + result.addAll(defaultsSection); result.add(blankLine); if (pools.isEmpty()) { // haproxy cannot handle empty listen / frontend or backend, so add // a dummy listener // on port 9 - result.addAll(Arrays.asList(defaultListen)); + result.addAll(defaultListen); } result.add(blankLine); @@ -617,8 +618,7 @@ private String generateStatsRule(final LoadBalancerConfigCommand lbCmd, final St @Override public String[] generateConfiguration(final LoadBalancerConfigCommand lbCmd) { final List result = new ArrayList(); - final List gSection = Arrays.asList(globalSection); - // note that this is overwritten on the String in the static ArrayList + final List gSection = new ArrayList<>(globalSection); gSection.set(2, "\tmaxconn " + lbCmd.maxconn); // TODO DH: write test for this function final String pipesLine = "\tmaxpipes " + Long.toString(Long.parseLong(lbCmd.maxconn) / 4); @@ -631,7 +631,7 @@ public String[] generateConfiguration(final LoadBalancerConfigCommand lbCmd) { result.addAll(gSection); result.add(blankLine); - final List dSection = Arrays.asList(defaultsSection); + final List dSection = new ArrayList<>(defaultsSection); if (lbCmd.keepAliveEnabled) { dSection.set(7, "\tno option httpclose"); } @@ -639,8 +639,7 @@ public String[] generateConfiguration(final LoadBalancerConfigCommand lbCmd) { dSection.set(9, "\ttimeout client " + Long.toString(lbCmd.idleTimeout)); dSection.set(10, "\ttimeout server " + Long.toString(lbCmd.idleTimeout)); } else if (lbCmd.idleTimeout == 0) { - // .remove() is not allowed, only .set() operations are allowed as the list - // is a fixed size. So lets just mark the entry as blank. + // blank rather than removed, so the indexes above stay valid dSection.set(9, ""); dSection.set(10, ""); } else { @@ -695,7 +694,7 @@ public String[] generateConfiguration(final LoadBalancerConfigCommand lbCmd) { // haproxy cannot handle empty listen / frontend or backend, so add // a dummy listener // on port 9 - result.addAll(Arrays.asList(defaultListen)); + result.addAll(defaultListen); } return result.toArray(new String[result.size()]); } diff --git a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java index 211e85d1fe2d..2f65d536e410 100644 --- a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java +++ b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java @@ -171,6 +171,34 @@ public void generateConfigurationTestKeepAliveWithSslOffloading() { Assert.assertTrue("keepalive should be requested explicitly", result.contains("\toption http-keep-alive")); } + @Test + public void generateConfigurationTestKeepAliveDoesNotLeakToNextConfig() { + LoadBalancerTO lb = new LoadBalancerTO("1", "10.2.0.1", 80, "http", "bla", false, false, false, null); + LoadBalancerTO[] lba = new LoadBalancerTO[1]; + lba[0] = lb; + HAProxyConfigurator hpg = new HAProxyConfigurator(); + + genConfig(hpg, new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", true, 0L)); + + String result = genConfig(hpg, new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", false, 0L)); + Assert.assertFalse("keepalive from an earlier config should not survive into this one", + result.contains("\tno option httpclose")); + } + + @Test + public void generateConfigurationTestIdleTimeoutDoesNotLeakToNextConfig() { + LoadBalancerTO lb = new LoadBalancerTO("1", "10.2.0.1", 80, "http", "bla", false, false, false, null); + LoadBalancerTO[] lba = new LoadBalancerTO[1]; + lba[0] = lb; + HAProxyConfigurator hpg = new HAProxyConfigurator(); + + genConfig(hpg, new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", false, 1234L)); + + String result = genConfig(hpg, new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", false, -1L)); + assertTrue("an unset idle timeout should fall back to the default", result.contains("\ttimeout client 50000")); + assertTrue("an unset idle timeout should fall back to the default", result.contains("\ttimeout server 50000")); + } + private String genConfig(HAProxyConfigurator hpg, LoadBalancerConfigCommand cmd) { String[] sa = hpg.generateConfiguration(cmd); StringBuilder sb = new StringBuilder(); From e0521a31d231176510702e5554cb0c5e92fa9bbe Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 16:03:11 +0000 Subject: [PATCH 03/10] Replace appsession, removed from HAProxy in 1.6 An LB rule with AppCookie stickiness emitted "appsession", which haproxy has refused since 1.6: [ALERT] config : 'appsession' is not supported anymore since HAProxy 1.6. [ALERT] config : Fatal errors found in configuration. The whole file is rejected, so every rule on that router keeps serving its previous config, not just the one with the policy. The VR advertises AppCookie as supported, so this is reachable from the API. A stick table on the cookie is the documented replacement: stick-table type string len size 10k expire stick store-response res.cook() stick match req.cook() stick store-request req.cook() # only with request-learn The prefix and mode options have no equivalent and are now logged and ignored. They were never applied - the directive carrying them was rejected - so no working behaviour changes. Signed-off-by: Brad House --- .../cloud/network/HAProxyConfigurator.java | 16 +++++++------ .../network/HAProxyConfiguratorTest.java | 24 +++++++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java index 230db113bbe8..fbe2ce3eb371 100644 --- a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java +++ b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java @@ -452,15 +452,17 @@ private String getLbSubRuleForStickiness(final LoadBalancerTO lbTO) { tempSb.append("appcookie_").append(srcip.hashCode()).append("_").append(lbTO.getSrcPort()); cookieName = tempSb.toString(); } - sb.append("\t").append("appsession ").append(cookieName).append(" len ").append(length).append(" timeout ").append(holdtime).append(" "); - if (prefix) { - sb.append("prefix "); - } + // "appsession" was removed in haproxy 1.6 and is a fatal parse error on the + // versions the system VM ships. A stick table on the cookie is the replacement. + sb.append("\t").append("stick-table type string len ").append(length).append(" size 10k expire ").append(holdtime).append("\n"); + sb.append("\t").append("stick store-response res.cook(").append(cookieName).append(")").append("\n"); + sb.append("\t").append("stick match req.cook(").append(cookieName).append(")").append("\n"); if (requestlearn) { - sb.append("request-learn").append(" "); + sb.append("\t").append("stick store-request req.cook(").append(cookieName).append(")").append("\n"); } - if (mode != null) { - sb.append("mode ").append(mode).append(" "); + if (prefix || mode != null) { + logger.warn("Haproxy stickiness policy for lb rule: {}:{}: prefix and mode are not supported since haproxy 1.6 and are ignored", + lbTO.getSrcIp(), lbTO.getSrcPort()); } } else { /* diff --git a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java index 2f65d536e410..ea5d0c6f31de 100644 --- a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java +++ b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java @@ -31,6 +31,9 @@ import com.cloud.agent.api.routing.LoadBalancerConfigCommand; import com.cloud.agent.api.to.LoadBalancerTO; import com.cloud.network.lb.LoadBalancingRule.LbDestination; +import com.cloud.utils.Pair; +import com.cloud.network.rules.LbStickinessMethod.StickinessMethodType; +import com.cloud.network.lb.LoadBalancingRule.LbStickinessPolicy; import com.cloud.network.lb.LoadBalancingRule.LbSslCert; import java.util.List; @@ -199,6 +202,27 @@ public void generateConfigurationTestIdleTimeoutDoesNotLeakToNextConfig() { assertTrue("an unset idle timeout should fall back to the default", result.contains("\ttimeout server 50000")); } + @Test + public void generateConfigurationTestAppCookieStickinessUsesAStickTable() { + List> params = new ArrayList<>(); + params.add(new Pair<>("cookie-name", "JSESSIONID")); + params.add(new Pair<>("length", "52")); + params.add(new Pair<>("holdtime", "3h")); + List policies = new ArrayList<>(); + policies.add(new LbStickinessPolicy(StickinessMethodType.AppCookieBased.getName(), params)); + List dests = new ArrayList<>(); + dests.add(new LbDestination(80, 80, "10.1.10.2", false)); + LoadBalancerTO lb = new LoadBalancerTO("1", "10.2.0.1", 80, "http", "roundrobin", false, false, false, dests, policies); + LoadBalancerTO[] lba = new LoadBalancerTO[1]; + lba[0] = lb; + String result = genConfig(new HAProxyConfigurator(), + new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", false, 50000L)); + Assert.assertFalse("appsession was removed in haproxy 1.6", result.contains("appsession")); + assertTrue(result.contains("stick-table type string len 52 size 10k expire 3h")); + assertTrue(result.contains("stick store-response res.cook(JSESSIONID)")); + assertTrue(result.contains("stick match req.cook(JSESSIONID)")); + } + private String genConfig(HAProxyConfigurator hpg, LoadBalancerConfigCommand cmd) { String[] sa = hpg.generateConfiguration(cmd); StringBuilder sb = new StringBuilder(); From 08327da074d4264886bf917c94babcbff5ed6258 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 16:03:11 +0000 Subject: [PATCH 04/10] Make keepalive and timeouts settable per load balancer rule #12586 made the idle timeout global. Keepalive is set on the network offering, at create time only, with no UI. Both land in the haproxy defaults section, so every rule on a router shares them. Three optional parameters on createLoadBalancerRule and updateLoadBalancerRule, each written to that rule's own listen section: | parameter | haproxy directive | unset | | keepalive | option http-keep-alive / httpclose | offering | | idletimeout | timeout client, timeout server | global | | keepalivetimeout | timeout http-keep-alive | idletimeout| Held as firewall rule details, so no schema change. A rule on port 80 can now have keepalive and X-Forwarded-For together, which no combination of the existing settings can produce: offering flag off mode http closes per response XFF works offering flag on mode tcp reuses connections XFF lost per rule mode http reuses connections XFF works A rule that sets nothing behaves exactly as before, including the fall back to tcp mode when the offering flag is on. Negative timeouts are rejected at the API and dropped in the generator. Haproxy treats one as a fatal parse error, which would strand every rule on the router. Applies to public LB rules. Application load balancers are created through a different command and are not covered. Signed-off-by: Brad House --- .../cloud/agent/api/to/LoadBalancerTO.java | 27 +++++ .../network/lb/LoadBalancingRulesService.java | 7 ++ .../com/cloud/network/rules/LoadBalancer.java | 5 + .../apache/cloudstack/api/ApiConstants.java | 3 + .../CreateLoadBalancerRuleCmd.java | 29 +++++ .../UpdateLoadBalancerRuleCmd.java | 28 +++++ .../api/response/LoadBalancerResponse.java | 24 ++++ .../cloud/network/HAProxyConfigurator.java | 40 ++++++- .../network/HAProxyConfiguratorTest.java | 112 +++++++++++++++++- .../java/com/cloud/api/ApiResponseHelper.java | 21 ++++ .../lb/LoadBalancingRulesManagerImpl.java | 45 ++++++- .../network/router/CommandSetupHelper.java | 29 +++++ .../lb/LoadBalancingRulesManagerImplTest.java | 12 ++ ui/public/locales/en.json | 7 ++ ui/src/views/network/LoadBalancing.vue | 74 +++++++++++- 15 files changed, 449 insertions(+), 14 deletions(-) diff --git a/api/src/main/java/com/cloud/agent/api/to/LoadBalancerTO.java b/api/src/main/java/com/cloud/agent/api/to/LoadBalancerTO.java index 6c4b9e607c51..f9ab5d2d2e6f 100644 --- a/api/src/main/java/com/cloud/agent/api/to/LoadBalancerTO.java +++ b/api/src/main/java/com/cloud/agent/api/to/LoadBalancerTO.java @@ -54,6 +54,9 @@ public class LoadBalancerTO { final static int MAX_HEALTHCHECK_POLICIES = 1; private String cidrList; + private Boolean keepAlive; + private Long idleTimeout; + private Long keepAliveTimeout; public LoadBalancerTO(String uuid, String srcIp, int srcPort, String protocol, String algorithm, boolean revoked, boolean alreadyAdded, boolean inline, List destinations) { @@ -249,6 +252,30 @@ public String getCidrList() { return cidrList; } + public void setKeepAlive(Boolean keepAlive) { + this.keepAlive = keepAlive; + } + + public Boolean getKeepAlive() { + return keepAlive; + } + + public void setIdleTimeout(Long idleTimeout) { + this.idleTimeout = idleTimeout; + } + + public Long getIdleTimeout() { + return idleTimeout; + } + + public void setKeepAliveTimeout(Long keepAliveTimeout) { + this.keepAliveTimeout = keepAliveTimeout; + } + + public Long getKeepAliveTimeout() { + return keepAliveTimeout; + } + public static class StickinessPolicyTO { private String methodName; private List> params; diff --git a/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java b/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java index b7fe3b26761c..0abe20e274cd 100644 --- a/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java +++ b/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java @@ -70,6 +70,13 @@ LoadBalancer createPublicLoadBalancerRule(String xId, String name, String descri LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd); + /** + * Stores the optional per rule haproxy settings. A null leaves the current value alone. + * + * @return true if any value changed + */ + boolean updateLoadBalancerConnectionSettings(long lbRuleId, Boolean keepAlive, Long idleTimeout, Long keepAliveTimeout); + boolean deleteLoadBalancerRule(long lbRuleId, boolean apply); /** diff --git a/api/src/main/java/com/cloud/network/rules/LoadBalancer.java b/api/src/main/java/com/cloud/network/rules/LoadBalancer.java index 9b4e991b5b5a..5c64e018acbe 100644 --- a/api/src/main/java/com/cloud/network/rules/LoadBalancer.java +++ b/api/src/main/java/com/cloud/network/rules/LoadBalancer.java @@ -21,6 +21,11 @@ */ public interface LoadBalancer extends FirewallRule, LoadBalancerContainer { + // Optional per rule haproxy settings, held as firewall rule details. Absent means inherit. + String KEEPALIVE = "keepalive"; + String IDLE_TIMEOUT = "idletimeout"; + String KEEPALIVE_TIMEOUT = "keepalivetimeout"; + int getDefaultPortStart(); int getDefaultPortEnd(); diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index f74c46161180..f58df6c200a1 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -357,6 +357,7 @@ public class ApiConstants { public static final String IS_ISO = "isiso"; public static final String IS_PORTABLE = "isportable"; public static final String IS_PUBLIC = "ispublic"; + public static final String IDLE_TIMEOUT = "idletimeout"; public static final String IS_PERSISTENT = "ispersistent"; public static final String EGRESS_DEFAULT_POLICY = "egressdefaultpolicy"; public static final String IS_READY = "isready"; @@ -368,6 +369,8 @@ public class ApiConstants { public static final String JAVA_VERSION = "javaversion"; public static final String JOB_ID = "jobid"; public static final String JOB_STATUS = "jobstatus"; + public static final String KEEPALIVE = "keepalive"; + public static final String KEEPALIVE_TIMEOUT = "keepalivetimeout"; public static final String KEEPALIVE_ENABLED = "keepaliveenabled"; public static final String KERNEL_VERSION = "kernelversion"; public static final String KEYPAIR_ID = "keypairid"; diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java index bd72f248364e..b6cb8cc9ca70 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java @@ -119,6 +119,22 @@ public class CreateLoadBalancerRuleCmd extends BaseAsyncCreateCmd /*implements L @Parameter(name = ApiConstants.FOR_DISPLAY, type = CommandType.BOOLEAN, description = "An optional field, whether to the display the rule to the end user or not", since = "4.4", authorized = {RoleType.Admin}) private Boolean display; + @Parameter(name = ApiConstants.KEEPALIVE, type = CommandType.BOOLEAN, since = "4.23.0", + description = "Whether the load balancer keeps client connections open between requests. " + + "Only applies to rules the router serves in HTTP mode. If not set, the network offering's setting is used.") + private Boolean keepAlive; + + @Parameter(name = ApiConstants.IDLE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + description = "How long an idle connection is held open, in milliseconds. Use 0 for infinite. " + + "If not set, the global setting network.loadbalancer.haproxy.idle.timeout is used.") + private Long idleTimeout; + + @Parameter(name = ApiConstants.KEEPALIVE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + description = "How long an idle keepalive connection is held open waiting for the next request, " + + "in milliseconds. Only applies to rules the router serves in HTTP mode. " + + "If not set, idletimeout applies.") + private Long keepAliveTimeout; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -135,6 +151,18 @@ public boolean isDisplay() { public String getAlgorithm() { return algorithm; } + public Boolean getKeepAlive() { + return keepAlive; + } + + public Long getIdleTimeout() { + return idleTimeout; + } + public Long getKeepAliveTimeout() { + return keepAliveTimeout; + } + + public String getDescription() { return description; @@ -307,6 +335,7 @@ public void create() { getCidrList()); this.setEntityId(result.getId()); this.setEntityUuid(result.getUuid()); + _lbService.updateLoadBalancerConnectionSettings(result.getId(), getKeepAlive(), getIdleTimeout(), getKeepAliveTimeout()); } catch (NetworkRuleConflictException e) { logger.warn("Exception: ", e); throw new ServerApiException(ApiErrorCode.NETWORK_RULE_CONFLICT_ERROR, e.getMessage()); 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 0ac99f1c760c..a66a2d22a548 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 @@ -68,6 +68,22 @@ public class UpdateLoadBalancerRuleCmd extends BaseAsyncCustomIdCmd { @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; + @Parameter(name = ApiConstants.KEEPALIVE, type = CommandType.BOOLEAN, since = "4.23.0", + description = "Whether the load balancer keeps client connections open between requests. " + + "Only applies to rules the router serves in HTTP mode. If not set, the network offering's setting is used.") + private Boolean keepAlive; + + @Parameter(name = ApiConstants.IDLE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + description = "How long an idle connection is held open, in milliseconds. Use 0 for infinite. " + + "If not set, the global setting network.loadbalancer.haproxy.idle.timeout is used.") + private Long idleTimeout; + + @Parameter(name = ApiConstants.KEEPALIVE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + description = "How long an idle keepalive connection is held open waiting for the next request, " + + "in milliseconds. Only applies to rules the router serves in HTTP mode. " + + "If not set, idletimeout applies.") + private Long keepAliveTimeout; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -75,6 +91,18 @@ public class UpdateLoadBalancerRuleCmd extends BaseAsyncCustomIdCmd { public String getAlgorithm() { return algorithm; } + public Boolean getKeepAlive() { + return keepAlive; + } + + public Long getIdleTimeout() { + return idleTimeout; + } + public Long getKeepAliveTimeout() { + return keepAliveTimeout; + } + + public String getDescription() { return description; diff --git a/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java index c57323d439dd..fd25c4e0042c 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java @@ -63,6 +63,18 @@ public class LoadBalancerResponse extends BaseResponse implements ControlledEnti @Param(description = "The ID of the guest Network the LB rule belongs to") private String networkId; + @SerializedName(ApiConstants.KEEPALIVE) + @Param(description = "whether the load balancer keeps client connections open between requests, unset means the network offering's setting is used", since = "4.23.0") + private Boolean keepAlive; + + @SerializedName(ApiConstants.IDLE_TIMEOUT) + @Param(description = "how long an idle connection is held open, in milliseconds, unset means the global setting is used", since = "4.23.0") + private Long idleTimeout; + + @SerializedName(ApiConstants.KEEPALIVE_TIMEOUT) + @Param(description = "how long an idle keepalive connection is held open waiting for the next request, in milliseconds", since = "4.23.0") + private Long keepAliveTimeout; + @SerializedName(ApiConstants.CIDR_LIST) @Param(description = "The CIDR list to allow traffic, all other CIDRs will be blocked. Multiple entries must be separated by a single comma character (,).") private String cidrList; @@ -143,6 +155,18 @@ public void setCidrList(String cidrs) { this.cidrList = cidrs; } + public void setKeepAlive(Boolean keepAlive) { + this.keepAlive = keepAlive; + } + + public void setIdleTimeout(Long idleTimeout) { + this.idleTimeout = idleTimeout; + } + + public void setKeepAliveTimeout(Long keepAliveTimeout) { + this.keepAliveTimeout = keepAliveTimeout; + } + public void setAlgorithm(String algorithm) { this.algorithm = algorithm; } diff --git a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java index fbe2ce3eb371..3be197953b26 100644 --- a/core/src/main/java/com/cloud/network/HAProxyConfigurator.java +++ b/core/src/main/java/com/cloud/network/HAProxyConfigurator.java @@ -480,6 +480,18 @@ private String getLbSubRuleForStickiness(final LoadBalancerTO lbTO) { return sb.toString(); } + /** + * Haproxy rejects a negative timeout, and one bad value costs the whole file. Drop it and keep + * whatever the defaults section says, the same way the global idle timeout does. + */ + private Long timeoutOrNull(final LoadBalancerTO lbTO, final String name, final Long value) { + if (value != null && value < 0) { + logger.warn("Ignoring negative {} [{}] on lb rule {}:{}", name, value, lbTO.getSrcIp(), lbTO.getSrcPort()); + return null; + } + return value; + } + private List getRulesForPool(final LoadBalancerTO lbTO, final LoadBalancerConfigCommand lbCmd) { StringBuilder sb = new StringBuilder(); final String poolName = sb.append(lbTO.getSrcIp().replace(".", "_")).append('-').append(lbTO.getSrcPort()).toString(); @@ -572,12 +584,30 @@ private List getRulesForPool(final LoadBalancerTO lbTO, final LoadBalanc if (stickinessSubRule != null && !destsAvailable) { logger.warn("Haproxy stickiness policy for lb rule: " + lbTO.getSrcIp() + ":" + lbTO.getSrcPort() + ": Not Applied, cause: backends are unavailable"); } - boolean keepAliveEnabled = lbCmd.keepAliveEnabled; - boolean http = (publicPort == NetUtils.HTTP_PORT && !keepAliveEnabled); - if (http || httpbasedStickiness || sslOffloading) { + final Boolean ruleKeepAlive = lbTO.getKeepAlive(); + final Long ruleIdleTimeout = timeoutOrNull(lbTO, "idletimeout", lbTO.getIdleTimeout()); + final Long ruleKeepAliveTimeout = timeoutOrNull(lbTO, "keepalivetimeout", lbTO.getKeepAliveTimeout()); + final boolean keepAliveEnabled = ruleKeepAlive != null ? ruleKeepAlive : lbCmd.keepAliveEnabled; + // A rule that asks for keepalive itself stays in http mode on port 80, so forwardfor keeps + // working. Without it, keepalive falls back to tcp mode as it always has. + final boolean port80HttpMode = publicPort == NetUtils.HTTP_PORT && (ruleKeepAlive != null || !keepAliveEnabled); + final boolean httpMode = port80HttpMode || httpbasedStickiness || sslOffloading; + if (httpMode) { frontendConfigs.add("\tmode http"); - String keepAliveLine = keepAliveEnabled ? "\toption http-keep-alive" : "\toption httpclose"; - frontendConfigs.add(keepAliveLine); + frontendConfigs.add(keepAliveEnabled ? "\toption http-keep-alive" : "\toption httpclose"); + if (keepAliveEnabled && ruleKeepAliveTimeout != null) { + frontendConfigs.add("\ttimeout http-keep-alive " + ruleKeepAliveTimeout); + } else if (ruleKeepAliveTimeout != null) { + logger.warn("Keepalive timeout ignored for lb rule {}:{}, keepalive is off for this rule", + lbTO.getSrcIp(), lbTO.getSrcPort()); + } + } else if (ruleKeepAlive != null || ruleKeepAliveTimeout != null) { + logger.warn("Keepalive ignored for lb rule {}:{}, it is served in tcp mode. Keepalive applies on port {}, " + + "with ssl offload, or with http based stickiness.", lbTO.getSrcIp(), lbTO.getSrcPort(), NetUtils.HTTP_PORT); + } + if (ruleIdleTimeout != null) { + frontendConfigs.add("\ttimeout client " + ruleIdleTimeout); + frontendConfigs.add("\ttimeout server " + ruleIdleTimeout); } // add line like this: "listen 65_37_141_30-80\n\tbind 65.37.141.30:80" diff --git a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java index ea5d0c6f31de..3bbbe20b357b 100644 --- a/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java +++ b/core/src/test/java/com/cloud/network/HAProxyConfiguratorTest.java @@ -31,9 +31,9 @@ import com.cloud.agent.api.routing.LoadBalancerConfigCommand; import com.cloud.agent.api.to.LoadBalancerTO; import com.cloud.network.lb.LoadBalancingRule.LbDestination; -import com.cloud.utils.Pair; -import com.cloud.network.rules.LbStickinessMethod.StickinessMethodType; import com.cloud.network.lb.LoadBalancingRule.LbStickinessPolicy; +import com.cloud.network.rules.LbStickinessMethod.StickinessMethodType; +import com.cloud.utils.Pair; import com.cloud.network.lb.LoadBalancingRule.LbSslCert; import java.util.List; @@ -202,6 +202,114 @@ public void generateConfigurationTestIdleTimeoutDoesNotLeakToNextConfig() { assertTrue("an unset idle timeout should fall back to the default", result.contains("\ttimeout server 50000")); } + private LoadBalancerConfigCommand cmdFor(LoadBalancerTO lb, boolean offeringKeepAlive) { + LoadBalancerTO[] lba = new LoadBalancerTO[1]; + lba[0] = lb; + return new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", offeringKeepAlive, 50000L); + } + + private LoadBalancerTO httpRule() { + return new LoadBalancerTO("1", "10.2.0.1", 80, "http", "roundrobin", false, false, false, null); + } + + /** Just the "listen" block for this rule, so assertions cannot match the defaults or stats sections. */ + private String poolSection(String config, String poolName) { + int start = config.indexOf("listen " + poolName + "\n"); + Assert.assertTrue("no listen block for " + poolName, start >= 0); + int next = config.indexOf("\nlisten ", start + 1); + return next < 0 ? config.substring(start) : config.substring(start, next); + } + + @Test + public void generateConfigurationTestPerRuleKeepAliveKeepsHttpMode() { + LoadBalancerTO lb = httpRule(); + lb.setKeepAlive(true); + String pool = poolSection(genConfig(new HAProxyConfigurator(), cmdFor(lb, false)), "10_2_0_1-80"); + assertTrue("the rule should stay in http mode so forwardfor still works", pool.contains("\tmode http")); + assertTrue(pool.contains("\toption http-keep-alive")); + Assert.assertFalse(pool.contains("\toption httpclose")); + } + + @Test + public void generateConfigurationTestPerRuleKeepAliveOverridesTheOffering() { + LoadBalancerTO lb = httpRule(); + lb.setKeepAlive(false); + String pool = poolSection(genConfig(new HAProxyConfigurator(), cmdFor(lb, true)), "10_2_0_1-80"); + assertTrue("the rule should win over the offering", pool.contains("\toption httpclose")); + Assert.assertFalse(pool.contains("\toption http-keep-alive")); + } + + @Test + public void generateConfigurationTestUnsetKeepAliveLeavesOldBehaviourAlone() { + String pool = poolSection(genConfig(new HAProxyConfigurator(), cmdFor(httpRule(), true)), "10_2_0_1-80"); + Assert.assertFalse("keepalive from the offering still drops to tcp mode", pool.contains("\tmode http")); + + pool = poolSection(genConfig(new HAProxyConfigurator(), cmdFor(httpRule(), false)), "10_2_0_1-80"); + assertTrue(pool.contains("\tmode http")); + assertTrue(pool.contains("\toption httpclose")); + } + + @Test + public void generateConfigurationTestKeepAliveTimeout() { + LoadBalancerTO lb = httpRule(); + lb.setKeepAlive(true); + lb.setKeepAliveTimeout(15000L); + String pool = poolSection(genConfig(new HAProxyConfigurator(), cmdFor(lb, false)), "10_2_0_1-80"); + assertTrue(pool.contains("\ttimeout http-keep-alive 15000")); + } + + @Test + public void generateConfigurationTestKeepAliveTimeoutNeedsKeepAlive() { + LoadBalancerTO lb = httpRule(); + lb.setKeepAliveTimeout(15000L); + String pool = poolSection(genConfig(new HAProxyConfigurator(), cmdFor(lb, false)), "10_2_0_1-80"); + Assert.assertFalse("without keepalive there is no idle connection to time out", pool.contains("timeout http-keep-alive")); + } + + @Test + public void generateConfigurationTestIdleTimeoutOverridesTheGlobalPerRule() { + LoadBalancerTO lb = new LoadBalancerTO("1", "10.2.0.1", 3306, "tcp", "roundrobin", false, false, false, null); + lb.setIdleTimeout(600000L); + String config = genConfig(new HAProxyConfigurator(), cmdFor(lb, false)); + String pool = poolSection(config, "10_2_0_1-3306"); + assertTrue("a tcp rule gets the timeouts too", pool.contains("\ttimeout client 600000")); + assertTrue(pool.contains("\ttimeout server 600000")); + assertTrue("the global still sets the defaults", config.contains("\ttimeout client 50000")); + } + + @Test + public void generateConfigurationTestNegativeTimeoutsAreDropped() { + LoadBalancerTO lb = httpRule(); + lb.setKeepAlive(true); + lb.setIdleTimeout(-1L); + lb.setKeepAliveTimeout(-5L); + String pool = poolSection(genConfig(new HAProxyConfigurator(), cmdFor(lb, false)), "10_2_0_1-80"); + Assert.assertFalse("a negative timeout is a fatal haproxy parse error", pool.contains("-1")); + Assert.assertFalse("a negative timeout is a fatal haproxy parse error", pool.contains("-5")); + Assert.assertFalse(pool.contains("timeout client")); + Assert.assertFalse(pool.contains("timeout http-keep-alive")); + } + + @Test + public void generateConfigurationTestPerRuleSettingsDoNotCrossContaminate() { + LoadBalancerTO tuned = httpRule(); + tuned.setKeepAlive(true); + tuned.setIdleTimeout(600000L); + tuned.setKeepAliveTimeout(15000L); + LoadBalancerTO plain = new LoadBalancerTO("2", "10.2.0.1", 8080, "tcp", "roundrobin", false, false, false, null); + LoadBalancerTO[] lba = new LoadBalancerTO[] {tuned, plain}; + LoadBalancerConfigCommand cmd = new LoadBalancerConfigCommand(lba, "10.0.0.1", "10.1.0.1", "10.1.1.1", null, 1L, "12", false, 50000L); + String config = genConfig(new HAProxyConfigurator(), cmd); + + String tunedPool = poolSection(config, "10_2_0_1-80"); + assertTrue(tunedPool.contains("\toption http-keep-alive")); + assertTrue(tunedPool.contains("\ttimeout client 600000")); + + String plainPool = poolSection(config, "10_2_0_1-8080"); + Assert.assertFalse("the other rule's settings must not leak here", plainPool.contains("http-keep-alive")); + Assert.assertFalse("the other rule's settings must not leak here", plainPool.contains("timeout client")); + } + @Test public void generateConfigurationTestAppCookieStickinessUsesAStickTable() { List> params = new ArrayList<>(); diff --git a/server/src/main/java/com/cloud/api/ApiResponseHelper.java b/server/src/main/java/com/cloud/api/ApiResponseHelper.java index f56cda6e557a..2eed2674bdd6 100644 --- a/server/src/main/java/com/cloud/api/ApiResponseHelper.java +++ b/server/src/main/java/com/cloud/api/ApiResponseHelper.java @@ -240,9 +240,12 @@ import org.apache.cloudstack.usage.UsageService; import org.apache.cloudstack.usage.UsageTypes; import org.apache.cloudstack.vm.UnmanagedInstanceTO; +import org.apache.cloudstack.resourcedetail.FirewallRuleDetailVO; +import org.apache.cloudstack.resourcedetail.dao.FirewallRuleDetailsDao; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.math.NumberUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -467,6 +470,8 @@ public class ApiResponseHelper implements ResponseGenerator, ResourceIdSupport { protected Logger logger = LogManager.getLogger(ApiResponseHelper.class); private static final DecimalFormat s_percentFormat = new DecimalFormat("##.##"); + @Inject + private FirewallRuleDetailsDao firewallRuleDetailsDao; @Inject private EntityManager _entityMgr; @Inject @@ -1305,6 +1310,21 @@ private void showVmInfoForSharedNetworks(boolean forVirtualNetworks, IpAddress i } } + private void setLbConnectionSettings(LoadBalancerResponse lbResponse, long lbRuleId) { + FirewallRuleDetailVO keepAlive = firewallRuleDetailsDao.findDetail(lbRuleId, LoadBalancer.KEEPALIVE); + if (keepAlive != null) { + lbResponse.setKeepAlive(Boolean.valueOf(keepAlive.getValue())); + } + FirewallRuleDetailVO idleTimeout = firewallRuleDetailsDao.findDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT); + if (idleTimeout != null) { + lbResponse.setIdleTimeout(NumberUtils.toLong(idleTimeout.getValue())); + } + FirewallRuleDetailVO keepAliveTimeout = firewallRuleDetailsDao.findDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT); + if (keepAliveTimeout != null) { + lbResponse.setKeepAliveTimeout(NumberUtils.toLong(keepAliveTimeout.getValue())); + } + } + @Override public LoadBalancerResponse createLoadBalancerResponse(LoadBalancer loadBalancer) { LoadBalancerResponse lbResponse = new LoadBalancerResponse(); @@ -1313,6 +1333,7 @@ public LoadBalancerResponse createLoadBalancerResponse(LoadBalancer loadBalancer lbResponse.setDescription(loadBalancer.getDescription()); List cidrs = ApiDBUtils.findFirewallSourceCidrs(loadBalancer.getId()); lbResponse.setCidrList(StringUtils.join(cidrs, ",")); + setLbConnectionSettings(lbResponse, loadBalancer.getId()); IPAddressVO publicIp = ApiDBUtils.findIpAddressById(loadBalancer.getSourceIpAddressId()); lbResponse.setPublicIpId(publicIp.getUuid()); 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 38f405fe1e4d..ae75b75f81c6 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -35,6 +35,8 @@ import org.apache.cloudstack.acl.ApiKeyPairVO; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.resourcedetail.FirewallRuleDetailVO; +import org.apache.cloudstack.resourcedetail.dao.FirewallRuleDetailsDao; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.command.user.loadbalancer.CreateLBHealthCheckPolicyCmd; @@ -183,6 +185,8 @@ public class LoadBalancingRulesManagerImpl extends ManagerBase implements LoadBalancingRulesManager, LoadBalancingRulesService { + @Inject + FirewallRuleDetailsDao _firewallRuleDetailsDao; @Inject NetworkOrchestrationService _networkMgr; @Inject @@ -2285,6 +2289,42 @@ public List getExistingDestinations(long lbId) { return dstList; } + /** + * Haproxy rejects a negative timeout, and a rejected file leaves every rule on the router + * running its previous config. Refuse the value here rather than let it reach the VR. + */ + protected void validateConnectionTimeout(String name, Long value) { + if (value != null && value < 0) { + throw new InvalidParameterValueException(String.format("%s must be 0 or greater, got [%s]. 0 means no timeout.", name, value)); + } + } + + @Override + public boolean updateLoadBalancerConnectionSettings(long lbRuleId, Boolean keepAlive, Long idleTimeout, Long keepAliveTimeout) { + validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, idleTimeout); + validateConnectionTimeout(ApiConstants.KEEPALIVE_TIMEOUT, keepAliveTimeout); + + boolean changed = storeDetail(lbRuleId, LoadBalancer.KEEPALIVE, keepAlive == null ? null : keepAlive.toString()); + changed |= storeDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT, idleTimeout == null ? null : idleTimeout.toString()); + changed |= storeDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT, keepAliveTimeout == null ? null : keepAliveTimeout.toString()); + return changed; + } + + private boolean storeDetail(long lbRuleId, String key, String value) { + if (value == null) { + return false; + } + FirewallRuleDetailVO existing = _firewallRuleDetailsDao.findDetail(lbRuleId, key); + if (existing != null && value.equals(existing.getValue())) { + return false; + } + if (existing != null) { + _firewallRuleDetailsDao.removeDetail(lbRuleId, key); + } + _firewallRuleDetailsDao.addDetail(lbRuleId, key, value, true); + return true; + } + @Override @ActionEvent(eventType = EventTypes.EVENT_LOAD_BALANCER_UPDATE, eventDescription = "updating load balancer", async = true) public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { @@ -2342,6 +2382,9 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { lb.setCidrList(cidrListStr); } + // lb.getId() rather than the id off the command, which is a Long and unboxes badly + boolean settingsChanged = updateLoadBalancerConnectionSettings(lb.getId(), cmd.getKeepAlive(), cmd.getIdleTimeout(), cmd.getKeepAliveTimeout()); + // Validate rule in LB provider LoadBalancingRule rule = getLoadBalancerRuleToApply(lb); if (!validateLbRule(rule)) { @@ -2356,7 +2399,7 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { boolean protocolChanged = !Objects.equals(lbProtocol, tmplbVo.getLbProtocol()); boolean cidrListChanged = !Objects.equals(tmplbVo.getCidrList(), lb.getCidrList()); - if (algorithmChanged || protocolChanged || cidrListChanged) { + if (algorithmChanged || protocolChanged || cidrListChanged || settingsChanged) { try { lb.setState(FirewallRule.State.Add); _lbDao.persist(lb); diff --git a/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java b/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java index 58b2892e09c3..05dd3d4bc0b0 100644 --- a/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java +++ b/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java @@ -34,6 +34,8 @@ import org.apache.cloudstack.network.BgpPeer; import org.apache.cloudstack.network.BgpPeerTO; import org.apache.cloudstack.network.dao.BgpPeerDetailsDao; +import org.apache.cloudstack.resourcedetail.FirewallRuleDetailVO; +import org.apache.cloudstack.resourcedetail.dao.FirewallRuleDetailsDao; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; @@ -119,6 +121,7 @@ import com.cloud.network.lb.LoadBalancingRule.LbDestination; import com.cloud.network.lb.LoadBalancingRule.LbStickinessPolicy; import com.cloud.network.rules.FirewallRule; +import com.cloud.network.rules.LoadBalancer; import com.cloud.network.rules.FirewallRule.Purpose; import com.cloud.network.rules.FirewallRuleVO; import com.cloud.network.rules.PortForwardingRule; @@ -169,6 +172,8 @@ public class CommandSetupHelper { @Inject private DomainDao domainDao; @Inject + private FirewallRuleDetailsDao _firewallRuleDetailsDao; + @Inject private NicDao _nicDao; @Inject private NetworkDao _networkDao; @@ -352,6 +357,29 @@ public void configDnsMasq(final VirtualRouter router, final Network network, fin cmds.addCommand("dnsMasqConfig", dnsMasqConfigCmd); } + /** Optional per rule haproxy settings. Absent means the rule inherits, so nothing is set. */ + protected void setConnectionSettings(final LoadBalancerTO lb, final long lbRuleId) { + final FirewallRuleDetailVO keepAlive = _firewallRuleDetailsDao.findDetail(lbRuleId, LoadBalancer.KEEPALIVE); + if (keepAlive != null) { + lb.setKeepAlive(Boolean.valueOf(keepAlive.getValue())); + } + lb.setIdleTimeout(longDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT)); + lb.setKeepAliveTimeout(longDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT)); + } + + private Long longDetail(final long lbRuleId, final String key) { + final FirewallRuleDetailVO detail = _firewallRuleDetailsDao.findDetail(lbRuleId, key); + if (detail == null) { + return null; + } + try { + return Long.valueOf(detail.getValue()); + } catch (final NumberFormatException e) { + logger.warn("Ignoring lb rule {} detail {}, [{}] is not a number", lbRuleId, key, detail.getValue()); + return null; + } + } + public void createApplyLoadBalancingRulesCommands(final List rules, final VirtualRouter router, final Commands cmds, final long guestNetworkId) { final LoadBalancerTO[] lbs = new LoadBalancerTO[rules.size()]; int i = 0; @@ -375,6 +403,7 @@ public void createApplyLoadBalancingRulesCommands(final List } lb.setLbProtocol(lb_protocol); lb.setLbSslCert(rule.getLbSslCert()); + setConnectionSettings(lb, rule.getId()); lbs[i++] = lb; } String routerPublicIp = null; diff --git a/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java b/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java index 282d892600b5..491cfab44196 100644 --- a/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java +++ b/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java @@ -378,4 +378,16 @@ public void createPublicLoadBalancerRuleWithDnsPortAndNoIpDoesNotNpe() throws Ex lbr.createPublicLoadBalancerRule("xid", "name", "desc", 53, 53, 53, 53, null, "tcp", "roundrobin", networkId, lbOwnerId, false, "tcp", null, null); } + + @Test + public void testValidateConnectionTimeoutAcceptsZeroAndAbove() { + lbr.validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, null); + lbr.validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, 0L); + lbr.validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, 600000L); + } + + @Test(expected = InvalidParameterValueException.class) + public void testValidateConnectionTimeoutRejectsNegative() { + lbr.validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, -1L); + } } diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index 99bf2cf7aef9..37dfa39e8a17 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -1370,6 +1370,7 @@ "label.in.progress": "in progress", "label.in.progress.for": "in progress for", "label.info": "Info", +"label.inherit.default": "Use default", "label.info.upper": "INFO", "label.infrastructure": "Infrastructure", "label.ingest.instance": "Ingest Instance", @@ -1459,6 +1460,8 @@ "label.isadvanced": "Show advanced settings", "label.iscsi": "iSCSI", "label.iscustomized": "Custom disk size", +"label.idletimeout": "Idle timeout (ms)", +"label.idletimeout.tooltip": "How long an idle connection is held open, in milliseconds. Use 0 for no timeout. Leave blank to use the global setting.", "label.iscustomizeddiskiops": "Custom IOPS", "label.iscustomizediops": "Custom IOPS", "label.isdedicated": "Dedicated", @@ -1508,6 +1511,10 @@ "label.key": "Key", "label.keybits": "Key Bits", "label.keyboard": "Keyboard language", +"label.keepalive": "Keepalive", +"label.keepalive.tooltip": "Whether the load balancer keeps client connections open between requests. Only applies to rules served in HTTP mode. Leave blank to use the Network offering setting.", +"label.keepalivetimeout": "Keepalive timeout (ms)", +"label.keepalivetimeout.tooltip": "How long an idle keepalive connection is held open waiting for the next request, in milliseconds. Only applies when keepalive is on.", "label.keyboardtype": "Keyboard type", "label.keypair": "SSH key pair", "label.keypairs": "SSH key pair(s)", diff --git a/ui/src/views/network/LoadBalancing.vue b/ui/src/views/network/LoadBalancing.vue index 0b9ed7684a89..ae37caee714c 100644 --- a/ui/src/views/network/LoadBalancing.vue +++ b/ui/src/views/network/LoadBalancing.vue @@ -472,6 +472,61 @@ :placeholder="$t('label.sourcecidrlist')" /> +
+

+ {{ $t('label.keepalive') }} + +

+ + {{ $t('label.inherit.default') }} + {{ $t('label.yes') }} + {{ $t('label.no') }} + +
+
+

+ {{ $t('label.idletimeout') }} + +

+ +
+
+

+ {{ $t('label.keepalivetimeout') }} + +

+ +
{{ $t('label.cancel') }} {{ $t('label.ok') }} @@ -859,7 +914,10 @@ export default { name: '', algorithm: '', protocol: '', - cidrlist: '' + cidrlist: '', + keepalive: undefined, + idletimeout: undefined, + keepalivetimeout: undefined }, newRule: { algorithm: 'roundrobin', @@ -1663,6 +1721,9 @@ export default { this.editRuleDetails.algorithm = this.lbProvider !== 'Netris' ? this.selectedRule.algorithm : undefined this.editRuleDetails.protocol = this.selectedRule.protocol // Normalize cidrlist: replace spaces with commas and clean up + this.editRuleDetails.keepalive = this.selectedRule.keepalive + this.editRuleDetails.idletimeout = this.selectedRule.idletimeout + this.editRuleDetails.keepalivetimeout = this.selectedRule.keepalivetimeout this.editRuleDetails.cidrlist = (this.selectedRule.cidrlist || '') .split(/[\s,]+/) // Split on spaces or commas .map(c => c.trim()) @@ -1680,11 +1741,12 @@ export default { cidrList: (this.editRuleDetails.cidrlist || '').split(',').map(c => c.trim()).filter(c => c) }) } - postAPI('updateLoadBalancerRule', { - ...this.editRuleDetails, - id: this.selectedRule.id, - ...payload - }).then(response => { + for (const key of ['keepalive', 'idletimeout', 'keepalivetimeout']) { + if (payload[key] === '' || payload[key] === null || payload[key] === undefined) { + delete payload[key] + } + } + postAPI('updateLoadBalancerRule', payload).then(response => { this.$pollJob({ jobId: response.updateloadbalancerruleresponse.jobid, successMessage: this.$t('message.success.edit.rule'), From c8cf0fdec01b64499a37629851c8ee70c129d9e4 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 18:18:20 +0000 Subject: [PATCH 05/10] Check the per rule settings in the VR health check haproxy_check.py compares the router's haproxy.cfg against what the management server thinks it configured. It knew about maxconn and the global idle timeout, so a rule's own keepalive and timeouts could drift without anything noticing. The health check data now carries the three per rule values, empty when the rule inherits, and the check validates them in that rule's listen section: ruleKeepAlive option http-keep-alive, or option httpclose ruleIdleTimeout timeout client, timeout server ruleKeepAliveTimeout timeout http-keep-alive The http mode test needed widening too. It read the offering flag alone, so a rule that sets keepalive itself stopped being checked at all: before port 80 and the offering has keepalive off after port 80 and (the rule sets keepalive, or the offering has it off) An older management server sends none of these keys, and a rule that sets nothing sends them empty. Both read as nothing to check. Signed-off-by: Brad House --- .../VirtualNetworkApplianceManagerImpl.java | 24 +++++++- .../root/health_checks/haproxy_check.py | 58 ++++++++++++++++++- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java index a166e894be1b..7f985b91e975 100644 --- a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java +++ b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java @@ -78,6 +78,8 @@ import org.apache.cloudstack.utils.CloudStackVersion; import org.apache.cloudstack.utils.identity.ManagementServerNode; import org.apache.cloudstack.utils.usage.UsageUtils; +import org.apache.cloudstack.resourcedetail.FirewallRuleDetailVO; +import org.apache.cloudstack.resourcedetail.dao.FirewallRuleDetailsDao; import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; @@ -199,6 +201,7 @@ import com.cloud.network.router.VirtualRouter.RedundantState; import com.cloud.network.router.VirtualRouter.Role; import com.cloud.network.rules.FirewallRule; +import com.cloud.network.rules.LoadBalancer; import com.cloud.network.rules.FirewallRule.Purpose; import com.cloud.network.rules.FirewallRuleVO; import com.cloud.network.rules.LoadBalancerContainer.Scheme; @@ -352,6 +355,8 @@ public class VirtualNetworkApplianceManagerImpl extends ManagerBase implements V @Inject protected CommandSetupHelper _commandSetupHelper; @Inject ManagementServer mgr; @Inject + private FirewallRuleDetailsDao _firewallRuleDetailsDao; + @Inject RoutedIpv4Manager routedIpv4Manager; @Inject BGPService bgpService; @@ -1711,7 +1716,9 @@ private void updateWithLbRules(final DomainRouterJoinVO routerJoinVO, final Stri .append(",protocol=").append(appLoadBalancerVO.getLbProtocol()); } loadBalancingData.append(",stickiness=").append(getStickinessPolicies(firewallRuleVO.getId())); - loadBalancingData.append(",keepAliveEnabled=").append(offering.isKeepAliveEnabled()).append(",vmIps="); + loadBalancingData.append(",keepAliveEnabled=").append(offering.isKeepAliveEnabled()); + appendLbRuleConnectionSettings(loadBalancingData, firewallRuleVO.getId()); + loadBalancingData.append(",vmIps="); for (LoadBalancerVMMapVO vmMapVO : vmMapVOs) { loadBalancingData.append(vmMapVO.getInstanceIp()).append(" "); } @@ -1720,6 +1727,21 @@ private void updateWithLbRules(final DomainRouterJoinVO routerJoinVO, final Stri } } + /** + * The per rule haproxy settings, so the health check can tell what the rule's own listen + * section should hold. Empty when the rule inherits. + */ + protected void appendLbRuleConnectionSettings(final StringBuilder loadBalancingData, long lbRuleId) { + loadBalancingData.append(",ruleKeepAlive=").append(lbRuleDetail(lbRuleId, LoadBalancer.KEEPALIVE)) + .append(",ruleIdleTimeout=").append(lbRuleDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT)) + .append(",ruleKeepAliveTimeout=").append(lbRuleDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT)); + } + + private String lbRuleDetail(long lbRuleId, String key) { + FirewallRuleDetailVO detail = _firewallRuleDetailsDao.findDetail(lbRuleId, key); + return detail == null ? "" : detail.getValue(); + } + protected void updateWithLbRuleSslCertificates(final StringBuilder loadBalancingData, LoadBalancerVO loadBalancerVO, String sourceIp) { if (NetUtils.SSL_PROTO.equals(loadBalancerVO.getLbProtocol())) { final LbSslCert sslCert = _lbMgr.getLbSslCert(loadBalancerVO.getId()); diff --git a/systemvm/debian/root/health_checks/haproxy_check.py b/systemvm/debian/root/health_checks/haproxy_check.py index cc9d90f7c18e..f06f7a685977 100644 --- a/systemvm/debian/root/health_checks/haproxy_check.py +++ b/systemvm/debian/root/health_checks/haproxy_check.py @@ -69,6 +69,52 @@ def checkIdletimeout(haproxyData, haCfgSections): return False return True +def hasOption(cfgSection, option): + return option in cfgSection.get("option", []) + + +def timeoutValue(cfgSection, kind): + for tline in cfgSection.get("timeout", []): + parts = tline.strip().split(None, 1) + if len(parts) == 2 and parts[0].strip() == kind: + return parts[1].strip() + return None + + +def checkRuleConnectionSettings(lbSec, cfgSection, secName, httpModeExpected): + """ + Per rule keepalive and timeouts, which live in the rule's own listen section rather than in + defaults. An empty value means the rule inherits, so there is nothing to check. + """ + correct = True + + keepAlive = lbSec.get("ruleKeepAlive", "") + if keepAlive != "" and httpModeExpected: + expected = "http-keep-alive" if keepAlive == "true" else "httpclose" + if not hasOption(cfgSection, expected): + print("Expected 'option " + expected + "' in " + secName) + correct = False + + idleTimeout = lbSec.get("ruleIdleTimeout", "") + if idleTimeout != "": + for kind in ("client", "server"): + found = timeoutValue(cfgSection, kind) + if found != idleTimeout: + print("Expected 'timeout " + kind + " " + idleTimeout + "' in " + secName + + " but found " + str(found)) + correct = False + + keepAliveTimeout = lbSec.get("ruleKeepAliveTimeout", "") + if keepAliveTimeout != "" and keepAlive == "true" and httpModeExpected: + found = timeoutValue(cfgSection, "http-keep-alive") + if found != keepAliveTimeout: + print("Expected 'timeout http-keep-alive " + keepAliveTimeout + "' in " + secName + + " but found " + str(found)) + correct = False + + return correct + + def checkLoadBalance(haproxyData, haCfgSections): correct = True for lbSec in haproxyData: @@ -94,12 +140,20 @@ def checkLoadBalance(haproxyData, haCfgSections): print("Incorrect bind string found. Expected " + bindStr + " but found " + cfgSection["bind"][0] + ".") correct = False - if (lbSec["sourcePortStart"] == "80" and lbSec["sourcePortEnd"] == "80" and lbSec["keepAliveEnabled"] == "false") \ - or (lbSec["stickiness"].find("AppCookie") != -1 or lbSec["stickiness"].find("LbCookie") != -1): + # A rule that sets keepalive itself stays in http mode on port 80. Without it, the + # network offering decides, and keepalive there drops the rule to tcp mode. + onHttpPort = lbSec["sourcePortStart"] == "80" and lbSec["sourcePortEnd"] == "80" + ruleKeepAlive = lbSec.get("ruleKeepAlive", "") + httpModeExpected = (onHttpPort and (ruleKeepAlive != "" or lbSec["keepAliveEnabled"] == "false")) \ + or lbSec["stickiness"].find("AppCookie") != -1 or lbSec["stickiness"].find("LbCookie") != -1 + if httpModeExpected: if not ("mode" in cfgSection and cfgSection["mode"][0] == "http"): print("Expected HTTP mode but not found") correct = False + if not checkRuleConnectionSettings(lbSec, cfgSection, secName, httpModeExpected): + correct = False + expectedServerIps = lbSec["vmIps"].split(" ") for expectedServerIp in expectedServerIps: pattern = expectedServerIp + ":" + \ From b5ff105b8eacfae5e5aeddc05f8c12bd1b2621d4 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 7 Oct 2026 15:06:07 +0000 Subject: [PATCH 06/10] Pass the update command's rule id to the connection settings lb was looked up by that same id and is non null by this point, so the Long unboxes safely. The comment claiming otherwise was wrong, and the id now matches the one applyLoadBalancerConfig gets a few lines later. Signed-off-by: Brad House --- .../com/cloud/network/lb/LoadBalancingRulesManagerImpl.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 ae75b75f81c6..71758798d7aa 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -2382,8 +2382,7 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { lb.setCidrList(cidrListStr); } - // lb.getId() rather than the id off the command, which is a Long and unboxes badly - boolean settingsChanged = updateLoadBalancerConnectionSettings(lb.getId(), cmd.getKeepAlive(), cmd.getIdleTimeout(), cmd.getKeepAliveTimeout()); + boolean settingsChanged = updateLoadBalancerConnectionSettings(lbRuleId, cmd.getKeepAlive(), cmd.getIdleTimeout(), cmd.getKeepAliveTimeout()); // Validate rule in LB provider LoadBalancingRule rule = getLoadBalancerRuleToApply(lb); From e974792cfa0ccb06a92191a8affa6b049c47b005 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 7 Oct 2026 15:08:53 +0000 Subject: [PATCH 07/10] Let updateLoadBalancerRule put a rule's settings back to the default Once a rule had its own keepalive or timeout there was no way back to inheriting. Leaving a parameter out keeps the current value, and there is no value that means unset: negative timeouts are refused, and keepalive is a boolean. updateLoadBalancerRule takes cleanupconnectionsettings, after the cleanupdetails flag on updateVirtualMachine and updateTemplate. It drops all three of the rule's own settings, then sets again any passed in the same call, so one call can reset one and keep the rest: rule holds call rule holds after idletimeout=5000 cleanupconnectionsettings=true nothing keepalive=true idletimeout=5000 cleanupconnectionsettings=true idletimeout=5000 keepalive=true idletimeout=5000 idletimeout=5000 keepalivetimeout=2000 idletimeout=5000 keepalivetimeout=2000 A change is now worked out by comparing the settings before and after, so a cleanup on a rule that holds none does not reapply the config. The update parameter descriptions said an unset value inherits, which is only true on create. They now say it keeps the current value. Signed-off-by: Brad House --- .../network/lb/LoadBalancingRulesService.java | 3 +- .../apache/cloudstack/api/ApiConstants.java | 1 + .../CreateLoadBalancerRuleCmd.java | 2 +- .../UpdateLoadBalancerRuleCmd.java | 19 +++- .../lb/LoadBalancingRulesManagerImpl.java | 68 +++++++++--- .../lb/LoadBalancingRulesManagerImplTest.java | 104 ++++++++++++++++++ 6 files changed, 175 insertions(+), 22 deletions(-) diff --git a/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java b/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java index 0abe20e274cd..3b1755e6d425 100644 --- a/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java +++ b/api/src/main/java/com/cloud/network/lb/LoadBalancingRulesService.java @@ -73,9 +73,10 @@ LoadBalancer createPublicLoadBalancerRule(String xId, String name, String descri /** * Stores the optional per rule haproxy settings. A null leaves the current value alone. * + * @param cleanup remove all three first, so the rule inherits again, then store any non null value given * @return true if any value changed */ - boolean updateLoadBalancerConnectionSettings(long lbRuleId, Boolean keepAlive, Long idleTimeout, Long keepAliveTimeout); + boolean updateLoadBalancerConnectionSettings(long lbRuleId, Boolean keepAlive, Long idleTimeout, Long keepAliveTimeout, boolean cleanup); boolean deleteLoadBalancerRule(long lbRuleId, boolean apply); diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index f58df6c200a1..6e3940533fd5 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -1210,6 +1210,7 @@ public class ApiConstants { public static final String OVM3_POOL = "ovm3pool"; public static final String OVM3_CLUSTER = "ovm3cluster"; public static final String OVM3_VIP = "ovm3vip"; + public static final String CLEAN_UP_CONNECTION_SETTINGS = "cleanupconnectionsettings"; public static final String CLEAN_UP_DETAILS = "cleanupdetails"; public static final String CLEAN_UP_EXTERNAL_DETAILS = "cleanupexternaldetails"; public static final String CLEAN_UP_EXTRA_CONFIG = "cleanupextraconfig"; diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java index b6cb8cc9ca70..9c881e3f31ef 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java @@ -335,7 +335,7 @@ public void create() { getCidrList()); this.setEntityId(result.getId()); this.setEntityUuid(result.getUuid()); - _lbService.updateLoadBalancerConnectionSettings(result.getId(), getKeepAlive(), getIdleTimeout(), getKeepAliveTimeout()); + _lbService.updateLoadBalancerConnectionSettings(result.getId(), getKeepAlive(), getIdleTimeout(), getKeepAliveTimeout(), false); } catch (NetworkRuleConflictException e) { logger.warn("Exception: ", e); throw new ServerApiException(ApiErrorCode.NETWORK_RULE_CONFLICT_ERROR, e.getMessage()); 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 a66a2d22a548..307b8f8cc91a 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 @@ -70,20 +70,29 @@ public class UpdateLoadBalancerRuleCmd extends BaseAsyncCustomIdCmd { @Parameter(name = ApiConstants.KEEPALIVE, type = CommandType.BOOLEAN, since = "4.23.0", description = "Whether the load balancer keeps client connections open between requests. " - + "Only applies to rules the router serves in HTTP mode. If not set, the network offering's setting is used.") + + "Only applies to rules the router serves in HTTP mode. If not set, the rule's current value is kept; " + + "use cleanupconnectionsettings to go back to the network offering's setting.") private Boolean keepAlive; @Parameter(name = ApiConstants.IDLE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", description = "How long an idle connection is held open, in milliseconds. Use 0 for infinite. " - + "If not set, the global setting network.loadbalancer.haproxy.idle.timeout is used.") + + "If not set, the rule's current value is kept; use cleanupconnectionsettings to go back to " + + "the global setting network.loadbalancer.haproxy.idle.timeout.") private Long idleTimeout; @Parameter(name = ApiConstants.KEEPALIVE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", description = "How long an idle keepalive connection is held open waiting for the next request, " + "in milliseconds. Only applies to rules the router serves in HTTP mode. " - + "If not set, idletimeout applies.") + + "If not set, the rule's current value is kept; use cleanupconnectionsettings to go back to " + + "following idletimeout.") private Long keepAliveTimeout; + @Parameter(name = ApiConstants.CLEAN_UP_CONNECTION_SETTINGS, type = CommandType.BOOLEAN, since = "4.23.0", + description = "If true, the rule's own keepalive, idletimeout and keepalivetimeout are removed, so the rule goes back " + + "to the network offering and global settings. Any of the three passed in the same call are then set again, " + + "so a single call can reset one and keep the others. If false or not set, no action.") + private Boolean cleanupConnectionSettings; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -102,6 +111,10 @@ public Long getKeepAliveTimeout() { return keepAliveTimeout; } + public boolean isCleanupConnectionSettings() { + return Boolean.TRUE.equals(cleanupConnectionSettings); + } + public String getDescription() { 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 71758798d7aa..446fa93f2a08 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -284,6 +284,7 @@ public class LoadBalancingRulesManagerImpl extends ManagerBase implements NicSecondaryIpDao _nicSecondaryIpDao; private static final int DNS_PORT = 53; + private static final List CONNECTION_SETTINGS = List.of(LoadBalancer.KEEPALIVE, LoadBalancer.IDLE_TIMEOUT, LoadBalancer.KEEPALIVE_TIMEOUT); // Will return a string. For LB Stickiness this will be a json, for // autoscale this will be "," separated values @Override @@ -2300,31 +2301,63 @@ protected void validateConnectionTimeout(String name, Long value) { } @Override - public boolean updateLoadBalancerConnectionSettings(long lbRuleId, Boolean keepAlive, Long idleTimeout, Long keepAliveTimeout) { + public boolean updateLoadBalancerConnectionSettings(long lbRuleId, Boolean keepAlive, Long idleTimeout, Long keepAliveTimeout, boolean cleanup) { validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, idleTimeout); validateConnectionTimeout(ApiConstants.KEEPALIVE_TIMEOUT, keepAliveTimeout); - boolean changed = storeDetail(lbRuleId, LoadBalancer.KEEPALIVE, keepAlive == null ? null : keepAlive.toString()); - changed |= storeDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT, idleTimeout == null ? null : idleTimeout.toString()); - changed |= storeDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT, keepAliveTimeout == null ? null : keepAliveTimeout.toString()); - return changed; - } - - private boolean storeDetail(long lbRuleId, String key, String value) { - if (value == null) { - return false; + Map current = getConnectionSettings(lbRuleId); + Map wanted = cleanup ? new HashMap<>() : new HashMap<>(current); + if (keepAlive != null) { + wanted.put(LoadBalancer.KEEPALIVE, keepAlive.toString()); } - FirewallRuleDetailVO existing = _firewallRuleDetailsDao.findDetail(lbRuleId, key); - if (existing != null && value.equals(existing.getValue())) { - return false; + if (idleTimeout != null) { + wanted.put(LoadBalancer.IDLE_TIMEOUT, idleTimeout.toString()); + } + if (keepAliveTimeout != null) { + wanted.put(LoadBalancer.KEEPALIVE_TIMEOUT, keepAliveTimeout.toString()); } - if (existing != null) { - _firewallRuleDetailsDao.removeDetail(lbRuleId, key); + if (wanted.equals(current)) { + return false; } - _firewallRuleDetailsDao.addDetail(lbRuleId, key, value, true); + setConnectionSettings(lbRuleId, wanted); return true; } + /** + * The per rule settings the rule holds itself, keyed by detail name. A setting the rule + * inherits is absent. + */ + protected Map getConnectionSettings(long lbRuleId) { + Map settings = new HashMap<>(); + for (String key : CONNECTION_SETTINGS) { + FirewallRuleDetailVO detail = _firewallRuleDetailsDao.findDetail(lbRuleId, key); + if (detail != null) { + settings.put(key, detail.getValue()); + } + } + return settings; + } + + /** + * Makes the rule hold exactly these settings: a key in the map is stored, a key missing + * from it is removed so the rule inherits it again. + */ + protected void setConnectionSettings(long lbRuleId, Map settings) { + for (String key : CONNECTION_SETTINGS) { + String value = settings.get(key); + FirewallRuleDetailVO existing = _firewallRuleDetailsDao.findDetail(lbRuleId, key); + if (existing != null && Objects.equals(value, existing.getValue())) { + continue; + } + if (existing != null) { + _firewallRuleDetailsDao.removeDetail(lbRuleId, key); + } + if (value != null) { + _firewallRuleDetailsDao.addDetail(lbRuleId, key, value, true); + } + } + } + @Override @ActionEvent(eventType = EventTypes.EVENT_LOAD_BALANCER_UPDATE, eventDescription = "updating load balancer", async = true) public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { @@ -2382,7 +2415,8 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { lb.setCidrList(cidrListStr); } - boolean settingsChanged = updateLoadBalancerConnectionSettings(lbRuleId, cmd.getKeepAlive(), cmd.getIdleTimeout(), cmd.getKeepAliveTimeout()); + boolean settingsChanged = updateLoadBalancerConnectionSettings(lbRuleId, cmd.getKeepAlive(), cmd.getIdleTimeout(), cmd.getKeepAliveTimeout(), + cmd.isCleanupConnectionSettings()); // Validate rule in LB provider LoadBalancingRule rule = getLoadBalancerRuleToApply(lb); diff --git a/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java b/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java index 491cfab44196..0a1a520761c4 100644 --- a/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java +++ b/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java @@ -36,6 +36,8 @@ import org.apache.cloudstack.api.command.user.loadbalancer.UpdateLoadBalancerRuleCmd; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService; +import org.apache.cloudstack.resourcedetail.FirewallRuleDetailVO; +import org.apache.cloudstack.resourcedetail.dao.FirewallRuleDetailsDao; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -57,6 +59,7 @@ import com.cloud.network.dao.NetworkDao; import com.cloud.network.dao.NetworkVO; import com.cloud.network.dao.SslCertVO; +import com.cloud.network.rules.LoadBalancer; import com.cloud.network.vpc.VpcManager; import com.cloud.offering.NetworkOffering; import com.cloud.offerings.dao.NetworkOfferingServiceMapDao; @@ -104,6 +107,9 @@ public class LoadBalancingRulesManagerImplTest{ @Mock VpcManager vpcManager; + @Mock + FirewallRuleDetailsDao _firewallRuleDetailsDao; + @Spy @InjectMocks LoadBalancingRulesManagerImpl lbr = new LoadBalancingRulesManagerImpl(); @@ -390,4 +396,102 @@ public void testValidateConnectionTimeoutAcceptsZeroAndAbove() { public void testValidateConnectionTimeoutRejectsNegative() { lbr.validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, -1L); } + + private void stubConnectionSettings(String keepAlive, String idleTimeout, String keepAliveTimeout) { + stubConnectionSetting(LoadBalancer.KEEPALIVE, keepAlive); + stubConnectionSetting(LoadBalancer.IDLE_TIMEOUT, idleTimeout); + stubConnectionSetting(LoadBalancer.KEEPALIVE_TIMEOUT, keepAliveTimeout); + } + + private void stubConnectionSetting(String key, String value) { + when(_firewallRuleDetailsDao.findDetail(lbRuleId, key)) + .thenReturn(value == null ? null : new FirewallRuleDetailVO(lbRuleId, key, value, true)); + } + + private void verifyNoConnectionSettingWrites() { + Mockito.verify(_firewallRuleDetailsDao, never()).removeDetail(anyLong(), Mockito.anyString()); + Mockito.verify(_firewallRuleDetailsDao, never()).addDetail(anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + } + + @Test + public void testConnectionSettingsCleanupResetsAllToInherit() { + stubConnectionSettings("true", "5000", "1000"); + + Assert.assertTrue(lbr.updateLoadBalancerConnectionSettings(lbRuleId, null, null, null, true)); + + Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.KEEPALIVE); + Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT); + Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT); + Mockito.verify(_firewallRuleDetailsDao, never()).addDetail(anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + } + + @Test + public void testConnectionSettingsCleanupKeepsValuesPassedAlongside() { + stubConnectionSettings("true", "5000", "1000"); + + Assert.assertTrue(lbr.updateLoadBalancerConnectionSettings(lbRuleId, null, 5000L, null, true)); + + Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.KEEPALIVE); + Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT); + Mockito.verify(_firewallRuleDetailsDao, never()).removeDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT); + Mockito.verify(_firewallRuleDetailsDao, never()).addDetail(anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + } + + @Test + public void testConnectionSettingsCleanupWithNothingSetIsNoChange() { + stubConnectionSettings(null, null, null); + + Assert.assertFalse(lbr.updateLoadBalancerConnectionSettings(lbRuleId, null, null, null, true)); + + verifyNoConnectionSettingWrites(); + } + + @Test + public void testConnectionSettingsNullKeepsCurrentValue() { + stubConnectionSettings(null, "5000", null); + + Assert.assertTrue(lbr.updateLoadBalancerConnectionSettings(lbRuleId, null, null, 2000L, false)); + + Mockito.verify(_firewallRuleDetailsDao).addDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT, "2000", true); + Mockito.verify(_firewallRuleDetailsDao, never()).removeDetail(anyLong(), Mockito.anyString()); + } + + @Test + public void testConnectionSettingsSameValueIsNoChange() { + stubConnectionSettings(null, "5000", null); + + Assert.assertFalse(lbr.updateLoadBalancerConnectionSettings(lbRuleId, null, 5000L, null, false)); + + verifyNoConnectionSettingWrites(); + } + + @Test + public void testConnectionSettingsChangedValueIsReplaced() { + stubConnectionSettings("false", null, null); + + Assert.assertTrue(lbr.updateLoadBalancerConnectionSettings(lbRuleId, true, null, null, false)); + + Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.KEEPALIVE); + Mockito.verify(_firewallRuleDetailsDao).addDetail(lbRuleId, LoadBalancer.KEEPALIVE, "true", true); + } + + @Test + public void testUpdateLoadBalancerRuleCleanupReappliesConfig() throws Exception { + setupUpdateLoadBalancerRule(); + stubConnectionSettings(null, "5000", null); + + // Only the connection settings change, algorithm and protocol are the same + UpdateLoadBalancerRuleCmd cmd = new UpdateLoadBalancerRuleCmd(); + ReflectionTestUtils.setField(cmd, ApiConstants.ID, lbRuleId); + ReflectionTestUtils.setField(cmd, "algorithm", "roundrobin"); + ReflectionTestUtils.setField(cmd, "lbProtocol", NetUtils.SSL_PROTO); + ReflectionTestUtils.setField(cmd, "cleanupConnectionSettings", true); + when(loadBalancerMock.getAlgorithm()).thenReturn("roundrobin"); + when(loadBalancerMock.getLbProtocol()).thenReturn(NetUtils.SSL_PROTO); + + lbr.updateLoadBalancerRule(cmd); + + Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT); + Mockito.verify(lbr, times(1)).applyLoadBalancerConfig(lbRuleId); + } } From 07af93829d01d82284287b3550040e108fa61181 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 7 Oct 2026 15:08:53 +0000 Subject: [PATCH 08/10] Send cleanupconnectionsettings when a field is cleared in the UI Clearing keepalive, idle timeout or keepalive timeout in the edit rule dialog dropped the parameter, which the API reads as keep the current value, so the old setting stayed. When a field the rule had set is now blank the dialog sends cleanupconnectionsettings, along with the fields still filled in. Signed-off-by: Brad House --- ui/src/views/network/LoadBalancing.vue | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/ui/src/views/network/LoadBalancing.vue b/ui/src/views/network/LoadBalancing.vue index ae37caee714c..5cb50d7e5d96 100644 --- a/ui/src/views/network/LoadBalancing.vue +++ b/ui/src/views/network/LoadBalancing.vue @@ -1720,10 +1720,10 @@ export default { this.editRuleDetails.name = this.selectedRule.name this.editRuleDetails.algorithm = this.lbProvider !== 'Netris' ? this.selectedRule.algorithm : undefined this.editRuleDetails.protocol = this.selectedRule.protocol - // Normalize cidrlist: replace spaces with commas and clean up this.editRuleDetails.keepalive = this.selectedRule.keepalive this.editRuleDetails.idletimeout = this.selectedRule.idletimeout this.editRuleDetails.keepalivetimeout = this.selectedRule.keepalivetimeout + // Normalize cidrlist: replace spaces with commas and clean up this.editRuleDetails.cidrlist = (this.selectedRule.cidrlist || '') .split(/[\s,]+/) // Split on spaces or commas .map(c => c.trim()) @@ -1741,9 +1741,16 @@ export default { cidrList: (this.editRuleDetails.cidrlist || '').split(',').map(c => c.trim()).filter(c => c) }) } + // A blank field sends nothing, which the API reads as keep the current value. When a + // field the rule had set is now blank, ask the API to drop the rule's own settings; + // the ones still filled in are sent alongside and set again. + const isBlank = (value) => value === '' || value === null || value === undefined for (const key of ['keepalive', 'idletimeout', 'keepalivetimeout']) { - if (payload[key] === '' || payload[key] === null || payload[key] === undefined) { + if (isBlank(payload[key])) { delete payload[key] + if (!isBlank(this.selectedRule[key])) { + payload.cleanupconnectionsettings = true + } } } postAPI('updateLoadBalancerRule', payload).then(response => { From 5ae177450c0e00d8175c8a05e8c65fca7318199c Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 7 Oct 2026 15:10:40 +0000 Subject: [PATCH 09/10] Roll back a rule's connection settings when the router update fails When applying an updated rule to the router fails, updateLoadBalancerRule puts back the name, description, algorithm and cidrs, but not the keepalive and timeouts. Those stayed in the database while the router still ran the old values, and the next successful apply of anything on that router would quietly push them. The settings held before the update are now restored along with the rest. They were also written before the provider validated the rule, so a rule it rejected kept the new settings. The other fields only reach the database at _lbDao.update, after validation; the settings are now written at the same point. Signed-off-by: Brad House --- .../lb/LoadBalancingRulesManagerImpl.java | 12 ++- .../lb/LoadBalancingRulesManagerImplTest.java | 77 +++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) 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 446fa93f2a08..d89808f091d7 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -2415,15 +2415,18 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { lb.setCidrList(cidrListStr); } - boolean settingsChanged = updateLoadBalancerConnectionSettings(lbRuleId, cmd.getKeepAlive(), cmd.getIdleTimeout(), cmd.getKeepAliveTimeout(), - cmd.isCleanupConnectionSettings()); - // Validate rule in LB provider LoadBalancingRule rule = getLoadBalancerRuleToApply(lb); if (!validateLbRule(rule)) { throw new InvalidParameterValueException(String.format("Modifications in lb rule %s are not supported.", lb)); } + // The settings are written straight to the details table, unlike the fields above that wait + // for _lbDao.update, so they are stored only once nothing else can reject the update + Map settingsBackup = getConnectionSettings(lbRuleId); + boolean settingsChanged = updateLoadBalancerConnectionSettings(lbRuleId, cmd.getKeepAlive(), cmd.getIdleTimeout(), cmd.getKeepAliveTimeout(), + cmd.isCleanupConnectionSettings()); + LoadBalancerVO tmplbVo = _lbDao.findById(lbRuleId); boolean success = _lbDao.update(lbRuleId, lb); @@ -2461,6 +2464,9 @@ public LoadBalancer updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) { lb.setState(lbBackup.getState()); _lbDao.update(lb.getId(), lb); _lbDao.persist(lb); + if (settingsChanged) { + setConnectionSettings(lbRuleId, settingsBackup); + } logger.debug("LB Rollback rule: {} while updating LB rule.", lb); } diff --git a/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java b/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java index 0a1a520761c4..b09ee878ecaf 100644 --- a/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java +++ b/server/src/test/java/com/cloud/network/lb/LoadBalancingRulesManagerImplTest.java @@ -494,4 +494,81 @@ public void testUpdateLoadBalancerRuleCleanupReappliesConfig() throws Exception Mockito.verify(_firewallRuleDetailsDao).removeDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT); Mockito.verify(lbr, times(1)).applyLoadBalancerConfig(lbRuleId); } + + /** + * Backs the details dao with a map, so a value written and then rolled back can be seen. + */ + private Map fakeConnectionSettings(Map initial) { + Map store = new HashMap<>(initial); + when(_firewallRuleDetailsDao.findDetail(Mockito.eq(lbRuleId), Mockito.anyString())).thenAnswer(invocation -> { + String key = invocation.getArgument(1); + String value = store.get(key); + return value == null ? null : new FirewallRuleDetailVO(lbRuleId, key, value, true); + }); + Mockito.doAnswer(invocation -> store.remove(invocation.getArgument(1))) + .when(_firewallRuleDetailsDao).removeDetail(Mockito.eq(lbRuleId), Mockito.anyString()); + Mockito.doAnswer(invocation -> store.put(invocation.getArgument(1), invocation.getArgument(2))) + .when(_firewallRuleDetailsDao).addDetail(Mockito.eq(lbRuleId), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + return store; + } + + @Test + public void testUpdateLoadBalancerRuleRollsBackSettingsWhenApplyFails() throws Exception { + setupUpdateLoadBalancerRule(); + Map before = Map.of(LoadBalancer.KEEPALIVE, "true", LoadBalancer.IDLE_TIMEOUT, "5000"); + Map store = fakeConnectionSettings(before); + + // Drop keepalive, change idletimeout, add keepalivetimeout, then fail to reach the router + UpdateLoadBalancerRuleCmd cmd = new UpdateLoadBalancerRuleCmd(); + ReflectionTestUtils.setField(cmd, ApiConstants.ID, lbRuleId); + ReflectionTestUtils.setField(cmd, "algorithm", "roundrobin"); + ReflectionTestUtils.setField(cmd, "lbProtocol", NetUtils.SSL_PROTO); + ReflectionTestUtils.setField(cmd, "idleTimeout", 2000L); + ReflectionTestUtils.setField(cmd, "keepAliveTimeout", 1000L); + ReflectionTestUtils.setField(cmd, "cleanupConnectionSettings", true); + when(loadBalancerMock.getAlgorithm()).thenReturn("roundrobin"); + when(loadBalancerMock.getLbProtocol()).thenReturn(NetUtils.SSL_PROTO); + Mockito.doThrow(ResourceUnavailableException.class).when(lbr).applyLoadBalancerConfig(lbRuleId); + when(_networkMgr.getProvidersForServiceInNetwork(networkMock, Network.Service.Lb)) + .thenReturn(Collections.singletonList(Network.Provider.VirtualRouter)); + + try { + lbr.updateLoadBalancerRule(cmd); + Assert.fail("Expected the update to fail when the router is unavailable"); + } catch (CloudRuntimeException e) { + // expected + } + + Mockito.verify(lbr, times(1)).applyLoadBalancerConfig(lbRuleId); + Assert.assertEquals(before, store); + } + + @Test + public void testUpdateLoadBalancerRuleLeavesSettingsAloneWhenValidationFails() { + AccountVO account = new AccountVO("testaccount", 1L, "networkdomain", Account.Type.NORMAL, "uuid"); + account.setId(accountId); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", + UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + when(_lbDao.findById(lbRuleId)).thenReturn(loadBalancerMock); + when(loadBalancerMock.getNetworkId()).thenReturn(networkId); + when(_networkDao.findById(networkId)).thenReturn(networkMock); + LoadBalancingRule loadBalancingRule = Mockito.mock(LoadBalancingRule.class); + Mockito.doReturn(loadBalancingRule).when(lbr).getLoadBalancerRuleToApply(loadBalancerMock); + Mockito.doReturn(false).when(lbr).validateLbRule(loadBalancingRule); + + UpdateLoadBalancerRuleCmd cmd = new UpdateLoadBalancerRuleCmd(); + ReflectionTestUtils.setField(cmd, ApiConstants.ID, lbRuleId); + ReflectionTestUtils.setField(cmd, "idleTimeout", 2000L); + + try { + lbr.updateLoadBalancerRule(cmd); + Assert.fail("Expected the provider to reject the update"); + } catch (InvalidParameterValueException e) { + // expected + } + + verifyNoConnectionSettingWrites(); + } } From 37801b9c6a6da1390aee4813b72181323067510e Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 7 Oct 2026 15:39:53 +0000 Subject: [PATCH 10/10] Mark the new load balancer parameters as since 24.0 The keepalive, idle timeout and keepalive timeout parameters, and the cleanupconnectionsettings flag, were marked since 4.23.0. That release shipped without them, and main is now 24.0.0-SNAPSHOT following the vote to drop the "4." prefix starting with release 24. Signed-off-by: Brad House --- .../user/loadbalancer/CreateLoadBalancerRuleCmd.java | 6 +++--- .../user/loadbalancer/UpdateLoadBalancerRuleCmd.java | 8 ++++---- .../cloudstack/api/response/LoadBalancerResponse.java | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java index 9c881e3f31ef..f16f2ac67c17 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/CreateLoadBalancerRuleCmd.java @@ -119,17 +119,17 @@ public class CreateLoadBalancerRuleCmd extends BaseAsyncCreateCmd /*implements L @Parameter(name = ApiConstants.FOR_DISPLAY, type = CommandType.BOOLEAN, description = "An optional field, whether to the display the rule to the end user or not", since = "4.4", authorized = {RoleType.Admin}) private Boolean display; - @Parameter(name = ApiConstants.KEEPALIVE, type = CommandType.BOOLEAN, since = "4.23.0", + @Parameter(name = ApiConstants.KEEPALIVE, type = CommandType.BOOLEAN, since = "24.0", description = "Whether the load balancer keeps client connections open between requests. " + "Only applies to rules the router serves in HTTP mode. If not set, the network offering's setting is used.") private Boolean keepAlive; - @Parameter(name = ApiConstants.IDLE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + @Parameter(name = ApiConstants.IDLE_TIMEOUT, type = CommandType.LONG, since = "24.0", description = "How long an idle connection is held open, in milliseconds. Use 0 for infinite. " + "If not set, the global setting network.loadbalancer.haproxy.idle.timeout is used.") private Long idleTimeout; - @Parameter(name = ApiConstants.KEEPALIVE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + @Parameter(name = ApiConstants.KEEPALIVE_TIMEOUT, type = CommandType.LONG, since = "24.0", description = "How long an idle keepalive connection is held open waiting for the next request, " + "in milliseconds. Only applies to rules the router serves in HTTP mode. " + "If not set, idletimeout applies.") 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 307b8f8cc91a..ebb57fbe9889 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 @@ -68,26 +68,26 @@ public class UpdateLoadBalancerRuleCmd extends BaseAsyncCustomIdCmd { @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; - @Parameter(name = ApiConstants.KEEPALIVE, type = CommandType.BOOLEAN, since = "4.23.0", + @Parameter(name = ApiConstants.KEEPALIVE, type = CommandType.BOOLEAN, since = "24.0", description = "Whether the load balancer keeps client connections open between requests. " + "Only applies to rules the router serves in HTTP mode. If not set, the rule's current value is kept; " + "use cleanupconnectionsettings to go back to the network offering's setting.") private Boolean keepAlive; - @Parameter(name = ApiConstants.IDLE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + @Parameter(name = ApiConstants.IDLE_TIMEOUT, type = CommandType.LONG, since = "24.0", description = "How long an idle connection is held open, in milliseconds. Use 0 for infinite. " + "If not set, the rule's current value is kept; use cleanupconnectionsettings to go back to " + "the global setting network.loadbalancer.haproxy.idle.timeout.") private Long idleTimeout; - @Parameter(name = ApiConstants.KEEPALIVE_TIMEOUT, type = CommandType.LONG, since = "4.23.0", + @Parameter(name = ApiConstants.KEEPALIVE_TIMEOUT, type = CommandType.LONG, since = "24.0", description = "How long an idle keepalive connection is held open waiting for the next request, " + "in milliseconds. Only applies to rules the router serves in HTTP mode. " + "If not set, the rule's current value is kept; use cleanupconnectionsettings to go back to " + "following idletimeout.") private Long keepAliveTimeout; - @Parameter(name = ApiConstants.CLEAN_UP_CONNECTION_SETTINGS, type = CommandType.BOOLEAN, since = "4.23.0", + @Parameter(name = ApiConstants.CLEAN_UP_CONNECTION_SETTINGS, type = CommandType.BOOLEAN, since = "24.0", description = "If true, the rule's own keepalive, idletimeout and keepalivetimeout are removed, so the rule goes back " + "to the network offering and global settings. Any of the three passed in the same call are then set again, " + "so a single call can reset one and keep the others. If false or not set, no action.") diff --git a/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java index fd25c4e0042c..9de970f205df 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/LoadBalancerResponse.java @@ -64,15 +64,15 @@ public class LoadBalancerResponse extends BaseResponse implements ControlledEnti private String networkId; @SerializedName(ApiConstants.KEEPALIVE) - @Param(description = "whether the load balancer keeps client connections open between requests, unset means the network offering's setting is used", since = "4.23.0") + @Param(description = "whether the load balancer keeps client connections open between requests, unset means the network offering's setting is used", since = "24.0") private Boolean keepAlive; @SerializedName(ApiConstants.IDLE_TIMEOUT) - @Param(description = "how long an idle connection is held open, in milliseconds, unset means the global setting is used", since = "4.23.0") + @Param(description = "how long an idle connection is held open, in milliseconds, unset means the global setting is used", since = "24.0") private Long idleTimeout; @SerializedName(ApiConstants.KEEPALIVE_TIMEOUT) - @Param(description = "how long an idle keepalive connection is held open waiting for the next request, in milliseconds", since = "4.23.0") + @Param(description = "how long an idle keepalive connection is held open waiting for the next request, in milliseconds", since = "24.0") private Long keepAliveTimeout; @SerializedName(ApiConstants.CIDR_LIST)