From 50156f8df5b1ec7fff98a25085a7e126ed7c6443 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Smolarek?= <34063647+Razz4780@users.noreply.github.com> Date: Thu, 4 Jul 2024 12:52:23 +0200 Subject: [PATCH] Always run discovery when fetching operator fails (#2570) * Always run discovery * Update mirrord/cli/src/error.rs Co-authored-by: meowjesty <43983236+meowjesty@users.noreply.github.com> * Improved error message --------- Co-authored-by: meowjesty <43983236+meowjesty@users.noreply.github.com> --- changelog.d/1730.changed.md | 1 + mirrord/cli/src/connection.rs | 10 ++++-- mirrord/cli/src/error.rs | 7 ++++ mirrord/cli/src/main.rs | 8 ++++- mirrord/cli/src/operator.rs | 8 +++-- mirrord/cli/src/operator/session.rs | 18 +++++----- mirrord/operator/src/client.rs | 54 +++-------------------------- 7 files changed, 43 insertions(+), 63 deletions(-) create mode 100644 changelog.d/1730.changed.md diff --git a/changelog.d/1730.changed.md b/changelog.d/1730.changed.md new file mode 100644 index 00000000000..bbe7de5a051 --- /dev/null +++ b/changelog.d/1730.changed.md @@ -0,0 +1 @@ +mirrord commands now provide a nicer error message when the operator required but not installed. \ No newline at end of file diff --git a/mirrord/cli/src/connection.rs b/mirrord/cli/src/connection.rs index 0fd95ce1360..587f1554fda 100644 --- a/mirrord/cli/src/connection.rs +++ b/mirrord/cli/src/connection.rs @@ -41,9 +41,13 @@ where return Ok(None); } - let Some(api) = OperatorApi::try_new(config, analytics).await? else { - operator_subtask.success(Some("operator not found")); - return Ok(None); + let api = match OperatorApi::try_new(config, analytics).await? { + Some(api) => api, + None if config.operator == Some(true) => return Err(CliError::OperatorNotInstalled), + None => { + operator_subtask.success(Some("operator not found")); + return Ok(None); + } }; let mut version_cmp_subtask = operator_subtask.subtask("checking version compatibility"); diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 509d134b5c6..eeaf4625d20 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -236,6 +236,13 @@ pub(crate) enum CliError { #[error("Failed to prepare mirrord operator client certificate: {0}")] #[diagnostic(help("{GENERAL_BUG}"))] OperatorClientCertError(String), + + #[error("mirrord operator was not found in the cluster.")] + #[diagnostic(help( + "Command requires the mirrord operator or operator usage was explicitly enabled in the configuration file. + Read more here: https://mirrord.dev/docs/overview/quick-start/#operator.{GENERAL_HELP}" + ))] + OperatorNotInstalled, } impl From for CliError { diff --git a/mirrord/cli/src/main.rs b/mirrord/cli/src/main.rs index 3828ca44dc9..48793a24cd1 100644 --- a/mirrord/cli/src/main.rs +++ b/mirrord/cli/src/main.rs @@ -566,7 +566,11 @@ async fn print_targets(args: &ListTargetArgs) -> Result<()> { } // Try operator first if relevant - let operator_api = OperatorApi::try_new(&layer_config, &mut NullReporter::default()).await?; + let operator_api = if layer_config.operator == Some(false) { + None + } else { + OperatorApi::try_new(&layer_config, &mut NullReporter::default()).await? + }; let mut targets = match operator_api { Some(api) => { let api = api.prepare_client_cert(&mut NullReporter::default()).await; @@ -588,6 +592,8 @@ async fn print_targets(args: &ListTargetArgs) -> Result<()> { .collect() } + None if layer_config.operator == Some(true) => return Err(CliError::OperatorNotInstalled), + None => list_pods(&layer_config, args).await?, }; diff --git a/mirrord/cli/src/operator.rs b/mirrord/cli/src/operator.rs index 0fe4a6ef471..56f250c46ab 100644 --- a/mirrord/cli/src/operator.rs +++ b/mirrord/cli/src/operator.rs @@ -159,11 +159,15 @@ async fn operator_status(config: Option<&Path>) -> Result<()> { } let mut status_progress = progress.subtask("fetching status"); - let api = OperatorApi::new(&layer_config, &mut NullReporter::default()) + let api = OperatorApi::try_new(&layer_config, &mut NullReporter::default()) .await .inspect_err(|_| { - status_progress.failure(Some("unable to get status")); + status_progress.failure(Some("failed to get status")); })?; + let Some(api) = api else { + status_progress.failure(Some("operator not found")); + return Err(CliError::OperatorNotInstalled); + }; status_progress.success(Some("fetched status")); progress.success(None); diff --git a/mirrord/cli/src/operator/session.rs b/mirrord/cli/src/operator/session.rs index 29d51d07133..2c090adf622 100644 --- a/mirrord/cli/src/operator/session.rs +++ b/mirrord/cli/src/operator/session.rs @@ -10,7 +10,7 @@ use mirrord_operator::{ }; use mirrord_progress::{Progress, ProgressTracker}; -use crate::{Result, SessionCommand}; +use crate::{CliError, Result, SessionCommand}; /// Handles the [`SessionCommand`]s that deal with session management in the operator. pub(super) struct SessionCommandHandler { @@ -38,13 +38,15 @@ impl SessionCommandHandler { progress.failure(Some(&format!("failed to read config from env: {error}"))); })?; - let operator_api = OperatorApi::new(&config, &mut NullReporter::default()) - .await - .inspect_err(|_| { - progress.failure(Some("failed to create operator API")); - })? - .prepare_client_cert(&mut NullReporter::default()) - .await; + let mut subtask = progress.subtask("checking operator"); + let operator_api = match OperatorApi::try_new(&config, &mut NullReporter::default()).await? + { + Some(api) => api.prepare_client_cert(&mut NullReporter::default()).await, + None => { + subtask.failure(Some("operator not found")); + return Err(CliError::OperatorNotInstalled); + } + }; operator_api.inspect_cert_error(|error| { progress.warning(&format!("Failed to prepare user certificate: {error}")); diff --git a/mirrord/operator/src/client.rs b/mirrord/operator/src/client.rs index b6da7c9b7e6..85fe58c34d1 100644 --- a/mirrord/operator/src/client.rs +++ b/mirrord/operator/src/client.rs @@ -259,51 +259,11 @@ where } impl OperatorApi { - /// Fetches [`MirrordOperatorCrd`] from the cluster and creates a new instance of this API. - #[tracing::instrument(level = Level::TRACE, skip_all, err)] - pub async fn new(config: &LayerConfig, reporter: &mut R) -> OperatorApiResult - where - R: Reporter, - { - let base_config = Self::base_client_config(config).await?; - let client = Client::try_from(base_config.clone()) - .map_err(KubeApiError::from) - .map_err(OperatorApiError::CreateKubeClient)?; - let operator: MirrordOperatorCrd = Api::all(client.clone()) - .get(OPERATOR_STATUS_NAME) - .await - .map_err(|error| OperatorApiError::KubeError { - error, - operation: OperatorOperation::FindingOperator, - })?; - - reporter.set_operator_properties(AnalyticsOperatorProperties { - client_hash: None, - license_hash: operator - .spec - .license - .fingerprint - .as_deref() - .map(AnalyticsHash::from_base64), - }); - - Ok(Self { - client, - client_cert: NoClientCert { base_config }, - operator, - }) - } - - /// If [`LayerConfig::operator`] is explicitly disabled, returns early with [`None`]. - /// - /// If [`LayerConfig::operator`] is explicitly enabled, this functions is equivalent to - /// [`OperatorApi::new`] and never returns [`None`]. + /// Attempts to fetch the [`MirrordOperatorCrd`] resource and create an instance of this API. + /// In case of error response from the Kubernetes API server, executes an extra API discovery + /// step to confirm that the operator is not installed. /// - /// If [`LayerConfig::operator`] is missing, tries to fetch [`MirrordOperatorCrd`] from the - /// cluster and create a new instance of this API. If fetching the resource fails, an extra - /// discovery step is made to determine whether the operator is installed. If this extra - /// step proves that the operator is installed, an error is returned. Otherwise, [`None`] is - /// returned. + /// If certain that the operator is not installed, returns [`None`]. #[tracing::instrument(level = Level::TRACE, skip_all, err)] pub async fn try_new( config: &LayerConfig, @@ -312,10 +272,6 @@ impl OperatorApi { where R: Reporter, { - if config.operator == Some(false) { - return Ok(None); - } - let base_config = Self::base_client_config(config).await?; let client = Client::try_from(base_config.clone()) .map_err(KubeApiError::from) @@ -343,7 +299,7 @@ impl OperatorApi { })); } - Err(error @ kube::Error::Api(..)) if config.operator.is_none() => { + Err(error @ kube::Error::Api(..)) => { match discovery::operator_installed(&client).await { Ok(false) | Err(..) => { return Ok(None);