Fix remote host collection RBAC checks

[diamonwiggins]

Description, Motivation and Context

• Move save node list code to run after we check for RBAC
• Update RBAC checks to cover everything needed for remote host collectors

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

[nvanthao]

many thanks Diamon! LGTM, just a few comments to make sure I understand the changes.

[nvanthao]

should we check on empty namespace?

[diamonwiggins]

Is there a use case where you think we might need to? Remote host collection can execute within a single namespace right?

[nvanthao]

I just think passing empty namespace should not allowed as we are checking RBAC in both cluster-scope and namespace-scoped such as get/pods

[diamonwiggins]

Are you saying we should default to default for the namespace or fail if no namespace is provided? Right now, if the user doesn't provide a namespace when executing remote host collectors via the CLI, then we pretty much assume "" for the namespace.

This may be a larger thing we want to address in the project as I've noticed that the namespace in the kubeconfig context isn't used as the default, and it's not clear if we intend to do so or if we default to assuming cluster scoped access if you don't specify "-n" when running troubleshoot binaries. This would be another one of those issues that's a larger thing we need to revisit in the project as the behavior is the same for in-cluster collectors today.

Let me know if that clears things up, or maybe i'm still not quite following what you're suggesting.

[nvanthao]

this could be a constant declared outside so that we will remember to update it in the future.

[diamonwiggins]

Can you clarify? I'm not sure I follow why moving it from here would make it more visible?

[nvanthao]

I mean setting resourceAttributesList as a package constant would make it more discoverable as a configuration rather than hiding it inside checkRemoteCollectorRBAC. E.g. when we have new collector with additional permission.

On this, I also think we are missing permission create pods/exec for executing collect binary.

[diamonwiggins]

On this, I also think we are missing permission create pods/exec for executing collect binary.

This PR is for the existing code which I don't think uses exec at the moment.

I mean setting resourceAttributesList as a package constant would make it more discoverable as a configuration rather than hiding it inside checkRemoteCollectorRBAC. E.g. when we have new collector with additional permission.

I don't have a super strong opinion here, but it doesn't feel like this list is going to be shared across multiple functions, so I'm not sure it needs to come out of this function.

[banjoh]

LGTM