BREAKING: add support for k8s disable-node-collector flag

[chen-keinan]

Description

Add support for k8s intrusive flag

Related issues

• Close trivy k8s: --disable-node-collector flag #6194

Checklist

• I've read the guidelines for contributing to this repository.
• I've followed the conventions in the PR title.
• I've added tests that prove my fix is effective or that my feature works.
• I've updated the documentation with the relevant information (if needed).

[knqyf263]

It's a breaking change, right? We need a BREAKING prefix in the PR title.

trivy/.github/workflows/semantic-pr.yaml

Line 31 in df024e8

BREAKING

Also, the integration test is failing.

[chen-keinan]

@knqyf263 @itaysk is --non-intrusive more appropriate than --skip-intrusive as a flag name ?

[knqyf263]

First of all, I don't mind. @itaysk and you can decide it. I'm just thinking out loud. If the default mode is intrusive, should we consider a more understated name for the non-intrusive mode (such as --passive or --unobtrusive) rather than --non-intrusive?

In some cases, negating a well-known or intuitive term can effectively convey the intended meaning. If "intrusive" is a term that users are likely to understand in the context of Trivy, then using "non-intrusive" as the flag name might be the most straightforward way to communicate the opposite behavior.

[itaysk]

In my opinion, --intrusive/--non-intrusive is fine. even though it doesn't convey the actual benefit for the user (like node-collect or deep or something like that), it gives the more important decision factor which is that this will actually create a job in the scanned cluster. I think generally a scanner is expected to be read-only, and manipulating the scan target is unconventional, and the user should understand the consequence.
This makes me think that perhaps it's even better to default to non-intrusive, and make the user explicitly say --intrusive in order to deploy the job, because there's inherent friction and risk in this action (as users reported). Even if we want and recommend that most users will do intrusive scan (and all our docs will include it by default), this puts the responsibility on the user for initiating the intrusive action rather than the tool to automatically do so. Akin to some tools requiring you to add --yes in order to perform the action.

TL;DR:
I think --intrusive/--non-intrusive is the best terminology, and I'm leaning towards non-intrusive by default (but not fixed on this)

[itaysk]

after discussing this offline with @chen-keinan we agreed to not change the current default behavior, meaning by default the scan will be intrusive and the user can set a --non-intrusive flag to opt out.

[knqyf263]

I know the context well and it is difficult to get an impartial view of how people feel about current behavior. I'd request a quick review from @simar7.

[simar7]

I know the context well and it is difficult to get an impartial view of how people feel about current behavior. I'd request a quick review from @simar7.

As I was summoned, I would like to provide my feedback.

I think there are two problems to solve here:

1. Flag name
2. Flag default behavior

Problem 1
I think personally, I would lean towards flags that are more "obvious" or "commonly used". To me "intrusive" (or lack thereof) doesn't tell me what's really intrusive about it. To understand this I would have to read the Trivy docs to know the default behavior (or read the help for CLI flags).

While as something like --node-collector-disable (or something similar) conveys exactly what is being done, aka node collector scan results are being disabled. This also lines up well with existing --node-collector-* flags that we currently have within Trivy.

Problem 2
If we change the flag name to something as I mentioned above, we can keep the current behavior as-is and have this flag opt-in. This would help us not make a breaking change.

[knqyf263]

Thanks @simar7. Feedback from someone unfamiliar with k8s scanning is valuable.

On the other hand, we may be able to make the assumption that people who use the k8s subcommand should be somewhat familiar with it.

We can also take the view that it is okay because the CLI help provides supplementary explanations.

      --non-intrusive                     When the flag is activated, the node-collector job will not be executed, thus skipping misconfiguration findings on the node.

The final decision should be made by the code owner, Chen, so I'll approve it.

[itaysk]

I agree with simar and wrote similar gist on the --deep PR. node-collector works too IMO

[chen-keinan]

I know the context well and it is difficult to get an impartial view of how people feel about current behavior. I'd request a quick review from @simar7.

As I was summoned, I would like to provide my feedback.

I think there are two problems to solve here:

1. Flag name
2. Flag default behavior

Problem 1 I think personally, I would lean towards flags that are more "obvious" or "commonly used". To me "intrusive" (or lack thereof) doesn't tell me what's really intrusive about it. To understand this I would have to read the Trivy docs to know the default behavior (or read the help for CLI flags).

While as something like --node-collector-disable (or something similar) conveys exactly what is being done, aka node collector scan results are being disabled. This also lines up well with existing --node-collector-* flags that we currently have within Trivy.

Problem 2 If we change the flag name to something as I mentioned above, we can keep the current behavior as-is and have this flag opt-in. This would help us not make a breaking change.

@simar7 thanks for the input, putting a flag --disable-node-collector make it more understandable and specific for what actually this flag do.

[simar7]

@chen-keinan reviewed again and left one comment but overall lgtm!

[chen-keinan]

@chen-keinan reviewed again and left one comment but overall lgtm!

@simar7 thanks for the review , I do not see the comment you mention

[simar7]

@chen-keinan reviewed again and left one comment but overall lgtm!

@simar7 thanks for the review , I do not see the comment you mention

#6311 (comment)

[simar7]

@chen-keinan lgtm!