[sled-agent] remove NexusClientWithResolver (#6283)

Previously, we would carry around a DNS resolver -- however, the resolver has been unused for a while. And in fact there's a warning about being careful around using it. Because the type was public, the rustc dead code detector didn't figure that out until my change to switch to an API trait in #6159 exposed this. Remove `NexusClientWithResolver`, adding a couple of free functions to make a `NexusClient` instead.
oxidecomputer · Aug 12, 2024 · afc564e · afc564e
1 parent 1d79114
commit afc564e
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 89 deletions.
diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs
@@ -12,7 +12,7 @@ use crate::instance_manager::{
     Error as ManagerError, InstanceManagerServices, InstanceTicket,
 };
 use crate::metrics::MetricsRequestQueue;
-use crate::nexus::NexusClientWithResolver;
+use crate::nexus::NexusClient;
 use crate::params::ZoneBundleMetadata;
 use crate::params::{InstanceExternalIpBody, ZoneBundleCause};
 use crate::params::{
@@ -349,7 +349,7 @@ struct InstanceRunner {
     running_state: Option<RunningState>,
 
     // Connection to Nexus
-    nexus_client: NexusClientWithResolver,
+    nexus_client: NexusClient,
 
     // Storage resources
     storage: StorageHandle,
@@ -528,7 +528,6 @@ impl InstanceRunner {
                 );
 
                 self.nexus_client
-                    .client()
                     .cpapi_instances_put(
                         &self.id().into_untyped_uuid(),
                         &state.into(),
@@ -1568,6 +1567,7 @@ mod tests {
     use super::*;
     use crate::fakes::nexus::{FakeNexusServer, ServerContext};
     use crate::metrics;
+    use crate::nexus::make_nexus_client_with_port;
     use crate::vmm_reservoir::VmmReservoirManagerHandle;
     use crate::zone_bundle::CleanupContext;
     use camino_tempfile::Utf8TempDir;
@@ -1634,7 +1634,7 @@ mod tests {
     }
 
     struct FakeNexusParts {
-        nexus_client: NexusClientWithResolver,
+        nexus_client: NexusClient,
         _nexus_server: HttpServer<ServerContext>,
         state_rx: Receiver<ReceivedInstanceState>,
         _dns_server: TransientServer,
@@ -1662,12 +1662,11 @@ mod tests {
                 .unwrap(),
             );
 
-            let nexus_client =
-                NexusClientWithResolver::new_from_resolver_with_port(
-                    &log,
-                    resolver,
-                    _nexus_server.local_addr().port(),
-                );
+            let nexus_client = make_nexus_client_with_port(
+                &log,
+                resolver,
+                _nexus_server.local_addr().port(),
+            );
 
             Self { nexus_client, _nexus_server, state_rx, _dns_server }
         }
@@ -1760,7 +1759,7 @@ mod tests {
     async fn instance_struct(
         log: &Logger,
         propolis_addr: SocketAddr,
-        nexus_client_with_resolver: NexusClientWithResolver,
+        nexus_client: NexusClient,
         storage_handle: StorageHandle,
         temp_dir: &String,
     ) -> (Instance, MetricsRx) {
@@ -1774,7 +1773,7 @@ mod tests {
         let (services, rx) = fake_instance_manager_services(
             log,
             storage_handle,
-            nexus_client_with_resolver,
+            nexus_client,
             temp_dir,
         );
 
@@ -1850,7 +1849,7 @@ mod tests {
     fn fake_instance_manager_services(
         log: &Logger,
         storage_handle: StorageHandle,
-        nexus_client_with_resolver: NexusClientWithResolver,
+        nexus_client: NexusClient,
         temp_dir: &String,
     ) -> (InstanceManagerServices, MetricsRx) {
         let vnic_allocator =
@@ -1869,7 +1868,7 @@ mod tests {
 
         let (metrics_queue, rx) = MetricsRequestQueue::for_test();
         let services = InstanceManagerServices {
-            nexus_client: nexus_client_with_resolver,
+            nexus_client,
             vnic_allocator,
             port_manager,
             storage: storage_handle,

diff --git a/sled-agent/src/instance_manager.rs b/sled-agent/src/instance_manager.rs
@@ -7,7 +7,7 @@
 use crate::instance::propolis_zone_name;
 use crate::instance::Instance;
 use crate::metrics::MetricsRequestQueue;
-use crate::nexus::NexusClientWithResolver;
+use crate::nexus::NexusClient;
 use crate::params::InstanceExternalIpBody;
 use crate::params::InstanceMetadata;
 use crate::params::ZoneBundleMetadata;
@@ -74,7 +74,7 @@ pub enum Error {
 }
 
 pub(crate) struct InstanceManagerServices {
-    pub nexus_client: NexusClientWithResolver,
+    pub nexus_client: NexusClient,
     pub vnic_allocator: VnicAllocator<Etherstub>,
     pub port_manager: PortManager,
     pub storage: StorageHandle,
@@ -103,7 +103,7 @@ impl InstanceManager {
     #[allow(clippy::too_many_arguments)]
     pub fn new(
         log: Logger,
-        nexus_client: NexusClientWithResolver,
+        nexus_client: NexusClient,
         etherstub: Etherstub,
         port_manager: PortManager,
         storage: StorageHandle,
@@ -422,7 +422,7 @@ struct InstanceManagerRunner {
     terminate_tx: mpsc::UnboundedSender<InstanceDeregisterRequest>,
     terminate_rx: mpsc::UnboundedReceiver<InstanceDeregisterRequest>,
 
-    nexus_client: NexusClientWithResolver,
+    nexus_client: NexusClient,
 
     // TODO: If we held an object representing an enum of "Created OR Running"
     // instance, we could avoid the methods within "instance.rs" that panic

diff --git a/sled-agent/src/nexus.rs b/sled-agent/src/nexus.rs
@@ -2,12 +2,11 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-pub use nexus_client::Client as NexusClient;
 use omicron_common::api::external::Generation;
 use omicron_common::disk::DiskVariant;
 
 use crate::vmm_reservoir::VmmReservoirManagerHandle;
-use internal_dns::resolver::{ResolveError, Resolver};
+use internal_dns::resolver::Resolver;
 use internal_dns::ServiceName;
 use nexus_client::types::SledAgentInfo;
 use omicron_common::address::NEXUS_INTERNAL_PORT;
@@ -19,62 +18,33 @@ use tokio::sync::{broadcast, mpsc, oneshot, Notify};
 use tokio::time::{interval, Duration, MissedTickBehavior};
 use uuid::Uuid;
 
-/// A thin wrapper over a progenitor-generated NexusClient.
-///
-/// Also attaches the "DNS resolver" for historical reasons.
-#[derive(Clone)]
-pub struct NexusClientWithResolver {
-    client: NexusClient,
+// Re-export the nexus_client::Client crate. (Use a type alias to be more
+// rust-analyzer friendly.)
+pub(crate) type NexusClient = nexus_client::Client;
+
+pub(crate) fn make_nexus_client(
+    log: &Logger,
     resolver: Arc<Resolver>,
+) -> NexusClient {
+    make_nexus_client_with_port(log, resolver, NEXUS_INTERNAL_PORT)
 }
 
-impl NexusClientWithResolver {
-    pub fn new(
-        log: &Logger,
-        resolver: Arc<Resolver>,
-    ) -> Result<Self, ResolveError> {
-        Ok(Self::new_from_resolver_with_port(
-            log,
-            resolver,
-            NEXUS_INTERNAL_PORT,
-        ))
-    }
-
-    pub fn new_from_resolver_with_port(
-        log: &Logger,
-        resolver: Arc<Resolver>,
-        port: u16,
-    ) -> Self {
-        let client = reqwest::ClientBuilder::new()
-            .dns_resolver(resolver.clone())
-            .build()
-            .expect("Failed to build client");
-
-        let dns_name = ServiceName::Nexus.srv_name();
-        Self {
-            client: NexusClient::new_with_client(
-                &format!("http://{dns_name}:{port}"),
-                client,
-                log.new(o!("component" => "NexusClient")),
-            ),
-            resolver,
-        }
-    }
-
-    /// Access the progenitor-based Nexus Client.
-    pub fn client(&self) -> &NexusClient {
-        &self.client
-    }
-
-    /// Access the DNS resolver used by the Nexus Client.
-    ///
-    /// WARNING: If you're using this resolver to access an IP address of
-    /// another service, be aware that it might change if that service moves
-    /// around! Be cautious when accessing and persisting IP addresses of other
-    /// services.
-    pub fn resolver(&self) -> &Arc<Resolver> {
-        &self.resolver
-    }
+pub(crate) fn make_nexus_client_with_port(
+    log: &Logger,
+    resolver: Arc<Resolver>,
+    port: u16,
+) -> NexusClient {
+    let client = reqwest::ClientBuilder::new()
+        .dns_resolver(resolver)
+        .build()
+        .expect("Failed to build client");
+
+    let dns_name = ServiceName::Nexus.srv_name();
+    NexusClient::new_with_client(
+        &format!("http://{dns_name}:{port}"),
+        client,
+        log.new(o!("component" => "NexusClient")),
+    )
 }
 
 pub fn d2n_params(

diff --git a/sled-agent/src/probe_manager.rs b/sled-agent/src/probe_manager.rs
@@ -1,5 +1,5 @@
 use crate::metrics::MetricsRequestQueue;
-use crate::nexus::NexusClientWithResolver;
+use crate::nexus::NexusClient;
 use anyhow::{anyhow, Result};
 use illumos_utils::dladm::Etherstub;
 use illumos_utils::link::VnicAllocator;
@@ -54,7 +54,7 @@ struct RunningProbes {
 
 pub(crate) struct ProbeManagerInner {
     join_handle: Mutex<Option<JoinHandle<()>>>,
-    nexus_client: NexusClientWithResolver,
+    nexus_client: NexusClient,
     log: Logger,
     sled_id: Uuid,
     vnic_allocator: VnicAllocator<Etherstub>,
@@ -67,7 +67,7 @@ pub(crate) struct ProbeManagerInner {
 impl ProbeManager {
     pub(crate) fn new(
         sled_id: Uuid,
-        nexus_client: NexusClientWithResolver,
+        nexus_client: NexusClient,
         etherstub: Etherstub,
         storage: StorageHandle,
         port_manager: PortManager,
@@ -248,7 +248,6 @@ impl ProbeManagerInner {
                 if n_added > 0 {
                     if let Err(e) = self
                         .nexus_client
-                        .client()
                         .bgtask_activate(&BackgroundTasksActivateRequest {
                             bgtask_names: vec!["vpc_route_manager".into()],
                         })
@@ -439,7 +438,6 @@ impl ProbeManagerInner {
     async fn target_state(self: &Arc<Self>) -> Result<HashSet<ProbeState>> {
         Ok(self
             .nexus_client
-            .client()
             .probes_get(
                 &self.sled_id,
                 None, //limit

diff --git a/sled-agent/src/server.rs b/sled-agent/src/server.rs
@@ -9,7 +9,7 @@ use super::http_entrypoints::api as http_api;
 use super::sled_agent::SledAgent;
 use crate::bootstrap::params::StartSledAgentRequest;
 use crate::long_running_tasks::LongRunningTaskHandles;
-use crate::nexus::NexusClientWithResolver;
+use crate::nexus::make_nexus_client;
 use crate::services::ServiceManager;
 use internal_dns::resolver::Resolver;
 use slog::Logger;
@@ -52,8 +52,7 @@ impl Server {
             .map_err(|e| e.to_string())?,
         );
 
-        let nexus_client = NexusClientWithResolver::new(&log, resolver)
-            .map_err(|e| e.to_string())?;
+        let nexus_client = make_nexus_client(&log, resolver);
 
         let sled_agent = SledAgent::new(
             &config,

diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs
@@ -13,8 +13,7 @@ use crate::instance_manager::InstanceManager;
 use crate::long_running_tasks::LongRunningTaskHandles;
 use crate::metrics::MetricsManager;
 use crate::nexus::{
-    NexusClientWithResolver, NexusNotifierHandle, NexusNotifierInput,
-    NexusNotifierTask,
+    NexusClient, NexusNotifierHandle, NexusNotifierInput, NexusNotifierTask,
 };
 use crate::params::{
     DiskStateRequested, InstanceExternalIpBody, InstanceHardware,
@@ -320,7 +319,7 @@ struct SledAgentInner {
     services: ServiceManager,
 
     // Connection to Nexus.
-    nexus_client: NexusClientWithResolver,
+    nexus_client: NexusClient,
 
     // A mechanism for notifiying nexus about sled-agent updates
     nexus_notifier: NexusNotifierHandle,
@@ -365,7 +364,7 @@ impl SledAgent {
     pub async fn new(
         config: &Config,
         log: Logger,
-        nexus_client: NexusClientWithResolver,
+        nexus_client: NexusClient,
         request: StartSledAgentRequest,
         services: ServiceManager,
         long_running_task_handles: LongRunningTaskHandles,
@@ -552,7 +551,7 @@ impl SledAgent {
         let nexus_notifier_input = NexusNotifierInput {
             sled_id: request.body.id,
             sled_address: get_sled_address(request.body.subnet),
-            nexus_client: nexus_client.client().clone(),
+            nexus_client: nexus_client.clone(),
             hardware: long_running_task_handles.hardware_manager.clone(),
             vmm_reservoir_manager: vmm_reservoir_manager.clone(),
         };
@@ -688,7 +687,6 @@ impl SledAgent {
 
         self.inner
             .nexus_client
-            .client()
             .sled_firewall_rules_request(&sled_id)
             .await
             .map_err(|err| Error::FirewallRequest(err))?;
@@ -1074,7 +1072,7 @@ impl SledAgent {
     ) -> Result<(), Error> {
         self.inner
             .updates
-            .download_artifact(artifact, &self.inner.nexus_client.client())
+            .download_artifact(artifact, &self.inner.nexus_client)
             .await?;
         Ok(())
     }

diff --git a/sled-agent/src/updates.rs b/sled-agent/src/updates.rs
@@ -252,8 +252,8 @@ impl UpdateManager {
 mod test {
     use super::*;
     use crate::fakes::nexus::FakeNexusServer;
+    use crate::nexus::NexusClient;
     use flate2::write::GzEncoder;
-    use nexus_client::Client as NexusClient;
     use omicron_common::api::external::{Error, SemverVersion};
     use omicron_common::api::internal::nexus::UpdateArtifactId;
     use omicron_test_utils::dev::test_setup_log;