Introduce new statuses: SKIP, MANU, ERRO #1035
Conversation
|
|
There was a problem hiding this comment.
First of all thanks for contributing !
Ok it seems good and nice :) there are a small changes I requested just a bit around the logic and about maybe change warn_reason in the json output but I'm not sure about since it will break backward compatibility but on the other hand we are changing a lot here.
| colors[check.WARN].Printf("== Remediations %s ==\n", r.Type) | ||
| for _, g := range r.Groups { | ||
| for _, c := range g.Checks { | ||
| if c.State == check.FAIL { | ||
| if c.State == check.FAIL || c.State == check.ERRO { | ||
| fmt.Printf("%s %s\n", c.ID, c.Remediation) |
There was a problem hiding this comment.
It should be different output from error to fail, because fail is fine and you just need to run remediation but error means something ran incorrectly, like the command doesn't even exist so remediation won't help here but stderr will.
There was a problem hiding this comment.
@yoavrotems Could you elaborate on this? The error is stored in Reason, is that right?
Do you mean something like this?
if c.State == check.ERRO {
fmt.Printf("%s audit test error: %s\n", c.ID, c.Reason)
}
cmd/common.go
Outdated
| fmt.Printf("%s %s\n", c.ID, c.Remediation) | ||
| } | ||
| if c.State == check.WARN { | ||
| if c.State == check.WARN || c.State == check.MANU { | ||
| // Print the error if test failed due to problem with the audit command | ||
| if c.Reason != "" && c.Type != "manual" { |
There was a problem hiding this comment.
WARN should be with FAIL and here it should be ERRO since Reason was a warn_reason which is why a test returned error, now WARN is just a fail test with no score. and ERRO is the test with reasons like command not exist or invalid parameter.
I think maybe we should consider change 'warn_reason' in the json output to error_reason or something like this.
There was a problem hiding this comment.
I'm not sure I understood what warn_reason is, since there is only reason in json output.
cmd/common.go
Outdated
| @@ -173,7 +173,9 @@ func prettyPrint(r *check.Controls, summary check.Summary) { | |||
| for _, c := range g.Checks { | |||
| colorPrint(c.State, fmt.Sprintf("%s %s\n", c.ID, c.Text)) | |||
|
|
|||
| if includeTestOutput && c.State == check.FAIL && len(c.ActualValue) > 0 { | |||
| if includeTestOutput && | |||
| (c.State == check.FAIL || c.State == check.ERRO) && | |||
There was a problem hiding this comment.
Also WARN should be printed since starting now WARN is a fail just not scored.
There was a problem hiding this comment.
Did I understand you correctly?
| (c.State == check.FAIL || c.State == check.ERRO) && | |
| (c.State == check.FAIL || c.State == check.WARN || c.State == check.ERRO) && |
Codecov Report
@@ Coverage Diff @@
## main #1035 +/- ##
==========================================
- Coverage 63.56% 63.34% -0.23%
==========================================
Files 14 14
Lines 1905 1934 +29
==========================================
+ Hits 1211 1225 +14
- Misses 633 647 +14
- Partials 61 62 +1
Continue to review full report at Codecov.
|
hypnoglow
force-pushed
the
916-new-statuses
branch
from
6f59025
to
85e4b94
Compare
|
@yoavrotems I've made a few changes based on your comments, PTAL. Although I'm not sure I got everything correct 馃檮 |
|
Before I'm reviewing those new changes, we talked about it with some kube-bench users, and those changes are breaking the output for automation and other projects using kube-bench so lets do it for now as a flag, for example --enhancedoutput, and then it will be default to true, and if someone needs backward compatibility they could run it as |
@hypnoglow Hi, Igor, Are you still working on this? |
This changeset introduces new statuses as proposed in #916
Probably I missed something, so I would be grateful any feedback and help 馃檹