diagnostics: enable per-diagnostic parameters

Adds the ability to specify parameters for individual diagnostics on the
command line (without proliferating flags).

Addresses #14640

Author: sosiouxme

contrib/completions/bash/oadm

@@ -2065,6 +2065,8 @@ _oadm_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")

contrib/completions/bash/oc

@@ -2230,6 +2230,8 @@ _oc_adm_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")

contrib/completions/bash/openshift

@@ -2065,6 +2065,8 @@ _openshift_admin_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")
@@ -7488,6 +7490,8 @@ _openshift_cli_adm_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")

contrib/completions/zsh/oadm

@@ -2214,6 +2214,8 @@ _oadm_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")

contrib/completions/zsh/oc

@@ -2379,6 +2379,8 @@ _oc_adm_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")

contrib/completions/zsh/openshift

@@ -2214,6 +2214,8 @@ _openshift_admin_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")
@@ -7637,6 +7639,8 @@ _openshift_cli_adm_diagnostics()
     local_nonpersistent_flags+=("--images=")
     flags+=("--latest-images")
     local_nonpersistent_flags+=("--latest-images")
+    flags+=("--list-params")
+    local_nonpersistent_flags+=("--list-params")
     flags+=("--loglevel=")
     local_nonpersistent_flags+=("--loglevel=")
     flags+=("--logspec=")

pkg/diagnostics/types/diagnostic.go

@@ -24,6 +24,15 @@ type Diagnostic interface {
 	Check() DiagnosticResult
 }
 
+// ParameterizedDiagnostic is a Diagnostic that can accept arbitrary parameters specifically for it.
+// AvailableParameters is used to describe or validate the parameters given on the command line.
+// Then SetParameters is called with those parameters.
+type ParameterizedDiagnostic interface {
+	Diagnostic
+	AvailableParameters() map[string]string
+	SetParameters(map[string]string) error
+}
+
 // DiagnosticResult provides a result object for diagnostics, accumulating the messages and errors
 // that the diagnostic generates as it runs.
 type DiagnosticResult interface {

pkg/oc/admin/diagnostics/diagnostics.go

@@ -33,6 +33,11 @@ import (
 type DiagnosticsOptions struct {
 	// list of diagnostic names to limit what is run
 	RequestedDiagnostics []string
+	// parameters to specify diagnostic-specific options (name => map of params)
+	Parameters map[string][]string
+	// after building diagnostics, list their parameters and exit
+	ListParameters bool
+
 	// specify locations of host config files
 	MasterConfigLocation string
 	NodeConfigLocation   string
@@ -102,9 +107,13 @@ var (
 		* If a client config file is not found, client and cluster diagnostics
 		  are skipped.
 
-		Diagnostics may be individually run by passing diagnostic name as arguments.
+		Diagnostics may be individually run by passing diagnostic name(s) as arguments.
+
+		    %[1]s DiagnosticName
+
+		Diagnostic parameters may be given values similarly:
 
-		    %[1]s <DiagnosticName>
+		    %[1]s DiagnosticName.parameter=value
 
 		The available diagnostic names are: %[2]s.`)
 )
@@ -113,6 +122,7 @@ var (
 func NewCmdDiagnostics(name string, fullName string, out io.Writer) *cobra.Command {
 	o := &DiagnosticsOptions{
 		RequestedDiagnostics: []string{},
+		Parameters:           map[string][]string{},
 		LogOptions:           &log.LoggerOptions{Out: out},
 		ImageTemplate:        variable.NewDefaultImageTemplate(),
 		NetworkOptions:       &NetworkDiagnosticsOptions{},
@@ -141,6 +151,7 @@ func NewCmdDiagnostics(name string, fullName string, out io.Writer) *cobra.Comma
 
 	o.ClientFlags = flag.NewFlagSet("client", flag.ContinueOnError) // hide the extensive set of client flags
 	o.Factory = osclientcmd.New(o.ClientFlags)                      // that would otherwise be added to this command
+	cmd.Flags().BoolVar(&o.ListParameters, options.FlagListParametersName, false, "List parameters for specified diagnostic(s) and exit")
 	cmd.Flags().AddFlag(o.ClientFlags.Lookup(config.OpenShiftConfigFlagName))
 	cmd.Flags().AddFlag(o.ClientFlags.Lookup("context")) // TODO: find k8s constant
 	cmd.Flags().StringVar(&o.ClientClusterContext, options.FlagClusterContextName, "", "Client context to use for cluster administrator")
@@ -182,6 +193,13 @@ func (o *DiagnosticsOptions) Complete(args []string) error {
 		}
 	}
 
+	if err = o.completeNetworkOptions(); err != nil {
+		return err
+	}
+	return o.processArgs(args)
+}
+
+func (o *DiagnosticsOptions) completeNetworkOptions() error {
 	if len(o.NetworkOptions.LogDir) == 0 {
 		o.NetworkOptions.LogDir = netutil.NetworkDiagDefaultLogDir
 	} else {
@@ -208,10 +226,39 @@ func (o *DiagnosticsOptions) Complete(args []string) error {
 	if kvalidation.IsValidPortNum(o.NetworkOptions.TestPodPort) != nil {
 		return fmt.Errorf("invalid port for network diagnostic test pod. Must be in the range 1 to 65535.")
 	}
+	return nil
+}
 
-	o.RequestedDiagnostics = append(o.RequestedDiagnostics, args...)
-	if len(o.RequestedDiagnostics) == 0 {
-		o.RequestedDiagnostics = availableDiagnostics().Difference(defaultSkipDiagnostics()).List()
+// Sort out the diagnostic names and any of their parameters.
+// oc adm diagnostics                   # all standard diagnostics
+// oc adm diagnostics Foo               # only Foo
+// oc adm diagnostics Foo Foo Foo       # only Foo (and only once)
+// oc adm diagnostics Foo.bar=baz       # all standard diagnostics and Foo, with param for Foo
+// oc adm diagnostics Foo Foo.bar=baz   # only Foo with param
+// oc adm diagnostics Foo Spam.eggs=ugh # only Foo and Spam with param
+func (o *DiagnosticsOptions) processArgs(args []string) error {
+	explicitDiagnostics := map[string]bool{}  // names by themselves, not from a parameter
+	requestedDiagnostics := map[string]bool{} // names by themselves or from a parameter
+	for _, arg := range args {
+		// if arg includes a . then it is a param; otherwise just a name
+		argsplit := strings.SplitN(arg, ".", 2)
+		name := argsplit[0]
+		if len(argsplit) == 1 { // name by itself
+			explicitDiagnostics[name] = true
+			requestedDiagnostics[name] = true
+		} else { // name.parameter
+			requestedDiagnostics[name] = true // note, not counted as explicit
+			o.Parameters[name] = append(o.Parameters[name], argsplit[1])
+		}
+	}
+	if len(explicitDiagnostics) == 0 {
+		// if none were explicitly given, include all available (except default skipped)
+		for _, name := range availableDiagnostics().Difference(defaultSkipDiagnostics()).List() {
+			requestedDiagnostics[name] = true
+		}
+	}
+	for name := range requestedDiagnostics {
+		o.RequestedDiagnostics = append(o.RequestedDiagnostics, name)
 	}
 
 	return nil
@@ -320,6 +367,27 @@ func (o DiagnosticsOptions) RunDiagnostics() (bool, error, int, int) {
 		if err != nil {
 			errors = append(errors, err)
 		}
+
+		// this has to run after the diagnostics are built to know what params they take
+		if o.ListParameters {
+			paramList := []string{}
+			for _, diag := range diagnostics {
+				if pd, ok := diag.(types.ParameterizedDiagnostic); ok {
+					paramList = append(paramList, listDiagnosticParameters(pd)...)
+				}
+			}
+			if len(paramList) > 0 {
+				o.Logger.Info("CED3021", strings.Join(paramList, "\n"))
+			} else {
+				o.Logger.Info("CED3022", "There are no parameters for the selected diagnostics.")
+			}
+			os.Exit(0)
+		}
+		// log parameter errors as user errors and fail
+		if err := o.parameterizeDiagnostics(diagnostics); err != nil {
+			failed = true
+			o.Logger.Error("CED3023", err.Error())
+		}
 	}()
 
 	if failed {
@@ -332,6 +400,63 @@ func (o DiagnosticsOptions) RunDiagnostics() (bool, error, int, int) {
 	return failed, err, numWarnings, numErrors
 }
 
+func (o DiagnosticsOptions) parameterizeDiagnostics(diagnostics []types.Diagnostic) error {
+	errors := []string{}
+	for _, diag := range diagnostics {
+		if pd, ok := diag.(types.ParameterizedDiagnostic); ok {
+			available := pd.AvailableParameters()
+			paramList := o.Parameters[pd.Name()]
+			paramMap := map[string]string{}
+			diagErrors := []string{}
+			for _, param := range paramList {
+				if split := strings.SplitN(param, "=", 2); len(split) == 2 {
+					name := split[0]
+					if available[name] == "" {
+						diagErrors = append(diagErrors, fmt.Sprintf("Diagnostic %s has no parameter '%s'.", pd.Name(), name))
+						continue
+					}
+					paramMap[name] = split[1]
+				} else { // Diagnostic.foo with no =value
+					diagErrors = append(errors, fmt.Sprintf("Diagnostic parameter sets no value: %s.%s", pd.Name(), param))
+				}
+
+			}
+			if len(diagErrors) > 0 {
+				errors = append(errors, diagErrors...)
+				errors = append(errors, listDiagnosticParameters(pd)...)
+				continue
+			}
+
+			if err := pd.SetParameters(paramMap); err != nil {
+				errors = append(errors, fmt.Sprintf("Error setting %s parameters: %v", pd.Name(), err))
+				errors = append(errors, listDiagnosticParameters(pd)...)
+				continue
+			}
+		} else {
+			if o.Parameters[diag.Name()] != nil {
+				errors = append(errors, fmt.Sprintf("Diagnostic %s accepts no parameters.", diag.Name()))
+				continue
+			}
+		}
+	}
+	if len(errors) == 0 {
+		return nil
+	}
+	return fmt.Errorf("Error with diagnostic parameters:\n%s", strings.Join(errors, "\n"))
+}
+
+func listDiagnosticParameters(pd types.ParameterizedDiagnostic) []string {
+	params := pd.AvailableParameters()
+	paramlist := []string{fmt.Sprintf("Valid parameters for %s are:", pd.Name())}
+	if params == nil {
+		return append(paramlist, "  None")
+	}
+	for _, key := range sets.StringKeySet(params).List() {
+		paramlist = append(paramlist, fmt.Sprintf("  %s: %s", key, params[key]))
+	}
+	return paramlist
+}
+
 // Run performs the actual execution of diagnostics once they're built.
 func (o DiagnosticsOptions) Run(diagnostics []types.Diagnostic) (bool, error, int, int) {
 	warnCount := 0

pkg/oc/admin/diagnostics/options/flaginfo.go

@@ -53,6 +53,7 @@ const (
 	FlagImageTemplateName       = "images"
 	FlagLatestImageName         = "latest-images"
 	FlagPreventModificationName = "prevent-modification"
+	FlagListParametersName      = "list-params"
 
 	FlagNetworkDiagLogDir          = "network-logdir"
 	FlagNetworkDiagPodImage        = "network-pod-image"