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

@@ -0,0 +1 @@
+mirrord commands now provide a nicer error message when the operator required but not installed.

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");

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<OperatorApiError> for CliError {

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?,
     };
 

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);

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}"));

mirrord/operator/src/client.rs

@@ -259,51 +259,11 @@ where
 }
 
 impl OperatorApi<NoClientCert> {
-    /// 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<R>(config: &LayerConfig, reporter: &mut R) -> OperatorApiResult<Self>
-    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<R>(
         config: &LayerConfig,
@@ -312,10 +272,6 @@ impl OperatorApi<NoClientCert> {
     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<NoClientCert> {
                 }));
             }
 
-            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);