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

[echu2013]

This a base and non-user-configurable change to limit maximum automatic reconnection retries when "reconnectable" client/tunnel error occurs.
Further implementations to do are:
1- set default behaviour as original one
2- add user (per connection?) setting to handle this
3- Show relevant message, in countdown ??

[necouchman]

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.

[echu2013]

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

[echu2013]

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

[necouchman]

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.

[necouchman]

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

[necouchman]

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.

[echu2013]

@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...

[mike-jumper]

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.

[echu2013]

Ok @mike-jumper I get your point.
So, you intend to extend this guacamole_connection table columns:

in order to include "reconnection" parameters?
Or, do you have another idea?

[mike-jumper]

So, you intend to extend this guacamole_connection table columns:

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.

[necouchman]

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.

[VTLee]

Would it be difficult to make the default value (suggested unlimited) something that can be easily configured? I saw this MR and it certainly got me excited -- noise from dead connections is certainly an annoyance, but 4 retries might be too short if the user triggered a reboot on the machine themselves, and the machine needs longer than (4*15) seconds to come back online. Feel free to ignore this suggestion/request, just wanted to provide some extra input.

[necouchman]

@VTLee I would agree that a configurable number is better than a static number of retries.

[ceharris]

Does @echu2013 plan to continue this work?

[necouchman]

@ceharris: As we have not heard from @echu2013 in quite a while, I think it's safe to say this is orphaned work. Are you looking to take it up and move it forward?

[echu2013]

Hello, where do you prefer to store this configurable setting @necouchman ? At a connection profile level (this requires altering SQL schema?) or within guacamole.properties level?

[necouchman]

Sorry @echu2013, I missed your question quite some time ago. I would say that the there should definitely be a per-connection mechanism for storing these, in the guacamole_connection_attributes table, as Mike mentioned previously.

At this same time, it might be nice to add a global setting in guacamole.properties that could be set as a default for the entire system.

Also, looks like there are two issues with the commits - one is that a master merge commit has been added, and the other is that there are conflicts that need to be resolved.