Unify API error types #450

MikailBag · 2021-03-05T14:01:16Z

Currently kube has two similar structs that are intended to hold API errors - Status and ErrorResponse. However, both types are incomplete. This pull request deletes both structs in favor of metav1::Status defined by k8s-openapi.

MikailBag · 2021-03-05T14:03:31Z

Alternatively, we can replace ErrorResponse with Status. This way we can still provide impl Error.

clux

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.

clux · 2021-03-07T22:42:50Z

kube/src/client/mod.rs

+                code: Some(s.as_u16().into()),
+                message: Some(format!("{:?}", text)),
+                reason: Some("Failed to parse error data".into()),
+                details: None,


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.

clux · 2021-03-07T22:48:44Z

kube/src/client/mod.rs

-/// A Kubernetes status object
-#[allow(missing_docs)]
-#[derive(Deserialize, Debug)]
-pub struct Status {


At least we are getting rid of these duplicate structs.

clux · 2021-03-07T22:50:13Z

kube-runtime/src/watcher.rs

-        backtrace: Backtrace,
-    },
+    #[snafu(display("error returned by apiserver during watch: {:?}", status))]
+    WatchError { status: Status, backtrace: Backtrace },


why is this now the debug print?

clux · 2021-03-07T22:51:20Z

examples/pod_api.rs

@@ -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


a bit uglier, and a breaking change, but a small one.

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.

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

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.

Can we move to #453? It's easier for others to join, and we can leave this PR alone.

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.

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

clux · 2021-03-07T22:53:12Z

kube/src/error.rs

-
-/// An error response from the API.
-#[derive(Error, Deserialize, Serialize, Debug, Clone, Eq, PartialEq)]
-#[error("{message}: {reason}")]


Is there a reason for expanding the nice shorthand to the full debug print?

MikailBag · 2021-03-07T23:02:42Z

Status from k8s-openapi does not implement Display.

What do you think about alternative?

Verify that kube::client::Status has all fields that metav1::Status has.
Replace ErrorResponse with kube::client::Status

cons:

we have hard-coded version of k8s-openapi stuff (but it's still better than current situation when we have two versions)

pros:

less Options
Display implementation
Error implementation

Based on your review I now think this approach is better.

Unify API error types

87cfa22

MikailBag added 2 commits March 5, 2021 17:05

Fix formatting

7d7c878

Fix doctests

3e4811e

clux reviewed Mar 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify API error types #450

Unify API error types #450

MikailBag commented Mar 5, 2021

MikailBag commented Mar 5, 2021 •

edited

Loading

clux left a comment

clux Mar 7, 2021

clux Mar 7, 2021

clux Mar 7, 2021

clux Mar 7, 2021

kazk Mar 8, 2021 •

edited

Loading

clux Mar 8, 2021

nightkr Mar 8, 2021

kazk Mar 8, 2021

nightkr Mar 8, 2021

MikailBag Mar 8, 2021

clux Mar 7, 2021

MikailBag commented Mar 7, 2021

Unify API error types #450

Are you sure you want to change the base?

Unify API error types #450

Conversation

MikailBag commented Mar 5, 2021

MikailBag commented Mar 5, 2021 • edited Loading

clux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazk Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikailBag commented Mar 7, 2021

MikailBag commented Mar 5, 2021 •

edited

Loading

kazk Mar 8, 2021 •

edited

Loading