Add check whether Crostini container is already configured

Adds check whether Crostini container is already configured, so that
default Crostini container configuration is triggered only when default
Crostini container is not yet successfully configured using currently
set CrostiniAnsiblePlaybook policy.

Before this CL: container is reconfigured each time container is started
if policy is specified & feature flag is set.

TEST: unit_tests --gtest_filter="*Ansible*"
Bug: 1024859
Change-Id: If1dc83a8130d35a9aa4eba5c8f5e8b39ea1e0dbf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2000701
Commit-Queue: Olya Kalitova <okalitova@chromium.org>
Reviewed-by: Nic Hollingum <hollingum@google.com>
Reviewed-by: Nikita Podguzov <nikitapodguzov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732810}

chrome/browser/chromeos/crostini/ansible/ansible_management_service.cc

@@ -40,6 +40,12 @@ AnsibleManagementService::~AnsibleManagementService() = default;
 
 void AnsibleManagementService::ConfigureDefaultContainer(
     base::OnceCallback<void(bool success)> callback) {
+  if (!ShouldConfigureDefaultContainer(profile_)) {
+    LOG(ERROR) << "Trying to configure default Crostini container when it "
+               << "should not be configured";
+    std::move(callback).Run(false);
+    return;
+  }
   DCHECK(!configuration_finished_callback_);
   configuration_finished_callback_ = std::move(callback);
 
@@ -86,15 +92,11 @@ void AnsibleManagementService::OnInstallLinuxPackageProgress(
 
   switch (status) {
     case InstallLinuxPackageProgressStatus::SUCCEEDED: {
-      CrostiniManager::GetForProfile(profile_)
-          ->RemoveLinuxPackageOperationProgressObserver(this);
       GetAnsiblePlaybookToApply();
       return;
     }
     case InstallLinuxPackageProgressStatus::FAILED:
       LOG(ERROR) << "Ansible installation failed";
-      CrostiniManager::GetForProfile(profile_)
-          ->RemoveLinuxPackageOperationProgressObserver(this);
       OnConfigurationFinished(false);
       return;
     // TODO(okalitova): Report Ansible downloading/installation progress.
@@ -176,6 +178,7 @@ void AnsibleManagementService::OnApplyAnsiblePlaybook(
 
   VLOG(1) << "Ansible playbook application has been started successfully";
   // Waiting for Ansible playbook application progress being reported.
+  // TODO(https://crbug.com/1043060): Add a timeout after which we stop waiting.
 }
 
 void AnsibleManagementService::OnApplyAnsiblePlaybookProgress(
@@ -216,6 +219,12 @@ void AnsibleManagementService::OnUninstallPackageProgress(
 
 void AnsibleManagementService::OnConfigurationFinished(bool success) {
   DCHECK(configuration_finished_callback_);
+  if (success) {
+    profile_->GetPrefs()->SetBoolean(prefs::kCrostiniDefaultContainerConfigured,
+                                     true);
+  }
+  CrostiniManager::GetForProfile(profile_)
+      ->RemoveLinuxPackageOperationProgressObserver(this);
   for (auto& observer : observers_) {
     observer.OnAnsibleSoftwareConfigurationFinished(success);
   }

chrome/browser/chromeos/crostini/ansible/ansible_management_service_unittest.cc

@@ -48,6 +48,17 @@ class AnsibleManagementServiceTest : public testing::Test {
     return ansible_management_service_;
   }
 
+  void ExecuteSuccessfulConfigurationFlow() {
+    ansible_management_service()->ConfigureDefaultContainer(
+        configuration_finished_mock_callback_.Get());
+    // Should wait for Cicerone response for Linux package install request.
+    base::RunLoop().RunUntilIdle();
+    test_helper_->SendSucceededInstallSignal();
+    // AnsibleManagementService should read playbook file content.
+    task_environment_.RunUntilIdle();
+    test_helper_->SendSucceededApplySignal();
+  }
+
   content::BrowserTaskEnvironment task_environment_;
   std::unique_ptr<AnsibleManagementTestHelper> test_helper_;
   base::MockCallback<base::OnceCallback<void(bool)>>
@@ -74,7 +85,6 @@ TEST_F(AnsibleManagementServiceTest, ConfigureDefaultContainerSuccess) {
   base::RunLoop().RunUntilIdle();
 
   test_helper_->SendSucceededInstallSignal();
-  base::RunLoop().RunUntilIdle();
   task_environment_.RunUntilIdle();
 
   test_helper_->SendSucceededApplySignal();
@@ -104,8 +114,47 @@ TEST_F(AnsibleManagementServiceTest, ConfigureDefaultContainerApplyFail) {
   base::RunLoop().RunUntilIdle();
 
   test_helper_->SendSucceededInstallSignal();
-  base::RunLoop().RunUntilIdle();
   task_environment_.RunUntilIdle();
 }
 
+TEST_F(AnsibleManagementServiceTest,
+       CouldNotConfigureContainerAfterSuccessfullConfiguration) {
+  test_helper_->SetUpAnsibleInstallation(
+      vm_tools::cicerone::InstallLinuxPackageResponse::STARTED);
+  test_helper_->SetUpPlaybookApplication(
+      vm_tools::cicerone::ApplyAnsiblePlaybookResponse::STARTED);
+
+  EXPECT_CALL(configuration_finished_mock_callback_, Run(true)).Times(1);
+  ExecuteSuccessfulConfigurationFlow();
+
+  EXPECT_CALL(configuration_finished_mock_callback_, Run(false)).Times(1);
+
+  ansible_management_service()->ConfigureDefaultContainer(
+      configuration_finished_mock_callback_.Get());
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(AnsibleManagementServiceTest,
+       CouldConfigureContainerAfterFailedConfiguration) {
+  test_helper_->SetUpAnsibleInstallation(
+      vm_tools::cicerone::InstallLinuxPackageResponse::FAILED);
+
+  EXPECT_CALL(configuration_finished_mock_callback_, Run(false)).Times(1);
+
+  // Unsuccessful sequence of events.
+  ansible_management_service()->ConfigureDefaultContainer(
+      configuration_finished_mock_callback_.Get());
+  base::RunLoop().RunUntilIdle();
+  CloseCrostiniAnsibleSoftwareConfigViewForTesting();
+
+  // Setup for success.
+  test_helper_->SetUpAnsibleInstallation(
+      vm_tools::cicerone::InstallLinuxPackageResponse::STARTED);
+  test_helper_->SetUpPlaybookApplication(
+      vm_tools::cicerone::ApplyAnsiblePlaybookResponse::STARTED);
+
+  EXPECT_CALL(configuration_finished_mock_callback_, Run(true)).Times(1);
+  ExecuteSuccessfulConfigurationFlow();
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_pref_names.cc

@@ -48,9 +48,13 @@ const char kUserCrostiniRootAccessAllowedByPolicy[] =
 // A file path preference representing a user level enterprise policy that
 // specifies Ansible playbook to be applied to the default Crostini container.
 // Value is empty when there is no playbook to be applied specified though
-// policy or playbook specified has already been applied successfully.
+// policy.
 const char kCrostiniAnsiblePlaybookFilePath[] =
     "crostini.ansible_playbook_file_path";
+// A boolean preference representing whether default Crostini container is
+// successfully configured by kCrostiniAnsiblePlaybook user policy.
+const char kCrostiniDefaultContainerConfigured[] =
+    "crostini.default_container_configured";
 
 // A boolean preference controlling Crostini usage reporting.
 const char kReportCrostiniUsageEnabled[] = "crostini.usage_reporting_enabled";
@@ -103,6 +107,7 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(kUserCrostiniRootAccessAllowedByPolicy, true);
   registry->RegisterFilePathPref(kCrostiniAnsiblePlaybookFilePath,
                                  base::FilePath());
+  registry->RegisterBooleanPref(kCrostiniDefaultContainerConfigured, false);
   registry->RegisterDictionaryPref(
       kCrostiniTerminalSettings, base::DictionaryValue(),
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);

chrome/browser/chromeos/crostini/crostini_pref_names.h

@@ -24,6 +24,7 @@ extern const char kUserCrostiniExportImportUIAllowedByPolicy[];
 extern const char kVmManagementCliAllowedByPolicy[];
 extern const char kUserCrostiniRootAccessAllowedByPolicy[];
 extern const char kCrostiniAnsiblePlaybookFilePath[];
+extern const char kCrostiniDefaultContainerConfigured[];
 
 extern const char kReportCrostiniUsageEnabled[];
 extern const char kCrostiniLastLaunchTerminaComponentVersion[];

chrome/browser/chromeos/crostini/crostini_util.cc

@@ -338,9 +338,11 @@ bool IsCrostiniRunning(Profile* profile) {
 bool ShouldConfigureDefaultContainer(Profile* profile) {
   const base::FilePath ansible_playbook_file_path =
       profile->GetPrefs()->GetFilePath(prefs::kCrostiniAnsiblePlaybookFilePath);
+  bool default_container_configured = profile->GetPrefs()->GetBoolean(
+      prefs::kCrostiniDefaultContainerConfigured);
   return base::FeatureList::IsEnabled(
              features::kCrostiniAnsibleInfrastructure) &&
-         !ansible_playbook_file_path.empty();
+         !default_container_configured && !ansible_playbook_file_path.empty();
 }
 
 // TODO(davidmunro): Answer based on flag and current container version.

chrome/browser/chromeos/policy/external_data_handlers/crostini_ansible_playbook_external_data_handler.cc

@@ -39,6 +39,8 @@ void CrostiniAnsiblePlaybookExternalDataHandler::OnExternalDataCleared(
   }
   profile->GetPrefs()->ClearPref(
       crostini::prefs::kCrostiniAnsiblePlaybookFilePath);
+  profile->GetPrefs()->ClearPref(
+      crostini::prefs::kCrostiniDefaultContainerConfigured);
 }
 
 void CrostiniAnsiblePlaybookExternalDataHandler::OnExternalDataFetched(
@@ -54,6 +56,8 @@ void CrostiniAnsiblePlaybookExternalDataHandler::OnExternalDataFetched(
   }
   profile->GetPrefs()->SetFilePath(
       crostini::prefs::kCrostiniAnsiblePlaybookFilePath, file_path);
+  profile->GetPrefs()->SetBoolean(
+      crostini::prefs::kCrostiniDefaultContainerConfigured, false);
 }
 
 void CrostiniAnsiblePlaybookExternalDataHandler::RemoveForAccountId(
@@ -66,6 +70,8 @@ void CrostiniAnsiblePlaybookExternalDataHandler::RemoveForAccountId(
   }
   profile->GetPrefs()->ClearPref(
       crostini::prefs::kCrostiniAnsiblePlaybookFilePath);
+  profile->GetPrefs()->ClearPref(
+      crostini::prefs::kCrostiniDefaultContainerConfigured);
 }
 
 }  // namespace policy

chrome/browser/ui/views/crostini/crostini_ansible_software_config_view_browsertest.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/views/crostini/crostini_browser_test_util.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/network_service_instance.h"
 #include "services/network/test/test_network_connection_tracker.h"
@@ -24,7 +25,10 @@ class CrostiniAnsibleSoftwareConfigViewBrowserTest
         container_id_(crostini::kCrostiniDefaultVmName,
                       crostini::kCrostiniDefaultContainerName),
         network_connection_tracker_(
-            network::TestNetworkConnectionTracker::CreateInstance()) {}
+            network::TestNetworkConnectionTracker::CreateInstance()) {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kCrostiniAnsibleInfrastructure);
+  }
 
   // CrostiniDialogBrowserTest:
   void ShowUi(const std::string& name) override {
@@ -114,6 +118,7 @@ class CrostiniAnsibleSoftwareConfigViewBrowserTest
   std::unique_ptr<network::TestNetworkConnectionTracker>
       network_connection_tracker_;
   std::unique_ptr<crostini::AnsibleManagementTestHelper> test_helper_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_F(CrostiniAnsibleSoftwareConfigViewBrowserTest,