fix(support-bundle): default in-cluster collectors in host support bundle (#1394)

* fix(support-bundle): default in-cluster collectors in host support bundle

Ensure cluster-resources and cluster-info collectors are present only
when a support bundle spec contains in-cluster collectors.

* Various improvements

* Improve error messages
* Util function appending elements to a nil slice that allows adding
  specs to an empty slice of collectors/analysers/redactors

* Fix failing test

cmd/analyze/cli/run.go

@@ -27,7 +27,7 @@ func runAnalyzers(v *viper.Viper, bundlePath string) error {
 	} else {
 		if !util.IsURL(specPath) {
 			// TODO: Better error message when we do not have a file/url etc
-			return fmt.Errorf("%s is not a URL and was not found (err %s)", specPath, err)
+			return fmt.Errorf("%s is not a URL and was not found", specPath)
 		}
 
 		req, err := http.NewRequest("GET", specPath, nil)

cmd/collect/cli/run.go

@@ -58,7 +58,7 @@ func runCollect(v *viper.Viper, arg string) error {
 		collectorContent = b
 	} else {
 		if !util.IsURL(arg) {
-			return fmt.Errorf("%s is not a URL and was not found (err %s)", arg, err)
+			return fmt.Errorf("%s is not a URL and was not found", arg)
 		}
 
 		req, err := http.NewRequest("GET", arg, nil)

cmd/troubleshoot/cli/analyze.go

@@ -91,7 +91,7 @@ func downloadAnalyzerSpec(specPath string) (string, error) {
 		specContent = string(b)
 	} else {
 		if !util.IsURL(specPath) {
-			return "", fmt.Errorf("%s is not a URL and was not found (err %s)", specPath, err)
+			return "", fmt.Errorf("%s is not a URL and was not found", specPath)
 		}
 
 		req, err := http.NewRequest("GET", specPath, nil)

cmd/troubleshoot/cli/run.go

@@ -233,7 +233,7 @@ the %s Admin Console to begin analysis.`
 }
 
 // loadSupportBundleSpecsFromURIs loads support bundle specs from URIs
-func loadSupportBundleSpecsFromURIs(ctx context.Context, kinds *loader.TroubleshootKinds) (*loader.TroubleshootKinds, error) {
+func loadSupportBundleSpecsFromURIs(ctx context.Context, kinds *loader.TroubleshootKinds) error {
 	remoteRawSpecs := []string{}
 	for _, s := range kinds.SupportBundlesV1Beta2 {
 		if s.Spec.Uri != "" && util.IsURL(s.Spec.Uri) {
@@ -252,12 +252,18 @@ func loadSupportBundleSpecsFromURIs(ctx context.Context, kinds *loader.Troublesh
 	}
 
 	if len(remoteRawSpecs) == 0 {
-		return kinds, nil
+		return nil
 	}
 
-	return loader.LoadSpecs(ctx, loader.LoadOptions{
+	moreKinds, err := loader.LoadSpecs(ctx, loader.LoadOptions{
 		RawSpecs: remoteRawSpecs,
 	})
+	if err != nil {
+		return err
+	}
+
+	kinds.Add(moreKinds)
+	return nil
 }
 
 func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface) (*troubleshootv1beta2.SupportBundle, *troubleshootv1beta2.Redactor, error) {
@@ -270,11 +276,9 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface)
 
 	// Load additional specs from support bundle URIs
 	if !viper.GetBool("no-uri") {
-		moreKinds, err := loadSupportBundleSpecsFromURIs(ctx, kinds)
+		err := loadSupportBundleSpecsFromURIs(ctx, kinds)
 		if err != nil {
 			klog.Warningf("unable to load support bundles from URIs: %v", err)
-		} else {
-			kinds.Add(moreKinds)
 		}
 	}
 
@@ -304,25 +308,27 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface)
 	}
 
 	for _, c := range kinds.CollectorsV1Beta2 {
-		mainBundle.Spec.Collectors = append(mainBundle.Spec.Collectors, c.Spec.Collectors...)
+		mainBundle.Spec.Collectors = util.Append(mainBundle.Spec.Collectors, c.Spec.Collectors)
 	}
 
 	for _, hc := range kinds.HostCollectorsV1Beta2 {
-		mainBundle.Spec.HostCollectors = append(mainBundle.Spec.HostCollectors, hc.Spec.Collectors...)
+		mainBundle.Spec.HostCollectors = util.Append(mainBundle.Spec.HostCollectors, hc.Spec.Collectors)
 	}
 
-	// Ensure cluster info and cluster resources collectors are in the merged spec
-	// We need to add them here so when we --dry-run, these collectors are included.
-	// supportbundle.runCollectors duplicates this bit. We'll need to refactor it out later
-	// when its clearer what other code depends on this logic e.g KOTS
-	mainBundle.Spec.Collectors = collect.EnsureCollectorInList(
-		mainBundle.Spec.Collectors,
-		troubleshootv1beta2.Collect{ClusterInfo: &troubleshootv1beta2.ClusterInfo{}},
-	)
-	mainBundle.Spec.Collectors = collect.EnsureCollectorInList(
-		mainBundle.Spec.Collectors,
-		troubleshootv1beta2.Collect{ClusterResources: &troubleshootv1beta2.ClusterResources{}},
-	)
+	if mainBundle.Spec.Collectors != nil {
+		// If we have in-cluster collectors, ensure cluster info and cluster resources
+		// collectors are in the merged spec. We need to add them here so when we --dry-run,
+		// these collectors are included. supportbundle.runCollectors duplicates this bit.
+		// We'll need to refactor it out later when its clearer what other code depends on this logic e.g KOTS
+		mainBundle.Spec.Collectors = collect.EnsureCollectorInList(
+			mainBundle.Spec.Collectors,
+			troubleshootv1beta2.Collect{ClusterInfo: &troubleshootv1beta2.ClusterInfo{}},
+		)
+		mainBundle.Spec.Collectors = collect.EnsureCollectorInList(
+			mainBundle.Spec.Collectors,
+			troubleshootv1beta2.Collect{ClusterResources: &troubleshootv1beta2.ClusterResources{}},
+		)
+	}
 
 	additionalRedactors := &troubleshootv1beta2.Redactor{
 		TypeMeta: metav1.TypeMeta{
@@ -334,7 +340,7 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface)
 		},
 	}
 	for _, r := range kinds.RedactorsV1Beta2 {
-		additionalRedactors.Spec.Redactors = append(additionalRedactors.Spec.Redactors, r.Spec.Redactors...)
+		additionalRedactors.Spec.Redactors = util.Append(additionalRedactors.Spec.Redactors, r.Spec.Redactors)
 	}
 
 	return mainBundle, additionalRedactors, nil

cmd/troubleshoot/cli/run_test.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"context"
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -48,11 +49,13 @@ spec:
 	require.NoError(t, err)
 	require.NotNil(t, kinds)
 
-	moreKinds, err := loadSupportBundleSpecsFromURIs(ctx, kinds)
+	assert.Len(t, kinds.SupportBundlesV1Beta2, 1)
+	assert.NotNil(t, kinds.SupportBundlesV1Beta2[0].Spec.Collectors[0].ConfigMap)
+	err = loadSupportBundleSpecsFromURIs(ctx, kinds)
 	require.NoError(t, err)
 
-	require.Len(t, moreKinds.SupportBundlesV1Beta2, 1)
-	assert.NotNil(t, moreKinds.SupportBundlesV1Beta2[0].Spec.Collectors[0].ClusterInfo)
+	require.Len(t, kinds.SupportBundlesV1Beta2, 2)
+	assert.NotNil(t, kinds.SupportBundlesV1Beta2[1].Spec.Collectors[0].ClusterInfo)
 }
 
 func Test_loadSupportBundleSpecsFromURIs_TimeoutError(t *testing.T) {
@@ -76,8 +79,14 @@ func Test_loadSupportBundleSpecsFromURIs_TimeoutError(t *testing.T) {
 		httputil.GetHttpClient().Timeout = before
 	}()
 
-	kindsAfter, err := loadSupportBundleSpecsFromURIs(ctx, kinds)
+	beforeJSON, err := json.Marshal(kinds)
 	require.NoError(t, err)
 
-	assert.Equal(t, kinds, kindsAfter)
+	err = loadSupportBundleSpecsFromURIs(ctx, kinds)
+	require.NoError(t, err)
+
+	afterJSON, err := json.Marshal(kinds)
+	require.NoError(t, err)
+
+	assert.JSONEq(t, string(beforeJSON), string(afterJSON))
 }