From dc50a9bef2aed630d72dd12fa0d70d29996ec938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Mon, 16 Dec 2024 12:37:50 +0100 Subject: [PATCH] Refactor how tsh lists apps as text (#49992) This way it's going to be easier to add a column with target ports, which should be displayed only if the list includes multi-port apps. Conflicts: tool/tsh/common/app_test.go tool/tsh/common/tsh.go --- tool/tsh/common/app_test.go | 150 +++++++++++++++++++++++++++++ tool/tsh/common/tsh.go | 186 ++++++++++++++++++++++++++---------- 2 files changed, 284 insertions(+), 52 deletions(-) diff --git a/tool/tsh/common/app_test.go b/tool/tsh/common/app_test.go index b1937912d4fa2..39fe28784e513 100644 --- a/tool/tsh/common/app_test.go +++ b/tool/tsh/common/app_test.go @@ -24,9 +24,11 @@ import ( "crypto/tls" "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "net/http/httputil" + "strings" "testing" "time" @@ -41,6 +43,7 @@ import ( "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/service" "github.com/gravitational/teleport/lib/service/servicecfg" + "github.com/gravitational/teleport/lib/tlsca" ) func startDummyHTTPServer(t *testing.T, name string) string { @@ -484,3 +487,150 @@ uri: https://test-tp.teleport:8443 }) } } + +func TestWriteAppTable(t *testing.T) { + defaultAppListings := []appListing{ + appListing{ + Proxy: "example.com", + Cluster: "foo-cluster", + App: mustMakeNewAppV3(t, types.Metadata{Name: "root-app"}, types.AppSpecV3{ + // Short URLs, because in tests the width of the term is just 80 characters and the public + // address column gets truncated very early. + PublicAddr: "https://root-app.example.com", + URI: "http://localhost:8080", + }), + }, + appListing{ + Proxy: "example.com", + Cluster: "bar-cluster", + App: mustMakeNewAppV3(t, types.Metadata{Name: "leaf-app"}, types.AppSpecV3{ + PublicAddr: "https://leaf-app.example.com", + URI: "http://localhost:4242", + }), + }, + } + + tests := []struct { + name string + config appTableConfig + appListings []appListing + wantHeaders []string + wantNoHeaders []string + wantValues []string + wantNoValues []string + }{ + { + name: "regular list", + config: appTableConfig{ + active: []tlsca.RouteToApp{}, + verbose: false, + listAll: false, + }, + appListings: defaultAppListings, + wantHeaders: []string{"Application", "Public Address"}, + // Public addresses are expected to be truncated when verbose mode is off. + wantValues: []string{"https://root-app...", "https://leaf-app...", "root-app", "leaf-app"}, + wantNoHeaders: []string{"URI", "Proxy", "Cluster"}, + wantNoValues: []string{"http://localhost:8080", "foo-cluster", "bar-cluster"}, + }, + { + name: "regular list with active app", + config: appTableConfig{ + active: []tlsca.RouteToApp{ + tlsca.RouteToApp{Name: "root-app"}, + }, + verbose: false, + listAll: false, + }, + appListings: defaultAppListings, + wantHeaders: []string{"Application"}, + wantValues: []string{"> root-app", "leaf-app"}, + }, + { + name: "regular list with no apps", + config: appTableConfig{ + active: []tlsca.RouteToApp{}, + verbose: false, + listAll: false, + }, + appListings: []appListing{}, + wantHeaders: []string{"Application", "Public Address"}, + }, + { + name: "verbose", + config: appTableConfig{ + active: []tlsca.RouteToApp{}, + verbose: true, + listAll: false, + }, + appListings: defaultAppListings, + wantHeaders: []string{"URI", "Application", "Public Address"}, + wantValues: []string{"http://localhost:8080", "http://localhost:4242", + "https://root-app.example.com", "https://leaf-app.example.com", "root-app", "leaf-app"}, + wantNoHeaders: []string{"Proxy", "Cluster"}, + wantNoValues: []string{"foo-cluster", "bar-cluster"}, + }, + { + name: "list all", + config: appTableConfig{ + active: []tlsca.RouteToApp{}, + verbose: false, + listAll: true, + }, + appListings: defaultAppListings, + wantHeaders: []string{"Proxy", "Cluster", "Application", "Public Address"}, + // Public addresses are expected to be truncated when verbose mode is off. + wantValues: []string{"foo-cluste...", "bar-cluste...", "example.co...", "https://ro...", "https://le...", "root-app", "leaf-app"}, + wantNoHeaders: []string{"URI"}, + wantNoValues: []string{"http://localhost:8080"}, + }, + { + name: "verbose and list all", + config: appTableConfig{ + active: []tlsca.RouteToApp{}, + verbose: true, + listAll: true, + }, + appListings: defaultAppListings, + wantHeaders: []string{"Proxy", "Cluster", "URI", "Application", "Public Address"}, + wantValues: []string{"foo-cluster", "bar-cluster", "http://localhost:8080", "http://localhost:4242", + "https://root-app.example.com", "https://leaf-app.example.com", "root-app", "leaf-app"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var b bytes.Buffer + w := io.Writer(&b) + + err := writeAppTable(w, test.appListings, test.config) + require.NoError(t, err) + + lines := strings.SplitN(b.String(), "\n", 3) + headers := lines[0] + // The second line contains header separators ("------"), that's why it's skipped here. + values := lines[2] + + for _, wantHeader := range test.wantHeaders { + assert.Contains(t, headers, wantHeader) + } + for _, wantNoHeader := range test.wantNoHeaders { + assert.NotContains(t, headers, wantNoHeader) + } + + for _, wantValue := range test.wantValues { + assert.Contains(t, values, wantValue) + } + for _, wantNoValue := range test.wantNoValues { + assert.NotContains(t, values, wantNoValue) + } + }) + } +} + +func mustMakeNewAppV3(t *testing.T, meta types.Metadata, spec types.AppSpecV3) *types.AppV3 { + t.Helper() + app, err := types.NewAppV3(meta, spec) + require.NoError(t, err) + return app +} diff --git a/tool/tsh/common/tsh.go b/tool/tsh/common/tsh.go index 9d091c188e3c6..d22e15ed554b3 100644 --- a/tool/tsh/common/tsh.go +++ b/tool/tsh/common/tsh.go @@ -2774,7 +2774,18 @@ func showApps(apps []types.Application, active []tlsca.RouteToApp, format string format = strings.ToLower(format) switch format { case teleport.Text, "": - showAppsAsText(apps, active, verbose) + appListings := make([]appListing, 0, len(apps)) + for _, app := range apps { + appListings = append(appListings, appListing{App: app}) + } + + if err := writeAppTable(os.Stdout, appListings, appTableConfig{ + listAll: false, // showApps lists apps from a single cluster. + active: active, + verbose: verbose, + }); err != nil { + return trace.Wrap(err) + } case teleport.JSON, teleport.YAML: out, err := serializeApps(apps, format) if err != nil { @@ -2801,46 +2812,118 @@ func serializeApps(apps []types.Application, format string) (string, error) { return string(out), trace.Wrap(err) } -func getAppRow(proxy, cluster string, app types.Application, active []tlsca.RouteToApp, verbose bool) []string { - var row []string - if proxy != "" && cluster != "" { - row = append(row, proxy, cluster) - } +type appTableConfig struct { + // active is a list of apps for which the user retrieved a short-lived cert with tsh app login. + active []tlsca.RouteToApp + // verbose makes the table show extra columns. + verbose bool + // listAll makes the table render two extra columns: Proxy and Cluster. + listAll bool +} - name := app.GetName() - for _, a := range active { - if name == a.Name { - name = fmt.Sprintf("> %v", name) - break - } - } +func writeAppTable(w io.Writer, appListings []appListing, config appTableConfig) error { + getName := func(app types.Application) string { + isActive := slices.ContainsFunc(config.active, func(route tlsca.RouteToApp) bool { + // TODO(ravicious): This should be based on name _and_ route.ClusterName, so that we don't + // incorrectly show multiple apps with the same name but from different clusters as active. + // However, to do this we'd need to double check if route.ClusterName always matches + // appListing.Cluster (and also fill out that field in showApps). + return route.Name == app.GetName() + }) - labels := common.FormatLabels(app.GetAllLabels(), verbose) - if verbose { - row = append(row, name, app.GetDescription(), app.GetProtocol(), app.GetPublicAddr(), app.GetURI(), labels) - } else { - row = append(row, name, app.GetDescription(), app.GetProtocol(), app.GetPublicAddr(), labels) - } + if isActive { + return fmt.Sprintf("> %s", app.GetName()) + } + + return app.GetName() + } + getLabels := func(app types.Application) string { + return common.FormatLabels(app.GetAllLabels(), config.verbose) + } + + const labelsColumn = "Labels" + allColumns := []appTableColumn{ + appTableColumn{ + name: "Proxy", + getFromListing: appListing.GetProxy, + hide: !config.listAll, + }, + appTableColumn{ + name: "Cluster", + getFromListing: appListing.GetCluster, + hide: !config.listAll, + }, + appTableColumn{ + name: "Application", + get: getName, + }, + appTableColumn{ + name: "Description", + get: types.Application.GetDescription, + }, + appTableColumn{ + name: "Type", + get: types.Application.GetProtocol, + }, + appTableColumn{ + name: "Public Address", + get: types.Application.GetPublicAddr, + }, + appTableColumn{ + name: "URI", + get: types.Application.GetURI, + hide: !config.verbose, + }, + appTableColumn{ + name: labelsColumn, + get: getLabels, + }, + } + columns := slices.DeleteFunc(allColumns, func(column appTableColumn) bool { return column.hide }) + + headers := make([]string, 0, len(columns)) + for _, column := range columns { + headers = append(headers, column.name) + } + + rows := make([][]string, 0, len(appListings)) + for _, appListing := range appListings { + appRow := make([]string, 0, len(columns)) + + for _, column := range columns { + var content string + switch { + case column.get != nil: + content = column.get(appListing.App) + case column.getFromListing != nil: + content = column.getFromListing(appListing) + } - return row -} + appRow = append(appRow, content) + } -func showAppsAsText(apps []types.Application, active []tlsca.RouteToApp, verbose bool) { - var rows [][]string - for _, app := range apps { - rows = append(rows, getAppRow("", "", app, active, verbose)) + rows = append(rows, appRow) } - // In verbose mode, print everything on a single line and include host UUID. + + // In verbose mode, print everything on a single line. // In normal mode, chunk the labels, print two per line and allow multiple - // lines per node. + // lines per app. var t asciitable.Table - if verbose { - t = asciitable.MakeTable([]string{"Application", "Description", "Type", "Public Address", "URI", "Labels"}, rows...) + if config.verbose { + t = asciitable.MakeTable(headers, rows...) } else { - t = asciitable.MakeTableWithTruncatedColumn( - []string{"Application", "Description", "Type", "Public Address", "Labels"}, rows, "Labels") + t = asciitable.MakeTableWithTruncatedColumn(headers, rows, labelsColumn) } - fmt.Println(t.AsBuffer().String()) + + _, err := fmt.Fprintln(w, t.AsBuffer().String()) + return trace.Wrap(err) +} + +type appTableColumn struct { + name string + get func(app types.Application) string + getFromListing func(listing appListing) string + hide bool } func showDatabases(cf *CLIConf, databases []types.Database, active []tlsca.RouteToDatabase, accessChecker services.AccessChecker) error { @@ -5130,6 +5213,14 @@ type appListing struct { App types.Application `json:"app"` } +func (al appListing) GetProxy() string { + return al.Proxy +} + +func (al appListing) GetCluster() string { + return al.Cluster +} + type appListings []appListing func (l appListings) Len() int { @@ -5186,37 +5277,28 @@ func listAppsAllClusters(cf *CLIConf) error { format := strings.ToLower(cf.Format) switch format { case teleport.Text, "": - printAppsWithClusters(listings, active, cf.Verbose) + if err := writeAppTable(cf.Stdout(), listings, appTableConfig{ + listAll: true, + active: active, + verbose: cf.Verbose, + }); err != nil { + return trace.Wrap(err) + } + case teleport.JSON, teleport.YAML: out, err := serializeAppsWithClusters(listings, format) if err != nil { return trace.Wrap(err) } - fmt.Println(out) + if _, err := fmt.Fprintln(cf.Stdout(), out); err != nil { + return trace.Wrap(err) + } default: return trace.BadParameter("unsupported format %q", format) } return nil } -func printAppsWithClusters(apps []appListing, active []tlsca.RouteToApp, verbose bool) { - var rows [][]string - for _, app := range apps { - rows = append(rows, getAppRow(app.Proxy, app.Cluster, app.App, active, verbose)) - } - // In verbose mode, print everything on a single line and include host UUID. - // In normal mode, chunk the labels, print two per line and allow multiple - // lines per node. - var t asciitable.Table - if verbose { - t = asciitable.MakeTable([]string{"Proxy", "Cluster", "Application", "Description", "Type", "Public Address", "URI", "Labels"}, rows...) - } else { - t = asciitable.MakeTableWithTruncatedColumn( - []string{"Proxy", "Cluster", "Application", "Description", "Type", "Public Address", "Labels"}, rows, "Labels") - } - fmt.Println(t.AsBuffer().String()) -} - func serializeAppsWithClusters(apps []appListing, format string) (string, error) { var out []byte var err error