Deduplication for In-Cluster Collectors (#972)

* adding dedup for in cluster collectors

* add tests

* return collector as is whenever marshalling to json fails

---------

Co-authored-by: Evans Mungai <[email protected]>

Author: diamonwiggins

cmd/troubleshoot/cli/run.go

@@ -328,7 +328,7 @@ the %s Admin Console to begin analysis.`
 			return nil
 		}
 
-		fmt.Printf("%s\n", response.ArchivePath)
+		fmt.Printf("\n%s\n", response.ArchivePath)
 		return nil
 	}
 

pkg/collect/collect.go

@@ -190,31 +190,3 @@ func CollectRemote(c *troubleshootv1beta2.RemoteCollector, additionalRedactors *
 	collectResult.AllCollectedData = allCollectedData
 	return collectResult, nil
 }
-
-// Ensure that the specified collector is in the list of collectors
-func EnsureCollectorInList(list []*troubleshootv1beta2.Collect, collector troubleshootv1beta2.Collect) []*troubleshootv1beta2.Collect {
-	for _, inList := range list {
-		if collector.ClusterResources != nil && inList.ClusterResources != nil {
-			return list
-		}
-		if collector.ClusterInfo != nil && inList.ClusterInfo != nil {
-			return list
-		}
-	}
-
-	return append(list, &collector)
-}
-
-// collect ClusterResources earliest in the list so the pod list does not include pods started by collectors
-func EnsureClusterResourcesFirst(list []*troubleshootv1beta2.Collect) []*troubleshootv1beta2.Collect {
-	sliceOfClusterResources := []*troubleshootv1beta2.Collect{}
-	sliceOfOtherCollectors := []*troubleshootv1beta2.Collect{}
-	for _, collector := range list {
-		if collector.ClusterResources != nil {
-			sliceOfClusterResources = append(sliceOfClusterResources, []*troubleshootv1beta2.Collect{collector}...)
-		} else {
-			sliceOfOtherCollectors = append(sliceOfOtherCollectors, []*troubleshootv1beta2.Collect{collector}...)
-		}
-	}
-	return append(sliceOfClusterResources, sliceOfOtherCollectors...)
-}

pkg/collect/collector.go

@@ -2,6 +2,7 @@ package collect
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strconv"
 	"strings"
@@ -189,3 +190,54 @@ func getCollectorName(c interface{}) string {
 	}
 	return collector
 }
+
+// Ensure that the specified collector is in the list of collectors
+func EnsureCollectorInList(list []*troubleshootv1beta2.Collect, collector troubleshootv1beta2.Collect) []*troubleshootv1beta2.Collect {
+	for _, inList := range list {
+		if collector.ClusterResources != nil && inList.ClusterResources != nil {
+			return list
+		}
+		if collector.ClusterInfo != nil && inList.ClusterInfo != nil {
+			return list
+		}
+	}
+
+	return append(list, &collector)
+}
+
+// collect ClusterResources earliest in the list so the pod list does not include pods started by collectors
+func EnsureClusterResourcesFirst(list []*troubleshootv1beta2.Collect) []*troubleshootv1beta2.Collect {
+	sliceOfClusterResources := []*troubleshootv1beta2.Collect{}
+	sliceOfOtherCollectors := []*troubleshootv1beta2.Collect{}
+	for _, collector := range list {
+		if collector.ClusterResources != nil {
+			sliceOfClusterResources = append(sliceOfClusterResources, []*troubleshootv1beta2.Collect{collector}...)
+		} else {
+			sliceOfOtherCollectors = append(sliceOfOtherCollectors, []*troubleshootv1beta2.Collect{collector}...)
+		}
+	}
+	return append(sliceOfClusterResources, sliceOfOtherCollectors...)
+}
+
+// deduplicates a list of troubleshootv1beta2.Collect objects
+// marshals object to json and then uses its string value to check for uniqueness
+// there is no sorting of the keys in the collect object's spec so if the spec isn't an exact match line for line as written, no dedup will occur
+func DedupCollectors(allCollectors []*troubleshootv1beta2.Collect) []*troubleshootv1beta2.Collect {
+	uniqueCollectors := make(map[string]bool)
+	finalCollectors := []*troubleshootv1beta2.Collect{}
+
+	for _, collector := range allCollectors {
+		data, err := json.Marshal(collector)
+		if err != nil {
+			// return collector as is if for whatever reason it can't be marshalled into json
+			finalCollectors = append(finalCollectors, collector)
+		} else {
+			stringData := string(data)
+			if _, value := uniqueCollectors[stringData]; !value {
+				uniqueCollectors[stringData] = true
+				finalCollectors = append(finalCollectors, collector)
+			}
+		}
+	}
+	return finalCollectors
+}

pkg/collect/collector_test.go

@@ -5,6 +5,7 @@ import (
 
 	troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2"
 	"github.com/replicatedhq/troubleshoot/pkg/multitype"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -364,3 +365,83 @@ pwd=somethinggoeshere;`,
 		})
 	}
 }
+
+func TestCollector_DedupCollectors(t *testing.T) {
+	tests := []struct {
+		name       string
+		Collectors []*troubleshootv1beta2.Collect
+		want       []*troubleshootv1beta2.Collect
+	}{
+		{
+			name: "multiple cluster info",
+			Collectors: []*troubleshootv1beta2.Collect{
+				{
+					ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
+				},
+				{
+					ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
+				},
+			},
+			want: []*troubleshootv1beta2.Collect{
+				{
+					ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
+				},
+			},
+		},
+		{
+			name: "multiple cluster resources with matching namespace lists",
+			Collectors: []*troubleshootv1beta2.Collect{
+				{
+					ClusterResources: &troubleshootv1beta2.ClusterResources{
+						Namespaces: []string{"namespace1", "namespace2"},
+					},
+				},
+				{
+					ClusterResources: &troubleshootv1beta2.ClusterResources{
+						Namespaces: []string{"namespace1", "namespace2"},
+					},
+				},
+			},
+			want: []*troubleshootv1beta2.Collect{
+				{
+					ClusterResources: &troubleshootv1beta2.ClusterResources{
+						Namespaces: []string{"namespace1", "namespace2"},
+					},
+				},
+			},
+		},
+		{
+			name: "multiple cluster resources with unnique namespace lists",
+			Collectors: []*troubleshootv1beta2.Collect{
+				{
+					ClusterResources: &troubleshootv1beta2.ClusterResources{
+						Namespaces: []string{"namespace1", "namespace2"},
+					},
+				},
+				{
+					ClusterResources: &troubleshootv1beta2.ClusterResources{
+						Namespaces: []string{"namespace1000", "namespace2000"},
+					},
+				},
+			},
+			want: []*troubleshootv1beta2.Collect{
+				{
+					ClusterResources: &troubleshootv1beta2.ClusterResources{
+						Namespaces: []string{"namespace1", "namespace2"},
+					},
+				},
+				{
+					ClusterResources: &troubleshootv1beta2.ClusterResources{
+						Namespaces: []string{"namespace1000", "namespace2000"},
+					},
+				},
+			},
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := DedupCollectors(tc.Collectors)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}

pkg/preflight/collect.go

@@ -135,6 +135,7 @@ func Collect(opts CollectOpts, p *troubleshootv1beta2.Preflight) (CollectResult,
 	}
 	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)
 
 	opts.KubernetesRestConfig.QPS = constants.DEFAULT_CLIENT_QPS

pkg/supportbundle/collect.go

@@ -76,6 +76,7 @@ func runCollectors(collectors []*troubleshootv1beta2.Collect, additionalRedactor
 	collectSpecs = append(collectSpecs, 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)
 
 	opts.KubernetesRestConfig.QPS = constants.DEFAULT_CLIENT_QPS