Copilot commented on code in PR #12586:
URL: https://github.com/apache/cloudstack/pull/12586#discussion_r2767711394
##########
core/src/main/java/com/cloud/network/HAProxyConfigurator.java:
##########
@@ -635,6 +635,15 @@ public String[] generateConfiguration(final
LoadBalancerConfigCommand lbCmd) {
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.
+ dSection.set(9, "");
+ dSection.set(10, "");
Review Comment:
The code doesn't handle negative values for idleTimeout. If a user somehow
configures a negative value, it would fall through the if-else logic and retain
the default timeout values from defaultsSection (50000). Consider adding
validation to ensure idleTimeout is non-negative, or explicitly handle negative
values in the else clause to make the behavior more predictable.
```suggestion
dSection.set(10, "");
} else {
// Negative idleTimeout values are considered invalid; retain the
// default HAProxy timeout values from defaultsSection for
predictability.
logger.warn("Negative idleTimeout ({}) configured; retaining
default HAProxy timeouts.", lbCmd.idleTimeout);
```
##########
systemvm/debian/root/health_checks/haproxy_check.py:
##########
@@ -28,6 +28,16 @@ def checkMaxconn(haproxyData, haCfgSections):
return True
+def checkIdletimeout(haproxyData, haCfgSections):
+ if "idletimeout" in haproxyData:
+ if "timeout client" not in haCfgSections["defaults"] or "timeout
server" not in haCfgSections["defaults"]:
+ print("defaults timeout client or timeout server missing")
+
+ if haproxyData["idletimeout"] != haCfgSections["defaults"]["timeout
client"][0].strip() or haproxyData["idletimeout"] !=
haCfgSections["defaults"]["timeout server"][0].strip():
+ print("defaults timeout client or timeout server mismatch
occurred")
+ return False
+
Review Comment:
The health check function has multiple critical bugs:
1. The config file parser splits lines by space with maxsplit=1 (line 126),
so "timeout client 1234" becomes key="timeout" with value="client 1234".
However, this function tries to access keys "timeout client" and "timeout
server" which don't exist in the parsed configuration.
2. When the timeout fields are missing (line 33-34), the function prints an
error but doesn't return False, allowing execution to continue to line 36 where
it will crash trying to access non-existent keys.
3. The check at line 36 cannot handle the idletimeout=0 case. When
idletimeout is 0, HAProxyConfigurator sets these lines to empty strings
(HAProxyConfigurator.java lines 644-645), which means they won't be present in
the parsed config.
The correct approach would be to:
- Check for the "timeout" key, not "timeout client"/"timeout server"
- Extract and parse the timeout values from
haCfgSections["defaults"]["timeout"] list
- Handle the case where idletimeout is 0 or "0" by verifying the timeout
entries are missing or empty
- Return False immediately when timeout fields are missing (if idletimeout
!= 0)
```suggestion
if "idletimeout" not in haproxyData:
return True
# Normalize idletimeout value to string for comparison
idle_value = str(haproxyData["idletimeout"]).strip()
# Safely get the defaults section and its timeout directives
defaults_section = haCfgSections.get("defaults", {})
timeout_lines = defaults_section.get("timeout", [])
# Extract client and server timeout values from the parsed "timeout"
entries
timeout_values = {}
for tline in timeout_lines:
tline = tline.strip()
if not tline:
continue
parts = tline.split(None, 1)
if len(parts) < 2:
continue
kind, value = parts[0].strip(), parts[1].strip()
if kind in ("client", "server"):
timeout_values[kind] = value
# Special handling for idletimeout == 0: there should be no
client/server timeouts configured
if idle_value == "0":
if "client" in timeout_values or "server" in timeout_values:
print("defaults timeout client or timeout server should be
absent when idletimeout is 0")
return False
return True
# Non-zero idletimeout: both client and server timeouts must be present
if "client" not in timeout_values or "server" not in timeout_values:
print("defaults timeout client or timeout server missing")
return False
if idle_value != timeout_values["client"] or idle_value !=
timeout_values["server"]:
print("defaults timeout client or timeout server mismatch occurred")
return False
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
