Add --audit and --audit-destination flags #40
Conversation
|
Here's a quick look at CLI output:
|
There was a problem hiding this comment.
Hey @bobby-brennan, thanks for this PR! It looks really promising, with some great changes. Just added some questions below. It looks like a rebase would be good here, and it may even be worth squashing a couple commits.
pkg/validator/fullaudit.go
Outdated
|
|
||
| // RunAudit runs a full Fairwinds audit and returns an AuditData object | ||
| func RunAudit(config conf.Configuration, kubeAPI *kube.API) (AuditData, error) { | ||
| // TODO: Validate pods and containers |
There was a problem hiding this comment.
This comment seems strange, we're already validating pods and containers that are part of deployments
There was a problem hiding this comment.
Yeah you're right, that's a bit confusing.
In the comment below (copied from elsewhere) it says Once we are validating more than deployments - I was trying to make this a more explicit TODO. Maybe Validate pods and containers that are not part of deployments? Or is there something else we should be validating?
There was a problem hiding this comment.
I think the next goal would be to add other parent resources of pods to this. That includes stateful sets, daemonsets, and cron jobs. I think it would be good to eventually capture pods that did not have a parent resource, but that could potentially be quite challenging. As far as containers specifically, they are always contained inside pods and can't exist outside of that context in Kubernetes.
There was a problem hiding this comment.
Got it, thanks, that makes sense. Updated the comment accordingly.
pkg/validator/fullaudit_test.go
Outdated
| Errors: uint(1), | ||
| } | ||
|
|
||
| acutalAudit, _ := RunAudit(c, k8s) |
There was a problem hiding this comment.
It seems like it would be good to also set the second variable here as err and verify that it's nil as one of the tests.
main.go
Outdated
| body, _ := ioutil.ReadAll(resp.Body) | ||
| glog.Println(string(body)) | ||
| } else { | ||
| printSummary := func(summary validator.ResultSummary, indent string) { |
There was a problem hiding this comment.
Instead of all the logic to manage indentation below this, could we get by with something like yaml output?
There was a problem hiding this comment.
You're right, that's probably easier to maintain and more useful in the long run. Done.
| @@ -43,8 +46,10 @@ var log = logf.Log.WithName("fairwinds") | |||
| func main() { | |||
| dashboard := flag.Bool("dashboard", false, "Runs the webserver for Fairwinds dashboard.") | |||
| webhook := flag.Bool("webhook", false, "Runs the webhook webserver.") | |||
| audit := flag.Bool("audit", false, "Runs a one-time audit.") | |||
There was a problem hiding this comment.
If I understand this correctly, audit, webhook, and dashboard are all separate modes Fairwinds can run in, but it can never run in more than one of them. Is that accurate? If so, it might make more sense to have a single mode/service/whatever argument with a default value, maybe dashboard?
There was a problem hiding this comment.
I was under the impression that you could run webhook and dashboard simultaneously. Is that wrong?
There was a problem hiding this comment.
I don't believe that's currently possible due to them both requiring a server to run, and those servers being different implementations (one of which is specific to controller-runtime's webhook implementation). I think it could be possible to run them together at some point in the future, but it may be simplest to keep them separate.
There was a problem hiding this comment.
If that's the case, it might make sense to split these operations into different binaries. I can take a crack at that in another PR if you agree.
For now, I set audit as the default, since that's the simplest operation. It'll also exit immediately, so if you intended a different operation you won't be sitting there waiting for it to finish.
rbren
force-pushed
the
bb/remote-audit
branch
2 times, most recently
from
3432f4b
to
41c247e
Compare
|
Thanks for the review! Rebased && squashed - let me know if I missed anything |
Add option to send audit results to a remote host add audit flag to print results to stdout add comments make comments more consistent move audit test fix fullaudit_test add test instructions to README update audit test simplify stdout output update comment fix import run audit by default
validator.AuditData, a struct meant to contain a full Fairwinds reportvalidator.RunAuditto populateAuditData--auditflag inmain.goto triggerRunAudit, prints to stdout by default--audit-destinationflag to submit the audit as a JSON POST request