Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

chen-keinan
Copy link
Contributor

Description

Add support for k8s intrusive flag

Related issues

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).

@chen-keinan chen-keinan force-pushed the feat/k8s-intrusive-flag-support branch from f9cdd94 to 98546fc Compare March 14, 2024 10:06
@chen-keinan chen-keinan changed the title feat: add support for k8s intrusive flag feat: add support for k8s --skip-intrusive flag Mar 17, 2024
@chen-keinan chen-keinan marked this pull request as ready for review March 18, 2024 14:41
@chen-keinan chen-keinan force-pushed the feat/k8s-intrusive-flag-support branch 2 times, most recently from b72cc65 to 74f8221 Compare March 31, 2024 12:25
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Also, the integration test is failing.

@chen-keinan chen-keinan changed the title feat: add support for k8s --skip-intrusive flag BREAKING: add support for k8s --skip-intrusive flag Apr 1, 2024
@chen-keinan
Copy link
Contributor Author

chen-keinan commented Apr 1, 2024

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

@knqyf263
Copy link
Collaborator

knqyf263 commented Apr 1, 2024

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
Copy link
Contributor

itaysk commented Apr 1, 2024

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)

@chen-keinan chen-keinan changed the title BREAKING: add support for k8s --skip-intrusive flag BREAKING: add support for k8s --non-intrusive flag Apr 2, 2024
@chen-keinan chen-keinan force-pushed the feat/k8s-intrusive-flag-support branch 2 times, most recently from 7f95e5d to c23dcbb Compare April 2, 2024 09:07
@itaysk
Copy link
Contributor

itaysk commented Apr 3, 2024

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 knqyf263 requested a review from simar7 April 3, 2024 16:29
@knqyf263
Copy link
Collaborator

knqyf263 commented Apr 3, 2024

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
Copy link
Member

simar7 commented Apr 3, 2024

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.

@chen-keinan chen-keinan force-pushed the feat/k8s-intrusive-flag-support branch from c23dcbb to 4179e48 Compare April 4, 2024 06:30
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

itaysk commented Apr 4, 2024

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

@chen-keinan
Copy link
Contributor Author

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.

@chen-keinan chen-keinan changed the title BREAKING: add support for k8s --non-intrusive flag BREAKING: add support for k8s disable-node-collector flag Apr 15, 2024
@chen-keinan chen-keinan force-pushed the feat/k8s-intrusive-flag-support branch from 4179e48 to 7e8c80d Compare April 15, 2024 06:00
@simar7
Copy link
Member

simar7 commented Apr 26, 2024

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

@chen-keinan chen-keinan requested a review from simar7 April 26, 2024 05:50
@chen-keinan
Copy link
Contributor Author

@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
Copy link
Member

simar7 commented Apr 27, 2024

@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)

Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
@chen-keinan chen-keinan force-pushed the feat/k8s-intrusive-flag-support branch from 224322f to 22e51a6 Compare April 27, 2024 15:35
Signed-off-by: chenk <hen.keinan@gmail.com>
@chen-keinan chen-keinan force-pushed the feat/k8s-intrusive-flag-support branch from 918c9ac to 7f2bd02 Compare April 27, 2024 16:07
@simar7
Copy link
Member

simar7 commented Apr 28, 2024

@chen-keinan lgtm!

@chen-keinan chen-keinan added this pull request to the merge queue May 2, 2024
Merged via the queue into aquasecurity:main with commit c6d5d85 May 2, 2024
17 checks passed
@chen-keinan chen-keinan deleted the feat/k8s-intrusive-flag-support branch May 2, 2024 11:28
fl0pp5 pushed a commit to altlinux/trivy that referenced this pull request May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trivy k8s: --disable-node-collector flag
4 participants