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 <bhouse@nexthop.ai>

Author: bhouse-nexthop

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<LbDestination> 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<Pair<String, String>> params;

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);
 
     /**

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();

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";

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());

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

@@ -68,13 +68,41 @@ 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<String> 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 ///////////////////////
     /////////////////////////////////////////////////////
 
     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;

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;
     }

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<String> 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<String> 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"

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<Pair<String, String>> params = new ArrayList<>();