kube-bench icon indicating copy to clipboard operation
kube-bench copied to clipboard

Add --status flag

Open mtpereira opened this issue 3 years ago β€’ 15 comments

Fixes #918

This change adds the --status flag as described on the referenced issue, so that kube-bench will only log tests results with a check.State contained in the --status flag. If the flag is not passed, all messages are logged. This does not affect the summary output.

mtpereira avatar Oct 23 '21 16:10 mtpereira

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 23 '21 16:10 CLAassistant

Hi! :wave: I'm looking at this issue in the context of Hacktoberfest 2021.

If you think this PR has a meaningful change, and it might not be merged/approved before the end of the month, could you please label it with hacktoberfest-accepted?

Thank you!

mtpereira avatar Oct 23 '21 16:10 mtpereira

πŸ“ I've just noticed I did not do any sort of error handling/input check on the --status flag. I'll handle that as well, although it would be great if I could get some feedback before that, to ensure I'm on the right track. πŸ™

mtpereira avatar Oct 24 '21 09:10 mtpereira

Codecov Report

Merging #1030 (5ed6c78) into main (dd68e85) will increase coverage by 0.18%. The diff coverage is 71.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   63.56%   63.75%   +0.18%     
==========================================
  Files          14       14              
  Lines        1905     1948      +43     
==========================================
+ Hits         1211     1242      +31     
- Misses        633      645      +12     
  Partials       61       61              
Impacted Files Coverage Ξ”
check/controls.go 75.11% <0.00%> (-4.49%) :arrow_down:
cmd/common.go 57.58% <92.50%> (+3.24%) :arrow_up:
cmd/root.go 34.50% <100.00%> (+0.46%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update dd68e85...5ed6c78. Read the comment docs.

codecov[bot] avatar Oct 27 '21 10:10 codecov[bot]

Thanks for contributing, First I will check about the hacktoberfest.
Secondly we would like --status to change the complete output, because the use of it will be to better know what tests in what status especially when we would support more statuses, for example if I would like to see what I need to fix in my system and thats it I will run: kube-bench --status="FAIL" And would expect only the failed tests to be outputted with the remediation and the summary of the different parts. (About the json I'm reluctant from changing the output since it is very easy to run manipulation on json data so maybe we should keep it as informative as possible)

yoavrotems avatar Oct 27 '21 10:10 yoavrotems

Hi @yoavrotems ! πŸ‘‹

I'm not sure I understood what you meant with your comment on what the output should be, so I'll try to clarify it. πŸ™

Do you mean that:

  1. when the --status flag is set, all of the stdout should be filtered, including summary and report;
  2. Or do you mean that other methods of outputting should also be affected, with the exception of JSON, as you've mentioned?

Thanks for the clarification!

mtpereira avatar Oct 28 '21 15:10 mtpereira

To make this more explicit and hopefully easier to communicate, here is the current output I get from integration tests when I modify them to pass --status=FAIL: https://gist.github.com/mtpereira/12e0acb97a209b2ae72389d3a640ca28

As you can see, the whole output is only about the checks that FAILed.

Is this not what we want?

mtpereira avatar Oct 30 '21 09:10 mtpereira

Hey you were right in the first part

Hi @yoavrotems ! πŸ‘‹

I'm not sure I understood what you meant with your comment on what the output should be, so I'll try to clarify it. πŸ™

Do you mean that:

  1. when the --status flag is set, all of the stdout should be filtered, including summary and report;
  2. Or do you mean that other methods of outputting should also be affected, with the exception of JSON, as you've mentioned?

Thanks for the clarification!

About the second part all of the output manipulation flags --noremediations --nosummary --noresults effects the stdout only and not the json. And yes it should all co-exist I don't see a reason why not using --noremediations and --status=FAIL

I saw the output you shared status wise it seems good, but I would skip printing remediation for not printed tests like you have with 3.1.1 3.2.1 3.2.2 those tests all are WARN so it not printed but the remediation do.

If someone wanted to see all fails tests the it remediation I guess that WARN tests remediation is not relevant here. if so you should run --status="FAIL,WARN"

wdyt?

yoavrotems avatar Nov 01 '21 09:11 yoavrotems

@yoavrotems Got it, that makes sense! I'll give it a look. :ok_hand:

mtpereira avatar Nov 01 '21 10:11 mtpereira

Hello @yoavrotems :wave:

How is this looking now? https://gist.github.com/mtpereira/12e0acb97a209b2ae72389d3a640ca28/revisions

:memo: The code needs some refactoring, it is still not ready, but tests are :green_circle: .

mtpereira avatar Nov 05 '21 09:11 mtpereira

Hey looks better the output that not including remediation when its not printing the test is great! This PR is great and part from a bigger change that we want to see, I think you should take in consideration this PR #1035

yoavrotems avatar Nov 08 '21 09:11 yoavrotems

Hey looks better the output that not including remediation when its not printing the test is great!

This PR is great and part from a bigger change that we want to see, I think you should take in consideration this PR #1035

Hi! πŸ‘‹

Oh this might be tricky to handle! πŸ˜… How do you suggest we go about this? Shall we merge one PR first and then proceed with the other? I'm fine either way. πŸ‘

mtpereira avatar Nov 08 '21 14:11 mtpereira

Let me check it and return to you on how we are going to accept the other PR because we might take as a flag to not break old behaviors

yoavrotems avatar Nov 11 '21 11:11 yoavrotems

Hey @mtpereira We would accept the #1035 PR but we will do it as a flag for now, the default will be using all of the statuses, but with flag --enhancedoutput=false to allow backward compatibility, so I think that this PR will need to support filtering all of the statuses. One thing we need to decide is the behavior when printing unused flag, for example: ./kube-bench --enhancedoutput=false --status="WARN,MANU"

I think that we can do in one of the two ways,

  1. When use flag status check what filtering is applying and if using new status with enhancedoutput=false, raise error and not running. --> kube-bench can't output non-supported status, either remove $STATUS from --status, or set --enhancedoutput=true P.S should also support for something like this one trying none exist status.

  2. When trying to filter out none exist flag just not print it since there isn't any $STATUS in the output, same as I would run --status="FAIL" and none of the tests failed.

I think that the first option is the best but open to suggestions. WDYT?

yoavrotems avatar Nov 14 '21 14:11 yoavrotems

Hello @yoavrotems ! :wave:

Option 1 seems better to me as well! I'll give it a look once I have some spare time. :bow:

mtpereira avatar Nov 18 '21 14:11 mtpereira