From 377e51bdb4e2415bd89d04abc404d16c3fab5103 Mon Sep 17 00:00:00 2001
From: Damon Barry <damonbarry@users.noreply.github.com>
Date: Tue, 26 May 2020 09:58:57 -0700
Subject: [PATCH] Don't delete provisioning backup when configuration changes
 (#2989)

At startup, if iotedged detects that `config.yaml` has changed it will remove the `{homedir}/cache/` folder, rebuild the `settings_state` file in that folder (the hash used to detect `config.yaml` changes), and remove any running containers so edge agent can recreate them. When this logic was originally introduced into iotedged, the _other_ file in that folder, `provisioning_backup.json`, would have also been recreated a little later in the startup cycle after provisioning took place.

The problem is that, in 1.0.9, the startup sequence was adjusted and provisioning now takes place _before_ we detect changes in `config.yaml`. As a result, `provisioning_backup.json` is created but then subsequently deleted if `config.yaml` changes are detected.

The impact is that users who specify some form of DPS provisioning in their `config.yaml`, and who upgrade their devices to 1.0.9 will not be able to reprovision their offline devices without doing an additional reboot.

This change removes the code that deleted the entire `cache/` folder. The existing code to recreate `settings_state` already creates the file if it didn't exist, or overwrites it otherwise. The provisioning backup file will already have been refreshed earlier in the startup sequence and doesn't need to be recreated.
---
 edgelet/iotedged/src/lib.rs | 84 +++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 12 deletions(-)

diff --git a/edgelet/iotedged/src/lib.rs b/edgelet/iotedged/src/lib.rs
index e11a6f192be..441d35e54b3 100644
--- a/edgelet/iotedged/src/lib.rs
+++ b/edgelet/iotedged/src/lib.rs
@@ -1077,7 +1077,6 @@ where
     M::Settings: Serialize,
     C: CreateCertificate + GetIssuerAlias + MasterEncryptionKey,
 {
-    // Remove all edge containers and destroy the cache (settings and dps backup)
     info!("Removing all modules...");
     tokio_runtime
         .block_on(runtime.remove_all())
@@ -1086,22 +1085,13 @@ where
         ))?;
     info!("Finished removing modules.");
 
-    // Ignore errors from this operation because we could be recovering from a previous bad
-    // configuration and shouldn't stall the current configuration because of that
-    let _u = fs::remove_dir_all(subdir);
-
     let path = subdir.join(filename);
 
-    DirBuilder::new()
-        .recursive(true)
-        .create(subdir)
-        .context(ErrorKind::Initialize(
-            InitializeErrorReason::CreateSettingsDirectory,
-        ))?;
-
     // regenerate the workload CA certificate
     destroy_workload_ca(crypto)?;
     prepare_workload_ca(crypto)?;
+
+    // regenerate settings_state
     let mut file =
         File::create(path).context(ErrorKind::Initialize(InitializeErrorReason::SaveSettings))?;
     let digest = compute_settings_digest(settings, id_cert_thumbprint)
@@ -2281,6 +2271,76 @@ mod tests {
         assert_ne!(written1, written);
     }
 
+    #[test]
+    fn check_settings_state_does_not_delete_other_files() {
+        let tmp_dir = TempDir::new("blah").unwrap();
+        let settings = Settings::new(Some(Path::new(GOOD_SETTINGS))).unwrap();
+        let config = DockerConfig::new(
+            "microsoft/test-image".to_string(),
+            ContainerCreateBody::new(),
+            None,
+        )
+        .unwrap();
+
+        // create baseline settings_state
+        let state = ModuleRuntimeState::default();
+        let module: TestModule<Error, _> =
+            TestModule::new_with_config("test-module".to_string(), config, Ok(state));
+        let runtime = TestRuntime::make_runtime(
+            settings.clone(),
+            TestProvisioningResult::new(),
+            TestHsm::default(),
+        )
+        .wait()
+        .unwrap()
+        .with_module(Ok(module));
+        let crypto = TestCrypto {
+            use_expired_ca: false,
+            fail_device_ca_alias: false,
+            fail_decrypt: false,
+            fail_encrypt: true,
+        };
+        let mut tokio_runtime = tokio::runtime::Runtime::new().unwrap();
+        check_settings_state::<TestRuntime<_, Settings>, _>(
+            tmp_dir.path(),
+            "settings_state",
+            &settings,
+            &runtime,
+            &crypto,
+            &mut tokio_runtime,
+            None,
+        )
+        .unwrap();
+
+        // create a couple extra files in the same folder as settings_state
+        let provisioning_backup_json = tmp_dir.path().join("provisioning_backup.json");
+        File::create(&provisioning_backup_json)
+            .unwrap()
+            .write_all(b"{}")
+            .unwrap();
+
+        let file1 = tmp_dir.path().join("file1.txt");
+        File::create(&file1).unwrap().write_all(b"hello").unwrap();
+
+        // change settings; will force update of settings_state
+        let settings1 = Settings::new(Some(Path::new(GOOD_SETTINGS1))).unwrap();
+        let mut tokio_runtime = tokio::runtime::Runtime::new().unwrap();
+        check_settings_state::<TestRuntime<_, Settings>, _>(
+            tmp_dir.path(),
+            "settings_state",
+            &settings1,
+            &runtime,
+            &crypto,
+            &mut tokio_runtime,
+            None,
+        )
+        .unwrap();
+
+        // verify extra files weren't deleted
+        assert!(provisioning_backup_json.exists());
+        assert!(file1.exists());
+    }
+
     #[test]
     fn get_proxy_uri_recognizes_https_proxy() {
         // Use existing "https_proxy" env var if it's set, otherwise invent one