-
Notifications
You must be signed in to change notification settings - Fork 624
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
Statically typed error handling PoC #1752
base: main
Are you sure you want to change the base?
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF Scorecard
Scanned Files |
4d824f9
to
24a12a2
Compare
Keep in mind that the io::ErrorKind is used to identify retry cases, see https://github.com/greenbone/openvas-scanner/blob/main/rust/src/nasl/interpreter/interpreter.rs#L196 for an incomplete example. The possibility to identify cases that can be retried transparently to the user should be considered. |
4626d27
to
58100c9
Compare
I think we need a more robust set up to identify these cases. Using IO errors to mean retryable might be a good heuristic, but is still not accurate, since there are both false positives (io::ErrorKind::NotADirectory is not something that will be solved by retrying) and false negatives (an ssh connection might fail due to a timeout but won't return an IO error).
(method naming very much up for debate) |
52b6113
to
2328196
Compare
Yes, it’s imperfect—I wasn’t suggesting we use it as-is. However, this concept is entirely missing from the current proposal, even though it’s available, albeit imperfectly, in the current handling. I’m not in favor of implementing it at a local level because it relies too heavily on the function developer's knowledge and could lead to inconsistent handling, making it harder to reason about, inconvenient, and adding cognitive load—especially when you just want to quickly create a built-in function. |
Yes, youre right.
I was also afraid of that, but if we don't trust the author of the builtin function to make the correct decision on whether an error is solvable by retrying or not, then I don't think we have a chance of solving this problem anyways. I would prefer doing this on a local level because it still gives us the room to discuss these things in code review etc.. I think making the overarching structure of the error type determine whether an error is "retryable" or not carries a larger risk because we will eventually have to fight our own system if we want more precise control over these errors. This also doesn't always have to be solved on the most local level, because you could, for example, write an impl From<io::Error> for MyCustomError {
fn from(val: io:Error) -> MyCustomError {
MyCustomError::Foo(val).retryable()
}
} which effectively reproduces the original behavior of making any |
1acb060
to
4fa701f
Compare
More specifically, allow autoconversion into the type of the match pattern, so that one does not need to specify the fully qualified error variant every time.
Also rename FunctionError within interpreter to FunctionCallError.
Add code location and origin statement to the message.
b161913
to
199b265
Compare
This is a proposal for revamped error type structure and handling for NASL builtin functions as well as some minor changes to error handling in general.
Error type structure
The main type this refactoring is concerned with is
FunctionErrorKind
which has been renamed toFnError
.In this proposed new error structure,
FnError
is an actual type that contains metadata about the error (such as whether the error can be solved by retrying the operation that caused it and the return value given to the NASL caller), as well as its kind, described by theFnErrorKind
enum. This enum corresponds to the previousFunctionErrorKind
and is structured as follows:The variants represent three different kinds of errors:
ArgumentError
refers to errors caused by calling a function with the wrong arguments which typically reflect an error in the usage of the function by the script authors.InternalError
refers to any error caused by failure of an internal process of the scanner (such as the storage). This reflects a deeper problem and may mean we cannot simply proceed with execution of the scan.Finally,
BuiltinError
represents any (more or less expected) error that occurred within a builtin function. This may include timeouts, authentication failures, etc..Error handling
I think the exact rules for how we treat each error type are something that is still open to discussion, but I'll write down the rules for the implementation in this PR as a basis for this discussion.
As stated above, the metadata (return value, retryable) on how to handle a specific error are stored in a
FnError
(as opposed to deriving them by matching on the kind of the error). This means that in principle, we always have the option of explicitly overriding any defaults for any specific case. However, I hope that we rarely need this in practice because we can choose sane defaults that reflect what we want most of the time.Return behavior
The return behavior of a given error specifies how an error should be handled during execution. This is specified by the following type:
This type is equivalent to
Option<NaslValue>
but we decided that having a custom type here would make the intent clearer.When the interpreter encounters an error with
ReturnBehavior::ExitScript
behavior, it will unsurprisingly exit the script. If it encounters an error withReturnBehavior::ReturnValue(val)
, it will returnval
and continue execution. This behavior is effectively equivalent to theDiagnostic
variant ofFunctionErrorKind
.The
Argument
andInternal
variants ofFnError
are automatically constructed withReturnBehavior::ExitScript
, meaning that they abort execution of the script. TheBuiltin
variant is constructed withReturnBehavior::ReturnValue(NaslValue::Null)
by default, but this value can easily be overwritten when the error is created, for example:Retry behavior
Certain errors can be flagged as being solvable by retrying the operation that caused them. This is represented by a
retryable
boolean onFnError
, which isfalse
by default for all variants except for a specific internal error in the storage. However, this default behavior can be overwritten at error creation if needed, for exampleI also added a small test to make sure that the interpreter does actually retry retryable errors.
Boilerplate / Cost
Since this was the main point of the discussion on implementation strategies for these errors, I figured I'd write down the amount of additional overhead for creating these custom error types that is currently needed when a new builtin module is created.
I personally like this approach anyways since it keeps error messages all in one place and makes it easy to get an overview of all the error messages and to keep them consistent.
BuiltinError
type described above. This is one line of code:From
impls andTryFrom
impls can make the error type easier to use by enabling use of the question mark operator. I added a tiny macro that implements these traits (because the implementations are usually trivial), so this comes down to one line too:builtin_error_variant!(Foo, FooError);
This is all that is needed to make this error usable in NASL functions:
As a side note, NASL functions can also return any concrete
impl Into<FnError>
directly, so for this case we can also writeNote that the above
From
impls that are automatically written by thebuiltin_error_variant!
macro can also be manually implemented if one wants to specify defaults for a specific error variant. For examplePartialEq
,Eq
andClone
Previously, all our error types derived
PartialEq
,Eq
,Clone
. This was basically unused in the codebase (with some minor exceptions where an easy rewrite made these derives unnecessary). In this PR I got rid of all of these derives. This has the advantage of making it easier to wrap certain internal library errors that may not implement any of the three traits (std::io::Error
implements neither and is used quite a bit, for example, causing us to always use theio::ErrorKind
instead.). I think there is a case to be made for either choice, but personally prefer not implementing these.Note that this will also be a problem in a
dyn FnError
-based solution, since the type will have to becomedyn FnError + PartialEq + Eq + Clone
or alternativelyFnError
will have to be defined astrait FnError: PartialEq + Eq + Clone
, either requiring each concrete type to implement the three traits.WithErrorInfo
As a last, opinionated part of this PR, I added a trait
WithErrorInfo
that is basically meant to attach some kind of information to an already existing error. This originated in the SSH implementation, where I wanted to add theSessionId
that caused the error to ~70% of error variants (but not all of them, which would have meant I could simply make it part of the parent error). I also often wanted to add the internal library error that caused the error to occur. This trait made it convenient to simply writeSshError::Foo.with(session_id).with(err)
. I then stumbled upon this concept again when I wanted to attach the retryable/return behavior information discussed above to certain errors, so I simply extended this trait to allow usage like.with(ReturnValue(-1))
or.with(Retryable)
whereReturnValue
andRetryable
are simply dummy types meant for just this one purpose. This has the nice advantage of allowing autoconversion toFnError
making the resulting code look really clean, but if this is too fancy I'm fine with getting rid of it. An alternative would be to implement methods likewith_return_value
and.with_retryable
for every type that needs them or to instead writeFnError::from(...).with_retryable()
which is more verbose.Todo
.md
document somewhere in the crate.FnError
/ArgumentError