Refactor lib/teleterm/clusters structs to make testing easier

[ravicious]

The problem

Currently, the execution path in tsh deamon looks like this, from the outermost layer:

gRPC handler → daemon.Service → clusters.Storage → clusters.Cluster → TeleportClient

• clusters.Storage is a struct that resolves URIs to clusters.Cluster structs.
• clusters.Cluster does all the dirty work, it's the only struct that has access to TeleportClient.
• daemon.Service is the struct responsible for orchestrating the work, it also holds a list of active db proxies and a mutex for it. When the tsh daemon starts, it creates one long-living instance of it.

As of now, the clusters.Cluster struct has about 18 methods defined across different files.

$ ls lib/teleterm/clusters
cluster.go           cluster_auth.go      cluster_gateways.go  cluster_leaves.go    config.go
cluster_apps.go      cluster_databases.go cluster_kubes.go     cluster_servers.go   storage.go

clusters.Cluster methods

``` $ rg "func.*Cluster\) [A-Z]" cluster_gateways.go 43:func (c *Cluster) CreateGateway(ctx context.Context, params CreateGatewayParams) (*gateway.Gateway, error) { cluster_leaves.go 41:func (c *Cluster) GetLeafClusters(ctx context.Context) ([]LeafCluster, error) { cluster.go 52:func (c *Cluster) Connected() bool { 57:func (c *Cluster) GetRoles(ctx context.Context) ([]*types.Role, error) { 77:func (c *Cluster) GetLoggedInUser() LoggedInUser { 87:func (c *Cluster) GetActualName() string { 92:func (c *Cluster) GetProxyHost() string { cluster_servers.go 39:func (c *Cluster) GetServers(ctx context.Context) ([]Server, error) { cluster_auth.go 34:func (c *Cluster) SyncAuthPreference(ctx context.Context) (*webclient.WebConfigAuthSettings, error) { 53:func (c *Cluster) Logout(ctx context.Context) error { 83:func (c *Cluster) LocalLogin(ctx context.Context, user, password, otpToken string) error { 133:func (c *Cluster) SSOLogin(ctx context.Context, providerType, providerName string) error { cluster_apps.go 39:func (c *Cluster) GetApps(ctx context.Context) ([]App, error) { cluster_kubes.go 37:func (c *Cluster) GetKubes(ctx context.Context) ([]Kube, error) { cluster_databases.go 43:func (c *Cluster) GetDatabase(ctx context.Context, dbURI string) (*Database, error) { 59:func (c *Cluster) GetDatabases(ctx context.Context) ([]Database, error) { 92:func (c *Cluster) ReissueDBCerts(ctx context.Context, user, dbName string, db types.Database) error { 139:func (c *Cluster) GetAllowedDatabaseUsers(ctx context.Context, dbURI string) ([]string, error) { ```

Because the surface area of clusters.Cluster is so big, it makes testing harder. The main problem is that when writing tests for daemon.Service, it's hard to mock out clusters.Cluster. Testing clusters.Cluster is harder too, as it's basically impossible to discern any collaborators as a Cluster method can call any other method on this struct, even if it's completely unrelated to the task at hand and could be replaced with a mock in tests.

The solution

Instead of having all methods defined on clusters.Cluster, let's break it up into individual structs.

• The individual structs will become collaborators of daemon.Service, which will need to be passed when daemon.Service is initialized.
  • Instead of having Cluster.CreateGateway, we'll have something like GatewayManager.CreateGateway (with perhaps a better name than SomethingManager).
• clusters.Cluster will become a simple value object, perhaps encapsulating client.ProfileStatus. It will be passed as a method argument to collaborators.
• client.TeleportClient is going to be passed as a method argument.
  • This means that daemon.Service tests won't have to worry about it at all, as they'll be able to mock out relevant collaborators.
  • The tests of individual collaborators will be able to mock TeleportClient by mocking individual methods, more or less like this:

    
    type mockClient struct {
    client.TeleportClient
    <state for mockClient>
    }

func (c *mockClient) GetApplicationServers() // …

---

* [Slack discussion on this topic](https://gravitational.slack.com/archives/C0DF0TPMY/p1654618039979239?thread_ts=1654618026.287449&cid=C0DF0TPMY)

[strideynet]

This looks good ! Breaking up the functionality of clusters.Cluster seems noble, especially from a point of view of unit-testing that itself as you mentioned.

I definitely like the idea of clusters.Cluster becoming a value object; I find a lot of good Go comes from roughly breaking all things down into "things that do" and "things that are". "Things that do" tend to be longer lived, and often have a single instance of them within the application, whereas "things that are" are created as needed and processed by the "things that do". It's a bit of a rough generalisation, but refactoring to have a series of Managers and this clusters.Cluster that just holds a piece of state moves towards that model... I should really write a blog post about this at some-point since this "fork" of breaking things up into "things that do" and "things that are" often comes to mind for me.