Repository: replicatedhq/troubleshoot
Pull request #644 (2022-08-03 10:17:18)

Allow custom messages to be passed if a deployment/statefulset is absent
This is an attempt to address internal shortcut 49195. 

We would like to be able to add custom messages when a deployment (e.g EKCO) is absent from the cluster to better explain how to resolve it's absence.

This change introduces a new special keyword "absent" that can be used in the "When" statement in a deploymentStatus or statefulSetStatus analyzer to allow absent deployments and statefulsets to be handled with custom messages.

for example:
```yaml
apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: example
spec:
  analyzers:
    - deploymentStatus:
        name: ekc-operator
        namespace: kurl
        outcomes:
          - fail:
              when: "absent"
              message: "The ekc-operator deployment is not instaled. Without it, this cluster may not behave as intended"
```

While the change acheives this objective, I do have some reservations that I'd like to discuss:

- currently the `when: "absent"` statement would need to be the first outcome for an analyzer due to how I'm catching it. comments on how I can improve this would be greatly appreciated.
- I'm currently having to repeat a block to allow for the default `the <resource> <name> does not exist` for each warn|pass|fail outcome when we aren't specifically looking for the absence of a deployment. I'd like to reduce this repitition if possible.


Patch:
diff --git a/pkg/analyze/common_status.go b/pkg/analyze/common_status.go
index 7c1e2adf..cce09d2d 100644
--- a/pkg/analyze/common_status.go
+++ b/pkg/analyze/common_status.go
@@ -3,21 +3,31 @@ package analyzer
 import (
 	"strconv"
 	"strings"
-
+	"fmt"
 	"github.com/pkg/errors"
 	troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2"
 )
 
-func commonStatus(outcomes []*troubleshootv1beta2.Outcome, title, iconKey string, iconURI string, readyReplicas int) (*AnalyzeResult, error) {
+func commonStatus(outcomes []*troubleshootv1beta2.Outcome, name string, iconKey string, iconURI string, readyReplicas int, exists bool, resourceType string) (*AnalyzeResult, error) {
 	result := &AnalyzeResult{
-		Title:   title,
+		Title:   fmt.Sprintf("%s Status", name),
 		IconKey: iconKey,
 		IconURI: iconURI,
 	}
 
 	// ordering from the spec is important, the first one that matches returns
 	for _, outcome := range outcomes {
+
 		if outcome.Fail != nil {
+
+			// if we're not checking that something is absent but it is, we should throw a default but meaningful error.
+			if exists == false && outcome.Fail.When != "absent" {
+				result.IsFail = true
+				result.Message = fmt.Sprintf("The %s %q was not found", resourceType, name)
+				result.URI = outcome.Fail.URI
+				return result, nil
+			}
+
 			if outcome.Fail.When == "" {
 				result.IsFail = true
 				result.Message = outcome.Fail.Message
@@ -26,7 +36,18 @@ func commonStatus(outcomes []*troubleshootv1beta2.Outcome, title, iconKey string
 				return result, nil
 			}
 
-			match, err := compareActualToWhen(outcome.Fail.When, readyReplicas)
+			if  outcome.Fail.When == "absent"  {
+				if exists == false {
+					result.IsFail = true
+					result.Message = outcome.Fail.Message
+					result.URI = outcome.Fail.URI
+					return result, nil
+				} else {
+					continue
+				}
+			}
+
+			match, err := compareActualToWhen(outcome.Fail.When, readyReplicas, exists)
 			if err != nil {
 				return nil, errors.Wrap(err, "failed to parse fail range")
 			}
@@ -39,6 +60,14 @@ func commonStatus(outcomes []*troubleshootv1beta2.Outcome, title, iconKey string
 				return result, nil
 			}
 		} else if outcome.Warn != nil {
+
+			if exists == false && outcome.Warn.When != "absent" {
+				result.IsFail = true
+				result.Message = fmt.Sprintf("The %s %q was not found", resourceType, name)
+				result.URI = outcome.Fail.URI
+				return result, nil
+			}
+
 			if outcome.Warn.When == "" {
 				result.IsWarn = true
 				result.Message = outcome.Warn.Message
@@ -47,7 +76,18 @@ func commonStatus(outcomes []*troubleshootv1beta2.Outcome, title, iconKey string
 				return result, nil
 			}
 
-			match, err := compareActualToWhen(outcome.Warn.When, readyReplicas)
+			if outcome.Warn.When == "absent" {
+				if exists == false {
+					result.IsWarn = true
+					result.Message = outcome.Warn.Message
+					result.URI = outcome.Warn.URI
+					return result, nil
+				} else {
+					continue
+				}
+			}
+
+			match, err := compareActualToWhen(outcome.Warn.When, readyReplicas, exists)
 			if err != nil {
 				return nil, errors.Wrap(err, "failed to parse warn range")
 			}
@@ -60,6 +100,14 @@ func commonStatus(outcomes []*troubleshootv1beta2.Outcome, title, iconKey string
 				return result, nil
 			}
 		} else if outcome.Pass != nil {
+
+			if exists == false && outcome.Pass.When != "absent" {
+				result.IsFail = true
+				result.Message = fmt.Sprintf("The %s %q was not found", resourceType, name)
+				result.URI = outcome.Fail.URI
+				return result, nil
+			}
+
 			if outcome.Pass.When == "" {
 				result.IsPass = true
 				result.Message = outcome.Pass.Message
@@ -68,7 +116,18 @@ func commonStatus(outcomes []*troubleshootv1beta2.Outcome, title, iconKey string
 				return result, nil
 			}
 
-			match, err := compareActualToWhen(outcome.Pass.When, readyReplicas)
+			if outcome.Pass.When == "absent" {
+				if exists == false {
+					result.IsPass = true
+					result.Message = outcome.Pass.Message
+					result.URI = outcome.Pass.URI
+					return result, nil
+				} else {
+					continue
+				}
+			}
+
+			match, err := compareActualToWhen(outcome.Pass.When, readyReplicas, exists)
 			if err != nil {
 				return nil, errors.Wrap(err, "failed to parse pass range")
 			}
@@ -86,7 +145,7 @@ func commonStatus(outcomes []*troubleshootv1beta2.Outcome, title, iconKey string
 	return result, nil
 }
 
-func compareActualToWhen(when string, actual int) (bool, error) {
+func compareActualToWhen(when string, actual int, exists bool) (bool, error) {
 	parts := strings.Split(strings.TrimSpace(when), " ")
 
 	// we can make this a lot more flexible
diff --git a/pkg/analyze/deployment_status.go b/pkg/analyze/deployment_status.go
index 9ad74c1e..e55fb9e7 100644
--- a/pkg/analyze/deployment_status.go
+++ b/pkg/analyze/deployment_status.go
@@ -26,6 +26,9 @@ func analyzeOneDeploymentStatus(analyzer *troubleshootv1beta2.DeploymentStatus,
 
 	var result *AnalyzeResult
 	for _, collected := range files { // only 1 file here
+		var exists bool = true
+		var readyReplicas int
+
 		var deployments appsv1.DeploymentList
 		if err := json.Unmarshal(collected, &deployments); err != nil {
 			return nil, errors.Wrap(err, "failed to unmarshal deployment list")
@@ -39,19 +42,15 @@ func analyzeOneDeploymentStatus(analyzer *troubleshootv1beta2.DeploymentStatus,
 		}
 
 		if status == nil {
-			// there's not an error, but maybe the requested deployment is not even deployed
-			result = &AnalyzeResult{
-				Title:   fmt.Sprintf("%s Deployment Status", analyzer.Name),
-				IconKey: "kubernetes_deployment_status",
-				IconURI: "https://troubleshoot.sh/images/analyzer-icons/deployment-status.svg?w=17&h=17",
-				IsFail:  true,
-				Message: fmt.Sprintf("The deployment %q was not found", analyzer.Name),
-			}
+			exists = false
+			readyReplicas = 0
 		} else {
-			result, err = commonStatus(analyzer.Outcomes, fmt.Sprintf("%s Status", analyzer.Name), "kubernetes_deployment_status", "https://troubleshoot.sh/images/analyzer-icons/deployment-status.svg?w=17&h=17", int(status.ReadyReplicas))
-			if err != nil {
-				return nil, errors.Wrap(err, "failed to process status")
-			}
+			readyReplicas = int(status.ReadyReplicas)
+		}
+
+		result, err = commonStatus(analyzer.Outcomes, analyzer.Name, "kubernetes_deployment_status", "https://troubleshoot.sh/images/analyzer-icons/deployment-status.svg?w=17&h=17", readyReplicas, exists, "deployment")
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to process status")
 		}
 	}
 
diff --git a/pkg/analyze/statefulset_status.go b/pkg/analyze/statefulset_status.go
index 84e7600e..d8c5391f 100644
--- a/pkg/analyze/statefulset_status.go
+++ b/pkg/analyze/statefulset_status.go
@@ -26,6 +26,9 @@ func analyzeOneStatefulsetStatus(analyzer *troubleshootv1beta2.StatefulsetStatus
 
 	var result *AnalyzeResult
 	for _, collected := range files { // only 1 file here
+		var exists bool = true
+		var readyReplicas int
+
 		var statefulsets appsv1.StatefulSetList
 		if err := json.Unmarshal(collected, &statefulsets); err != nil {
 			return nil, errors.Wrap(err, "failed to unmarshal statefulset list")
@@ -40,15 +43,13 @@ func analyzeOneStatefulsetStatus(analyzer *troubleshootv1beta2.StatefulsetStatus
 		}
 
 		if statefulset == nil {
-			result = &AnalyzeResult{
-				Title:   fmt.Sprintf("%s Statefulset Status", analyzer.Name),
-				IconKey: "kubernetes_statefulset_status",
-				IconURI: "https://troubleshoot.sh/images/analyzer-icons/statefulset-status.svg?w=23&h=14",
-				IsFail:  true,
-				Message: fmt.Sprintf("The statefulset %q was not found", analyzer.Name),
-			}
-		} else if len(analyzer.Outcomes) > 0 {
-			result, err = commonStatus(analyzer.Outcomes, fmt.Sprintf("%s Status", analyzer.Name), "kubernetes_statefulset_status", "https://troubleshoot.sh/images/analyzer-icons/statefulset-status.svg?w=23&h=14", int(statefulset.Status.ReadyReplicas))
+			exists = false
+			readyReplicas = 0
+		} else {
+			readyReplicas = int(statefulset.Status.ReadyReplicas)
+		}
+		if len(analyzer.Outcomes) > 0 {
+			result, err = commonStatus(analyzer.Outcomes, analyzer.Name, "kubernetes_statefulset_status", "https://troubleshoot.sh/images/analyzer-icons/statefulset-status.svg?w=23&h=14", readyReplicas, exists, "statefulset")
 			if err != nil {
 				return nil, errors.Wrap(err, "failed to process status")
 			}
diff --git a/pkg/preflight/collect.go b/pkg/preflight/collect.go
index 27b31bcc..c26b6b30 100644
--- a/pkg/preflight/collect.go
+++ b/pkg/preflight/collect.go
@@ -146,7 +146,7 @@ func Collect(opts CollectOpts, p *troubleshootv1beta2.Preflight) (CollectResult,
 
 	k8sClient, err := kubernetes.NewForConfig(opts.KubernetesRestConfig)
 	if err != nil {
-		return collectResult, errors.Wrap(err, "failed to instantiate Kubernetes client")
+		return collectResult, errors.Wrap(err, "failed to instantiate kuberentes client")
 	}
 
 	if err := collectors.CheckRBAC(context.Background()); err != nil {
diff --git a/pkg/supportbundle/collect.go b/pkg/supportbundle/collect.go
index 36251f8c..fbe4564f 100644
--- a/pkg/supportbundle/collect.go
+++ b/pkg/supportbundle/collect.go
@@ -42,10 +42,10 @@ func runHostCollectors(hostCollectors []*troubleshootv1beta2.HostCollect, additi
 			continue
 		}
 
-		opts.ProgressChan <- fmt.Sprintf("[%s] Running host collector...", collector.Title())
+		opts.ProgressChan <- fmt.Sprintf("[%s] Running collector...", collector.Title())
 		result, err := collector.Collect(opts.ProgressChan)
 		if err != nil {
-			opts.ProgressChan <- errors.Errorf("failed to run host collector: %s: %v", collector.Title(), err)
+			opts.ProgressChan <- errors.Errorf("failed to run collector: %s: %v", collector.Title(), err)
 		}
 		for k, v := range result {
 			allCollectedData[k] = v
@@ -92,7 +92,7 @@ func runCollectors(collectors []*troubleshootv1beta2.Collect, additionalRedactor
 
 	k8sClient, err := kubernetes.NewForConfig(opts.KubernetesRestConfig)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to instantiate Kubernetes client")
+		return nil, errors.Wrap(err, "failed to instantiate kuberentes client")
 	}
 
 	if err := cleanedCollectors.CheckRBAC(context.Background()); err != nil {
diff --git a/pkg/supportbundle/supportbundle.go b/pkg/supportbundle/supportbundle.go
index 07424b80..50fe9912 100644
--- a/pkg/supportbundle/supportbundle.go
+++ b/pkg/supportbundle/supportbundle.go
@@ -86,35 +86,19 @@ func CollectSupportBundleFromSpec(spec *troubleshootv1beta2.SupportBundleSpec, a
 		return nil, errors.Wrap(err, "create bundle dir")
 	}
 
-	var result, files, hostFiles collect.CollectorResult
-
-	if spec.HostCollectors != nil {
-		// Run host collectors
-		hostFiles, err = runHostCollectors(spec.HostCollectors, additionalRedactors, bundlePath, opts)
-		if err != nil {
-			fmt.Println(errors.Wrap(err, "failed to run host collectors"))
-		}
+	hostFiles, err := runHostCollectors(spec.HostCollectors, additionalRedactors, bundlePath, opts)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to run host collectors")
 	}
 
-	if spec.Collectors != nil {
-		// Run collectors
-		files, err = runCollectors(spec.Collectors, additionalRedactors, bundlePath, opts)
-		if err != nil {
-			fmt.Println(errors.Wrap(err, "failed to run collectors"))
-		}
+	// Run collectors
+	files, err := runCollectors(spec.Collectors, additionalRedactors, bundlePath, opts)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to run collectors")
 	}
 
-	if files != nil && hostFiles != nil {
-		result = files
-		for k, v := range hostFiles {
-			result[k] = v
-		}
-	} else if files != nil {
-		result = files
-	} else if hostFiles != nil {
-		result = hostFiles
-	} else {
-		return nil, errors.Wrap(err, "failed to generate support bundle")
+	for k, v := range hostFiles {
+		files[k] = v
 	}
 
 	version, err := getVersionFile()
@@ -122,7 +106,7 @@ func CollectSupportBundleFromSpec(spec *troubleshootv1beta2.SupportBundleSpec, a
 		return nil, errors.Wrap(err, "failed to get version file")
 	}
 
-	err = result.SaveResult(bundlePath, VersionFilename, version)
+	err = files.SaveResult(bundlePath, VersionFilename, version)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to write version")
 	}
@@ -145,12 +129,12 @@ func CollectSupportBundleFromSpec(spec *troubleshootv1beta2.SupportBundleSpec, a
 		return nil, errors.Wrap(err, "failed to get analysis file")
 	}
 
-	err = result.SaveResult(bundlePath, AnalysisFilename, analysis)
+	err = files.SaveResult(bundlePath, AnalysisFilename, analysis)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to write analysis")
 	}
 
-	if err := collect.TarSupportBundleDir(bundlePath, result, filename); err != nil {
+	if err := collect.TarSupportBundleDir(bundlePath, files, filename); err != nil {
 		return nil, errors.Wrap(err, "create bundle file")
 	}
 


Test patch:
diff --git a/pkg/analyze/deployment_status_test.go b/pkg/analyze/deployment_status_test.go
index 09bb90f2..66382082 100644
--- a/pkg/analyze/deployment_status_test.go
+++ b/pkg/analyze/deployment_status_test.go
@@ -15,10 +15,53 @@ func Test_deploymentStatus(t *testing.T) {
 		expectResult []*AnalyzeResult
 		files        map[string][]byte
 	}{
+		{
+			name: "1/1, fail when absent",
+			analyzer: troubleshootv1beta2.DeploymentStatus{
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							When: "absent",
+							Message: "fail",
+						},
+					},
+					{
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							When:    "= 1",
+							Message: "pass",
+						},
+					},
+				},
+				Namespace: "default",
+				Name: "nonexistant-deployment",
+			},
+			expectResult: []*AnalyzeResult{
+				{
+					IsPass: false,
+					IsWarn: false,
+					IsFail: true,
+					Title: "nonexistant-deployment Status",
+					Message: "fail",
+					IconKey: "kubernetes_deployment_status",
+					IconURI: "https://troubleshoot.sh/images/analyzer-icons/deployment-status.svg?w=17&h=17",
+				},
+			},
+			files: map[string][]byte{
+				"cluster-resources/deployments/default.json":     []byte(defaultDeployments),
+				"cluster-resources/deployments/monitoring.json":  []byte(monitoringDeployments),
+				"cluster-resources/deployments/kube-system.json": []byte(kubeSystemDeployments),
+			},
+		},
 		{
 			name: "1/1, pass when = 1",
 			analyzer: troubleshootv1beta2.DeploymentStatus{
 				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							When: "absent",
+							Message: "fail",
+						},
+					},
 					{
 						Pass: &troubleshootv1beta2.SingleOutcome{
 							When:    "= 1",
@@ -55,6 +98,12 @@ func Test_deploymentStatus(t *testing.T) {
 			name: "1/1, pass when = 2",
 			analyzer: troubleshootv1beta2.DeploymentStatus{
 				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							When: "absent",
+							Message: "fail",
+						},
+					},
 					{
 						Pass: &troubleshootv1beta2.SingleOutcome{
 							When:    "= 2",
@@ -91,6 +140,12 @@ func Test_deploymentStatus(t *testing.T) {
 			name: "1/1, pass when >= 2, warn when = 1, fail when 0",
 			analyzer: troubleshootv1beta2.DeploymentStatus{
 				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							When: "absent",
+							Message: "fail",
+						},
+					},
 					{
 						Pass: &troubleshootv1beta2.SingleOutcome{
 							When:    ">= 2",
diff --git a/pkg/analyze/statefulset_status_test.go b/pkg/analyze/statefulset_status_test.go
index 78f5a00c..2a088776 100644
--- a/pkg/analyze/statefulset_status_test.go
+++ b/pkg/analyze/statefulset_status_test.go
@@ -15,6 +15,41 @@ func Test_analyzeStatefulsetStatus(t *testing.T) {
 		expectResult []*AnalyzeResult
 		files        map[string][]byte
 	}{
+		{
+			name: "fail when absent",
+			analyzer: troubleshootv1beta2.StatefulsetStatus{
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							When: "absent",
+							Message: "fail",
+						},
+					},
+					{
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							When:    "= 1",
+							Message: "pass",
+						},
+					},
+				},
+				Namespace: "default",
+				Name: "nonexistant",
+			},
+			expectResult: []*AnalyzeResult{
+				{
+					IsPass: false,
+					IsWarn: false,
+					IsFail: true,
+					Title: "nonexistant Status",
+					Message: "fail",
+					IconKey: "kubernetes_statefulset_status",
+					IconURI: "https://troubleshoot.sh/images/analyzer-icons/statefulset-status.svg?w=23&h=14",
+				},
+			},
+			files: map[string][]byte{
+				"cluster-resources/statefulsets/default.json":    []byte(defaultStatefulSets),
+			},
+		},
 		{
 			name:     "analyze all statefulsets",
 			analyzer: troubleshootv1beta2.StatefulsetStatus{},