-
Notifications
You must be signed in to change notification settings - Fork 32
API, control, netstack: error refactoring #154
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
Open
nrc
wants to merge
4
commits into
main
Choose a base branch
from
nrc/errors
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,50 +1,127 @@ | ||
| use std::fmt; | ||
|
|
||
| use crate::netstack::Error as NetstackError; | ||
|
|
||
| /// Errors that may occur while interacting with a device. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, thiserror::Error)] | ||
| #[derive(Debug, thiserror::Error, Clone, Copy, Eq, PartialEq)] | ||
| pub enum Error { | ||
| /// Internal operation failed, likely a bug. | ||
| #[error("internal operation returned an error")] | ||
| InternalFailure, | ||
|
|
||
| /// The runtime state was degraded: a component that we expected to be able to | ||
| /// communicate with hung up or could not be reached. | ||
| /// An operation timed-out. | ||
| /// | ||
| /// This usually means that an internal component has panicked or is wedged. | ||
| #[error("runtime degraded, component unreachable")] | ||
| RuntimeDegraded, | ||
|
|
||
| /// An operation timed out. | ||
| #[error("operation timed out")] | ||
| /// This error can often be handled by retrying. | ||
| #[error("operation timed-out")] | ||
| Timeout, | ||
|
|
||
| /// A connection was reset. | ||
| /// | ||
| /// This error can often be handled by retrying. | ||
| #[error("connection reset")] | ||
| ConnectionReset, | ||
|
|
||
| /// An error reading or parsing the key file. | ||
| #[error("an error reading or parsing the key file")] | ||
| KeyFileRead, | ||
|
|
||
| /// An error writing out the key file. | ||
| #[error("an error writing out the key file")] | ||
| KeyFileWrite, | ||
|
|
||
| /// The environment variable `TS_RS_EXPERIMENT` was not set. | ||
| /// | ||
| /// The end-user must set `TS_RS_EXPERIMENT=this_is_unstable_software` to acknowledge that tailscale-rs | ||
| /// is early-days experimental software containing bugs, unvalidated cryptography, and no stability | ||
| /// or compatibility guarantees. | ||
| #[error("the environment variable `{}` was not set", crate::ENV_MAGIC_VAR)] | ||
| UnstableEnvVar, | ||
|
|
||
| /// An error occurred which can not be anticipated or handled by a library user. | ||
| /// | ||
| /// This is likely due to a bug in our code or a rare and unexpected error. | ||
| /// | ||
| /// [`InternalErrorKind`] is intended to be informational (might be used to improve error reporting | ||
| /// in logs or to the end-user), rather then inspected during handling. | ||
| #[error("internal error ({0})")] | ||
| Internal(InternalErrorKind), | ||
| } | ||
|
|
||
| /// Informational detail on the kind of internal error. | ||
| #[non_exhaustive] | ||
| #[derive(Debug, Clone, Copy, Eq, PartialEq)] | ||
| pub enum InternalErrorKind { | ||
| /// Invalid socket state. | ||
| InvalidSocketState, | ||
| /// Response type mismatched to request type. | ||
| InternalResponseMismatch, | ||
| /// Channel closed. | ||
| InternalChannelClosed, | ||
| /// Handle to invalid TCP listener. | ||
| BadListenerHandle, | ||
| /// Handle to invalid socket. | ||
| BadSocketHandle, | ||
| /// Bad request. | ||
| BadRequest, | ||
| /// Invalid buffer. | ||
| BadBuffer, | ||
| /// Actor missing or shutdown. | ||
| Actor, | ||
| } | ||
|
|
||
| impl fmt::Display for InternalErrorKind { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| InternalErrorKind::InvalidSocketState => write!(f, "invalid socket state"), | ||
| InternalErrorKind::InternalResponseMismatch => { | ||
| write!(f, "response type mismatched to request type") | ||
| } | ||
| InternalErrorKind::InternalChannelClosed => write!(f, "channel closed"), | ||
| InternalErrorKind::BadListenerHandle => write!(f, "handle to invalid TCP listener"), | ||
| InternalErrorKind::BadSocketHandle => write!(f, "handle to invalid socket"), | ||
| InternalErrorKind::BadRequest => write!(f, "bad request"), | ||
| InternalErrorKind::BadBuffer => write!(f, "invalid buffer"), | ||
| InternalErrorKind::Actor => write!(f, "actor missing or shutdown"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<crate::netstack::InternalErrorKind> for InternalErrorKind { | ||
| fn from(e: crate::netstack::InternalErrorKind) -> Self { | ||
| match e { | ||
| crate::netstack::InternalErrorKind::InvalidSocketState => { | ||
| InternalErrorKind::InvalidSocketState | ||
| } | ||
| crate::netstack::InternalErrorKind::InternalResponseMismatch => { | ||
| InternalErrorKind::InternalResponseMismatch | ||
| } | ||
| crate::netstack::InternalErrorKind::InternalChannelClosed => { | ||
| InternalErrorKind::InternalChannelClosed | ||
| } | ||
| crate::netstack::InternalErrorKind::BadListenerHandle => { | ||
| InternalErrorKind::BadListenerHandle | ||
| } | ||
| crate::netstack::InternalErrorKind::BadSocketHandle => { | ||
| InternalErrorKind::BadSocketHandle | ||
| } | ||
| _ => unreachable!(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<ts_runtime::Error> for Error { | ||
| fn from(value: ts_runtime::Error) -> Self { | ||
| match value.kind { | ||
| ts_runtime::ErrorKind::Timeout => Error::Timeout, | ||
| ts_runtime::ErrorKind::ActorGone => Error::RuntimeDegraded, | ||
| ts_runtime::ErrorKind::MailboxFull | ts_runtime::ErrorKind::ReplyErr => { | ||
| Error::InternalFailure | ||
| } | ||
| ts_runtime::ErrorKind::ActorGone | ||
| | ts_runtime::ErrorKind::MailboxFull | ||
| | ts_runtime::ErrorKind::ReplyErr => Error::Internal(InternalErrorKind::Actor), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<NetstackError> for Error { | ||
| fn from(value: NetstackError) -> Self { | ||
| match value { | ||
| NetstackError::ChannelClosed => Error::RuntimeDegraded, | ||
|
|
||
| NetstackError::WrongType | ||
| | NetstackError::BadRequest | ||
| | NetstackError::InvariantViolated => Error::InternalFailure, | ||
|
|
||
| NetstackError::TcpStream(_) => Error::ConnectionReset, | ||
| NetstackError::Internal(k) => Error::Internal(k.into()), | ||
| NetstackError::ConnectionReset => Error::ConnectionReset, | ||
| NetstackError::BadRequest(_) => Error::Internal(InternalErrorKind::BadRequest), | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,7 +133,7 @@ pub async fn set_closest_derp( | |
| ) | ||
| }), | ||
| ) | ||
| .await?; | ||
| .await; | ||
|
|
||
| Ok((*id, map.get(id).unwrap().servers.clone())) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ use ts_capabilityversion::CapabilityVersion; | |
| use ts_http_util::{BytesBody, Http2}; | ||
| use url::Url; | ||
|
|
||
| use crate::{DialCandidate, DialMode, DialPlan, tokio::ConnectionError}; | ||
| use crate::{DialCandidate, DialMode, DialPlan, Error, InternalErrorKind, Operation}; | ||
|
|
||
| /// Manages state for control dial plan and handles selection of successive dial candidates. | ||
| pub struct ControlDialer { | ||
|
|
@@ -189,18 +189,18 @@ impl ControlDialer { | |
| &mut self, | ||
| url: &Url, | ||
| machine_keys: &ts_keys::MachineKeyPair, | ||
| ) -> Result<Http2<BytesBody>, ConnectionError> { | ||
| ) -> Result<Http2<BytesBody>, Error> { | ||
| let next = self.next_dialer(); | ||
| tracing::trace!(selected_control_dialer = ?next); | ||
|
|
||
| let host = url.host_str().ok_or(ConnectionError::ConnectionFailed)?; | ||
| let host = url.host_str().ok_or(Error::InvalidUrl(url.clone()))?; | ||
| let port = url | ||
| .port_or_known_default() | ||
| .ok_or(ConnectionError::ConnectionFailed)?; | ||
| .ok_or(Error::InvalidUrl(url.clone()))?; | ||
|
|
||
| let conn = next.dial(host, port).await.map_err(|e| { | ||
| tracing::error!(error = %e, %url, %host, port, "dialing tcp"); | ||
| ConnectionError::ConnectionFailed | ||
| Error::Internal(InternalErrorKind::Io, Operation::ConnectToControlServer) | ||
| })?; | ||
|
|
||
| tracing::debug!( | ||
|
|
@@ -223,27 +223,27 @@ pub async fn complete_connection<Io>( | |
| url: &Url, | ||
| machine_keys: &ts_keys::MachineKeyPair, | ||
| stream: Io, | ||
| ) -> Result<Http2<BytesBody>, ConnectionError> | ||
| ) -> Result<Http2<BytesBody>, Error> | ||
| where | ||
| Io: AsyncRead + AsyncWrite + Send + Unpin + 'static, | ||
| { | ||
| let h1_client = match url.scheme() { | ||
| "https" => { | ||
| let conn = ts_tls_util::connect( | ||
| ts_tls_util::server_name(url).ok_or(ConnectionError::ConnectionFailed)?, | ||
| ts_tls_util::server_name(url).ok_or(Error::InvalidUrl(url.clone()))?, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
| stream, | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "establishing tls connection"); | ||
| ConnectionError::ConnectionFailed | ||
| Error::io_error(e, Operation::ConnectToControlServer) | ||
| })?; | ||
| ts_http_util::http1::connect(conn).await? | ||
| } | ||
| "http" => ts_http_util::http1::connect(stream).await?, | ||
| other => { | ||
| tracing::error!(invalid_scheme = other); | ||
| return Err(ConnectionError::ConnectionFailed); | ||
| return Err(Error::InvalidUrl(url.clone())); | ||
| } | ||
| }; | ||
| let control_public_key = crate::tokio::fetch_control_key(url).await?; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
nit:
ok_or_elseto skip the clone on the happy path