Merge pull request #128 from omertuc/unwrap

Don't use `unwrap`

clippy.toml

@@ -0,0 +1 @@
+allow-unwrap-in-tests = true

src/cluster_crypto/cert_key_pair.rs

@@ -422,7 +422,7 @@ fn fix_akid(
     akid: &mut x509_cert::ext::pkix::AuthorityKeyIdentifier,
     skids: &mut SkidEdits,
     serial_numbers: &mut SerialNumberEdits,
-) -> Result<(), anyhow::Error> {
+) -> Result<()> {
     if let Some(key_identifier) = &akid.key_identifier {
         let matching_skid = skids
             .get(&HashableKeyID(key_identifier.clone()))

src/cluster_crypto/cert_key_pair/cert_mutations.rs

@@ -82,10 +82,7 @@ fn get_certificate_expiration(tbs_certificate: &mut TbsCertificate) -> Result<(D
     Ok((current_not_before, current_not_after))
 }
 
-pub(crate) fn mutate_cert_cn_san(
-    tbs_certificate: &mut TbsCertificate,
-    cn_san_replace_rules: &CnSanReplaceRules,
-) -> Result<(), anyhow::Error> {
+pub(crate) fn mutate_cert_cn_san(tbs_certificate: &mut TbsCertificate, cn_san_replace_rules: &CnSanReplaceRules) -> Result<()> {
     mutate_cert_common_name(&mut tbs_certificate.subject, cn_san_replace_rules).context("mutating subject Common Name")?;
     mutate_cert_common_name(&mut tbs_certificate.issuer, cn_san_replace_rules).context("mutating issuer Common Name")?;
     mutate_cert_subject_alternative_name(tbs_certificate, cn_san_replace_rules).context("mutating Subject Alternative Name")?;

src/cluster_crypto/crypto_objects.rs

@@ -125,12 +125,15 @@ pub(crate) fn process_jwt(value: &str, location: &Location) -> Result<Option<Dis
 pub(crate) fn process_pem_bundle(value: &str, location: &Location, external_certs: &HashSet<String>) -> Result<Vec<DiscoveredCryptoObect>> {
     let pems = pem::parse_many(value).context("parsing pem")?;
 
+    #[allow(clippy::unwrap_used)] // The filter ensures that unwrap will never panic. We can't use
+    // a filter_map because we want to maintain the index of the pem in the bundle.
     pems.iter()
         .enumerate()
         .map(|(pem_index, pem)| {
             process_single_pem(pem, external_certs).with_context(|| format!("processing pem at index {} in the bundle", pem_index))
         })
-        .collect::<Result<Vec<_>>>()?
+        .collect::<Result<Vec<_>>>()
+        .context("error processing PEM")?
         .into_iter()
         .enumerate()
         .filter(|(_, crypto_object)| crypto_object.is_some())

src/cluster_crypto/crypto_utils.rs

@@ -17,12 +17,13 @@ pub(crate) mod jwt;
 
 pub(crate) struct SigningKey {
     pub in_memory_signing_key_pair: InMemorySigningKeyPair,
-    pub pkcs8_pem: Vec<u8>,
+    pkcs8_pem: Vec<u8>,
 }
 
 impl Clone for SigningKey {
     fn clone(&self) -> Self {
         Self {
+            #[allow(clippy::unwrap_used)] // This can never panic because a SigningKey could never be created with an invalid pkcs8_pem
             in_memory_signing_key_pair: InMemorySigningKeyPair::from_pkcs8_pem(&self.pkcs8_pem).unwrap(),
             pkcs8_pem: self.pkcs8_pem.clone(),
         }
@@ -126,7 +127,7 @@ pub(crate) fn generate_ec_key(ec_curve: EcdsaCurve) -> Result<SigningKey> {
 
     let pkcs8_pem_data = StdCommand::new("openssl")
         .args(["pkcs8", "-topk8", "-nocrypt", "-inform", "DER"])
-        .stdin(gen_sec1_ec.stdout.unwrap())
+        .stdin(gen_sec1_ec.stdout.context("no stdout")?)
         .output()
         .context("openssl pkcs8")?
         .stdout;

src/cluster_crypto/distributed_jwt.rs

@@ -107,7 +107,7 @@ impl DistributedJwt {
         Ok(())
     }
 
-    async fn commit_at_location(location: &Location, jwt_regenerated: &Jwt, etcd_client: &InMemoryK8sEtcd) -> Result<(), anyhow::Error> {
+    async fn commit_at_location(location: &Location, jwt_regenerated: &Jwt, etcd_client: &InMemoryK8sEtcd) -> Result<()> {
         match location {
             Location::K8s(k8slocation) => {
                 Self::commit_to_etcd(jwt_regenerated, etcd_client, k8slocation)

src/config.rs

@@ -183,8 +183,10 @@ impl RecertConfig {
             .context("force_expire must be a boolean")?;
         let cluster_rename = match value.remove("cluster_rename") {
             Some(value) => Some(
-                ClusterNamesRename::parse(value.as_str().context("cluster_rename must be a string")?)
-                    .context(format!("cluster_rename {}", value.as_str().unwrap()))?,
+                ClusterNamesRename::parse(value.as_str().context("cluster_rename must be a string")?).context(format!(
+                    "cluster_rename {}",
+                    value.as_str().context("cluster_rename must be a string")?
+                ))?,
             ),
             None => None,
         };
@@ -202,7 +204,8 @@ impl RecertConfig {
         };
         let proxy = match value.remove("proxy") {
             Some(value) => Some(
-                Proxy::parse(value.as_str().context("proxy must be a string")?).context(format!("proxy {}", value.as_str().unwrap()))?,
+                Proxy::parse(value.as_str().context("proxy must be a string")?)
+                    .context(format!("proxy {}", value.as_str().context("proxy must be a string")?))?,
             ),
             None => None,
         };
@@ -376,29 +379,33 @@ impl RecertConfig {
     }
 }
 
-fn parse_summary_file_clean(value: Value) -> Result<Option<ConfigPath>, anyhow::Error> {
+fn parse_summary_file_clean(value: Value) -> Result<Option<ConfigPath>> {
     Ok(Some(
-        ConfigPath::new(value.as_str().context("summary_file_clean must be a string")?)
-            .context(format!("summary_file_clean {}", value.as_str().unwrap()))?,
+        ConfigPath::new(value.as_str().context("summary_file_clean must be a string")?).context(format!(
+            "summary_file_clean {}",
+            value.as_str().context("summary_file_clean must be a string")?
+        ))?,
     ))
 }
 
-fn parse_summary_file(value: Value) -> Result<Option<ConfigPath>, anyhow::Error> {
+fn parse_summary_file(value: Value) -> Result<Option<ConfigPath>> {
     Ok(Some(
         ConfigPath::new(value.as_str().context("summary_file must be a string")?)
-            .context(format!("summary_file {}", value.as_str().unwrap()))?,
+            .context(format!("summary_file {}", value.as_str().context("summary_file must be a string")?))?,
     ))
 }
 
-fn parse_server_ssh_keys(value: Value) -> Result<Option<ConfigPath>, anyhow::Error> {
-    let config_path = ConfigPath::new(value.as_str().context("regenerate_server_ssh_keys must be a string")?)
-        .context(format!("regenerate_server_ssh_keys {}", value.as_str().unwrap()))?;
+fn parse_server_ssh_keys(value: Value) -> Result<Option<ConfigPath>> {
+    let config_path = ConfigPath::new(value.as_str().context("regenerate_server_ssh_keys must be a string")?).context(format!(
+        "regenerate_server_ssh_keys {}",
+        value.as_str().context("regenerate_server_ssh_keys must be a string")?
+    ))?;
     ensure!(config_path.try_exists()?, "regenerate_server_ssh_keys must exist");
     ensure!(config_path.is_dir(), "regenerate_server_ssh_keys must be a directory");
     Ok(Some(config_path))
 }
 
-fn parse_threads(value: Value) -> Result<Option<usize>, anyhow::Error> {
+fn parse_threads(value: Value) -> Result<Option<usize>> {
     Ok(Some(
         value
             .as_u64()
@@ -415,8 +422,10 @@ fn parse_cert_rules(value: Value) -> Result<UseCertRules> {
             .context("use_cert_rules must be an array")?
             .iter()
             .map(|value| {
-                UseCert::parse(value.as_str().context("use_cert_rules must be an array of strings")?)
-                    .context(format!("use_cert_rule {}", value.as_str().unwrap()))
+                UseCert::parse(value.as_str().context("use_cert_rules must be an array of strings")?).context(format!(
+                    "use_cert_rule {}",
+                    value.as_str().context("use_cert_rules must be an array of strings")?
+                ))
             })
             .collect::<Result<Vec<UseCert>>>()?,
     ))
@@ -429,8 +438,10 @@ fn parse_use_key_rules(value: Value) -> Result<UseKeyRules> {
             .context("use_key_rules must be an array")?
             .iter()
             .map(|value| {
-                UseKey::parse(value.as_str().context("use_key_rules must be an array of strings")?)
-                    .context(format!("use_key_rule {}", value.as_str().unwrap()))
+                UseKey::parse(value.as_str().context("use_key_rules must be an array of strings")?).context(format!(
+                    "use_key_rule {}",
+                    value.as_str().context("use_key_rules must be an array of strings")?
+                ))
             })
             .collect::<Result<Vec<UseKey>>>()?,
     ))
@@ -443,8 +454,10 @@ fn parse_cs_san_rules(value: Value) -> Result<CnSanReplaceRules> {
             .context("cn_san_replace_rules must be an array")?
             .iter()
             .map(|value| {
-                CnSanReplace::parse(value.as_str().context("cn_san_replace_rules must be an array of strings")?)
-                    .context(format!("cn_san_replace_rule {}", value.as_str().unwrap()))
+                CnSanReplace::parse(value.as_str().context("cn_san_replace_rules must be an array of strings")?).context(format!(
+                    "cn_san_replace_rule {}",
+                    value.as_str().context("cn_san_replace_rules must be an array of strings")?
+                ))
             })
             .collect::<Result<Vec<CnSanReplace>>>()?,
     ))
@@ -474,8 +487,9 @@ fn parse_dir_file_config(
                 .context("static_dirs must be an array")?
                 .iter()
                 .map(|value| {
-                    let config_path = ConfigPath::new(value.as_str().context("static_dirs must be an array of strings")?)
-                        .context(format!("config dir {}", value.as_str().unwrap()))?;
+                    let config_path = ConfigPath::new(value.as_str().context("static_dirs must be an array of strings")?).context(
+                        format!("config dir {}", value.as_str().context("static_dirs must be an array of strings")?),
+                    )?;
 
                     ensure!(config_path.try_exists()?, format!("static_dir must exist: {}", config_path));
                     ensure!(config_path.is_dir(), format!("static_dir must be a directory: {}", config_path));
@@ -506,8 +520,11 @@ fn parse_dir_file_config(
                 .context("static_files must be an array")?
                 .iter()
                 .map(|value| {
-                    let config_path = ConfigPath::new(value.as_str().context("static_files must be an array of strings")?)
-                        .context(format!("config file {}", value.as_str().unwrap()))?;
+                    let config_path =
+                        ConfigPath::new(value.as_str().context("static_files must be an array of strings")?).context(format!(
+                            "config file {}",
+                            value.as_str().context("static_files must be an array of strings")?
+                        ))?;
 
                     ensure!(config_path.try_exists()?, format!("static_file must exist: {}", config_path));
                     ensure!(config_path.is_file(), format!("static_file must be a file: {}", config_path));
@@ -525,8 +542,9 @@ fn parse_dir_file_config(
                 .context("crypto_dirs must be an array")?
                 .iter()
                 .map(|value| {
-                    let config_path = ConfigPath::new(value.as_str().context("crypto_dirs must be an array of strings")?)
-                        .context(format!("crypto dir {}", value.as_str().unwrap()))?;
+                    let config_path = ConfigPath::new(value.as_str().context("crypto_dirs must be an array of strings")?).context(
+                        format!("crypto dir {}", value.as_str().context("crypto_dirs must be an array of strings")?),
+                    )?;
 
                     ensure!(config_path.try_exists()?, format!("crypto_dir must exist: {}", config_path));
                     ensure!(config_path.is_dir(), format!("crypto_dir must be a directory: {}", config_path));
@@ -546,8 +564,11 @@ fn parse_dir_file_config(
                 .context("crypto_files must be an array")?
                 .iter()
                 .map(|value| {
-                    let config_path = ConfigPath::new(value.as_str().context("crypto_files must be an array of strings")?)
-                        .context(format!("crypto file {}", value.as_str().unwrap()))?;
+                    let config_path =
+                        ConfigPath::new(value.as_str().context("crypto_files must be an array of strings")?).context(format!(
+                            "crypto file {}",
+                            value.as_str().context("crypto_files must be an array of strings")?
+                        ))?;
 
                     ensure!(config_path.try_exists()?, format!("crypto_file must exist: {}", config_path));
                     ensure!(config_path.is_file(), format!("crypto_file must be a file: {}", config_path));
@@ -569,7 +590,10 @@ fn parse_dir_file_config(
                 .iter()
                 .map(|value| {
                     let config_path = ConfigPath::new(value.as_str().context("cluster_customization_dirs must be an array of strings")?)
-                        .context(format!("cluster_customization dir {}", value.as_str().unwrap()))?;
+                        .context(format!(
+                            "cluster_customization dir {}",
+                            value.as_str().context("cluster_customization_dirs must be an array of strings")?
+                        ))?;
 
                     ensure!(
                         config_path.try_exists()?,
@@ -597,7 +621,10 @@ fn parse_dir_file_config(
                 .iter()
                 .map(|value| {
                     let config_path = ConfigPath::new(value.as_str().context("cluster_customization_files must be an array of strings")?)
-                        .context(format!("cluster_customization file {}", value.as_str().unwrap()))?;
+                        .context(format!(
+                        "cluster_customization file {}",
+                        value.as_str().context("cluster_customization_files must be an array of strings")?
+                    ))?;
 
                     ensure!(
                         config_path.try_exists()?,

src/etcd_encoding.rs

@@ -68,7 +68,15 @@ pub(crate) async fn decode(data: &[u8]) -> Result<Vec<u8>> {
 
     let data = &data[4..];
     let unknown = Unknown::decode(data)?;
-    let kind = unknown.type_meta.as_ref().unwrap().kind.as_ref().unwrap().as_str();
+    let kind = unknown
+        .type_meta
+        .as_ref()
+        .context("missing meta")?
+        .kind
+        .as_ref()
+        .context("missing kind")?
+        .as_str();
+
     Ok(match kind {
         "Route" => serde_json::to_vec(&RouteWithMeta::try_from(unknown)?)?,
         "Deployment" => serde_json::to_vec(&DeploymentWithMeta::try_from(unknown)?)?,

src/main.rs

@@ -1,3 +1,5 @@
+#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic)]
+
 use anyhow::{Context, Result};
 use cluster_crypto::ClusterCryptoObjects;
 use config::RecertConfig;

src/ocp_postprocess.rs

@@ -87,7 +87,7 @@ pub(crate) async fn ocp_postprocess(
 async fn run_cluster_customizations(
     cluster_customizations: &ClusterCustomizations,
     in_memory_etcd_client: &Arc<InMemoryK8sEtcd>,
-) -> Result<(), anyhow::Error> {
+) -> Result<()> {
     let dirs = &cluster_customizations.dirs;
     let files = &cluster_customizations.files;
 
@@ -435,7 +435,7 @@ async fn fix_dep_annotations(
     annotations: &mut serde_json::Map<String, serde_json::Value>,
     k8s_resource_location: &K8sResourceLocation,
     etcd_client: &Arc<InMemoryK8sEtcd>,
-) -> Result<(), anyhow::Error> {
+) -> Result<()> {
     for annotation_key in annotations.keys().cloned().collect::<Vec<_>>() {
         if !annotation_key.starts_with("operator.openshift.io/dep-") {
             continue;
@@ -654,7 +654,7 @@ pub(crate) async fn sync_webhook_authenticators(in_memory_etcd_client: &Arc<InMe
         .iter()
         .filter_map(|file_pathbuf| {
             let file_path = file_pathbuf.to_str()?;
-            Some((regex.captures(file_path)?[1].parse::<u32>().unwrap(), file_pathbuf))
+            Some((regex.captures(file_path)?[1].parse::<u32>().ok()?, file_pathbuf))
         })
         .collect::<HashSet<_>>();
     let (latest_revision, latest_kubeconfig) = captures

src/ocp_postprocess/cluster_domain_rename.rs

@@ -13,7 +13,7 @@ pub(crate) async fn rename_all(
     cluster_rename: &ClusterNamesRename,
     static_dirs: &Vec<ConfigPath>,
     static_files: &Vec<ConfigPath>,
-) -> Result<(), anyhow::Error> {
+) -> Result<()> {
     let cluster_domain = cluster_rename.cluster_domain();
     let cluster_name = cluster_rename.cluster_name.clone();
 
@@ -45,7 +45,7 @@ async fn fix_filesystem_resources(
     static_dirs: &Vec<ConfigPath>,
     static_files: &Vec<ConfigPath>,
     generated_infra_id: String,
-) -> Result<(), anyhow::Error> {
+) -> Result<()> {
     for dir in static_dirs {
         fix_dir_resources(cluster_name, cluster_domain, dir, &generated_infra_id).await?;
     }
@@ -57,7 +57,7 @@ async fn fix_filesystem_resources(
     Ok(())
 }
 
-async fn fix_dir_resources(cluster_name: &str, cluster_domain: &str, dir: &Path, generated_infra_id: &str) -> Result<(), anyhow::Error> {
+async fn fix_dir_resources(cluster_name: &str, cluster_domain: &str, dir: &Path, generated_infra_id: &str) -> Result<()> {
     filesystem_rename::fix_filesystem_kubeconfigs(cluster_name, cluster_domain, dir)
         .await
         .context("renaming kubeconfigs")?;
@@ -82,7 +82,7 @@ async fn fix_dir_resources(cluster_name: &str, cluster_domain: &str, dir: &Path,
     Ok(())
 }
 
-async fn fix_file_resources(cluster_domain: &str, file: &Path) -> Result<(), anyhow::Error> {
+async fn fix_file_resources(cluster_domain: &str, file: &Path) -> Result<()> {
     filesystem_rename::fix_filesystem_mcs_machine_config_content(cluster_domain, file)
         .await
         .context("fix filesystem mcs machine config content")?;
@@ -94,7 +94,7 @@ async fn fix_etcd_resources(
     cluster_domain: &str,
     generated_infra_id: String,
     cluster_rename: &ClusterNamesRename,
-) -> Result<(), anyhow::Error> {
+) -> Result<()> {
     etcd_rename::fix_router_certs(
         etcd_client,
         cluster_domain,

src/ocp_postprocess/cluster_domain_rename/etcd_rename.rs

@@ -734,7 +734,7 @@ pub(crate) async fn fix_dns_cluster_config(etcd_client: &Arc<InMemoryK8sEtcd>, c
     Ok(())
 }
 
-fn fix_dns(config: &mut Value, cluster_domain: &str) -> Result<(), anyhow::Error> {
+fn fix_dns(config: &mut Value, cluster_domain: &str) -> Result<()> {
     let spec = &mut config
         .pointer_mut("/spec")
         .context("no /spec")?
@@ -849,7 +849,7 @@ pub(crate) async fn fix_infrastructure_cluster_config(
     Ok(())
 }
 
-fn fix_infra(config: &mut Value, infra_id: &str, cluster_domain: &str) -> Result<(), anyhow::Error> {
+fn fix_infra(config: &mut Value, infra_id: &str, cluster_domain: &str) -> Result<()> {
     let status = &mut config
         .pointer_mut("/status")
         .context("no /status")?

src/ocp_postprocess/cluster_domain_rename/rename_utils.rs

@@ -366,7 +366,7 @@ pub(crate) fn fix_kcm_pod(pod: &mut Value, generated_infra_id: &str) -> Result<(
 
             *arg = serde_json::Value::String(
                 regex::Regex::new(r"--cluster-name=[^ ]+")
-                    .unwrap()
+                    .context("compiling regex")?
                     .replace_all(
                         arg.as_str().context("arg not string")?,
                         format!("--cluster-name={}", generated_infra_id).as_str(),

src/ocp_postprocess/go_base32.rs

@@ -18,6 +18,9 @@ pub(crate) fn base32_encode(mut num: u64) -> String {
     output_index -= 1;
     output_array[output_index] = BASE32_DIGITS[num as usize];
 
+    // Unwrap can't panic because the array the above loop outputs is guaranteed to be valid UTF-8,
+    // we don't want this function to return a Result
+    #[allow(clippy::unwrap_used)]
     String::from_utf8(output_array[output_index..].to_vec()).unwrap()
 }