Extend tsh clusters output with "Cluster Type" and "Selected" columns

[andrejtokarcik]

Fixes #4965, implementing the output format proposed therein. The additional columns are printed by default, it didn't seem necessary to introduce a -v flag.

[r0mant]

Curious what others think, but I wonder why you opted for a more complicated approach with splitting the proxy GetAllClusters into 2 methods, rather than just keeping the API as-is and comparing each cluster name with the name of the root cluster.

Something like:

rootClusterName, err := proxyClient.RootClusterName()
// handle err

sort.Slice(clusters, func(i, j int) bool { clusters[i].GetName() == rootClusterName })
for _, cluster := range clusters {
  kind := "root"
  if cluster.GetName() != rootClusterName {
    kind = "leaf"
  }
  t.AddRow(...)
}

Think the implementation would be a bit simpler/cleaner and proxy client interface wouldn't need to change.

[awly]

I prefer the GetLeafClusters over GetAllClusters, so that we don't have to pretend like the local (root) cluster is a services.RemoteCluster.
But maybe remove GetRootCluster, and call proxyClient.RootClusterName directly in tsh.

Wrapping root cluster in a services.RemoteCluster feels like we're fighting the type system for the sake of some code de-duplication in tsh.

[andrejtokarcik]

Curious what others think, but I wonder why you opted for a more complicated approach with splitting the proxy GetAllClusters into 2 methods.

My thought process was basically the following:

1. The notions of root/leaf clusters seem to be significant enough to be encoded in a more general API instead of relying on ad-hoc name comparison.

2. I realize that it's probably a given but it wasn't entirely clear that cluster names are unique. Mere cluster name comparison could therefore possibly yield multiple clusters marked as "root". My alternative seems to prevent this possibility quite clearly.

3. Your code snippet also demonstrates why we should prefer to encode as much information as possible in types and code structure: the old GetAllClusters seemed to intend an invariant that the root cluster must be returned as the first entry. However, since this is not exposed/guaranteed in any way, your code performs this sorting "defensively" once again.

I originally contemplated an option to retain GetAllClusters but change its return type into [](struct {services.RemoteCluster; isRoot bool}) or some such. But then, I think it's a bad practice to rely on boolean flags too much - their meaning is often difficult to trace and they tend to be difficult to extend to multiple choices/values.

[andrejtokarcik]

@awly Note that GetRootCluster doesn't wrap only the cluster's name but also the connection status (as well as the last heartbeat but that doesn't seem to come into play at the moment). Once again, I think it's better to provide this info uniformly to any consumer, even though it might be just the tsh clusters handler at this point.

To avoid the impression that we're fighting the type system we could perhaps just abstract away those methods of RemoteCluster that are relevant for the root cluster into a different struct/interface Cluster and then embed this into RemoteCluster.

[awly]

The connection status and last heartbeat are set to constant values and we're not likely to change that (if you're talking to the root cluster, it's obviously online and last seen at the time of request).
Those are only set to shoehorn the root cluster into the services.RemtoeCluster type.