fix (supportbundle): Add default collectors when expected

[banjoh]

Description, Motivation and Context

Ensure default collectors (clusterInfo and clusterResources) are added by default whenever required when running support-bundle. Below are specs representing possible combinations

• Defaults added

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  collectors: []

• Defaults added

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  collectors:
  - logs: {}

• Defaults added (similar to when spec: {} is present. spec is not mandatory. This depicts the minimum spec one can create)

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle

• Defaults NOT added

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  hostCollectors:
  - cpu: {}

• Defaults NOT added in the resulting merged spec

apiVersion: troubleshoot.sh/v1beta2
kind: HostCollector
spec:
  collectors:
  - cpu: {}

• Defaults added

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  hostCollectors:
  - cpu: {}
  collectors:
  - logs: {}

• Defaults NOT added. collectors: [] is treated as if its absent

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  hostCollectors:
  - cpu: {}
  collectors: []

Fixes: #1417

Checklist

• New and existing tests pass locally with introduced changes.
• Tests for the changes have been added (for bug fixes / features)
• The commit message(s) are informative and highlight any breaking changes
• Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

• Yes
• No

[chris-sanders]

So this I expected

Defaults added:

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  collectors: []

But this one I'm surprised by:
Defaults NOT added:

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  hostCollectors:
  - cpu: {}
  collectors: []

Is that latter an intentional choice or a side effect of how the object is created. I would prefer these both add defaults, it looks like they would.

Wait this doesn't have defaults either?

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  hostCollectors:
  - cpu: {}

So is there an intentional rule, if hostCollectors are added don't add default collectors? Is that the logic?

[diamonwiggins]

So is there an intentional rule, if hostCollectors are added don't add default collectors? Is that the logic?

I think this is what we did in the past to avoid any code paths that would try contacting a Kubernetes cluster if there were only host collectors/analyzers in the spec. As in we figured it was intentional that's the behavior you wanted it you didn't at least include an empty collector/analyzer property.

[DexterYan]

According the test cases, I saw

• Defaults added

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  analyzers: []

just to confirm it will not have Defaults as follow, right?

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  hostCollectors:
  - cpu: {}
  analyzers: []

[banjoh]

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
hostCollectors:

• cpu: {}
  analyzers: []

Correct. Defaults will not be added. analyzers: [] is also treated the same as being absent

[banjoh]

But this one I'm surprised by: Defaults NOT added:

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  hostCollectors:
  - cpu: {}
  collectors: []

Is that latter an intentional choice or a side effect of how the object is created.

Yes, its intentional. Its code that was added a while back as commented in #1418 (comment)

[banjoh]

The logic in troubleshoot treats zero length lists the same as being absent for collectors, analyzers, and redactors

[xavpaice]

I read this as a bugfix, making things behave as we originally intended.