Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GUACAMOLE-1126: Set a maximum of 4 auto-reconnect attempts #547

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

GUACAMOLE-1126: Set a maximum of 4 auto-reconnect attempts #547

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ba5f18
GUACAMOLE-1126: Set a maximum of 4 auto-reconnect attempts
echu2013 Jul 6, 2020
7c91cde
GUACAMOLE-1126: Add Auto-Reconnect options and translations
echu2013 Jul 6, 2020
1c030d7
GUACAMOLE-1126: Minor corrections in translations
Jul 6, 2020
9ec8b11
Merge remote-tracking branch 'remotes/upstream-apache/master' into JI…
echu2013 Jul 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions guacamole/src/main/webapp/app/client/controllers/clientController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams
var requestService = $injector.get('requestService');
var tunnelService = $injector.get('tunnelService');
var userPageService = $injector.get('userPageService');

/**
* Counter and limiter for maximum auto-reconnect attempts that we should
* automatically try to establish a connection when CLIENT_AUTO_RECONNECT or
* TUNNEL_AUTO_RECONNECT error types occur.
*
* @type Number
*/
var AUTO_RECONNECT_TRY = 0;
var AUTO_RECONNECT_MAXTRY = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, rather than setting a constant, here, I would say:

  • Add an attribute to the Connection for tracking whether auto-reconnect is disabled or not (AUTO_RECONNECT_DISABLE, for example), which defaults to having auto-reconnect enabled.
  • Add an attribute to the Connection for setting the maximum number of retries, which should default to unlimited.
  • Might even be worth making the retry interval configurable with a similar attribute, such that it defaults to Guacamole's global default, but could be set per-connection to something other than 15 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we get a profile's parameter value within clientController.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance we can get any parameter without modifying guacamole-server @necouchman ??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we definitely won't need to modify guacamole-server, as the connection attributes all stay on the guacamole-client side and are available via the REST API. However, there will be some changes required to the various Java classes in guacamole-client, along with the JDBC extensions, to support storage of the attributes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least a couple of places that would need to be changed would be:

https://github.com/apache/guacamole-client/blob/master/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java

https://github.com/apache/guacamole-client/blob/master/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of this similar in light of the VNC autoretry parameter, but maybe the goals and implementation details are slightly different. I guess that parameter does the retries transparently in the background (guacd side), while to the client it only looks like a single connection attempt. There was mention when implementing WoL support (GUACAMOLE-513) of the possibility of supporting autoretry with some configurable timers/limits across the board.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-jumper @necouchman I am sorry but I don't understand why would be need to modify guacamole's schema to achieve this..
Why can't we just use guacamole_connection_parameter which already holds all kind of profile parameters under column named parameter_name???
What I've done so far is an addition of 3 parameters (autoreconnect-disable, autoreconnect-maxretries, autoreconnect-wait-seconds) which I need to get in clientController.js somehow...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just use guacamole_connection_parameter which already holds all kind of profile parameters under column named parameter_name???

Because those aren't connection parameters. Connection parameters are a specific type of name/value pairs and are defined by the protocol plugins used by guacd. Connection parameter storage should not be used for anything that isn't a connection parameter.

There is a built-in facility for arbitrary name/value pair storage for connections (connection attributes), and this would be the appropriate vehicle for these changes. New connection attributes do not generally require schema changes, at least in the case of non-standard attributes.

Thus: connection parameters would be a non-starter, but connection attributes would be (generally) fine.

The awkwardness in this particular change comes from:

  • If the web application will have built-in handling for these attributes, they should be standard attributes, perhaps part of the API.
  • Standard attributes (those which are standardized by the extension API) really should be stored within properly-typed columns. This is already done for the existing standard connection attributes.
  • We desire to not modify the database schema, as doing so breaks backward compatibility ... but not modifying the schema in this case would mean that we deviate from established convention that is grounded in good technical reasoning (attributes that are standard really should be able to be queried directly).

So ... yes, you're not wrong that it could be done as you describe, but it really should not be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok @mike-jumper I get your point.
So, you intend to extend this guacamole_connection table columns:
image
in order to include "reconnection" parameters?
Or, do you have another idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, you intend to extend this guacamole_connection table columns:
image
in order to include "reconnection" parameters?

Not exactly. This would be the correct approach if standard attributes are going to be used here, but I strongly wish to avoid this.

Or, do you have another idea?

Yes, listed above in an earlier comment. Specifically:

  1. Modifying the handling of automatic reconnect such that extensions can easily override it. An extension could then add this functionality, and any attributes used would not need to be standardized.
  2. Avoiding the issue entirely by ensuring that automated reconnect attempts are not sufficient to prevent a session from expiring.

I would lean toward the latter (ensuring automated reconnect attempts do not prevent a session from expiring). It avoids schema changes, API changes, and any additional configuration, and instead simply leverages existing behavior to solve the issue motivating GUACAMOLE-1126.


/**
* The minimum number of pixels a drag gesture must move to result in the
Expand Down Expand Up @@ -770,7 +780,13 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams
var errorName = (status in CLIENT_ERRORS) ? status.toString(16).toUpperCase() : "DEFAULT";

// Determine whether the reconnect countdown applies
var countdown = (status in CLIENT_AUTO_RECONNECT) ? RECONNECT_COUNTDOWN : null;
if (status in CLIENT_AUTO_RECONNECT && AUTO_RECONNECT_TRY < AUTO_RECONNECT_MAXTRY ) {
var countdown = RECONNECT_COUNTDOWN;
AUTO_RECONNECT_TRY++;
} else {
var countdown = null;
AUTO_RECONNECT_TRY = 0;
}

// Show error status
notifyConnectionClosed({
Expand All @@ -792,7 +808,13 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams
var errorName = (status in TUNNEL_ERRORS) ? status.toString(16).toUpperCase() : "DEFAULT";

// Determine whether the reconnect countdown applies
var countdown = (status in TUNNEL_AUTO_RECONNECT) ? RECONNECT_COUNTDOWN : null;
if (status in TUNNEL_AUTO_RECONNECT && AUTO_RECONNECT_TRY < AUTO_RECONNECT_MAXTRY ) {
var countdown = RECONNECT_COUNTDOWN;
AUTO_RECONNECT_TRY++;
} else {
var countdown = null;
AUTO_RECONNECT_TRY = 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the countdown is already part of the notification, I would think it would be pretty easy to add the number of tries and max tries pretty easily.


// Show error status
notifyConnectionClosed({
Expand Down Expand Up @@ -828,6 +850,8 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams

// Hide status notification
guacNotification.showStatus(false);
// Reset auto-reconnect attempts counter
AUTO_RECONNECT_TRY = 0;

}

Expand Down