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

Author: banjoh

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))
 }

internal/specs/specs.go

@@ -167,7 +167,7 @@ func LoadFromCLIArgs(ctx context.Context, client kubernetes.Interface, args []st
 				rawSpecs = append(rawSpecs, content...)
 			} else {
 				if !util.IsURL(v) {
-					return nil, types.NewExitCodeError(constants.EXIT_CODE_SPEC_ISSUES, fmt.Errorf("%s is not a URL and was not found (err %s)", v, err))
+					return nil, types.NewExitCodeError(constants.EXIT_CODE_SPEC_ISSUES, fmt.Errorf("%s is not a URL and was not found", v))
 				}
 
 				// Download preflight specs

internal/util/util.go

@@ -52,3 +52,25 @@ func EstimateNumberOfLines(text string) int {
 	}
 	return n
 }
+
+// Append appends elements in src to target.
+// We have this function because of how append()
+// treats nil slices the same as empty slices.
+// An empty array in YAML like below is not the
+// same as when the array is not specified.
+//
+//	 spec:
+//		  collectors: []
+func Append[T any](target []T, src []T) []T {
+	// Do nothing only if src is nil
+	if src == nil {
+		return target
+	}
+
+	// In case target is nil, we need to initialize it
+	// since append() will not do it for us when len(src) == 0
+	if target == nil {
+		target = []T{}
+	}
+	return append(target, src...)
+}

internal/util/util_test.go

@@ -194,3 +194,55 @@ func Test_EstimateNumberOfLines(t *testing.T) {
 		})
 	}
 }
+
+func TestAppend(t *testing.T) {
+	tests := []struct {
+		name   string
+		target []string
+		src    []string
+		want   []string
+	}{
+		{
+			name:   "empty target",
+			target: []string{},
+			src:    []string{"a", "b", "c"},
+			want:   []string{"a", "b", "c"},
+		},
+		{
+			name:   "empty src",
+			target: []string{"a", "b", "c"},
+			src:    []string{},
+			want:   []string{"a", "b", "c"},
+		},
+		{
+			name:   "non-empty target and src",
+			target: []string{"a", "b", "c"},
+			src:    []string{"d", "e", "f"},
+			want:   []string{"a", "b", "c", "d", "e", "f"},
+		},
+		{
+			name:   "nil target",
+			target: nil,
+			src:    []string{"a", "b", "c"},
+			want:   []string{"a", "b", "c"},
+		},
+		{
+			name:   "nil src",
+			target: []string{"a", "b", "c"},
+			src:    nil,
+			want:   []string{"a", "b", "c"},
+		},
+		{
+			name:   "nil target and empty src",
+			target: nil,
+			src:    []string{},
+			want:   []string{},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := Append(tt.target, tt.src)
+			assert.Equal(t, tt.want, got, "Append() = %v, want %v", got, tt.want)
+		})
+	}
+}

pkg/loader/loader.go

@@ -153,6 +153,10 @@ func (l *specLoader) loadFromStrings(rawSpecs ...string) (*TroubleshootKinds, er
 	// For secrets and configmaps, extract support bundle, redactor or preflight specs
 	// For troubleshoot kinds, pass them through
 	for _, rawDoc := range multiRawDocs {
+		if rawDoc == "" {
+			continue
+		}
+
 		var parsed parsedDoc
 
 		err := yaml.Unmarshal([]byte(rawDoc), &parsed)

pkg/supportbundle/load.go

@@ -225,7 +225,7 @@ func loadSpec(arg string) ([]byte, error) {
 	}
 
 	if !util.IsURL(arg) {
-		return nil, fmt.Errorf("%s is not a URL and was not found (err %s)", arg, err)
+		return nil, fmt.Errorf("%s is not a URL and was not found", arg)
 	}
 
 	spec, err := loadSpecFromURL(arg)