From a6c84b8bb4da7406a82469f2ca20b7ad6d51ddf6 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Wed, 16 Oct 2024 16:19:11 -0300 Subject: [PATCH 01/11] Better error for invalid certificate. --- mirrord/cli/src/connection.rs | 8 ++++++-- mirrord/kube/src/error.rs | 22 ++++++++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/mirrord/cli/src/connection.rs b/mirrord/cli/src/connection.rs index b310d346219..eef34fddfcc 100644 --- a/mirrord/cli/src/connection.rs +++ b/mirrord/cli/src/connection.rs @@ -164,8 +164,12 @@ where .await .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateAgentFailed))?; - if let Err(error) = k8s_api.detect_openshift(progress).await { - tracing::debug!(?error, "Failed to detect OpenShift"); + if let Err(fail) = k8s_api.detect_openshift(progress).await { + tracing::debug!(?fail, "Failed to detect OpenShift"); + + if let KubeApiError::InvalidCertificate(fail) = fail { + tracing::warn!(fail); + } }; let agent_connect_info = tokio::time::timeout( diff --git a/mirrord/kube/src/error.rs b/mirrord/kube/src/error.rs index ce898f0501f..11723d7942e 100644 --- a/mirrord/kube/src/error.rs +++ b/mirrord/kube/src/error.rs @@ -7,8 +7,8 @@ pub type Result = std::result::Result; #[derive(Debug, Error)] pub enum KubeApiError { - #[error("Kube failed: {0}")] - KubeError(#[from] kube::Error), + #[error(transparent)] + KubeError(kube::Error), #[error("Connection to agent failed: {0}")] KubeConnectionError(#[from] std::io::Error), @@ -57,6 +57,13 @@ pub enum KubeApiError { /// Should be plural name of the resource String, ), + + #[error( + r"Kube API operation failed due to an invalid certificate: `{0}`! + - Consider enabling `accept_invalid_certificates` in your `mirrord.json`, or; + - Running `mirrord exec` with the `-c` flag." + )] + InvalidCertificate(Box), } impl KubeApiError { @@ -127,3 +134,14 @@ impl KubeApiError { Self::RequiresCopy(R::plural(&()).into_owned()) } } + +impl From for KubeApiError { + fn from(kube_fail: kube::Error) -> Self { + match kube_fail { + kube::Error::Service(fail) if fail.to_string().contains("InvalidCertificate") => { + Self::InvalidCertificate(fail) + } + other => Self::KubeError(other), + } + } +} From 80924b50c9201f9093e06a7d62ab26681837c6cb Mon Sep 17 00:00:00 2001 From: meowjesty Date: Wed, 16 Oct 2024 16:36:39 -0300 Subject: [PATCH 02/11] docs --- mirrord/kube/src/error.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mirrord/kube/src/error.rs b/mirrord/kube/src/error.rs index 11723d7942e..96e07c064ce 100644 --- a/mirrord/kube/src/error.rs +++ b/mirrord/kube/src/error.rs @@ -7,6 +7,8 @@ pub type Result = std::result::Result; #[derive(Debug, Error)] pub enum KubeApiError { + /// We manually implement `From` to give a better error in case of + /// kube failing due to an invalid certificate. #[error(transparent)] KubeError(kube::Error), @@ -58,6 +60,8 @@ pub enum KubeApiError { String, ), + /// Friendlier version of the invalid certificate error that comes from a + /// [`kube::Error::Service`]. #[error( r"Kube API operation failed due to an invalid certificate: `{0}`! - Consider enabling `accept_invalid_certificates` in your `mirrord.json`, or; From 0356f591af6601191763533d03d2cab826138ffe Mon Sep 17 00:00:00 2001 From: meowjesty Date: Thu, 17 Oct 2024 16:52:25 -0300 Subject: [PATCH 03/11] move error to cli --- mirrord/cli/src/connection.rs | 13 ++++++------- mirrord/cli/src/error.rs | 16 +++++++++++++++- mirrord/kube/src/error.rs | 22 +--------------------- 3 files changed, 22 insertions(+), 29 deletions(-) diff --git a/mirrord/cli/src/connection.rs b/mirrord/cli/src/connection.rs index eef34fddfcc..833f57ecb96 100644 --- a/mirrord/cli/src/connection.rs +++ b/mirrord/cli/src/connection.rs @@ -164,13 +164,12 @@ where .await .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateAgentFailed))?; - if let Err(fail) = k8s_api.detect_openshift(progress).await { - tracing::debug!(?fail, "Failed to detect OpenShift"); - - if let KubeApiError::InvalidCertificate(fail) = fail { - tracing::warn!(fail); - } - }; + k8s_api + .detect_openshift(progress) + .await + .map_err(|fail| CliError::auth_exec_error_or(fail, CliError::CreateAgentFailed)) + .inspect_err(|fail| tracing::debug!(?fail, "Failed to detect OpenShift!")) + .ok(); let agent_connect_info = tokio::time::timeout( Duration::from_secs(config.agent.startup_timeout), diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 2e2a0634f3c..6d596cf5951 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -187,6 +187,15 @@ pub(crate) enum CliError { ))] AgentConnectionFailed(KubeApiError), + /// Friendlier version of the invalid certificate error that comes from a + /// [`kube::Error::Service`]. + #[error("Kube API operation failed due to missing or invalid certificate: {0}")] + #[diagnostic(help( + r"1. Consider enabling `accept_invalid_certificates` in your `mirrord.json`, or; + 2. Running `mirrord exec` with the `-c` flag." + ))] + InvalidCertificate(KubeApiError), + #[error("Failed to communicate with the agent: {0}")] #[diagnostic(help("Please check agent status and logs.{GENERAL_HELP}"))] InitialAgentCommFailed(String), @@ -400,7 +409,12 @@ impl CliError { match error { KubeApiError::KubeError(Error::Auth(AuthError::AuthExec(error))) => { - Self::KubeAuthExecFailed(error) + Self::KubeAuthExecFailed(error.to_owned()) + } + KubeApiError::KubeError(Error::Service(ref fail)) + if fail.to_string().contains("InvalidCertificate") => + { + Self::InvalidCertificate(error) } error => fallback(error), } diff --git a/mirrord/kube/src/error.rs b/mirrord/kube/src/error.rs index 96e07c064ce..4865d964828 100644 --- a/mirrord/kube/src/error.rs +++ b/mirrord/kube/src/error.rs @@ -10,7 +10,7 @@ pub enum KubeApiError { /// We manually implement `From` to give a better error in case of /// kube failing due to an invalid certificate. #[error(transparent)] - KubeError(kube::Error), + KubeError(#[from] kube::Error), #[error("Connection to agent failed: {0}")] KubeConnectionError(#[from] std::io::Error), @@ -59,15 +59,6 @@ pub enum KubeApiError { /// Should be plural name of the resource String, ), - - /// Friendlier version of the invalid certificate error that comes from a - /// [`kube::Error::Service`]. - #[error( - r"Kube API operation failed due to an invalid certificate: `{0}`! - - Consider enabling `accept_invalid_certificates` in your `mirrord.json`, or; - - Running `mirrord exec` with the `-c` flag." - )] - InvalidCertificate(Box), } impl KubeApiError { @@ -138,14 +129,3 @@ impl KubeApiError { Self::RequiresCopy(R::plural(&()).into_owned()) } } - -impl From for KubeApiError { - fn from(kube_fail: kube::Error) -> Self { - match kube_fail { - kube::Error::Service(fail) if fail.to_string().contains("InvalidCertificate") => { - Self::InvalidCertificate(fail) - } - other => Self::KubeError(other), - } - } -} From cae90f3466468c83895c2833d3a0c4dc92535844 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Thu, 17 Oct 2024 18:51:53 -0300 Subject: [PATCH 04/11] better error message and fn --- mirrord/cli/src/connection.rs | 8 ++++---- mirrord/cli/src/error.rs | 16 +++++++++------- mirrord/cli/src/main.rs | 23 +++++++++++------------ mirrord/cli/src/operator.rs | 2 +- mirrord/cli/src/vpn.rs | 2 +- mirrord/kube/src/error.rs | 2 -- 6 files changed, 26 insertions(+), 27 deletions(-) diff --git a/mirrord/cli/src/connection.rs b/mirrord/cli/src/connection.rs index 833f57ecb96..bbd29efa8b8 100644 --- a/mirrord/cli/src/connection.rs +++ b/mirrord/cli/src/connection.rs @@ -162,12 +162,12 @@ where let k8s_api = KubernetesAPI::create(config) .await - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateAgentFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateAgentFailed))?; k8s_api .detect_openshift(progress) .await - .map_err(|fail| CliError::auth_exec_error_or(fail, CliError::CreateAgentFailed)) + .map_err(|fail| CliError::friendlier_error_or_else(fail, CliError::CreateAgentFailed)) .inspect_err(|fail| tracing::debug!(?fail, "Failed to detect OpenShift!")) .ok(); @@ -177,14 +177,14 @@ where ) .await .unwrap_or(Err(KubeApiError::AgentReadyTimeout)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateAgentFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateAgentFailed))?; let (sender, receiver) = wrap_raw_connection( k8s_api .create_connection(agent_connect_info.clone()) .await .map_err(|error| { - CliError::auth_exec_error_or(error, CliError::AgentConnectionFailed) + CliError::friendlier_error_or_else(error, CliError::AgentConnectionFailed) })?, ); diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 6d596cf5951..a29a38fbad2 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -191,8 +191,8 @@ pub(crate) enum CliError { /// [`kube::Error::Service`]. #[error("Kube API operation failed due to missing or invalid certificate: {0}")] #[diagnostic(help( - r"1. Consider enabling `accept_invalid_certificates` in your `mirrord.json`, or; - 2. Running `mirrord exec` with the `-c` flag." + "Consider enabling `accept_invalid_certificates` in your \ + `mirrord.json`, or running `mirrord exec` with the `-c` flag." ))] InvalidCertificate(KubeApiError), @@ -398,10 +398,12 @@ pub(crate) enum CliError { } impl CliError { - /// If the given [`KubeApiError`] originates from failed authentication command exec, produces - /// [`CliError::KubeAuthExecFailed`]. Otherwise, uses the given `fallback` function to - /// produce the result. - pub fn auth_exec_error_or Self>( + /// Here we give more meaning to some errors, instead of just letting them pass as + /// whatever [`KubeApiError`] we're getting. + /// + /// If `error` is not something we're interested in (no need for a special diagnostic message), + /// then we turn it into a `fallback` [`CliError`]. + pub fn friendlier_error_or_else Self>( error: KubeApiError, fallback: F, ) -> Self { @@ -434,7 +436,7 @@ impl From for CliError { operator_version, }, OperatorApiError::CreateKubeClient(e) => { - Self::auth_exec_error_or(e, Self::CreateKubeApiFailed) + Self::friendlier_error_or_else(e, Self::CreateKubeApiFailed) } OperatorApiError::ConnectRequestBuildError(e) => Self::ConnectRequestBuildError(e), OperatorApiError::KubeError { diff --git a/mirrord/cli/src/main.rs b/mirrord/cli/src/main.rs index de645b1f824..fcae81024af 100644 --- a/mirrord/cli/src/main.rs +++ b/mirrord/cli/src/main.rs @@ -388,7 +388,7 @@ async fn list_targets(layer_config: &LayerConfig, args: &ListTargetArgs) -> Resu ) .await .and_then(|config| Client::try_from(config).map_err(From::from)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateKubeApiFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateKubeApiFailed))?; let namespace = args .namespace @@ -422,15 +422,13 @@ async fn list_targets(layer_config: &LayerConfig, args: &ListTargetArgs) -> Resu if ALL_TARGETS_SUPPORTED_OPERATOR_VERSION .matches(&api.operator().spec.operator_version) => { - seeker - .all() - .await - .map_err(|error| CliError::auth_exec_error_or(error, CliError::ListTargetsFailed)) + seeker.all().await.map_err(|error| { + CliError::friendlier_error_or_else(error, CliError::ListTargetsFailed) + }) } - _ => seeker - .all_open_source() - .await - .map_err(|error| CliError::auth_exec_error_or(error, CliError::ListTargetsFailed)), + _ => seeker.all_open_source().await.map_err(|error| { + CliError::friendlier_error_or_else(error, CliError::ListTargetsFailed) + }), } } @@ -593,9 +591,10 @@ async fn port_forward(args: &PortForwardArgs, watch: drain::Watch) -> Result<()> .map_err(|agent_con_error| match agent_con_error { AgentConnectionError::Io(error) => CliError::PortForwardingSetupError(error.into()), AgentConnectionError::Operator(operator_api_error) => operator_api_error.into(), - AgentConnectionError::Kube(kube_api_error) => { - CliError::auth_exec_error_or(kube_api_error, CliError::PortForwardingSetupError) - } + AgentConnectionError::Kube(kube_api_error) => CliError::friendlier_error_or_else( + kube_api_error, + CliError::PortForwardingSetupError, + ), AgentConnectionError::Tls(connection_tls_error) => connection_tls_error.into(), AgentConnectionError::NoConnectionMethod => CliError::PortForwardingNoConnectionMethod, })?; diff --git a/mirrord/cli/src/operator.rs b/mirrord/cli/src/operator.rs index bdc1d8e3ff3..840355fa418 100644 --- a/mirrord/cli/src/operator.rs +++ b/mirrord/cli/src/operator.rs @@ -142,7 +142,7 @@ async fn get_status_api(config: Option<&Path>) -> Result ) .await .and_then(|config| Client::try_from(config).map_err(From::from)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateKubeApiFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateKubeApiFailed))?; Ok(Api::all(client)) } diff --git a/mirrord/cli/src/vpn.rs b/mirrord/cli/src/vpn.rs index f2768ee9efa..a6734e0824c 100644 --- a/mirrord/cli/src/vpn.rs +++ b/mirrord/cli/src/vpn.rs @@ -36,7 +36,7 @@ pub async fn vpn_command(args: VpnArgs) -> Result<()> { ) .await .and_then(|config| kube::Client::try_from(config).map_err(From::from)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateKubeApiFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateKubeApiFailed))?; let mut sub_progress = progress.subtask("fetching vpn info"); diff --git a/mirrord/kube/src/error.rs b/mirrord/kube/src/error.rs index 4865d964828..df816ab8565 100644 --- a/mirrord/kube/src/error.rs +++ b/mirrord/kube/src/error.rs @@ -7,8 +7,6 @@ pub type Result = std::result::Result; #[derive(Debug, Error)] pub enum KubeApiError { - /// We manually implement `From` to give a better error in case of - /// kube failing due to an invalid certificate. #[error(transparent)] KubeError(#[from] kube::Error), From 5af3a0a3c2790de1a041a1a6aa1e388f5a626925 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Fri, 18 Oct 2024 23:23:50 -0300 Subject: [PATCH 05/11] add tests for InvalidCertificate. // Fix contains to use Debug. --- Cargo.lock | 4 + mirrord/agent/src/main.rs | 1 + mirrord/cli/Cargo.toml | 5 ++ mirrord/cli/src/error.rs | 142 +++++++++++++++++++++++++++++++- tests/issue-2824-certs/cert.pem | 22 +++++ tests/issue-2824-certs/key.pem | 28 +++++++ 6 files changed, 201 insertions(+), 1 deletion(-) create mode 100644 tests/issue-2824-certs/cert.pem create mode 100644 tests/issue-2824-certs/key.pem diff --git a/Cargo.lock b/Cargo.lock index 3cef4580e36..96ff36f814e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3996,7 +3996,11 @@ dependencies = [ "drain", "exec", "futures", + "http-body 1.0.1", + "http-body-util", "humantime", + "hyper 1.4.1", + "hyper-util", "k8s-openapi", "kube", "local-ip-address", diff --git a/mirrord/agent/src/main.rs b/mirrord/agent/src/main.rs index 3c1bb982308..57ab399d72e 100644 --- a/mirrord/agent/src/main.rs +++ b/mirrord/agent/src/main.rs @@ -1,6 +1,7 @@ #![feature(hash_extract_if)] #![feature(let_chains)] #![feature(iterator_try_collect)] +#![feature(entry_insert)] #![cfg_attr(target_os = "linux", feature(tcp_quickack))] #![warn(clippy::indexing_slicing)] diff --git a/mirrord/cli/Cargo.toml b/mirrord/cli/Cargo.toml index 47b0298b8d1..c72bfc6a6a5 100644 --- a/mirrord/cli/Cargo.toml +++ b/mirrord/cli/Cargo.toml @@ -80,3 +80,8 @@ mirrord-layer = { artifact = "cdylib", path = "../layer" } [dev-dependencies] rstest = "0.23" +hyper.workspace = true +hyper-util = { workspace = true, "features" = ["server"] } +http-body.workspace = true +http-body-util.workspace = true +kube.workspace = true diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index a29a38fbad2..6615c4260b0 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -413,8 +413,10 @@ impl CliError { KubeApiError::KubeError(Error::Auth(AuthError::AuthExec(error))) => { Self::KubeAuthExecFailed(error.to_owned()) } + // UGH(alex): Type-erased errors are messy, and this one is especially bad. + // See `kube_service_error_dependency_is_in_sync` for a "what's going on here". KubeApiError::KubeError(Error::Service(ref fail)) - if fail.to_string().contains("InvalidCertificate") => + if format!("{fail:?}").contains("InvalidCertificate") => { Self::InvalidCertificate(error) } @@ -478,3 +480,141 @@ impl From for CliError { #[derive(Debug, Error)] #[error("unsupported runtime version")] pub struct UnsupportedRuntimeVariant; + +#[cfg(test)] +mod tests { + use std::{ + fs, io, + net::{Ipv4Addr, SocketAddr}, + path::PathBuf, + sync::Arc, + }; + + use http_body_util::Full; + use hyper::{ + body::{Bytes, Incoming}, + service::service_fn, + Request, Response, + }; + use hyper_util::{ + rt::{TokioExecutor, TokioIo}, + server::conn::auto::Builder, + }; + use k8s_openapi::api::core::v1::Pod; + use kube::{api::ListParams, Api}; + use rustls::{crypto::aws_lc_rs::default_provider, pki_types::CertificateDer, ServerConfig}; + use tokio::{net::TcpListener, sync::Notify}; + use tokio_rustls::TlsAcceptor; + + /// With this test we're trying to `assert` that our [`kube`] crate is (somewhat) + /// version-synced with [`rustls`]. To give a friendlier error message on kube requests + /// when there's a certificate problem, we must dig down into the [`kube::Error`]. + /// + /// Certificate errors come under the [`kube::Error::Service`] variant, which holds + /// a very annoying type-erased boxed error. Trying to `downcast` this is messy, so + /// instead we check the debug message of the error. + /// + /// We want this error (or something similar) to happen: + /// + /// `Service(hyper_util::client::legacy::Error(Connect, Custom { kind: Other, error: Custom { + /// kind: InvalidData, error: InvalidCertificate(Expired) } }))`. + /// + /// The part that we care about is the `InvalidCertificate`, the rest is not relevant. + /// + /// This test may fail if [`rustls`] changes the `error: InvalidCertificate` message, + /// or any of the upper errors change. + #[tokio::test] + async fn kube_service_error_dependency_is_in_sync() { + use kube::{Client, Config}; + + let provider = default_provider(); + provider.install_default().unwrap(); + + let notify = Arc::new(Notify::new()); + let wait_notify = notify.clone(); + + tokio::spawn(async move { + run_hyper_tls_server(notify).await; + }); + + // Wait until the server is listening before we start connecting. + wait_notify.notified().await; + + let kube_config = Config::new("https://127.0.0.1:9669".parse().unwrap()); + let client = Client::try_from(kube_config).unwrap(); + + // Manage pods + let pods: Api = Api::default_namespaced(client); + + let lp = ListParams::default().fields(&format!("legend={}", "hank")); + + if let Err(fail) = pods.list(&lp).await { + assert!(format!("{fail:?}").contains("InvalidCertificate"), + "We were expecting this error {fail:?} to have `InvalidCertificate` but it's {fail}!" + ); + } + } + + /// Creates an [`hyper`] server with a broken certificate and a _catch-all_ route on + /// `localhost:9669`. + /// + /// The `.pem` files live in `/mirrord/tests/issue-2824-certs`. + async fn run_hyper_tls_server(notify: Arc) { + let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 9669); + + let incoming = TcpListener::bind(&addr).await.unwrap(); + + let mut certs_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + certs_path.push("../.."); + certs_path.push("tests"); + certs_path.push("issue-2824-certs"); + + println!("{certs_path:?}"); + + let cert_file = fs::File::open(certs_path.join("cert.pem")).unwrap(); + let mut reader = io::BufReader::new(cert_file); + let certs = rustls_pemfile::certs(&mut reader) + .collect::>>>() + .unwrap(); + + let key_file = fs::File::open(certs_path.join("key.pem")).unwrap(); + let mut reader = io::BufReader::new(key_file); + let key = rustls_pemfile::private_key(&mut reader) + .map(|key| key.unwrap()) + .unwrap(); + + // Build TLS configuration. + let mut server_config = ServerConfig::builder() + .with_no_client_auth() + .with_single_cert(certs, key) + .unwrap(); + + server_config.alpn_protocols = + vec![b"h2".to_vec(), b"http/1.1".to_vec(), b"http/1.0".to_vec()]; + let tls_acceptor = TlsAcceptor::from(Arc::new(server_config)); + + /// Catch-all handler, we don't care about the response, any request is supposed + /// to fail due to `InvalidCertificate`. + async fn handle_any_route( + _: Request, + ) -> Result>, hyper::Error> { + Ok(Response::new(Full::default())) + } + + let service = service_fn(handle_any_route); + + // We're ready for the client side to start. + notify.notify_waiters(); + let (tcp_stream, _remote_addr) = incoming.accept().await.unwrap(); + + let tls_acceptor = tls_acceptor.clone(); + tokio::spawn(async move { + let tls_stream = tls_acceptor.accept(tcp_stream).await.unwrap(); + + Builder::new(TokioExecutor::new()) + .serve_connection(TokioIo::new(tls_stream), service) + .await + .unwrap(); + }); + } +} diff --git a/tests/issue-2824-certs/cert.pem b/tests/issue-2824-certs/cert.pem new file mode 100644 index 00000000000..656aa880552 --- /dev/null +++ b/tests/issue-2824-certs/cert.pem @@ -0,0 +1,22 @@ +-----BEGIN CERTIFICATE----- +MIIDkzCCAnugAwIBAgIUXVYkRCrM/ge03DVymDtXCuybp7gwDQYJKoZIhvcNAQEL +BQAwWTELMAkGA1UEBhMCVVMxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM +GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDESMBAGA1UEAwwJbG9jYWxob3N0MB4X +DTIxMDczMTE0MjIxMloXDTIyMDczMTE0MjIxMlowWTELMAkGA1UEBhMCVVMxEzAR +BgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5 +IEx0ZDESMBAGA1UEAwwJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A +MIIBCgKCAQEA02V5ZjmqLB/VQwTarrz/35qsa83L+DbAoa0001+jVmmC+G9Nufi0 +daroFWj/Uicv2fZWETU8JoZKUrX4BK9og5cg5rln/CtBRWCUYIwRgY9R/CdBGPn4 +kp+XkSJaCw74ZIyLy/Zfux6h8ES1m9YRnBza+s7U+ImRBRf4MRPtXQ3/mqJxAZYq +dOnKnvssRyD2qutgVTAxwMUvJWIivRhRYDj7WOpS4CEEeQxP1iH1/T5P7FdtTGdT +bVBABCA8JhL96uFGPpOYHcM/7R5EIA3yZ5FNg931QzoDITjtXGtQ6y9/l/IYkWm6 +J67RWcN0IoTsZhz0WNU4gAeslVtJLofn8QIDAQABo1MwUTAdBgNVHQ4EFgQUzFnK +NfS4LAYuKeWwHbzooER0yZ0wHwYDVR0jBBgwFoAUzFnKNfS4LAYuKeWwHbzooER0 +yZ0wDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAk4O+e9jia59W +ZwetN4GU7OWcYhmOgSizRSs6u7mTfp62LDMt96WKU3THksOnZ44HnqWQxsSfdFVU +XJD12tjvVU8Z4FWzQajcHeemUYiDze8EAh6TnxnUcOrU8IcwiKGxCWRY/908jnWg ++MMscfMCMYTRdeTPqD8fGzAlUCtmyzH6KLE3s4Oo/r5+NR+Uvrwpdvb7xe0MwwO9 +Q/zR4N8ep/HwHVEObcaBofE1ssZLksX7ZgCP9wMgXRWpNAtC5EWxMbxYjBfWFH24 +fDJlBMiGJWg8HHcxK7wQhFh+fuyNzE+xEWPsI9VL1zDftd9x8/QsOagyEOnY8Vxr +AopvZ09uEQ== +-----END CERTIFICATE----- diff --git a/tests/issue-2824-certs/key.pem b/tests/issue-2824-certs/key.pem new file mode 100644 index 00000000000..3de14eb32f6 --- /dev/null +++ b/tests/issue-2824-certs/key.pem @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDTZXlmOaosH9VD +BNquvP/fmqxrzcv4NsChrTTTX6NWaYL4b025+LR1qugVaP9SJy/Z9lYRNTwmhkpS +tfgEr2iDlyDmuWf8K0FFYJRgjBGBj1H8J0EY+fiSn5eRIloLDvhkjIvL9l+7HqHw +RLWb1hGcHNr6ztT4iZEFF/gxE+1dDf+aonEBlip06cqe+yxHIPaq62BVMDHAxS8l +YiK9GFFgOPtY6lLgIQR5DE/WIfX9Pk/sV21MZ1NtUEAEIDwmEv3q4UY+k5gdwz/t +HkQgDfJnkU2D3fVDOgMhOO1ca1DrL3+X8hiRabonrtFZw3QihOxmHPRY1TiAB6yV +W0kuh+fxAgMBAAECggEADltu8k1qTFLhJgsXWxTFAAe+PBgfCT2WuaRM2So+qqjB +12Of0MieYPt5hbK63HaC3nfHgqWt7yPhulpXfOH45C8IcgMXl93MMg0MJr58leMI ++2ojFrIrerHSFm5R1TxwDEwrVm/mMowzDWFtQCc6zPJ8wNn5RuP48HKfTZ3/2fjw +zEjSwPO2wFMfo1EJNTjlI303lFbdFBs67NaX6puh30M7Tn+gznHKyO5a7F57wkIt +fkgnEy/sgMedQlwX7bRpUoD6f0fZzV8Qz4cHFywtYErczZJh3VGitJoO/VCIDdty +RPXOAqVDd7EpP1UUehZlKVWZ0OZMEfRgKbRCel5abQKBgQDwgwrIQ5+BiZv6a0VT +ETeXB+hRbvBinRykNo/RvLc3j1enRh9/zO/ShadZIXgOAiM1Jnr5Gp8KkNGca6K1 +myhtad7xYPODYzNXXp6T1OPgZxHZLIYzVUj6ypXeV64Te5ZiDaJ1D49czsq+PqsQ +XRcgBJSNpFtDFiXWpjXWfx8PxwKBgQDhAnLY5Sl2eeQo+ud0MvjwftB/mN2qCzJY +5AlQpRI4ThWxJgGPuHTR29zVa5iWNYuA5LWrC1y/wx+t5HKUwq+5kxvs+npYpDJD +ZX/w0Glc6s0Jc/mFySkbw9B2LePedL7lRF5OiAyC6D106Sc9V2jlL4IflmOzt4CD +ZTNbLtC6hwKBgHfIzBXxl/9sCcMuqdg1Ovp9dbcZCaATn7ApfHd5BccmHQGyav27 +k7XF2xMJGEHhzqcqAxUNrSgV+E9vTBomrHvRvrd5Ec7eGTPqbBA0d0nMC5eeFTh7 +wV0miH20LX6Gjt9G6yJiHYSbeV5G1+vOcTYBEft5X/qJjU7aePXbWh0BAoGBAJlV +5tgCCuhvFloK6fHYzqZtdT6O+PfpW20SMXrgkvMF22h2YvgDFrDwqKRUB47NfHzg +3yBpxNH1ccA5/w97QO8w3gX3h6qicpJVOAPusu6cIBACFZfjRv1hyszOZwvw+Soa +Fj5kHkqTY1YpkREPYS9V2dIW1Wjic1SXgZDw7VM/AoGAP/cZ3ZHTSCDTFlItqy5C +rIy2AiY0WJsx+K0qcvtosPOOwtnGjWHb1gdaVdfX/IRkSsX4PAOdnsyidNC5/l/m +y8oa+5WEeGFclWFhr4dnTA766o8HrM2UjIgWWYBF2VKdptGnHxFeJWFUmeQC/xeW +w37pCS7ykL+7gp7V0WShYsw= +-----END PRIVATE KEY----- From 766e692f31f481669f2ee574ef2a9cb52fc40ab1 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Fri, 18 Oct 2024 23:25:40 -0300 Subject: [PATCH 06/11] docs --- mirrord/cli/src/error.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 6615c4260b0..8f45d2ca6fb 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -523,6 +523,9 @@ mod tests { /// /// This test may fail if [`rustls`] changes the `error: InvalidCertificate` message, /// or any of the upper errors change. + /// + /// Relying on the `Debug` string version of the error, as the `Display` version is + /// just a generic `ServiceError: client error (Connect)`. #[tokio::test] async fn kube_service_error_dependency_is_in_sync() { use kube::{Client, Config}; From e4d5095524cb67ac0cb564b358e5a889ff263eee Mon Sep 17 00:00:00 2001 From: meowjesty Date: Fri, 18 Oct 2024 23:36:52 -0300 Subject: [PATCH 07/11] fix docs --- mirrord/cli/src/error.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 8f45d2ca6fb..08590df4366 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -157,8 +157,8 @@ pub(crate) enum OperatorSetupError { #[derive(Debug, Error, Diagnostic)] pub(crate) enum CliError { - /// Do not construct this variant directly, use [`CliError::auth_exec_error_or`] to allow for - /// more granular error detection. + /// Do not construct this variant directly, use [`CliError::friendlier_error_or_else`] to allow + /// for more granular error detection. #[error("Failed to create Kubernetes API client: {0}")] #[diagnostic(help("Please check that Kubernetes is configured correctly and test your connection with `kubectl get pods`.{GENERAL_HELP}"))] CreateKubeApiFailed(KubeApiError), @@ -167,8 +167,8 @@ pub(crate) enum CliError { #[diagnostic(help("Please check that Kubernetes is configured correctly and test your connection with `kubectl get pods`.{GENERAL_HELP}"))] ListTargetsFailed(KubeApiError), - /// Do not construct this variant directly, use [`CliError::auth_exec_error_or`] to allow for - /// more granular error detection. + /// Do not construct this variant directly, use [`CliError::friendlier_error_or_else`] to allow + /// for more granular error detection. #[error("Failed to create mirrord-agent: {0}")] #[diagnostic(help( r"1. Please check the status of the agent pod, using `kubectl get pods` in the relevant namespace. @@ -177,8 +177,8 @@ pub(crate) enum CliError { ))] CreateAgentFailed(KubeApiError), - /// Do not construct this variant directly, use [`CliError::auth_exec_error_or`] to allow for - /// more granular error detection. + /// Do not construct this variant directly, use [`CliError::friendlier_error_or_else`] to allow + /// for more granular error detection. #[error("Failed to connect to the created mirrord-agent: {0}")] #[diagnostic(help( "Please check the following: From c84b584bbb4e70e06660e4e41220756baaae20a7 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Mon, 21 Oct 2024 14:12:38 -0300 Subject: [PATCH 08/11] Generate certificate in the test. --- mirrord/cli/src/error.rs | 46 ++++++++++++++------------------- tests/issue-2824-certs/cert.pem | 22 ---------------- tests/issue-2824-certs/key.pem | 29 --------------------- 3 files changed, 20 insertions(+), 77 deletions(-) delete mode 100644 tests/issue-2824-certs/cert.pem delete mode 100644 tests/issue-2824-certs/key.pem diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 08590df4366..005179748ac 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -484,9 +484,7 @@ pub struct UnsupportedRuntimeVariant; #[cfg(test)] mod tests { use std::{ - fs, io, net::{Ipv4Addr, SocketAddr}, - path::PathBuf, sync::Arc, }; @@ -502,7 +500,11 @@ mod tests { }; use k8s_openapi::api::core::v1::Pod; use kube::{api::ListParams, Api}; - use rustls::{crypto::aws_lc_rs::default_provider, pki_types::CertificateDer, ServerConfig}; + use rustls::{ + crypto::aws_lc_rs::default_provider, + pki_types::{pem::PemObject, CertificateDer, PrivateKeyDer}, + ServerConfig, + }; use tokio::{net::TcpListener, sync::Notify}; use tokio_rustls::TlsAcceptor; @@ -551,11 +553,12 @@ mod tests { let lp = ListParams::default().fields(&format!("legend={}", "hank")); - if let Err(fail) = pods.list(&lp).await { - assert!(format!("{fail:?}").contains("InvalidCertificate"), - "We were expecting this error {fail:?} to have `InvalidCertificate` but it's {fail}!" - ); - } + let list = pods.list(&lp).await; + assert!( + list.as_ref() + .is_err_and(|fail| { format!("{fail:?}").contains("InvalidCertificate") }), + "We were expecting an error with `InvalidCertificate`, but got {list:?}!" + ); } /// Creates an [`hyper`] server with a broken certificate and a _catch-all_ route on @@ -563,33 +566,24 @@ mod tests { /// /// The `.pem` files live in `/mirrord/tests/issue-2824-certs`. async fn run_hyper_tls_server(notify: Arc) { + use rcgen::generate_simple_self_signed; + let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 9669); let incoming = TcpListener::bind(&addr).await.unwrap(); - let mut certs_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - certs_path.push("../.."); - certs_path.push("tests"); - certs_path.push("issue-2824-certs"); - - println!("{certs_path:?}"); + // Generate a certificate that should not work with kube. + let cert_key = generate_simple_self_signed(vec!["mieszko.i".to_string()]).unwrap(); + let cert_pem = cert_key.cert.pem().into_bytes(); + let cert = CertificateDer::from_pem_slice(&cert_pem).unwrap(); - let cert_file = fs::File::open(certs_path.join("cert.pem")).unwrap(); - let mut reader = io::BufReader::new(cert_file); - let certs = rustls_pemfile::certs(&mut reader) - .collect::>>>() - .unwrap(); - - let key_file = fs::File::open(certs_path.join("key.pem")).unwrap(); - let mut reader = io::BufReader::new(key_file); - let key = rustls_pemfile::private_key(&mut reader) - .map(|key| key.unwrap()) - .unwrap(); + let key_pem = cert_key.key_pair.serialize_pem().into_bytes(); + let key = PrivateKeyDer::from_pem_slice(&key_pem).unwrap(); // Build TLS configuration. let mut server_config = ServerConfig::builder() .with_no_client_auth() - .with_single_cert(certs, key) + .with_single_cert(vec![cert], key) .unwrap(); server_config.alpn_protocols = diff --git a/tests/issue-2824-certs/cert.pem b/tests/issue-2824-certs/cert.pem deleted file mode 100644 index 656aa880552..00000000000 --- a/tests/issue-2824-certs/cert.pem +++ /dev/null @@ -1,22 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIDkzCCAnugAwIBAgIUXVYkRCrM/ge03DVymDtXCuybp7gwDQYJKoZIhvcNAQEL -BQAwWTELMAkGA1UEBhMCVVMxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM -GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDESMBAGA1UEAwwJbG9jYWxob3N0MB4X -DTIxMDczMTE0MjIxMloXDTIyMDczMTE0MjIxMlowWTELMAkGA1UEBhMCVVMxEzAR -BgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5 -IEx0ZDESMBAGA1UEAwwJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A -MIIBCgKCAQEA02V5ZjmqLB/VQwTarrz/35qsa83L+DbAoa0001+jVmmC+G9Nufi0 -daroFWj/Uicv2fZWETU8JoZKUrX4BK9og5cg5rln/CtBRWCUYIwRgY9R/CdBGPn4 -kp+XkSJaCw74ZIyLy/Zfux6h8ES1m9YRnBza+s7U+ImRBRf4MRPtXQ3/mqJxAZYq -dOnKnvssRyD2qutgVTAxwMUvJWIivRhRYDj7WOpS4CEEeQxP1iH1/T5P7FdtTGdT -bVBABCA8JhL96uFGPpOYHcM/7R5EIA3yZ5FNg931QzoDITjtXGtQ6y9/l/IYkWm6 -J67RWcN0IoTsZhz0WNU4gAeslVtJLofn8QIDAQABo1MwUTAdBgNVHQ4EFgQUzFnK -NfS4LAYuKeWwHbzooER0yZ0wHwYDVR0jBBgwFoAUzFnKNfS4LAYuKeWwHbzooER0 -yZ0wDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAk4O+e9jia59W -ZwetN4GU7OWcYhmOgSizRSs6u7mTfp62LDMt96WKU3THksOnZ44HnqWQxsSfdFVU -XJD12tjvVU8Z4FWzQajcHeemUYiDze8EAh6TnxnUcOrU8IcwiKGxCWRY/908jnWg -+MMscfMCMYTRdeTPqD8fGzAlUCtmyzH6KLE3s4Oo/r5+NR+Uvrwpdvb7xe0MwwO9 -Q/zR4N8ep/HwHVEObcaBofE1ssZLksX7ZgCP9wMgXRWpNAtC5EWxMbxYjBfWFH24 -fDJlBMiGJWg8HHcxK7wQhFh+fuyNzE+xEWPsI9VL1zDftd9x8/QsOagyEOnY8Vxr -AopvZ09uEQ== ------END CERTIFICATE----- diff --git a/tests/issue-2824-certs/key.pem b/tests/issue-2824-certs/key.pem deleted file mode 100644 index bd2404debf1..00000000000 --- a/tests/issue-2824-certs/key.pem +++ /dev/null @@ -1,29 +0,0 @@ ------BEGIN PRIVATE KEY----- -MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDTZXlmOaosH9VD -BNquvP/fmqxrzcv4NsChrTTTX6NWaYL4b025+LR1qugVaP9SJy/Z9lYRNTwmhkpS -tfgEr2iDlyDmuWf8K0FFYJRgjBGBj1H8J0EY+fiSn5eRIloLDvhkjIvL9l+7HqHw -RLWb1hGcHNr6ztT4iZEFF/gxE+1dDf+aonEBlip06cqe+yxHIPaq62BVMDHAxS8l -YiK9GFFgOPtY6lLgIQR5DE/WIfX9Pk/sV21MZ1NtUEAEIDwmEv3q4UY+k5gdwz/t -HkQgDfJnkU2D3fVDOgMhOO1ca1DrL3+X8hiRabonrtFZw3QihOxmHPRY1TiAB6yV -W0kuh+fxAgMBAAECggEADltu8k1qTFLhJgsXWxTFAAe+PBgfCT2WuaRM2So+qqjB -12Of0MieYPt5hbK63HaC3nfHgqWt7yPhulpXfOH45C8IcgMXl93MMg0MJr58leMI -+2ojFrIrerHSFm5R1TxwDEwrVm/mMowzDWFtQCc6zPJ8wNn5RuP48HKfTZ3/2fjw -zEjSwPO2wFMfo1EJNTjlI303lFbdFBs67NaX6puh30M7Tn+gznHKyO5a7F57wkIt -fkgnEy/sgMedQlwX7bRpUoD6f0fZzV8Qz4cHFywtYErczZJh3VGitJoO/VCIDdty -RPXOAqVDd7EpP1UUehZlKVWZ0OZMEfRgKbRCel5abQKBgQDwgwrIQ5+BiZv6a0VT -ETeXB+hRbvBinRykNo/RvLc3j1enRh9/zO/ShadZIXgOAiM1Jnr5Gp8KkNGca6K1 -myhtad7xYPODYzNXXp6T1OPgZxHZLIYzVUj6ypXeV64Te5ZiDaJ1D49czsq+PqsQ -XRcgBJSNpFtDFiXWpjXWfx8PxwKBgQDhAnLY5Sl2eeQo+ud0MvjwftB/mN2qCzJY -5AlQpRI4ThWxJgGPuHTR29zVa5iWNYuA5LWrC1y/wx+t5HKUwq+5kxvs+npYpDJD -ZX/w0Glc6s0Jc/mFySkbw9B2LePedL7lRF5OiAyC6D106Sc9V2jlL4IflmOzt4CD -ZTNbLtC6hwKBgHfIzBXxl/9sCcMuqdg1Ovp9dbcZCaATn7ApfHd5BccmHQGyav27 -k7XF2xMJGEHhzqcqAxUNrSgV+E9vTBomrHvRvrd5Ec7eGTPqbBA0d0nMC5eeFTh7 -wV0miH20LX6Gjt9G6yJiHYSbeV5G1+vOcTYBEft5X/qJjU7aePXbWh0BAoGBAJlV -5tgCCuhvFloK6fHYzqZtdT6O+PfpW20SMXrgkvMF22h2YvgDFrDwqKRUB47NfHzg -3yBpxNH1ccA5/w97QO8w3gX3h6qicpJVOAPusu6cIBACFZfjRv1hyszOZwvw+Soa -Fj5kHkqTY1YpkREPYS9V2dIW1Wjic1SXgZDw7VM/AoGAP/cZ3ZHTSCDTFlItqy5C -rIy2AiY0WJsx+K0qcvtosPOOwtnGjWHb1gdaVdfX/IRkSsX4PAOdnsyidNC5/l/m -y8oa+5WEeGFclWFhr4dnTA766o8HrM2UjIgWWYBF2VKdptGnHxFeJWFUmeQC/xeW -w37pCS7ykL+7gp7V0WShYsw= ------END PRIVATE KEY----- -ServiceError: client error (Connect) From fd94fb1ae70945adfce5e3d0c29acfb0339e1d15 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Mon, 21 Oct 2024 14:14:11 -0300 Subject: [PATCH 09/11] increase test timeout for outgoing_tcp_bound_socket --- mirrord/layer/tests/outgoing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mirrord/layer/tests/outgoing.rs b/mirrord/layer/tests/outgoing.rs index 355d4266f2f..6366552576d 100644 --- a/mirrord/layer/tests/outgoing.rs +++ b/mirrord/layer/tests/outgoing.rs @@ -187,7 +187,7 @@ async fn outgoing_tcp_from_the_local_app_broken( /// application. #[rstest] #[tokio::test] -#[timeout(Duration::from_secs(10))] +#[timeout(Duration::from_secs(15))] async fn outgoing_tcp_bound_socket(dylib_path: &Path) { let (mut test_process, mut intproxy) = Application::RustIssue2438 .start_process_with_layer(dylib_path, vec![], None) From c5aab4dde360d856fb76d029901808c0cc06dc5a Mon Sep 17 00:00:00 2001 From: meowjesty Date: Mon, 21 Oct 2024 14:15:36 -0300 Subject: [PATCH 10/11] fix docs --- mirrord/cli/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 26146d788ff..99c2996a36b 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -572,7 +572,7 @@ mod tests { /// Creates an [`hyper`] server with a broken certificate and a _catch-all_ route on /// `localhost:9669`. /// - /// The `.pem` files live in `/mirrord/tests/issue-2824-certs`. + /// The certificate is generated here with [`rcgen::generate_simple_self_signed`]. async fn run_hyper_tls_server(notify: Arc) { use rcgen::generate_simple_self_signed; From 38fdd799be53232babdde8c4302c791beacc301d Mon Sep 17 00:00:00 2001 From: meowjesty Date: Mon, 21 Oct 2024 14:16:33 -0300 Subject: [PATCH 11/11] changelog --- changelog.d/2824.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/2824.changed.md diff --git a/changelog.d/2824.changed.md b/changelog.d/2824.changed.md new file mode 100644 index 00000000000..470c0c0faf8 --- /dev/null +++ b/changelog.d/2824.changed.md @@ -0,0 +1 @@ +Improve error checking for InvalidCertificate errors in mirrord-cli. \ No newline at end of file