Unify API error types #450
Conversation
|
Alternatively, we can replace |
There was a problem hiding this comment.
I think in general this is reasonable, and it uncouples us a bit more from kubernetes code.
It's a bit awkward because the openapi types are a lot more option heavy and this is making the error interface arguably uglier, but it is unifying the types, which is probably the smarter thing to do here.
Left one question on the debug print.
| code: Some(s.as_u16().into()), | ||
| message: Some(format!("{:?}", text)), | ||
| reason: Some("Failed to parse error data".into()), | ||
| details: None, |
There was a problem hiding this comment.
This now causes a lot more option types that always exist (like most k8s-openapi structs), and that was the main reason for the separation of the structs.
| /// A Kubernetes status object | ||
| #[allow(missing_docs)] | ||
| #[derive(Deserialize, Debug)] | ||
| pub struct Status { |
There was a problem hiding this comment.
At least we are getting rid of these duplicate structs.
| backtrace: Backtrace, | ||
| }, | ||
| #[snafu(display("error returned by apiserver during watch: {:?}", status))] | ||
| WatchError { status: Status, backtrace: Backtrace }, |
| @@ -41,8 +41,8 @@ async fn main() -> anyhow::Result<()> { | |||
| // wait for it.. | |||
| std::thread::sleep(std::time::Duration::from_millis(5_000)); | |||
| } | |||
| Err(kube::Error::Api(ae)) => assert_eq!(ae.code, 409), // if you skipped delete, for instance | |||
| Err(e) => return Err(e.into()), // any other case is probably bad | |||
| Err(kube::Error::Api(ae)) => assert_eq!(ae.code, Some(409)), // if you skipped delete, for instance | |||
There was a problem hiding this comment.
a bit uglier, and a breaking change, but a small one.
There was a problem hiding this comment.
Yeah, API error handling already felt awkward when writing controllers. Matches like Err(kube::Error::Api(kube::error::ErrorResponse { code: 404, .. })) are common.
I was going to propose switching to snafu and add new Error specific for client/API (let's say kube::client::Error for now). Providing specific variants like Error::NotFound and Error::Conflict makes sense because these are very common. We can include our wrapper for meta::v1::Status in variants for those who need them.
Also, I'm not sure if there's any reason to keep the deprecated runtime, but I think it makes sense to move kube-runtime under kube soon and this (snafu) is a necessary step.
There was a problem hiding this comment.
Yeah, there's definitely bigger improvements to be made in client error handling, separating it out more like how it's done for ConfigError, and special purpose creating specific errors would help a lot like you say.
A small question though. How are you finding working with snafu? What benefits do you find with it over thiserror?
I ask because I was kind of thinking to propose unifying under thiserror instead because every time i make another app, I have started reaching for it instead because of ergonomics:
- snafu's error enum ends up being 5 lines per impl (when using a source + backtrace) rather than thiserror's 2
- snafu's error variants all have to be imported into scope and causes extra cycles when developing
There was a problem hiding this comment.
A small question though. How are you finding working with snafu? What benefits do you find with it over thiserror?
IMO, the big benefit is that the context selectors push you towards having many smaller and more specific error types that act as a sort of semantic backtrace ("file not found while finding Y while reconciling X"), rather than the more or less unmanageable kube::Error ("something has gone wrong in library X, have fun figuring out why").
- snafu's error enum ends up being 5 lines per impl (when using a source + backtrace) rather than thiserror's 2
snafu is agnostic to struct vs tuple variants, you can do:
#[derive(Debug, Snafu)
pub enum Error {
#[snafu(display("IO error"))]
Io(#[snafu(source)] std::io::Error, #[snafu(backtrace)] Backtrace)
}
The current state in kube-derive is more about my own bias towards struct-style enums.
Backtraces can also be inherited from other snafu errors, which would save you an item.
- snafu's error variants all have to be imported into scope and causes extra cycles when developing
Not really, if you follow snafu's recommended error-enum-per-module style. The enum is the external interface of the module, the context selectors are an implementation detail.
There was a problem hiding this comment.
Can we move to #453? It's easier for others to join, and we can leave this PR alone.
There was a problem hiding this comment.
Providing specific variants like Error::NotFound and Error::Conflict makes sense because these are very common. We can include our wrapper for meta::v1::Status in variants for those who need them.
I agree that that would be more comfortable, but it's a big compatibility footgun, sadly.
Adding a Conflict arm would mean that a match on Err(kube::Error::Api(kube::error::ErrorResponse { code: 409, .. })) would still compile just fine, but silently never match anything.
There was a problem hiding this comment.
If we keep kube::client::Status, we can add something like:
impl Status {
pub fn classify(&self) -> Reason;
}
#[non_exhaustive]
pub enum Reason {
NotExists,
Confict,
PermissionDenied,
Unknown
}Otherwise, it can be implemented as a free function or an extension trait for k8s_openapi::Status
|
|
||
| /// An error response from the API. | ||
| #[derive(Error, Deserialize, Serialize, Debug, Clone, Eq, PartialEq)] | ||
| #[error("{message}: {reason}")] |
There was a problem hiding this comment.
Is there a reason for expanding the nice shorthand to the full debug print?
|
What do you think about alternative?
cons:
pros:
Based on your review I now think this approach is better. |
Currently
kubehas two similar structs that are intended to hold API errors -StatusandErrorResponse. However, both types are incomplete. This pull request deletes both structs in favor ofmetav1::Statusdefined byk8s-openapi.