From e45e2cadd3c91f0c2dcda28f3b8d77ccdc70c5f3 Mon Sep 17 00:00:00 2001 From: Xav Paice Date: Wed, 26 Nov 2025 15:34:17 +1300 Subject: [PATCH] Fix collector ordering: preserve order when grouping by type (#1935) - Fix issue where EnsureClusterResourcesFirst ordering was lost when collectors were grouped by type into a map (Go maps have random iteration order) - Preserve collector type order by tracking collectorTypeOrder slice as collectors are added to the map - Apply fix to both pkg/preflight/collect.go and pkg/supportbundle/collect.go - Add comprehensive tests to verify clusterResources runs first and relative order of other collectors is preserved - Enhance EnsureClusterResourcesFirst tests with additional edge cases --- pkg/collect/collect_test.go | 95 ++++++++++++++- pkg/preflight/collect.go | 8 +- pkg/preflight/collect_test.go | 211 ++++++++++++++++++++++++++++++++++ pkg/supportbundle/collect.go | 8 +- 4 files changed, 319 insertions(+), 3 deletions(-) create mode 100644 pkg/preflight/collect_test.go diff --git a/pkg/collect/collect_test.go b/pkg/collect/collect_test.go index 93f7eeaa..b6c97769 100644 --- a/pkg/collect/collect_test.go +++ b/pkg/collect/collect_test.go @@ -15,7 +15,7 @@ func Test_ensureClusterResourcesFirst(t *testing.T) { list []*troubleshootv1beta2.Collect }{ { - name: "Reorg OK", + name: "Reorg OK - clusterResources moved to front", want: []*troubleshootv1beta2.Collect{ { ClusterResources: &troubleshootv1beta2.ClusterResources{}, @@ -33,6 +33,99 @@ func Test_ensureClusterResourcesFirst(t *testing.T) { }, }, }, + { + name: "Already first - no change", + want: []*troubleshootv1beta2.Collect{ + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + { + Data: &troubleshootv1beta2.Data{}, + }, + { + Secret: &troubleshootv1beta2.Secret{}, + }, + }, + list: []*troubleshootv1beta2.Collect{ + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + { + Data: &troubleshootv1beta2.Data{}, + }, + { + Secret: &troubleshootv1beta2.Secret{}, + }, + }, + }, + { + name: "Multiple clusterResources - all moved to front", + want: []*troubleshootv1beta2.Collect{ + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + { + Data: &troubleshootv1beta2.Data{}, + }, + { + Secret: &troubleshootv1beta2.Secret{}, + }, + }, + list: []*troubleshootv1beta2.Collect{ + { + Data: &troubleshootv1beta2.Data{}, + }, + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + { + Secret: &troubleshootv1beta2.Secret{}, + }, + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + }, + }, + { + name: "No clusterResources - no change", + want: []*troubleshootv1beta2.Collect{ + { + Data: &troubleshootv1beta2.Data{}, + }, + { + Secret: &troubleshootv1beta2.Secret{}, + }, + }, + list: []*troubleshootv1beta2.Collect{ + { + Data: &troubleshootv1beta2.Data{}, + }, + { + Secret: &troubleshootv1beta2.Secret{}, + }, + }, + }, + { + name: "Empty list - no change", + want: []*troubleshootv1beta2.Collect{}, + list: []*troubleshootv1beta2.Collect{}, + }, + { + name: "Only clusterResources - no change", + want: []*troubleshootv1beta2.Collect{ + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + }, + list: []*troubleshootv1beta2.Collect{ + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + }, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/preflight/collect.go b/pkg/preflight/collect.go index 9d4e6588..101455da 100644 --- a/pkg/preflight/collect.go +++ b/pkg/preflight/collect.go @@ -183,6 +183,7 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1 } allCollectorsMap := make(map[reflect.Type][]collect.Collector) + collectorTypeOrder := make([]reflect.Type, 0) // Preserve order of collector types allCollectedData := make(map[string][]byte) for _, desiredCollector := range collectSpecs { @@ -193,6 +194,9 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1 return nil, errors.Wrap(err, "failed to check RBAC for collectors") } collectorType := reflect.TypeOf(collector) + if _, exists := allCollectorsMap[collectorType]; !exists { + collectorTypeOrder = append(collectorTypeOrder, collectorType) + } allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector) } } @@ -200,7 +204,9 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1 collectorList := map[string]CollectorStatus{} - for _, collectors := range allCollectorsMap { + // Iterate over collector types in the order they appeared in collectSpecs + for _, collectorType := range collectorTypeOrder { + collectors := allCollectorsMap[collectorType] if mergeCollector, ok := collectors[0].(collect.MergeableCollector); ok { mergedCollectors, err := mergeCollector.Merge(collectors) if err != nil { diff --git a/pkg/preflight/collect_test.go b/pkg/preflight/collect_test.go new file mode 100644 index 00000000..7c09501d --- /dev/null +++ b/pkg/preflight/collect_test.go @@ -0,0 +1,211 @@ +package preflight + +import ( + "reflect" + "testing" + + troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2" + "github.com/replicatedhq/troubleshoot/pkg/collect" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" +) + +// TestCollectWithContext_ClusterResourcesFirst verifies that clusterResources +// collector runs first, even when it's not first in the spec. +func TestCollectWithContext_ClusterResourcesFirst(t *testing.T) { + // Create a preflight spec with collectors in a specific order + // where clusterResources is NOT first + preflight := &troubleshootv1beta2.Preflight{ + Spec: troubleshootv1beta2.PreflightSpec{ + Collectors: []*troubleshootv1beta2.Collect{ + { + Data: &troubleshootv1beta2.Data{ + CollectorMeta: troubleshootv1beta2.CollectorMeta{ + CollectorName: "test-data", + }, + Name: "test.json", + Data: `{"test": "data"}`, + }, + }, + { + ClusterResources: &troubleshootv1beta2.ClusterResources{}, + }, + }, + }, + } + + // Use a fake Kubernetes client to avoid network calls + fakeClient := fake.NewSimpleClientset() + restConfig := &rest.Config{ + Host: "https://fake-host", + } + + opts := CollectOpts{ + Namespace: "default", + KubernetesRestConfig: restConfig, + ProgressChan: make(chan interface{}, 100), + BundlePath: t.TempDir(), + IgnorePermissionErrors: true, // Ignore RBAC errors in tests + } + + // Manually test the ordering logic by simulating what CollectWithContext does + collectSpecs := make([]*troubleshootv1beta2.Collect, 0) + if preflight.Spec.Collectors != nil { + collectSpecs = append(collectSpecs, preflight.Spec.Collectors...) + } + collectSpecs = collect.EnsureCollectorInList( + collectSpecs, troubleshootv1beta2.Collect{ClusterInfo: &troubleshootv1beta2.ClusterInfo{}}, + ) + collectSpecs = collect.EnsureCollectorInList( + collectSpecs, troubleshootv1beta2.Collect{ClusterResources: &troubleshootv1beta2.ClusterResources{}}, + ) + collectSpecs = collect.DedupCollectors(collectSpecs) + collectSpecs = collect.EnsureClusterResourcesFirst(collectSpecs) + + // Verify clusterResources is first in the specs + require.NotEmpty(t, collectSpecs, "should have collectors") + require.NotNil(t, collectSpecs[0].ClusterResources, "first collector should be clusterResources") + + // Now simulate the map grouping and order preservation + allCollectorsMap := make(map[reflect.Type][]collect.Collector) + collectorTypeOrder := make([]reflect.Type, 0) + + for _, desiredCollector := range collectSpecs { + if collectorInterface, ok := collect.GetCollector(desiredCollector, opts.BundlePath, opts.Namespace, opts.KubernetesRestConfig, fakeClient, nil); ok { + if collector, ok := collectorInterface.(collect.Collector); ok { + // Skip RBAC check for this unit test + collectorType := reflect.TypeOf(collector) + if _, exists := allCollectorsMap[collectorType]; !exists { + collectorTypeOrder = append(collectorTypeOrder, collectorType) + } + allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector) + } + } + } + + // Verify that clusterResources type is first in the order + require.NotEmpty(t, collectorTypeOrder, "should have collector types") + + // Find the clusterResources type by checking the actual collectors + var clusterResourcesType reflect.Type + for collectorType, collectors := range allCollectorsMap { + if len(collectors) > 0 { + if _, ok := collectors[0].(*collect.CollectClusterResources); ok { + clusterResourcesType = collectorType + break + } + } + } + require.NotNil(t, clusterResourcesType, "should find clusterResources type") + assert.Equal(t, clusterResourcesType, collectorTypeOrder[0], "clusterResources type should be first in collectorTypeOrder") +} + +// TestCollectWithContext_PreservesOrderAfterClusterResources verifies that +// after clusterResources, other collectors maintain their relative order. +func TestCollectWithContext_PreservesOrderAfterClusterResources(t *testing.T) { + // Create a preflight spec with multiple collectors in a specific order + preflight := &troubleshootv1beta2.Preflight{ + Spec: troubleshootv1beta2.PreflightSpec{ + Collectors: []*troubleshootv1beta2.Collect{ + { + Data: &troubleshootv1beta2.Data{ + CollectorMeta: troubleshootv1beta2.CollectorMeta{ + CollectorName: "data-first", + }, + Name: "first.json", + Data: `{"first": "data"}`, + }, + }, + { + Secret: &troubleshootv1beta2.Secret{ + CollectorMeta: troubleshootv1beta2.CollectorMeta{ + CollectorName: "secret-second", + }, + }, + }, + }, + }, + } + + // Use a fake Kubernetes client + fakeClient := fake.NewSimpleClientset() + restConfig := &rest.Config{ + Host: "https://fake-host", + } + + opts := CollectOpts{ + Namespace: "default", + KubernetesRestConfig: restConfig, + ProgressChan: make(chan interface{}, 100), + BundlePath: t.TempDir(), + IgnorePermissionErrors: true, + } + + // Simulate the ordering logic + collectSpecs := make([]*troubleshootv1beta2.Collect, 0) + if preflight.Spec.Collectors != nil { + collectSpecs = append(collectSpecs, preflight.Spec.Collectors...) + } + collectSpecs = collect.EnsureCollectorInList( + collectSpecs, troubleshootv1beta2.Collect{ClusterInfo: &troubleshootv1beta2.ClusterInfo{}}, + ) + collectSpecs = collect.EnsureCollectorInList( + collectSpecs, troubleshootv1beta2.Collect{ClusterResources: &troubleshootv1beta2.ClusterResources{}}, + ) + collectSpecs = collect.DedupCollectors(collectSpecs) + collectSpecs = collect.EnsureClusterResourcesFirst(collectSpecs) + + // Group collectors by type and track order + allCollectorsMap := make(map[reflect.Type][]collect.Collector) + collectorTypeOrder := make([]reflect.Type, 0) + + for _, desiredCollector := range collectSpecs { + if collectorInterface, ok := collect.GetCollector(desiredCollector, opts.BundlePath, opts.Namespace, opts.KubernetesRestConfig, fakeClient, nil); ok { + if collector, ok := collectorInterface.(collect.Collector); ok { + collectorType := reflect.TypeOf(collector) + if _, exists := allCollectorsMap[collectorType]; !exists { + collectorTypeOrder = append(collectorTypeOrder, collectorType) + } + allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector) + } + } + } + + // Verify clusterResources is first + require.NotEmpty(t, collectorTypeOrder, "should have collector types") + + // Find the actual types from the collectors + var clusterResourcesType, dataType, secretType reflect.Type + for collectorType, collectors := range allCollectorsMap { + if len(collectors) > 0 { + switch collectors[0].(type) { + case *collect.CollectClusterResources: + clusterResourcesType = collectorType + case *collect.CollectData: + dataType = collectorType + case *collect.CollectSecret: + secretType = collectorType + } + } + } + + require.NotNil(t, clusterResourcesType, "should find clusterResources type") + assert.Equal(t, clusterResourcesType, collectorTypeOrder[0], "clusterResources should be first") + + dataIndex := -1 + secretIndex := -1 + for i, ct := range collectorTypeOrder { + if ct == dataType { + dataIndex = i + } + if ct == secretType { + secretIndex = i + } + } + + if dataIndex >= 0 && secretIndex >= 0 { + assert.Less(t, dataIndex, secretIndex, "data collectors should come before secret collectors, preserving relative order") + } +} diff --git a/pkg/supportbundle/collect.go b/pkg/supportbundle/collect.go index 83202cde..fa5043c9 100644 --- a/pkg/supportbundle/collect.go +++ b/pkg/supportbundle/collect.go @@ -106,6 +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) for _, desiredCollector := range collectSpecs { @@ -116,12 +117,17 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec return nil, errors.Wrap(err, "failed to check RBAC for collectors") } collectorType := reflect.TypeOf(collector) + if _, exists := allCollectorsMap[collectorType]; !exists { + collectorTypeOrder = append(collectorTypeOrder, collectorType) + } allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector) } } } - for _, collectors := range allCollectorsMap { + // Iterate over collector types in the order they appeared in collectSpecs + for _, collectorType := range collectorTypeOrder { + collectors := allCollectorsMap[collectorType] if mergeCollector, ok := collectors[0].(collect.MergeableCollector); ok { mergedCollectors, err := mergeCollector.Merge(collectors) if err != nil {