LB: per-rule keepalive and timeouts, plus three HAProxy config fixes

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

@@ -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<String> 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<String> 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<String> 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<PortForwardingRuleTO> fwRules)
 
         final List<String> result = new ArrayList<String>();
 
-        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);
 
@@ -451,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 {
                 /*
@@ -477,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();
@@ -569,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 ? "\tno option forceclose" : "\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"
@@ -617,8 +650,7 @@ private String generateStatsRule(final LoadBalancerConfigCommand lbCmd, final St
     @Override
     public String[] generateConfiguration(final LoadBalancerConfigCommand lbCmd) {
         final List<String> result = new ArrayList<String>();
-        final List<String> gSection = Arrays.asList(globalSection);
-        //        note that this is overwritten on the String in the static ArrayList<String>
+        final List<String> 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,16 +663,15 @@ public String[] generateConfiguration(final LoadBalancerConfigCommand lbCmd) {
         result.addAll(gSection);
 
         result.add(blankLine);
-        final List<String> dSection = Arrays.asList(defaultsSection);
+        final List<String> dSection = new ArrayList<>(defaultsSection);
         if (lbCmd.keepAliveEnabled) {
             dSection.set(7, "\tno option httpclose");
         }
         if (lbCmd.idleTimeout > 0) {
             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 +726,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()]);
     }