auditing clone usage (istio#418)

* auditing clone usage:
- some clones removed
- some clones were changing borrowed to owned, replaced with .to_owned() to make this clear

* addressing review feedback & clippy feedback

* addressing check-clean-repo error

src/config.rs

@@ -267,9 +267,7 @@ pub struct ProxyConfig {
 
 impl ProxyConfig {
     fn merge(mut self, other: Self) -> Self {
-        self.discovery_address = other
-            .discovery_address
-            .or_else(|| self.discovery_address.clone());
+        self.discovery_address = other.discovery_address.or(self.discovery_address); // clone not required; self is moved and discovery_address is an owned type
         self.proxy_admin_port = other.proxy_admin_port.or(self.proxy_admin_port);
         self.status_port = other.status_port.or(self.status_port);
         self.concurrency = other.concurrency.or(self.concurrency);

src/identity/caclient.rs

@@ -82,7 +82,7 @@ impl CertificateProvider for CaClient {
         let leaf = resp
             .cert_chain
             .first()
-            .ok_or_else(|| Error::EmptyResponse(id.clone()))?
+            .ok_or_else(|| Error::EmptyResponse(id.to_owned()))?
             .as_bytes();
         let chain = if resp.cert_chain.len() > 1 {
             resp.cert_chain[1..].iter().map(|s| s.as_bytes()).collect()
@@ -93,7 +93,7 @@ impl CertificateProvider for CaClient {
         let certs = tls::cert_from(&pkey, leaf, chain);
         certs
             .verify_san(id)
-            .map_err(|_| Error::SanError(id.clone()))?;
+            .map_err(|_| Error::SanError(id.to_owned()))?;
         Ok(certs)
     }
 }
@@ -181,9 +181,9 @@ pub mod mock {
                 .expect("SystemTime cannot represent current time. Was the process started in extreme future?");
             let not_after = not_before + self.cfg.cert_lifetime;
 
-            let certs = generate_test_certs_at(&id.clone().into(), not_before, not_after);
+            let certs = generate_test_certs_at(&id.to_owned().into(), not_before, not_after);
 
-            self.state.write().await.fetches.push(id.clone());
+            self.state.write().await.fetches.push(id.to_owned());
             return Ok(certs);
         }
     }

src/identity/manager.rs

@@ -251,8 +251,8 @@ impl<C: CertificateProvider> Worker<C> {
                 },
                 true = maybe_sleep_until(next), if workers.len() < self.concurrency as usize => {
                     let (id, _) = pending.pop().unwrap();
-                    processing.insert(id.clone());
-                    workers.push(self.refresh(id.clone()));
+                    processing.insert(id.to_owned());
+                    workers.push(self.refresh(id.to_owned()));
                 },
             };
         }
@@ -265,7 +265,7 @@ impl<C: CertificateProvider> Worker<C> {
         id: Identity,
     ) -> Result<(Identity, Option<tls::Certs>), watch::error::SendError<CertState>> {
         let (state, res) = match self.client.fetch_certificate(&id).await {
-            Ok(certs) => (CertState::Available(certs.clone()), Some(certs)),
+            Ok(certs) => (CertState::Available(certs.clone()), Some(certs)), // TODO: must we return 2 copies of certs as different types?
             Err(err) => (CertState::Unavailable(err), None),
         };
         match self.update_certs(&id, state).await {
@@ -351,10 +351,10 @@ impl<T: CertificateProvider> SecretManager<T> {
             // New identity, start managing it and return the newly created channel.
             None => {
                 let (tx, rx) = watch::channel(CertState::Initializing);
-                certs.insert(id.clone(), CertChannel { rx: rx.clone(), tx });
+                certs.insert(id.to_owned(), CertChannel { rx: rx.clone(), tx });
                 drop(certs);
                 // Notify the background worker to start refreshing the certificate.
-                match self.requests.send((id.clone(), pri)).await {
+                match self.requests.send((id.to_owned(), pri)).await {
                     Err(e) => unreachable!("SecretManager worker died: {e}"),
                     Ok(()) => Ok(rx),
                 }
@@ -368,8 +368,8 @@ impl<T: CertificateProvider> SecretManager<T> {
             res = rx.changed() => match res {
                 Err(e) => unreachable!("the send end of the channel should still be reachable via self: {e}"),
                 Ok(()) => match *rx.borrow() {
-                    CertState::Unavailable(ref err) => Err(err.clone()),
-                    CertState::Available(ref certs) => Ok(certs.clone()),
+                    CertState::Unavailable(ref err) => Err(err.to_owned()),
+                    CertState::Available(ref certs) => Ok(certs.to_owned()),
                     CertState::Initializing => unreachable!("Only the initial state can be Initializing, but the state has changed"),
                 },
             },
@@ -401,7 +401,7 @@ impl<T: CertificateProvider + Clone> CertificateProvider for SecretManager<T> {
 
 impl SecretManager<CaClient> {
     pub fn new(cfg: crate::config::Config) -> Result<Self, Error> {
-        let caclient = CaClient::new(cfg.ca_address.unwrap(), cfg.ca_root_cert.clone(), cfg.auth)?;
+        let caclient = CaClient::new(cfg.ca_address.unwrap(), cfg.ca_root_cert, cfg.auth)?;
         Ok(Self::new_with_client(caclient))
     }
 }

src/proxy/inbound.rs

@@ -315,7 +315,7 @@ impl Inbound {
                     reporter: Reporter::destination,
                     source,
                     derived_source: Some(derived_source),
-                    destination: Some(upstream.clone()),
+                    destination: Some(upstream),
                     connection_security_policy: traffic::SecurityPolicy::mutual_tls,
                     destination_service: None,
                     destination_service_namespace: None,

src/proxy/inbound_passthrough.rs

@@ -57,7 +57,7 @@ impl InboundPassthrough {
                 Ok((stream, remote)) => {
                     tokio::spawn(async move {
                         if let Err(e) = Self::proxy_inbound_plaintext(
-                            pi.clone(),
+                            pi, // pi cloned above; OK to move
                             socket::to_canonical(remote),
                             stream,
                         )
@@ -145,7 +145,7 @@ impl InboundPassthrough {
             reporter: Reporter::destination,
             source: source_workload,
             derived_source: Some(derived_source),
-            destination: Some(upstream.clone()),
+            destination: Some(upstream),
             connection_security_policy: traffic::SecurityPolicy::unknown,
             destination_service: None,
             destination_service_namespace: None,

src/proxy/outbound.rs

@@ -200,7 +200,7 @@ impl OutboundConnection {
                 InboundConnect::DirectPath(stream),
                 origin_src,
                 req.destination,
-                self.pi.metrics.clone(),
+                self.pi.metrics.to_owned(), // self is a borrow so this clone is to return an owned
                 connection_metrics,
                 Some(inbound_connection_metrics),
             )
@@ -372,7 +372,7 @@ impl OutboundConnection {
         }
         // For case source client and upstream server are on the same node
         if !us.workload.node.is_empty()
-            && self.pi.cfg.local_node == Some(us.workload.node.clone())
+            && self.pi.cfg.local_node.as_ref() == Some(&us.workload.node) // looks weird but in Rust borrows can be compared and will behave the same as owned (https://doc.rust-lang.org/std/primitive.reference.html)
             && us.workload.protocol == Protocol::HBONE
         {
             trace!(

src/rbac.rs

@@ -87,7 +87,7 @@ impl Authorization {
             .src_identity
             .as_ref()
             .map(|i| match i {
-                Identity::Spiffe { namespace, .. } => namespace.clone(),
+                Identity::Spiffe { namespace, .. } => namespace.to_owned(), // may be more clear if we use to_owned() to denote change from borrowed to owned
             })
             .unwrap_or_default();
         if self.groups.is_empty() {
@@ -383,9 +383,9 @@ impl TryFrom<&XdsAddress> for IpNet {
 impl From<&XdsStringMatch> for Option<StringMatch> {
     fn from(resource: &XdsStringMatch) -> Self {
         resource.match_type.as_ref().map(|m| match m {
-            MatchType::Exact(s) => StringMatch::Exact(s.clone()),
-            MatchType::Prefix(s) => StringMatch::Prefix(s.clone()),
-            MatchType::Suffix(s) => StringMatch::Suffix(s.clone()),
+            MatchType::Exact(s) => StringMatch::Exact(s.to_owned()),
+            MatchType::Prefix(s) => StringMatch::Prefix(s.to_owned()),
+            MatchType::Suffix(s) => StringMatch::Suffix(s.to_owned()),
             MatchType::Presence(_) => StringMatch::Presence(),
         })
     }

src/readiness.rs

@@ -32,7 +32,7 @@ impl Ready {
     pub fn register_task(&self, name: &str) -> BlockReady {
         self.0.lock().unwrap().insert(name.to_string());
         BlockReady {
-            parent: self.clone(),
+            parent: self.to_owned(),
             name: name.to_string(),
         }
     }

src/tls/boring.rs

@@ -121,9 +121,9 @@ pub struct ZtunnelCert {
 impl ZtunnelCert {
     pub fn new(cert: x509::X509) -> ZtunnelCert {
         ZtunnelCert {
-            x509: cert.clone(),
             not_before: asn1_time_to_system_time(cert.not_before()),
             not_after: asn1_time_to_system_time(cert.not_after()),
+            x509: cert, // cert is already owned, the asn1_ functions borrow cert so as long as we move cert to ZtunnelCert after the borrows this doesn't need cloning
         }
     }
 }
@@ -385,7 +385,7 @@ impl SanChecker for x509::X509 {
         let sans = extract_sans(self);
         sans.iter()
             .find(|id| id == &identity)
-            .ok_or_else(|| TlsError::SanError(identity.clone(), sans.clone()))
+            .ok_or_else(|| TlsError::SanError(identity.to_owned(), sans.clone()))
             .map(|_| ())
     }
 }
@@ -401,9 +401,9 @@ impl Service<Request<BoxBody>> for TlsGrpcChannel {
 
     fn call(&mut self, mut req: Request<BoxBody>) -> Self::Future {
         let uri = Uri::builder()
-            .scheme(self.uri.scheme().unwrap().clone())
-            .authority(self.uri.authority().unwrap().clone())
-            .path_and_query(req.uri().path_and_query().unwrap().clone())
+            .scheme(self.uri.scheme().unwrap().to_owned())
+            .authority(self.uri.authority().unwrap().to_owned())
+            .path_and_query(req.uri().path_and_query().unwrap().to_owned())
             .build()
             .unwrap();
         *req.uri_mut() = uri;

src/workload.rs

@@ -689,7 +689,7 @@ impl WorkloadStore {
                 wl_vips.iter().choose(&mut rand::thread_rng()).unwrap();
             if let Some(wl) = self.workloads.get(workload_ip) {
                 let mut us = Upstream {
-                    workload: wl.clone(),
+                    workload: wl.to_owned(),
                     port: *target_port,
                 };
                 Self::set_gateway_address(&mut us, hbone_port);
@@ -699,7 +699,7 @@ impl WorkloadStore {
         }
         if let Some(wl) = self.workloads.get(&addr.ip()) {
             let mut us = Upstream {
-                workload: wl.clone(),
+                workload: wl.to_owned(),
                 port: addr.port(),
             };
             Self::set_gateway_address(&mut us, hbone_port);
@@ -961,9 +961,9 @@ mod tests {
         // at least once, and no unexpected results
         for _ in 0..1000 {
             if let Some(us) = wi.find_upstream("127.0.1.1:80".parse().unwrap(), 15008) {
-                let n = us.workload.name.clone();
-                found.insert(n.clone());
-                wants.remove(&n);
+                let n = &us.workload.name; // borrow name instead of cloning
+                found.insert(n.to_owned()); // insert an owned copy of the borrowed n
+                wants.remove(n); // remove using the borrow
             }
         }
         if !wants.is_empty() {

src/xds/client.rs

@@ -427,7 +427,7 @@ impl AdsClient {
                     .get(request_type)
                     .map(|hs| {
                         hs.iter()
-                            .map(|n| (n.clone(), "".to_string())) // Proto expects Name -> Version. We don't care about version
+                            .map(|n| (n.to_owned(), "".to_string())) // Proto expects Name -> Version. We don't care about version
                             .collect()
                     })
                     .unwrap_or_default();
@@ -439,7 +439,7 @@ impl AdsClient {
                     (vec![], vec![])
                 };
                 DeltaDiscoveryRequest {
-                    type_url: request_type.clone(),
+                    type_url: request_type.to_owned(),
                     node: Some(node.clone()),
                     initial_resource_versions: irv,
                     resource_names_subscribe: sub,
@@ -462,7 +462,7 @@ impl AdsClient {
         let type_url = response.type_url.clone();
         let nonce = response.nonce.clone();
         info!(
-            type_url = type_url.clone(),
+            type_url = type_url, // this is a borrow, it's OK
             size = response.resources.len(),
             "received response"
         );
@@ -495,14 +495,14 @@ impl AdsClient {
         };
 
         debug!(
-            type_url=type_url.clone(),
+            type_url=type_url,
             nonce,
             "type"=?response_type,
             "sending response",
         );
         send.send(DeltaDiscoveryRequest {
-            type_url: type_url.clone(),
-            response_nonce: nonce.clone(),
+            type_url,              // this is owned, OK to move
+            response_nonce: nonce, // this is owned, OK to move
             error_detail: error.map(|msg| Status {
                 message: msg,
                 ..Default::default()
@@ -551,7 +551,7 @@ impl AdsClient {
             .iter()
             .map(|res| {
                 let k = ResourceKey {
-                    name: res.clone(),
+                    name: res.to_owned(),
                     type_url: resp.type_url.clone(),
                 };
                 debug!("received delete resource {k}");
@@ -674,15 +674,12 @@ mod tests {
         let start_time = SystemTime::now();
         let converted: Option<Workload> = expected_workload
             .as_ref()
-            .map(|expected_workload| Workload::try_from(&expected_workload.clone()).unwrap());
+            .map(|expected_workload| Workload::try_from(expected_workload).unwrap()); // this is a borrow, Ok not to clone
         let mut matched = false;
         while start_time.elapsed().unwrap() < TEST_TIMEOUT && !matched {
             sleep(POLL_RATE).await;
             let wl = source.fetch_workload(&ip).await;
-            matched = converted.is_none() && wl.is_none()
-                || (wl.is_some()
-                    && converted.is_some()
-                    && wl.unwrap() == converted.clone().unwrap());
+            matched = wl == converted; // Option<Workload> is Ok to compare without needing to unwrap
         }
     }