Skip to content

Commit e4363a1

Browse files
banjohbanjoh
authored
fix (supportbundle): Add default collectors when expected (#1418)
* fix (supportbundle): Add default collectors when expected * Remove unnecessary change * Add default collectors to a empty spec * Add more tests
1 parent b5996d0 commit e4363a1

File tree

5 files changed

+214
-18
lines changed

5 files changed

+214
-18
lines changed

‎cmd/troubleshoot/cli/run.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -315,11 +315,12 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface)
315315
mainBundle.Spec.HostCollectors = util.Append(mainBundle.Spec.HostCollectors, hc.Spec.Collectors)
316316
}
317317

318-
if mainBundle.Spec.Collectors != nil {
319-
// If we have in-cluster collectors, ensure cluster info and cluster resources
320-
// collectors are in the merged spec. We need to add them here so when we --dry-run,
321-
// these collectors are included. supportbundle.runCollectors duplicates this bit.
322-
// We'll need to refactor it out later when its clearer what other code depends on this logic e.g KOTS
318+
if !(len(mainBundle.Spec.HostCollectors) > 0 && len(mainBundle.Spec.Collectors) == 0) {
319+
// Always add default collectors unless we only have host collectors
320+
// We need to add them here so when we --dry-run, these collectors
321+
// are included. supportbundle.runCollectors duplicates this bit.
322+
// We'll need to refactor it out later when its clearer what other
323+
// code depends on this logic e.g KOTS
323324
mainBundle.Spec.Collectors = collect.EnsureCollectorInList(
324325
mainBundle.Spec.Collectors,
325326
troubleshootv1beta2.Collect{ClusterInfo: &troubleshootv1beta2.ClusterInfo{}},

‎cmd/troubleshoot/cli/run_test.go‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,13 @@ import (
99
"testing"
1010
"time"
1111

12+
"github.com/replicatedhq/troubleshoot/internal/testutils"
13+
troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2"
1214
"github.com/replicatedhq/troubleshoot/pkg/httputil"
1315
"github.com/replicatedhq/troubleshoot/pkg/loader"
1416
"github.com/stretchr/testify/assert"
1517
"github.com/stretchr/testify/require"
18+
testclient "k8s.io/client-go/kubernetes/fake"
1619
)
1720

1821
var orig = `
@@ -90,3 +93,186 @@ func Test_loadSupportBundleSpecsFromURIs_TimeoutError(t *testing.T) {
9093

9194
assert.JSONEq(t, string(beforeJSON), string(afterJSON))
9295
}
96+
97+
func Test_loadSupportBundleSpecs(t *testing.T) {
98+
tests := []struct {
99+
name string
100+
args []string
101+
expected troubleshootv1beta2.SupportBundleSpec
102+
}{
103+
{
104+
name: "empty collectors array in spec, default collectors added",
105+
args: []string{testutils.ServeFromFilePath(t, `
106+
apiVersion: troubleshoot.sh/v1beta2
107+
kind: SupportBundle
108+
spec:
109+
collectors: []
110+
`)},
111+
expected: troubleshootv1beta2.SupportBundleSpec{
112+
Collectors: []*troubleshootv1beta2.Collect{
113+
{
114+
ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
115+
},
116+
{
117+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
118+
},
119+
},
120+
},
121+
},
122+
{
123+
name: "no collectors defined in spec, default collectors added",
124+
args: []string{testutils.ServeFromFilePath(t, `
125+
apiVersion: troubleshoot.sh/v1beta2
126+
kind: SupportBundle
127+
spec:
128+
analyzers: []
129+
`)},
130+
expected: troubleshootv1beta2.SupportBundleSpec{
131+
Collectors: []*troubleshootv1beta2.Collect{
132+
{
133+
ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
134+
},
135+
{
136+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
137+
},
138+
},
139+
Analyzers: []*troubleshootv1beta2.Analyze{},
140+
},
141+
},
142+
{
143+
name: "collectors present but defaults missing, default collectors added",
144+
args: []string{testutils.ServeFromFilePath(t, `
145+
apiVersion: troubleshoot.sh/v1beta2
146+
kind: SupportBundle
147+
spec:
148+
collectors:
149+
- logs: {}
150+
`)},
151+
expected: troubleshootv1beta2.SupportBundleSpec{
152+
Collectors: []*troubleshootv1beta2.Collect{
153+
{
154+
Logs: &troubleshootv1beta2.Logs{},
155+
},
156+
{
157+
ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
158+
},
159+
{
160+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
161+
},
162+
},
163+
},
164+
},
165+
{
166+
name: "empty support bundle spec adds default collectors",
167+
args: []string{testutils.ServeFromFilePath(t, `
168+
apiVersion: troubleshoot.sh/v1beta2
169+
kind: SupportBundle
170+
`)},
171+
expected: troubleshootv1beta2.SupportBundleSpec{
172+
Collectors: []*troubleshootv1beta2.Collect{
173+
{
174+
ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
175+
},
176+
{
177+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
178+
},
179+
},
180+
},
181+
},
182+
{
183+
name: "sb spec with host collectors does not add default collectors",
184+
args: []string{testutils.ServeFromFilePath(t, `
185+
apiVersion: troubleshoot.sh/v1beta2
186+
kind: SupportBundle
187+
spec:
188+
hostCollectors:
189+
- cpu: {}
190+
`)},
191+
expected: troubleshootv1beta2.SupportBundleSpec{
192+
HostCollectors: []*troubleshootv1beta2.HostCollect{
193+
{
194+
CPU: &troubleshootv1beta2.CPU{},
195+
},
196+
},
197+
},
198+
},
199+
{
200+
name: "host collector spec with collectors does not add default collectors",
201+
args: []string{testutils.ServeFromFilePath(t, `
202+
apiVersion: troubleshoot.sh/v1beta2
203+
kind: HostCollector
204+
spec:
205+
collectors:
206+
- cpu: {}
207+
`)},
208+
expected: troubleshootv1beta2.SupportBundleSpec{
209+
HostCollectors: []*troubleshootv1beta2.HostCollect{
210+
{
211+
CPU: &troubleshootv1beta2.CPU{},
212+
},
213+
},
214+
},
215+
},
216+
{
217+
name: "sb spec with host and in-cluster collectors adds default collectors",
218+
args: []string{testutils.ServeFromFilePath(t, `
219+
apiVersion: troubleshoot.sh/v1beta2
220+
kind: SupportBundle
221+
spec:
222+
hostCollectors:
223+
- cpu: {}
224+
collectors:
225+
- logs: {}
226+
`)},
227+
expected: troubleshootv1beta2.SupportBundleSpec{
228+
HostCollectors: []*troubleshootv1beta2.HostCollect{
229+
{
230+
CPU: &troubleshootv1beta2.CPU{},
231+
},
232+
},
233+
Collectors: []*troubleshootv1beta2.Collect{
234+
{
235+
Logs: &troubleshootv1beta2.Logs{},
236+
},
237+
{
238+
ClusterInfo: &troubleshootv1beta2.ClusterInfo{},
239+
},
240+
{
241+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
242+
},
243+
},
244+
},
245+
},
246+
{
247+
name: "sb spec with host collectors and empty in-cluster collectors does not default collectors",
248+
args: []string{testutils.ServeFromFilePath(t, `
249+
apiVersion: troubleshoot.sh/v1beta2
250+
kind: SupportBundle
251+
spec:
252+
hostCollectors:
253+
- cpu: {}
254+
collectors: []
255+
`)},
256+
expected: troubleshootv1beta2.SupportBundleSpec{
257+
HostCollectors: []*troubleshootv1beta2.HostCollect{
258+
{
259+
CPU: &troubleshootv1beta2.CPU{},
260+
},
261+
},
262+
Collectors: []*troubleshootv1beta2.Collect{},
263+
},
264+
},
265+
}
266+
267+
for _, test := range tests {
268+
t.Run(test.name, func(t *testing.T) {
269+
ctx := context.Background()
270+
client := testclient.NewSimpleClientset()
271+
272+
sb, _, err := loadSpecs(ctx, test.args, client)
273+
require.NoError(t, err)
274+
275+
assert.Equal(t, test.expected, sb.Spec)
276+
})
277+
}
278+
}

‎internal/testutils/utils.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,13 @@ func FileDir() string {
4141

4242
// Generates a temporary filename
4343
func TempFilename(prefix string) string {
44+
return filepath.Join(os.TempDir(), generateTempName(prefix))
45+
}
46+
47+
func generateTempName(prefix string) string {
4448
randBytes := make([]byte, 16)
4549
rand.Read(randBytes)
46-
return filepath.Join(os.TempDir(), fmt.Sprintf("%s_%s", prefix, hex.EncodeToString(randBytes)))
50+
return fmt.Sprintf("%s_%s", prefix, hex.EncodeToString(randBytes))
4751
}
4852

4953
func CreateTestFile(t *testing.T, path string) {
@@ -78,3 +82,11 @@ func AsJSON(t *testing.T, v interface{}) string {
7882
return string(b)
7983
}
8084
}
85+
86+
func ServeFromFilePath(t *testing.T, data string) string {
87+
t.Helper()
88+
89+
path := filepath.Join(t.TempDir(), generateTempName("testfile"))
90+
CreateTestFileWithData(t, path, data)
91+
return path
92+
}

‎internal/util/util.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,13 +54,9 @@ func EstimateNumberOfLines(text string) int {
5454
}
5555

5656
// Append appends elements in src to target.
57-
// We have this function because of how append()
58-
// treats nil slices the same as empty slices.
59-
// An empty array in YAML like below is not the
60-
// same as when the array is not specified.
61-
//
62-
// spec:
63-
// collectors: []
57+
// We have this function because of how the
58+
// builtin append() function works. It treats
59+
// target nil slices the same as empty slices.
6460
func Append[T any](target []T, src []T) []T {
6561
// Do nothing only if src is nil
6662
if src == nil {

‎pkg/supportbundle/supportbundle.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/fatih/color"
1515
"github.com/pkg/errors"
1616
"github.com/replicatedhq/troubleshoot/internal/traces"
17+
"github.com/replicatedhq/troubleshoot/internal/util"
1718
analyzer "github.com/replicatedhq/troubleshoot/pkg/analyze"
1819
troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2"
1920
"github.com/replicatedhq/troubleshoot/pkg/collect"
@@ -276,11 +277,11 @@ func ConcatSpec(target *troubleshootv1beta2.SupportBundle, source *troubleshootv
276277
newBundle = source
277278
} else {
278279
newBundle = target.DeepCopy()
279-
newBundle.Spec.Collectors = append(target.Spec.Collectors, source.Spec.Collectors...)
280-
newBundle.Spec.AfterCollection = append(target.Spec.AfterCollection, source.Spec.AfterCollection...)
281-
newBundle.Spec.HostCollectors = append(target.Spec.HostCollectors, source.Spec.HostCollectors...)
282-
newBundle.Spec.HostAnalyzers = append(target.Spec.HostAnalyzers, source.Spec.HostAnalyzers...)
283-
newBundle.Spec.Analyzers = append(target.Spec.Analyzers, source.Spec.Analyzers...)
280+
newBundle.Spec.Collectors = util.Append(target.Spec.Collectors, source.Spec.Collectors)
281+
newBundle.Spec.AfterCollection = util.Append(target.Spec.AfterCollection, source.Spec.AfterCollection)
282+
newBundle.Spec.HostCollectors = util.Append(target.Spec.HostCollectors, source.Spec.HostCollectors)
283+
newBundle.Spec.HostAnalyzers = util.Append(target.Spec.HostAnalyzers, source.Spec.HostAnalyzers)
284+
newBundle.Spec.Analyzers = util.Append(target.Spec.Analyzers, source.Spec.Analyzers)
284285
// TODO: What to do with the Uri field?
285286
}
286287
return newBundle

0 commit comments

Comments
 (0)