cmd: improve command usage message by grouping related flags

I wanted to create multiple commits but it turns out to be complicated.
Let's summarize what this commit does:

- Add `cmd/common/template` where a `Usage()` can be used by commands to
  generate a custom template based on the provided `pflag.FlagSets`.
  This allows grouping related flags when displaying help/usage of a
  command or subcommand.
- With the help of `Usage()` described above, the custom usage function
  for the `observer` subcommand can be dropped. It was quite hacky and
  easy to get out of sync by adding a flag to the `observe` command and
  forgetting to update the usage formatting function to indicate to
  which flag group the newly added flag belongs. The `observe` command
  now has several defines several `pflag.FlagSet` for each flag group
  (selectors, filters, formatting, others).
- Global flags (ie flags that are persisted from the root command) have
  been split into to `pflag.FlagSet` in `cmd/common/config`:
  `GlobalFlags` and `ServerFlags`. The former contains all global flags
  (ie: debug and config) and the latter all flags related to connecting
  to a Hubble server instance.
- The global flags section is always printed for each command or
  subcommand as it apply to any command. However, the new `ServerFlags`
  flag set needs to be added to the help message by any command or
  subcommand that makes use of it. For the user, this is really
  convenient as these flags appear in the help message of a command only
  when relevant instead of always (eg: the `completion` command does not
  use the server flags and this does not display help for these when
  running `hubble completion -h`). The `ServerFlags` are nonetheless
  global in the sense that they are persistent flags added to the root
  command. This is done for 2 reasons: not breaking the existing
  behavior and allowing users to use these flags in any order (e.g.
  `hubble --server foo observe` also works).
- Instead of using hardcoded strings for every configuration key in
  every command where they need to be used, constants have been defined
  in `cmd/common/config` and shall be used instead.
- Defaults for the configuration directory, fallback configuration
  directory and configuration file have been moved to `pkg/defaults`.

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

cmd/common/config/flags.go

@@ -0,0 +1,95 @@
+// Copyright 2020 Authors of Hubble
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	"github.com/cilium/hubble/pkg/defaults"
+	"github.com/spf13/pflag"
+)
+
+// Keys can be used to retrieve values from GlobalFlags and ServerFlags (e.g.
+// when bound to a viper instance).
+const (
+	// GlobalFlags keys.
+	KeyConfig = "config"
+	KeyDebug  = "debug"
+
+	// ServerFlags keys.
+	KeyServer            = "server"
+	KeyTLS               = "tls"
+	KeyTLSAllowInsecure  = "tls-allow-insecure"
+	KeyTLSCACertFiles    = "tls-ca-cert-files"
+	KeyTLSClientCertFile = "tls-client-cert-file"
+	KeyTLSClientKeyFile  = "tls-client-key-file"
+	KeyTLSServerName     = "tls-server-name"
+	KeyTimeout           = "timeout"
+)
+
+// GlobalFlags are flags that apply to any command.
+var GlobalFlags = pflag.NewFlagSet("global", pflag.ContinueOnError)
+
+// ServerFlags are flags that configure how to connect to a Hubble server.
+var ServerFlags = pflag.NewFlagSet("server", pflag.ContinueOnError)
+
+func init() {
+	initGlobalFlags()
+	initServerFlags()
+}
+
+func initGlobalFlags() {
+	GlobalFlags.String(KeyConfig, defaults.ConfigFile, "Optional config file")
+	GlobalFlags.BoolP(KeyDebug, "D", false, "Enable debug messages")
+}
+
+func initServerFlags() {
+	ServerFlags.String(KeyServer, defaults.GetSocketPath(), "Address of a Hubble server")
+	ServerFlags.Duration(KeyTimeout, defaults.DialTimeout, "Hubble server dialing timeout")
+	ServerFlags.Bool(
+		KeyTLS,
+		false,
+		"Specify that TLS must be used when establishing a connection to a Hubble server.\r\n"+
+			"By default, TLS is only enabled if the server address starts with 'tls://'.",
+	)
+	ServerFlags.Bool(
+		KeyTLSAllowInsecure,
+		false,
+		"Allows the client to skip verifying the server's certificate chain and host name.\r\n"+
+			"This option is NOT recommended as, in this mode, TLS is susceptible to machine-in-the-middle attacks.\r\n"+
+			"See also the 'tls-server-name' option which allows setting the server name.",
+	)
+	ServerFlags.StringSlice(
+		KeyTLSCACertFiles,
+		nil,
+		"Paths to custom Certificate Authority (CA) certificate files."+
+			"The files must contain PEM encoded data.",
+	)
+	ServerFlags.String(
+		KeyTLSClientCertFile,
+		"",
+		"Path to the public key file for the client certificate to connect to a Hubble server (implies TLS).\r\n"+
+			"The file must contain PEM encoded data.",
+	)
+	ServerFlags.String(
+		KeyTLSClientKeyFile,
+		"",
+		"Path to the private key file for the client certificate to connect a Hubble server (implies TLS).\r\n"+
+			"The file must contain PEM encoded data.",
+	)
+	ServerFlags.String(
+		KeyTLSServerName,
+		"",
+		"Specify a server name to verify the hostname on the returned certificate (eg: 'instance.hubble-relay.cilium.io').",
+	)
+}

cmd/common/config/viper.go

@@ -0,0 +1,46 @@
+// Copyright 2020 Authors of Hubble
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	"strings"
+
+	"github.com/cilium/hubble/pkg/defaults"
+	"github.com/spf13/viper"
+)
+
+// NewViper creates a new viper instance configured for Hubble.
+func NewViper() *viper.Viper {
+	vp := viper.New()
+
+	// read config from a file
+	vp.SetConfigName("config") // name of config file (without extension)
+	vp.SetConfigType("yaml")   // useful if the given config file does not have the extension in the name
+	vp.AddConfigPath(".")      // look for a config in the working directory first
+	if defaults.ConfigDir != "" {
+		vp.AddConfigPath(defaults.ConfigDir)
+	}
+	if defaults.ConfigDirFallback != "" {
+		vp.AddConfigPath(defaults.ConfigDirFallback)
+	}
+
+	// read config from environment variables
+	vp.SetEnvPrefix("hubble") // env var must start with HUBBLE_
+	// replace - by _ for environment variable names
+	// (eg: the env var for tls-server-name is TLS_SERVER_NAME)
+	vp.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
+	vp.AutomaticEnv() // read in environment variables that match
+	return vp
+}

cmd/common/template/usage.go

@@ -0,0 +1,63 @@
+// Copyright 2020 Authors of Hubble
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package template
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/cilium/hubble/cmd/common/config"
+	"github.com/spf13/pflag"
+)
+
+const (
+	header = `Usage:{{if .Runnable}}
+  {{.UseLine}}{{end}}{{if .HasAvailableSubCommands}}
+  {{.CommandPath}} [command]{{end}}{{if gt (len .Aliases) 0}}
+
+Aliases:
+  {{.NameAndAliases}}{{end}}{{if .HasExample}}
+
+Examples:
+{{.Example}}{{end}}{{if .HasAvailableSubCommands}}
+
+Available Commands:{{range .Commands}}{{if (or .IsAvailableCommand (eq .Name "help"))}}
+  {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}
+
+`
+	footer = `{{- if .HasHelpSubCommands}}Additional help topics:{{range .Commands}}{{if .IsAdditionalHelpTopicCommand}}
+  {{rpad .CommandPath .CommandPathPadding}} {{.Short}}{{end}}{{end}}{{end}}{{if .HasAvailableSubCommands}}
+
+Use "{{.CommandPath}} [command] --help" for more information about a command.{{end}}
+`
+)
+
+// Usage returns a usage template string with the given sets of flags.
+// Each flag set is separated in a new flags section with the flagset name as
+// section title.
+// When used with cobra commands, the resulting template may be passed as a
+// parameter to command.SetUsageTemplate().
+func Usage(flagSets ...*pflag.FlagSet) string {
+	var b strings.Builder
+	b.WriteString(header)
+	for _, fs := range append(flagSets, config.GlobalFlags) {
+		fmt.Fprintf(&b, "%s Flags:\n", strings.Title(fs.Name()))
+		fmt.Fprintln(&b, fs.FlagUsages())
+	}
+	// treat the special --help flag separately
+	b.WriteString("Get help:\n  -h, --help	Help for any command or subcommand")
+	b.WriteString(footer)
+	return b.String()
+}

cmd/common/template/usage_test.go

@@ -0,0 +1,86 @@
+// Copyright 2020 Authors of Hubble
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package template
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/cilium/hubble/pkg/defaults"
+	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/require"
+)
+
+func TestUsage(t *testing.T) {
+	cmd := &cobra.Command{
+		Use:     "cmd",
+		Aliases: []string{"and", "conquer"},
+		Example: "I'm afraid this is not a good an example.",
+		Short:   "Do foo with bar",
+		Long:    "Do foo with bar and pay attention to baz and more.",
+		Run: func(_ *cobra.Command, _ []string) {
+			// noop
+		},
+	}
+	flags := pflag.NewFlagSet("bar", pflag.ContinueOnError)
+	flags.String("baz", "", "baz usage")
+	cmd.Flags().AddFlagSet(flags)
+	cmd.SetUsageTemplate(Usage(flags))
+
+	subCmd := &cobra.Command{
+		Use: "subcmd",
+		Run: func(_ *cobra.Command, _ []string) {
+			// noop
+		},
+	}
+	cmd.AddCommand(subCmd)
+
+	var out strings.Builder
+	cmd.SetOut(&out)
+	cmd.Usage()
+
+	var expect strings.Builder
+	expect.WriteString(`Usage:
+  cmd [flags]
+  cmd [command]
+
+Aliases:
+  cmd, and, conquer
+
+Examples:
+I'm afraid this is not a good an example.
+
+Available Commands:
+  subcmd      
+
+Bar Flags:
+      --baz string   baz usage
+
+Global Flags:
+      --config string   Optional config file (default "`)
+
+	expect.WriteString(defaults.ConfigFile)
+	expect.WriteString(`")
+  -D, --debug           Enable debug messages
+
+Get help:
+  -h, --help	Help for any command or subcommand
+
+Use "cmd [command] --help" for more information about a command.
+`)
+
+	require.Equal(t, expect.String(), out.String())
+}

cmd/config/config.go

@@ -52,6 +52,7 @@ HUBBLE_TLS_ALLOW_INSECURE and so on.`,
 			return cmd.Help()
 		},
 	}
+
 	configCmd.AddCommand(
 		newGetCommand(vp),
 		newResetCommand(vp),

cmd/node/list.go

@@ -25,8 +25,11 @@ import (
 
 	observerpb "github.com/cilium/cilium/api/v1/observer"
 	relaypb "github.com/cilium/cilium/api/v1/relay"
+	"github.com/cilium/hubble/cmd/common/config"
 	"github.com/cilium/hubble/cmd/common/conn"
+	"github.com/cilium/hubble/cmd/common/template"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 	"github.com/spf13/viper"
 	"google.golang.org/grpc"
 )
@@ -38,7 +41,7 @@ var listOpts struct {
 }
 
 func newListCommand(vp *viper.Viper) *cobra.Command {
-	cmd := &cobra.Command{
+	listCmd := &cobra.Command{
 		Use:   "list",
 		Short: "List Hubble nodes",
 		RunE: func(cmd *cobra.Command, _ []string) error {
@@ -53,20 +56,24 @@ func newListCommand(vp *viper.Viper) *cobra.Command {
 		},
 	}
 
-	cmd.Flags().StringVarP(
+	formattingFlags := pflag.NewFlagSet("Formatting", pflag.ContinueOnError)
+	formattingFlags.StringVarP(
 		&listOpts.output, "output", "o", "table",
 		`Specify the output format, one of:
  json:     JSON encoding
  table:    Tab-aligned columns
  wide:     Tab-aligned columns with additional information`)
-	cmd.RegisterFlagCompletionFunc("output", func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) {
+	listCmd.RegisterFlagCompletionFunc("output", func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) {
 		return []string{
 			"json",
 			"table",
 			"wide",
 		}, cobra.ShellCompDirectiveDefault
 	})
-	return cmd
+	listCmd.Flags().AddFlagSet(formattingFlags)
+
+	listCmd.SetUsageTemplate(template.Usage(formattingFlags, config.ServerFlags))
+	return listCmd
 }
 
 func runList(ctx context.Context, cmd *cobra.Command, conn *grpc.ClientConn) error {

cmd/node/node.go

@@ -15,20 +15,27 @@
 package node
 
 import (
+	"github.com/cilium/hubble/cmd/common/config"
+	"github.com/cilium/hubble/cmd/common/template"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 )
 
 // New creates a new hidden peer command.
 func New(vp *viper.Viper) *cobra.Command {
-	cmd := &cobra.Command{
+	nodeCmd := &cobra.Command{
 		Use:     "nodes",
 		Aliases: []string{"node"},
 		Short:   "Get information about Hubble nodes",
 		Long:    `Get information about Hubble nodes.`,
 	}
-	cmd.AddCommand(
+
+	// add config.ServerFlags to the help template as these flags are used by
+	// this command
+	nodeCmd.SetUsageTemplate(template.Usage(config.ServerFlags))
+
+	nodeCmd.AddCommand(
 		newListCommand(vp),
 	)
-	return cmd
+	return nodeCmd
 }