`linkerd check`: handle warnings and remove extra newlines (#7379)

When running `linkerd check -o short` there can still be formatting issues:
- When there are no core warnings but there are extension warnings, a newline is printed at the start of the output
- When there are warnings, there is no newline printed between the warnings and the result

Below you can see the extra newline (before `Linkerd extensions checks`) and the lack of a newline on line before `Status check results ...`.

Old:

```shell
$ linkerd check -o short

Linkerd extensions checks
=========================

linkerd-viz
-----------
...
Status check results are √
```

New:

```shell
$ linkerd check -o short
Linkerd extensions checks
=========================

linkerd-viz
-----------
...

Status check results are √
```

---

This fixes the above issues by moving the newline printing to the end of a category—which right now is Core and Extension.

If there is no output for either, then no newline is printed. This results in no stray newlines when running in short output and there are no warnings.

```shell
$ linkerd check -o short
Status check results are √
```

If there is output for a category, then the category handles the newline printing itself meaning we don't need to track if a newline needs to be printed _before_ a category or _before_ the results.

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

cli/cmd/check.go

@@ -213,11 +213,11 @@ func configureAndRunChecks(cmd *cobra.Command, wout io.Writer, werr io.Writer, s
 	})
 
 	if options.output == tableOutput {
-		healthcheck.PrintChecksHeader(wout, true)
+		healthcheck.PrintChecksHeader(wout, healthcheck.CoreHeader)
 	}
-	success := healthcheck.RunChecks(wout, werr, hc, options.output)
+	success, warning := healthcheck.RunChecks(wout, werr, hc, options.output)
 
-	extensionSuccess, err := runExtensionChecks(cmd, wout, werr, options)
+	extensionSuccess, extensionWarning, err := runExtensionChecks(cmd, wout, werr, options)
 	if err != nil {
 		err = fmt.Errorf("failed to run extensions checks: %s", err)
 		fmt.Fprintln(werr, err)
@@ -225,7 +225,8 @@ func configureAndRunChecks(cmd *cobra.Command, wout io.Writer, werr io.Writer, s
 	}
 
 	totalSuccess := success && extensionSuccess
-	healthcheck.PrintChecksResult(wout, options.output, totalSuccess)
+	totalWarning := warning || extensionWarning
+	healthcheck.PrintChecksResult(wout, options.output, totalSuccess, totalWarning)
 
 	if !totalSuccess {
 		os.Exit(1)
@@ -234,21 +235,21 @@ func configureAndRunChecks(cmd *cobra.Command, wout io.Writer, werr io.Writer, s
 	return nil
 }
 
-func runExtensionChecks(cmd *cobra.Command, wout io.Writer, werr io.Writer, opts *checkOptions) (bool, error) {
+func runExtensionChecks(cmd *cobra.Command, wout io.Writer, werr io.Writer, opts *checkOptions) (bool, bool, error) {
 	kubeAPI, err := k8s.NewAPI(kubeconfigPath, kubeContext, impersonate, impersonateGroup, 0)
 	if err != nil {
-		return false, err
+		return false, false, err
 	}
 
 	namespaces, err := kubeAPI.GetAllNamespacesWithExtensionLabel(cmd.Context())
 	if err != nil {
-		return false, err
+		return false, false, err
 	}
 
 	success := true
 	// no extensions to check
 	if len(namespaces) == 0 {
-		return success, nil
+		return success, false, nil
 	}
 
 	nsLabels := make([]string, len(namespaces))
@@ -256,8 +257,8 @@ func runExtensionChecks(cmd *cobra.Command, wout io.Writer, werr io.Writer, opts
 		nsLabels[i] = ns.Labels[k8s.LinkerdExtensionLabel]
 	}
 
-	extensionSuccess := healthcheck.RunExtensionsChecks(wout, werr, nsLabels, getExtensionCheckFlags(cmd.Flags()), opts.output)
-	return extensionSuccess, nil
+	extensionSuccess, extensionWarning := healthcheck.RunExtensionsChecks(wout, werr, nsLabels, getExtensionCheckFlags(cmd.Flags()), opts.output)
+	return extensionSuccess, extensionWarning, nil
 }
 
 func getExtensionCheckFlags(lf *pflag.FlagSet) []string {

cli/cmd/testdata/check_output.golden

@@ -4,3 +4,4 @@ category
 × check2
     This should contain instructions for fail
     see https://linkerd.io/2/checks/#hint-anchor for hints
+

jaeger/cmd/check.go

@@ -240,8 +240,8 @@ func configureAndRunChecks(wout io.Writer, werr io.Writer, options *checkOptions
 
 	hc.AppendCategories(jaegerCategory(hc))
 
-	success := healthcheck.RunChecks(wout, werr, hc, options.output)
-	healthcheck.PrintChecksResult(wout, options.output, success)
+	success, warning := healthcheck.RunChecks(wout, werr, hc, options.output)
+	healthcheck.PrintChecksResult(wout, options.output, success, warning)
 
 	if !success {
 		os.Exit(1)

multicluster/cmd/check.go

@@ -156,8 +156,8 @@ func configureAndRunChecks(wout io.Writer, werr io.Writer, options *checkOptions
 	hc := newHealthChecker(linkerdHC)
 	category := multiclusterCategory(hc)
 	hc.AppendCategories(category)
-	success := healthcheck.RunChecks(wout, werr, hc, options.output)
-	healthcheck.PrintChecksResult(wout, options.output, success)
+	success, warning := healthcheck.RunChecks(wout, werr, hc, options.output)
+	healthcheck.PrintChecksResult(wout, options.output, success, warning)
 	if !success {
 		os.Exit(1)
 	}

pkg/healthcheck/healthcheck.go

@@ -422,7 +422,7 @@ type HealthChecker struct {
 
 // Runner is implemented by any health-checkers that can be triggered with RunChecks()
 type Runner interface {
-	RunChecks(observer CheckObserver) bool
+	RunChecks(observer CheckObserver) (bool, bool)
 }
 
 // NewHealthChecker returns an initialized HealthChecker
@@ -1586,8 +1586,9 @@ func (hc *HealthChecker) checkMinReplicasAvailable(ctx context.Context) error {
 // remaining checks are skipped. If at least one check fails, RunChecks returns
 // false; if all checks passed, RunChecks returns true.  Checks which are
 // designated as warnings will not cause RunCheck to return false, however.
-func (hc *HealthChecker) RunChecks(observer CheckObserver) bool {
+func (hc *HealthChecker) RunChecks(observer CheckObserver) (bool, bool) {
 	success := true
+	warning := false
 	for _, c := range hc.categories {
 		if c.enabled {
 			for _, checker := range c.checkers {
@@ -1596,9 +1597,11 @@ func (hc *HealthChecker) RunChecks(observer CheckObserver) bool {
 					if !hc.runCheck(c, &checker, observer) {
 						if !checker.warning {
 							success = false
+						} else {
+							warning = true
 						}
 						if checker.fatal {
-							return success
+							return success, warning
 						}
 					}
 				}
@@ -1606,7 +1609,7 @@ func (hc *HealthChecker) RunChecks(observer CheckObserver) bool {
 		}
 	}
 
-	return success
+	return success, warning
 }
 
 // LinkerdConfig gets the Linkerd configuration values.

pkg/healthcheck/healthcheck_output.go

@@ -28,6 +28,11 @@ const (
 	// ShortOutput is used to specify the short output format
 	ShortOutput = "short"
 
+	// CoreHeader is used when printing core header checks
+	CoreHeader = "core"
+	// extensionsHeader is used when printing extensions header checks
+	extensionsHeader = "extensions"
+
 	// DefaultHintBaseURL is the default base URL on the linkerd.io website
 	// that all check hints for the latest linkerd version point to. Each
 	// check adds its own `hintAnchor` to specify a location on the page.
@@ -49,47 +54,41 @@ type CheckResults struct {
 
 // RunChecks submits each of the individual CheckResult structs to the given
 // observer.
-func (cr CheckResults) RunChecks(observer CheckObserver) bool {
+func (cr CheckResults) RunChecks(observer CheckObserver) (bool, bool) {
 	success := true
+	warning := false
 	for _, result := range cr.Results {
 		result := result // Copy loop variable to make lint happy.
-		if result.Err != nil && !result.Warning {
-			success = false
+		if result.Err != nil {
+			if !result.Warning {
+				success = false
+			} else {
+				warning = true
+			}
 		}
 		observer(&result)
 	}
-	return success
+	return success, warning
 }
 
-// PrintChecksHeader writes the core/extension checks header.
-func PrintChecksHeader(wout io.Writer, isCore bool) {
-	var headerTxt string
-	if isCore {
-		headerTxt = "Linkerd core checks"
-	} else {
-		headerTxt = "Linkerd extensions checks"
-		// this ensures there is a new line between the core check status and the extensions header
-		fmt.Fprintln(wout)
-	}
-	fmt.Fprintln(wout, headerTxt)
-	fmt.Fprintln(wout, strings.Repeat("=", len(headerTxt)))
+// PrintChecksHeader writes the header text for a check.
+func PrintChecksHeader(wout io.Writer, header string) {
+	headerText := fmt.Sprintf("Linkerd %s checks", header)
+	fmt.Fprintln(wout, headerText)
+	fmt.Fprintln(wout, strings.Repeat("=", len(headerText)))
 	fmt.Fprintln(wout)
 }
 
 // PrintChecksResult writes the checks result.
-func PrintChecksResult(wout io.Writer, output string, success bool) {
+func PrintChecksResult(wout io.Writer, output string, success bool, warning bool) {
 	if output == JSONOutput {
 		return
 	}
 
 	switch success {
 	case true:
-		if output != ShortOutput {
-			fmt.Fprintln(wout, "")
-		}
 		fmt.Fprintf(wout, "Status check results are %s\n", okStatus)
 	case false:
-		fmt.Fprintln(wout, "")
 		fmt.Fprintf(wout, "Status check results are %s\n", failStatus)
 	}
 }
@@ -97,15 +96,16 @@ func PrintChecksResult(wout io.Writer, output string, success bool) {
 // RunExtensionsChecks runs checks for each extension name passed into the `extensions` parameter
 // and handles formatting the output for each extension's check. This function also handles
 // finding the extension in the user's path and runs it.
-func RunExtensionsChecks(wout io.Writer, werr io.Writer, extensions []string, flags []string, output string) bool {
+func RunExtensionsChecks(wout io.Writer, werr io.Writer, extensions []string, flags []string, output string) (bool, bool) {
 	if output == TableOutput {
-		PrintChecksHeader(wout, false)
+		PrintChecksHeader(wout, extensionsHeader)
 	}
 
 	spin := spinner.New(spinner.CharSets[9], 100*time.Millisecond)
 	spin.Writer = wout
 
 	success := true
+	warning := false
 	for _, extension := range extensions {
 		var path string
 		args := append([]string{"check"}, flags...)
@@ -169,17 +169,18 @@ func RunExtensionsChecks(wout io.Writer, werr io.Writer, extensions []string, fl
 			}
 		}
 
-		extensionSuccess := RunChecks(wout, werr, results, output)
+		var extensionSuccess bool
+		extensionSuccess, warning = RunChecks(wout, werr, results, output)
 		if !extensionSuccess {
 			success = false
 		}
 	}
 
-	return success
+	return success, warning
 }
 
 // RunChecks runs the checks that are part of hc
-func RunChecks(wout io.Writer, werr io.Writer, hc Runner, output string) bool {
+func RunChecks(wout io.Writer, werr io.Writer, hc Runner, output string) (bool, bool) {
 	if output == JSONOutput {
 		return runChecksJSON(wout, werr, hc)
 	}
@@ -187,7 +188,7 @@ func RunChecks(wout io.Writer, werr io.Writer, hc Runner, output string) bool {
 	return runChecksTable(wout, hc, output)
 }
 
-func runChecksTable(wout io.Writer, hc Runner, output string) bool {
+func runChecksTable(wout io.Writer, hc Runner, output string) (bool, bool) {
 	var lastCategory CategoryID
 	spin := spinner.New(spinner.CharSets[9], 100*time.Millisecond)
 	spin.Writer = wout
@@ -231,15 +232,25 @@ func runChecksTable(wout io.Writer, hc Runner, output string) bool {
 		printResultDescription(wout, status, result)
 	}
 
-	var success bool
+	var (
+		success bool
+		warning bool
+	)
 	switch output {
 	case ShortOutput:
-		success = hc.RunChecks(prettyPrintResultsShort)
+		success, warning = hc.RunChecks(prettyPrintResultsShort)
 	default:
-		success = hc.RunChecks(prettyPrintResults)
+		success, warning = hc.RunChecks(prettyPrintResults)
 	}
 
-	return success
+	// This ensures there is a newline separating check categories from each
+	// other as well as the check result. When running in ShortOutput mode and
+	// there are no warnings, there is no newline printed.
+	if output != ShortOutput || !success || warning {
+		fmt.Fprintln(wout)
+	}
+
+	return success, warning
 }
 
 type checkOutput struct {
@@ -269,7 +280,7 @@ const (
 	checkErr     checkResult = "error"
 )
 
-func runChecksJSON(wout io.Writer, werr io.Writer, hc Runner) bool {
+func runChecksJSON(wout io.Writer, werr io.Writer, hc Runner) (bool, bool) {
 	var categories []*checkCategory
 
 	collectJSONOutput := func(result *CheckResult) {
@@ -308,10 +319,10 @@ func runChecksJSON(wout io.Writer, werr io.Writer, hc Runner) bool {
 		}
 	}
 
-	result := hc.RunChecks(collectJSONOutput)
+	success, warning := hc.RunChecks(collectJSONOutput)
 
 	outputJSON := checkOutput{
-		Success:    result,
+		Success:    success,
 		Categories: categories,
 	}
 
@@ -321,7 +332,7 @@ func runChecksJSON(wout io.Writer, werr io.Writer, hc Runner) bool {
 	} else {
 		fmt.Fprintf(werr, "JSON serialization of the check result failed with %s", err)
 	}
-	return result
+	return success, warning
 }
 
 // ParseJSONCheckOutput parses the output of a check command run with json
@@ -395,12 +406,12 @@ func printHeader(wout io.Writer, headerPrinted bool, hc Runner) bool {
 	switch v := hc.(type) {
 	case *HealthChecker:
 		if v.IsMainCheckCommand {
-			PrintChecksHeader(wout, true)
+			PrintChecksHeader(wout, CoreHeader)
 			headerPrinted = true
 		}
 	// When RunExtensionChecks called
 	case CheckResults:
-		PrintChecksHeader(wout, false)
+		PrintChecksHeader(wout, extensionsHeader)
 		headerPrinted = true
 	}
 

pkg/healthcheck/healthcheck_test.go

@@ -210,7 +210,7 @@ func TestHealthChecker(t *testing.T) {
 		hc.AppendCategories(passingCheck1)
 		hc.AppendCategories(passingCheck2)
 
-		success := hc.RunChecks(nullObserver)
+		success, _ := hc.RunChecks(nullObserver)
 
 		if !success {
 			t.Fatalf("Expecting checks to be successful, but got [%t]", success)
@@ -226,7 +226,7 @@ func TestHealthChecker(t *testing.T) {
 		hc.AppendCategories(failingCheck)
 		hc.AppendCategories(passingCheck2)
 
-		success := hc.RunChecks(nullObserver)
+		success, _ := hc.RunChecks(nullObserver)
 
 		if success {
 			t.Fatalf("Expecting checks to not be successful, but got [%t]", success)
@@ -1991,7 +1991,8 @@ func TestLinkerdPreInstallGlobalResourcesChecks(t *testing.T) {
 		}
 
 		observer := newObserver()
-		if !hc.RunChecks(observer.resultFn) {
+		success, _ := hc.RunChecks(observer.resultFn)
+		if !success {
 			t.Errorf("Expect RunChecks to return true")
 		}
 
@@ -2049,7 +2050,8 @@ metadata:
 		}
 
 		observer := newObserver()
-		if hc.RunChecks(observer.resultFn) {
+		success, _ := hc.RunChecks(observer.resultFn)
+		if success {
 			testutil.Error(t, "Expect RunChecks to return false")
 		}
 

viz/cmd/check.go

@@ -92,8 +92,8 @@ func configureAndRunChecks(wout io.Writer, werr io.Writer, options *checkOptions
 	if options.proxy {
 		hc.AppendCategories(hc.VizDataPlaneCategory())
 	}
-	success := healthcheck.RunChecks(wout, werr, hc, options.output)
-	healthcheck.PrintChecksResult(wout, options.output, success)
+	success, warning := healthcheck.RunChecks(wout, werr, hc, options.output)
+	healthcheck.PrintChecksResult(wout, options.output, success, warning)
 
 	if !success {
 		os.Exit(1)

viz/pkg/healthcheck/healthcheck.go

@@ -62,7 +62,7 @@ func (hc *HealthChecker) VizAPIClient() pb.ApiClient {
 }
 
 // RunChecks implements the healthcheck.Runner interface
-func (hc *HealthChecker) RunChecks(observer healthcheck.CheckObserver) bool {
+func (hc *HealthChecker) RunChecks(observer healthcheck.CheckObserver) (bool, bool) {
 	return hc.HealthChecker.RunChecks(observer)
 }
 

web/srv/api_handlers.go

@@ -372,7 +372,7 @@ func (h *handler) handleAPICheck(w http.ResponseWriter, req *http.Request, p htt
 	}
 	// TODO (tegioz): ignore runchecks results until we stop filtering checks
 	// in this method (see #3670 for more details)
-	_ = h.hc.RunChecks(collectResults)
+	_, _ = h.hc.RunChecks(collectResults)
 
 	renderJSON(w, map[string]interface{}{
 		"success": success,
@@ -389,7 +389,7 @@ func (h *handler) handleAPIResourceDefinition(w http.ResponseWriter, req *http.R
 		}
 	}
 	if len(missingParams) != 0 {
-		renderJSONError(w, fmt.Errorf("Required params not provided: %s", strings.Join(missingParams, ", ")), http.StatusBadRequest)
+		renderJSONError(w, fmt.Errorf("required params not provided: %s", strings.Join(missingParams, ", ")), http.StatusBadRequest)
 		return
 	}
 

web/srv/api_handlers_test.go

@@ -20,38 +20,38 @@ type mockHealthChecker struct {
 	results []*healthcheck.CheckResult
 }
 
-func (c *mockHealthChecker) RunChecks(observer healthcheck.CheckObserver) bool {
+func (c *mockHealthChecker) RunChecks(observer healthcheck.CheckObserver) (bool, bool) {
 	for _, result := range c.results {
 		observer(result)
 	}
-	return true
+	return true, false
 }
 
 func TestHandleApiCheck(t *testing.T) {
 	// Setup handler using a mock health checker
 	mockResults := []*healthcheck.CheckResult{
-		&healthcheck.CheckResult{
+		{
 			Category:    healthcheck.LinkerdConfigChecks,
 			Description: "check3-description",
 			HintURL:     healthcheck.DefaultHintBaseURL + "check3-hint-anchor",
 			Warning:     false,
 			Err:         nil,
 		},
-		&healthcheck.CheckResult{
+		{
 			Category:    healthcheck.LinkerdConfigChecks,
 			Description: "check4-description-kubectl",
 			HintURL:     healthcheck.DefaultHintBaseURL + "check4-hint-anchor",
 			Warning:     true,
 			Err:         nil,
 		},
-		&healthcheck.CheckResult{
+		{
 			Category:    healthcheck.KubernetesAPIChecks,
 			Description: "check1-description",
 			HintURL:     healthcheck.DefaultHintBaseURL + "check1-hint-anchor",
 			Warning:     false,
 			Err:         nil,
 		},
-		&healthcheck.CheckResult{
+		{
 			Category:    healthcheck.KubernetesAPIChecks,
 			Description: "check2-description",
 			HintURL:     healthcheck.DefaultHintBaseURL + "check2-hint-anchor",

web/srv/server.go

@@ -56,7 +56,7 @@ type (
 	}
 
 	healthChecker interface {
-		RunChecks(observer healthcheck.CheckObserver) bool
+		RunChecks(observer healthcheck.CheckObserver) (bool, bool)
 	}
 )