From a3c453d1d6630991f80d97dbbb32906fd9998372 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 15 Apr 2026 22:29:35 +0100 Subject: [PATCH] fix: record skipped collectors in bundle and improve CLI warnings (#2019) * fix: record skipped collectors in bundle and improve CLI warnings - Write `skipped-collectors.json` to the support bundle and preflight bundle recording which collectors were skipped (RBAC permissions or spec exclusion), with reason, errors, and timestamp - Surface RBAC errors and skipped-collector messages as `klog.Warningf` in non-interactive mode so they're visible without `-v` flags - Exec collector file naming now falls back to the resolved container name (from the pod spec) when `collectorName` is not set, instead of only checking `containerName` from the spec Signed-off-by: Evans Mungai * Address comment Signed-off-by: Evans Mungai * fix bugbot comment Signed-off-by: Evans Mungai * Remove unused var Signed-off-by: Evans Mungai * Address comment Signed-off-by: Evans Mungai * Use results.SaveResult and add automated tests Signed-off-by: Evans Mungai * Improvements Signed-off-by: Evans Mungai * Fix bugbot Signed-off-by: Evans Mungai * Handle bugbot Signed-off-by: Evans Mungai --------- Signed-off-by: Evans Mungai --- cmd/troubleshoot/cli/run.go | 13 ++- pkg/collect/collector.go | 32 +++++++ pkg/collect/collector_test.go | 88 +++++++++++++++++++ pkg/collect/exec.go | 7 +- pkg/collect/result.go | 5 +- pkg/preflight/collect.go | 25 ++++++ pkg/supportbundle/collect.go | 32 ++++++- .../skipped_collectors_e2e_test.go | 72 +++++++++++++++ .../spec/skippedCollectors.yaml | 10 +++ 9 files changed, 275 insertions(+), 9 deletions(-) create mode 100644 test/e2e/support-bundle/skipped_collectors_e2e_test.go create mode 100644 test/e2e/support-bundle/spec/skippedCollectors.yaml diff --git a/cmd/troubleshoot/cli/run.go b/cmd/troubleshoot/cli/run.go index b806ef2d..786b67a7 100644 --- a/cmd/troubleshoot/cli/run.go +++ b/cmd/troubleshoot/cli/run.go @@ -166,7 +166,18 @@ func runTroubleshoot(v *viper.Viper, args []string) error { go func() { defer wg.Done() for msg := range progressChan { - klog.Infof("Collecting support bundle: %v", msg) + switch msg := msg.(type) { + case error: + klog.Warningf("Collecting support bundle: %v", msg) + case string: + if strings.Contains(msg, "skipping collector") { + klog.Warningf("Collecting support bundle: %s", msg) + } else { + klog.Infof("Collecting support bundle: %s", msg) + } + default: + klog.Infof("Collecting support bundle: %v", msg) + } } }() } else { diff --git a/pkg/collect/collector.go b/pkg/collect/collector.go index ed42f3dd..bf8a815f 100644 --- a/pkg/collect/collector.go +++ b/pkg/collect/collector.go @@ -1,9 +1,11 @@ package collect import ( + "bytes" "context" "encoding/json" "fmt" + "maps" "strconv" "strings" "time" @@ -13,6 +15,7 @@ import ( "github.com/replicatedhq/troubleshoot/pkg/multitype" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "k8s.io/klog/v2" ) type Collector interface { @@ -296,6 +299,35 @@ func DedupCollectors(allCollectors []*troubleshootv1beta2.Collect) []*troublesho return finalCollectors } +// SkippedCollector records information about a collector that was skipped during collection. +type SkippedCollector struct { + Collector string `json:"collector"` + Reason string `json:"reason"` + Errors []string `json:"errors"` + Timestamp string `json:"timestamp"` +} + +// WriteSkippedCollectors marshals the skipped collectors list and saves it +// using SaveResult which handles both in-memory and on-disk storage. +func WriteSkippedCollectors(skipped []SkippedCollector, allCollectedData map[string][]byte, bundlePath string) { + if len(skipped) == 0 { + return + } + skippedJSON, err := json.Marshal(skipped) + if err != nil { + return + } + + // Either write to bundle path or memory + c := CollectorResult{} + if err := c.SaveResult(bundlePath, "skipped-collectors.json", bytes.NewReader(skippedJSON)); err != nil { + klog.Errorf("Failed to save skipped collectors: %v", err) + } else { + // Write to collected data to return downstream + maps.Copy(allCollectedData, c) + } +} + // Ensure Copy collectors are last in the list // This is because copy collectors are expected to copy files from other collectors such as Exec, RunPod, RunDaemonSet func EnsureCopyLast(allCollectors []Collector) []Collector { diff --git a/pkg/collect/collector_test.go b/pkg/collect/collector_test.go index 9144dbb5..c3ed8c2e 100644 --- a/pkg/collect/collector_test.go +++ b/pkg/collect/collector_test.go @@ -1,6 +1,9 @@ package collect import ( + "encoding/json" + "os" + "path/filepath" "testing" troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2" @@ -519,3 +522,88 @@ func TestEnsureCopyLast(t *testing.T) { }) } } + +func TestWriteSkippedCollectors(t *testing.T) { + tests := []struct { + name string + skipped []SkippedCollector + bundlePath string + useTempDir bool + wantInMap bool + wantOnDisk bool + wantEntries []SkippedCollector + }{ + { + name: "empty skipped list does nothing", + skipped: nil, + bundlePath: "", + wantInMap: false, + }, + { + name: "in-memory only when bundlePath is empty", + skipped: []SkippedCollector{ + {Collector: "clusterResources", Reason: "excluded", Timestamp: "2026-01-01T00:00:00Z"}, + }, + bundlePath: "", + wantInMap: true, + wantOnDisk: false, + wantEntries: []SkippedCollector{ + {Collector: "clusterResources", Reason: "excluded", Timestamp: "2026-01-01T00:00:00Z"}, + }, + }, + { + name: "writes to disk when bundlePath is set", + skipped: []SkippedCollector{ + {Collector: "clusterResources", Reason: "excluded", Timestamp: "2026-01-01T00:00:00Z"}, + {Collector: "logs", Reason: "insufficient RBAC permissions", Errors: []string{"pods is forbidden"}, Timestamp: "2026-01-01T00:00:01Z"}, + }, + useTempDir: true, + wantInMap: true, + wantOnDisk: true, + wantEntries: []SkippedCollector{ + {Collector: "clusterResources", Reason: "excluded", Timestamp: "2026-01-01T00:00:00Z"}, + {Collector: "logs", Reason: "insufficient RBAC permissions", Errors: []string{"pods is forbidden"}, Timestamp: "2026-01-01T00:00:01Z"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + bundlePath := tt.bundlePath + if tt.useTempDir { + bundlePath = t.TempDir() + } + + result := CollectorResult{} + WriteSkippedCollectors(tt.skipped, result, bundlePath) + + if !tt.wantInMap { + assert.Empty(t, result) + return + } + + // Verify in-memory entry exists + if bundlePath == "" { + // In-memory mode: data is stored in the map + data, ok := result["skipped-collectors.json"] + require.True(t, ok, "skipped-collectors.json should be in result map") + require.NotNil(t, data) + + var got []SkippedCollector + require.NoError(t, json.Unmarshal(data, &got)) + assert.Equal(t, tt.wantEntries, got) + } else { + // On-disk mode: map entry exists with nil value, file is on disk + _, ok := result["skipped-collectors.json"] + require.True(t, ok, "skipped-collectors.json should be in result map") + + diskData, err := os.ReadFile(filepath.Join(bundlePath, "skipped-collectors.json")) + require.NoError(t, err) + + var got []SkippedCollector + require.NoError(t, json.Unmarshal(diskData, &got)) + assert.Equal(t, tt.wantEntries, got) + } + }) + } +} diff --git a/pkg/collect/exec.go b/pkg/collect/exec.go index 27ae956d..1aaeb853 100644 --- a/pkg/collect/exec.go +++ b/pkg/collect/exec.go @@ -87,9 +87,14 @@ func execWithoutTimeout(clientConfig *rest.Config, bundlePath string, execCollec pod := pods[0] stdout, stderr, execErrors := getExecOutputs(ctx, clientConfig, client, pod, execCollector) + container := pod.Spec.Containers[0].Name + if execCollector.ContainerName != "" { + container = execCollector.ContainerName + } + filePrefix := execCollector.CollectorName if filePrefix == "" { - filePrefix = execCollector.ContainerName + filePrefix = container } path := filepath.Join(execCollector.Name, pod.Namespace, pod.Name) diff --git a/pkg/collect/result.go b/pkg/collect/result.go index c48fb524..6bbdb216 100644 --- a/pkg/collect/result.go +++ b/pkg/collect/result.go @@ -6,6 +6,7 @@ import ( "compress/gzip" "fmt" "io" + "maps" "os" "path" "path/filepath" @@ -87,9 +88,7 @@ func (r CollectorResult) SymLinkResult(bundlePath, relativeLinkPath, relativeFil // It also ensures that when operating on the results in memory (e.g preflights), // all files are included. func (r CollectorResult) AddResult(other CollectorResult) { - for k, v := range other { - r[k] = v - } + maps.Copy(r, other) } // SaveResult saves the collector result to relativePath file on disk. If bundlePath is diff --git a/pkg/preflight/collect.go b/pkg/preflight/collect.go index 101455da..4ee6be98 100644 --- a/pkg/preflight/collect.go +++ b/pkg/preflight/collect.go @@ -245,6 +245,8 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1 // move Copy Collectors if any to the end of the execution list allCollectors = collect.EnsureCopyLast(allCollectors) + var skippedCollectors []collect.SkippedCollector + for i, collector := range allCollectors { _, span := otel.Tracer(constants.LIB_TRACER_NAME).Start(ctx, collector.Title()) span.SetAttributes(attribute.String("type", reflect.TypeOf(collector).String())) @@ -254,6 +256,13 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1 klog.V(1).Infof("excluding %q collector", collector.Title()) span.SetAttributes(attribute.Bool(constants.EXCLUDED, true)) span.End() + + skippedCollectors = append(skippedCollectors, collect.SkippedCollector{ + Collector: collector.Title(), + Reason: "excluded", + Timestamp: time.Now().Format(time.RFC3339), + }) + continue } @@ -270,6 +279,19 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1 } span.SetStatus(codes.Error, "skipping collector, insufficient RBAC permissions") span.End() + + rbacErrors := collector.GetRBACErrors() + errorMessages := make([]string, 0, len(rbacErrors)) + for _, e := range rbacErrors { + errorMessages = append(errorMessages, e.Error()) + } + skippedCollectors = append(skippedCollectors, collect.SkippedCollector{ + Collector: collector.Title(), + Reason: "insufficient RBAC permissions", + Errors: errorMessages, + Timestamp: time.Now().Format(time.RFC3339), + }) + continue } } @@ -320,6 +342,9 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1 span.End() } + // Write skipped collectors manifest so users can see what was missed + collect.WriteSkippedCollectors(skippedCollectors, allCollectedData, opts.BundlePath) + // The values of map entries will contain the collected data in bytes if the data was not stored to disk collectResult.AllCollectedData = allCollectedData diff --git a/pkg/supportbundle/collect.go b/pkg/supportbundle/collect.go index f4d572d1..0867b348 100644 --- a/pkg/supportbundle/collect.go +++ b/pkg/supportbundle/collect.go @@ -39,9 +39,8 @@ import ( ) const ( - selectorLabelKey = "ds-selector-label" - selectorLabelValue = "remote-host-collector" - defaultTimeout = 30 + selectorLabelKey = "ds-selector-label" + defaultTimeout = 30 ) func runHostCollectors(ctx context.Context, hostCollectors []*troubleshootv1beta2.HostCollect, additionalRedactors *troubleshootv1beta2.Redactor, bundlePath string, opts SupportBundleCreateOpts) (collect.CollectorResult, error) { @@ -107,7 +106,7 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec allCollectorsMap := make(map[reflect.Type][]collect.Collector) collectorTypeOrder := make([]reflect.Type, 0) // Preserve order of collector types - allCollectedData := make(map[string][]byte) + allCollectedData := map[string][]byte{} for _, desiredCollector := range collectSpecs { if collectorInterface, ok := collect.GetCollector(desiredCollector, bundlePath, opts.Namespace, opts.KubernetesRestConfig, k8sClient, opts.SinceTime); ok { @@ -155,6 +154,8 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec // move Copy Collectors if any to the end of the execution list allCollectors = collect.EnsureCopyLast(allCollectors) + var skippedCollectors []collect.SkippedCollector + for _, collector := range allCollectors { _, span := otel.Tracer(constants.LIB_TRACER_NAME).Start(ctx, collector.Title()) span.SetAttributes(attribute.String("type", reflect.TypeOf(collector).String())) @@ -165,6 +166,13 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec opts.CollectorProgressCallback(opts.ProgressChan, msg) span.SetAttributes(attribute.Bool(constants.EXCLUDED, true)) span.End() + + skippedCollectors = append(skippedCollectors, collect.SkippedCollector{ + Collector: collector.Title(), + Reason: "excluded", + Timestamp: time.Now().Format(time.RFC3339), + }) + continue } @@ -175,6 +183,19 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec opts.CollectorProgressCallback(opts.ProgressChan, msg) span.SetStatus(codes.Error, "skipping collector, insufficient RBAC permissions") span.End() + + rbacErrors := collector.GetRBACErrors() + errorMessages := make([]string, 0, len(rbacErrors)) + for _, e := range rbacErrors { + errorMessages = append(errorMessages, e.Error()) + } + skippedCollectors = append(skippedCollectors, collect.SkippedCollector{ + Collector: collector.Title(), + Reason: "insufficient RBAC permissions", + Errors: errorMessages, + Timestamp: time.Now().Format(time.RFC3339), + }) + continue } } @@ -207,6 +228,9 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec span.End() } + // Write skipped collectors manifest to the bundle so users can see what was missed + collect.WriteSkippedCollectors(skippedCollectors, allCollectedData, bundlePath) + collectResult := allCollectedData globalRedactors := []*troubleshootv1beta2.Redact{} diff --git a/test/e2e/support-bundle/skipped_collectors_e2e_test.go b/test/e2e/support-bundle/skipped_collectors_e2e_test.go new file mode 100644 index 00000000..dd0da603 --- /dev/null +++ b/test/e2e/support-bundle/skipped_collectors_e2e_test.go @@ -0,0 +1,72 @@ +package e2e + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/e2e-framework/pkg/envconf" + "sigs.k8s.io/e2e-framework/pkg/features" +) + +func TestSkippedCollectors(t *testing.T) { + feature := features.New("Skipped Collectors"). + Assess("bundle contains skipped-collectors.json for excluded collectors", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + var out bytes.Buffer + + supportBundleName := "skipped-collectors-test" + tarPath := fmt.Sprintf("%s.tar.gz", supportBundleName) + cmd := exec.CommandContext(ctx, sbBinary(), "spec/skippedCollectors.yaml", + "--interactive=false", + fmt.Sprintf("-o=%s", supportBundleName), + ) + cmd.Stdout = &out + cmd.Stderr = &out + err := cmd.Run() + if err != nil { + t.Fatalf("support-bundle command failed: %v\nOutput: %s", err, out.String()) + } + + defer func() { + err := os.Remove(tarPath) + if err != nil { + t.Fatal("Error removing file:", err) + } + }() + + // Read skipped-collectors.json from the bundle + skippedJSON, err := readFileFromTar(tarPath, fmt.Sprintf("%s/skipped-collectors.json", supportBundleName)) + require.NoError(t, err, "skipped-collectors.json should exist in the bundle") + + var skipped []struct { + Collector string `json:"collector"` + Reason string `json:"reason"` + Errors []string `json:"errors"` + Timestamp string `json:"timestamp"` + } + err = json.Unmarshal(skippedJSON, &skipped) + require.NoError(t, err) + + // Both excluded collectors should be recorded + assert.Len(t, skipped, 2) + + collectors := map[string]string{} + for _, s := range skipped { + collectors[s.Collector] = s.Reason + assert.NotEmpty(t, s.Timestamp, "timestamp should be set") + } + + assert.Equal(t, "excluded", collectors["cluster-resources"]) + assert.Equal(t, "excluded", collectors["cluster-info"]) + + return ctx + }).Feature() + + testenv.Test(t, feature) +} diff --git a/test/e2e/support-bundle/spec/skippedCollectors.yaml b/test/e2e/support-bundle/spec/skippedCollectors.yaml new file mode 100644 index 00000000..8a5d14bd --- /dev/null +++ b/test/e2e/support-bundle/spec/skippedCollectors.yaml @@ -0,0 +1,10 @@ +apiVersion: troubleshoot.sh/v1beta2 +kind: SupportBundle +metadata: + name: skipped-collectors-test +spec: + collectors: + - clusterResources: + exclude: true + - clusterInfo: + exclude: true