-
Notifications
You must be signed in to change notification settings - Fork 492
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
Refactor to use new scope.Error method #2208
base: master
Are you sure you want to change the base?
Conversation
The problem with this approach is that we use the Error() log as an indication that the collection should be marked as failed. A lot of errors are not really fatal so the use of Error() should be done judiciously only in cases where it is obvious that something went very wrong. Probably the default level should be fine in most cases, or maybe use WARN or INFO but ERROR should be reserved to cases where it is really bad. Errors should be used in cases where the user needs to pay attention to the collection. For example:
|
Perhaps just scope.Warn then? Will have to slowly comb through everything to figure out what warrants halting the collection, but marking errors as WARN for now at least indicates something has gone wrong. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great PR because it makes us think about what errors should be flagged for user attention. It is great to work through them and see what each error represents.
Some of the errors are not always obvious - sometimes they are critical other times not so much. Maybe we need a way for the VQL to elevate errors in particular scopes? So when we get a WARN it elevates it to an error? It has to be somewhat VQL dependent and use case dependent.
I went through the first part and I think we can summarise:
- ExtractArgsWithContext - definitely an error but this is already taken care of
- Permission checks - not an error maybe WARN level?
- API errors not critical maybe WARN?
- Compiling regex/yara definitely error
@@ -49,7 +49,7 @@ func (self ClockPlugin) Call( | |||
arg := &ClockPluginArgs{} | |||
err := arg_parser.ExtractArgsWithContext(ctx, scope, args, arg) | |||
if err != nil { | |||
scope.Log("clock: %v", err) | |||
scope.Error("clock: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are probably fine for Error - it means the args are not valid somehow and may be a syntax error (BTW these are automatically marked as error anyway here
Line 80 in 2d77179
getLogErrorRegex(config_obj).FindStringIndex(row.Message) != nil { |
@@ -46,13 +46,13 @@ func (self *EnvFunction) Call(ctx context.Context, | |||
|
|||
err := vql_subsystem.CheckAccess(scope, acls.MACHINE_STATE) | |||
if err != nil { | |||
scope.Log("environ: %s", err) | |||
scope.Error("environ: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May not be a critical error - if running with lower privileges we do want to just ignore
@@ -149,7 +149,7 @@ func (self MailPlugin) Call( | |||
// Send the email to Bob, Cora and Dan. | |||
err = d.DialAndSend(m) | |||
if err != nil { | |||
scope.Log("mail: %v", err) | |||
scope.Error("mail: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may not be critical - can be an intermittant failure
@@ -121,19 +121,19 @@ func (self ShellPlugin) Call( | |||
|
|||
stdout_pipe, err := command.StdoutPipe() | |||
if err != nil { | |||
scope.Log("shell: no command to run") | |||
scope.Error("shell: no command to run") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the wrong message :-)
IDK - is this critical or not? it might be or it might be intermittent (e.g. tool failed to download etc)
@@ -99,7 +100,7 @@ func (self YaraScanPlugin) Call( | |||
|
|||
err = vql_subsystem.CheckFilesystemAccess(scope, arg.Accessor) | |||
if err != nil { | |||
scope.Log("yara: %s", err.Error()) | |||
scope.Error("yara: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not critical error
return false | ||
} | ||
|
||
dir, err := ioutil.TempDir("", "tmp") | ||
if err != nil { | ||
scope.Log("tempdir: %v", err) | ||
scope.Error("tempdir: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an error
@@ -159,7 +159,7 @@ func (self *FilterFunction) Call(ctx context.Context, | |||
if arg.Condition != "" { | |||
lambda, err = vfilter.ParseLambda(arg.Condition) | |||
if err != nil { | |||
scope.Log("filter: Unable to compile lambda %s", arg.Condition) | |||
scope.Error("filter: Unable to compile lambda %s", arg.Condition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is definitely an error
return false | ||
} | ||
|
||
if arg.Parse != "" { | ||
result, err := url.Parse(arg.Parse) | ||
if err != nil { | ||
scope.Log("url: %v", err) | ||
scope.Error("url: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not critical
@@ -51,6 +51,14 @@ var ( | |||
mu sync.Mutex | |||
|
|||
proxyHandler = http.ProxyFromEnvironment | |||
|
|||
validHttpMethods = map[string]bool{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one disappeared no? maybe you need to rebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thought I did. Weird
@@ -368,7 +384,7 @@ func (self *_HttpPlugin) Call( | |||
req, err = http.NewRequestWithContext( | |||
ctx, method, arg.Url, strings.NewReader(arg.Data)) | |||
if err != nil { | |||
scope.Log("%s: %v", self.Name(), err) | |||
scope.Error("%s: %v", self.Name(), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these may or may not be errors?
I think the default level is DEFAULT and makes no practical difference from WARN. I guess we need to think about the purpose of the log levels:
|
For most of the calls to scope.Log, it's because there's an error, but sometimes that's not the case. It'd be helpful to be able to differentiate between the two by just looking at the level. I think it makes sense for the default level to be INFO, things that might be an issue are WARN, ERROR is reserved solely for showstoppers, and DEBUG is just highly verbose output you probably don't care about. |
Yes agree 👍 this is a great way to put it. |
This PR changes most instances where scope.Log is used to report an error to scope.Error in order to make sure errors occur at the correct log level.