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.

Sign up for GitHub

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

Jump to bottom

cmd/run: detect simulation errors and exit with appropriate rc #53

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

cmd/run: detect simulation errors and exit with appropriate rc #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions cmd/run/print.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ The current time is %s
`, Version, ip, dnsIntfIP, time.Now().Format("02-Jan-06 15:04:05"))
}

func printGoodbye() {
fmt.Printf("\nAll done! Check your SIEM for alerts using the timestamps and details above.\n")
func printGoodbye(simErrsDetected bool) {
if simErrsDetected {
fmt.Printf("\nAll done, but simulation errors occurred! ")
} else {
fmt.Printf("\nAll done! ")
}
fmt.Printf("Check your SIEM for alerts using the timestamps and details above.\n")
}
33 changes: 31 additions & 2 deletions cmd/run/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package run

import (
"context"
"errors"
"flag"
"fmt"
"net"
Expand Down Expand Up @@ -358,6 +359,22 @@ func getDefaultDNSIntf() (string, error) {
return dnsIntfIP, nil
}

// failedSimulationNames returns a sorted slice of failed simulation names extracted from
// them sims map.
func failedSimulationNames(sims map[string]bool) []string {
if len(sims) == 0 {
return nil
}
failedSims := make([]string, len(sims))
i := 0
for sim := range sims {
failedSims[i] = sim
i++
}
sort.Strings(failedSims)
return failedSims
}

func run(sims []*Simulation, bind simulator.BindAddr, size int) error {
// If user override on iface, both IP and DNS traffic will flow through bind.Addr.
// NOTE: not passing the DNS server to printWelcome(), as it may be confusing in cases
Expand All @@ -374,12 +391,15 @@ func run(sims []*Simulation, bind simulator.BindAddr, size int) error {
}
printHeader()

// Log failed simulations.
failedSims := make(map[string]bool)
for simN, sim := range sims {
fmt.Print("\n")

okHosts := 0
err := sim.Init(bind)
if err != nil {
failedSims[sim.Name()] = true
printMsg(sim, msgPrefixErrorInit+fmt.Sprint(err))
} else {
printMsg(sim, sim.HeaderMsg)
Expand All @@ -391,6 +411,7 @@ func run(sims []*Simulation, bind simulator.BindAddr, size int) error {

hosts, err := sim.Module.Hosts(sim.Scope, numOfHosts)
if err != nil {
failedSims[sim.Name()] = true
printMsg(sim, msgPrefixErrorInit+err.Error())
continue
}
Expand All @@ -412,6 +433,7 @@ func run(sims []*Simulation, bind simulator.BindAddr, size int) error {
// TODO: some module can return custom messages (e.g. hijack)
// and "ERROR" prefix shouldn't be printed then
printMsg(sim, fmt.Sprintf("ERROR: %s: %s", host, err.Error()))
failedSims[sim.Name()] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this won't be misleading in some scenarios. What if we run the simulation against 10 hosts and one of them fails? Should we report the whole module as failed?

I don't have an answer here, but we need to define what it means for the module to fail. Maybe simple boolean flag is not enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also most probably we should extract this for loop body into a separate function/structure so we don't repeat the errors handling (printMsg+failedSims). But we before we do so we should address what we really want to report at the end and how to handle partial failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK... I was a little concerned that a single host failure was a bit too coarse. I'll mark this as a draft PR until we discuss further.

Thanks for the review.

} else {
okHosts++
}
Expand All @@ -436,7 +458,14 @@ func run(sims []*Simulation, bind simulator.BindAddr, size int) error {
}
sim.Cleanup()
}

printGoodbye()
// Extract list of failed simulations and return as an error.
failedSimNames := failedSimulationNames(failedSims)
printGoodbye(failedSimNames != nil)
if failedSimNames != nil {
msg := fmt.Sprintf(
"The following simulations experienced errors: %v",
strings.Join(failedSimNames, ", "))
return errors.New(msg)
}
Comment on lines +463 to +469
Copy link
Contributor

Choose a reason for hiding this comment

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

printGoodbye is a function which is called only once, so the only reason for its existence is to make the code cleaner. Splitting half of the goodbye message into printGoodbye and leaving another half inlined in the function doesn't make sense. We should either pass the failed simulations into the printGoodbye or remove printGoodbye and inline everything.

return nil
}