Roll back Satellite provisioning if subscription attempt fails with e…

…rror

Previously Satellite provisioning was only rolled back when the
"Unregister" button has been pressed.

Turns out this is not sufficient, as provisioning happens quite early,
yet the registration and subscription attempt can still end with an
error. The system will then be provisioned for Satellite, but not
registered to it, so no "Unregister" button would be available to
roll back the provisioning.

So instead, make sure Satellite provisioning is always rolled back
if registration to satellite fails in one of tis phases.

And while we are at it refactor the RHSM config dict flattening method
to a function in an utility module and cover all the new and changed
code with unit tests.

Resolves: rhbz#2010865

pyanaconda/modules/subscription/runtime.py

@@ -44,6 +44,7 @@
     SERVER_HOSTNAME_NOT_SATELLITE_PREFIX
 from pyanaconda.modules.subscription.subscription_interface import \
     RetrieveOrganizationsTaskInterface
+from pyanaconda.modules.subscription.utils import flatten_rhsm_nested_dict
 from pyanaconda.anaconda_loggers import get_module_logger
 
 import gi
@@ -801,6 +802,7 @@ def __init__(self, rhsm_observer, subscription_request, system_purpose_data,
         self._rhsm_observer = rhsm_observer
         self._subscription_request = subscription_request
         self._system_purpose_data = system_purpose_data
+        self._rhsm_configuration = {}
         # callback for nested tasks
         self._registered_callback = registered_callback
         self._registered_to_satellite_callback = registered_to_satellite_callback
@@ -883,7 +885,16 @@ def _provision_system_for_satellite(self):
         backup_task = BackupRHSMConfBeforeSatelliteProvisioningTask(
             rhsm_config_proxy=rhsm_config_proxy
         )
-        self._config_backup_callback(backup_task.run())
+        # Run the task and flatten the returned configuration
+        # (so that it can be fed to SetAll()) now, so we don't have to do that later.
+        flat_rhsm_configuration = {}
+        nested_rhsm_configuration = backup_task.run()
+        if nested_rhsm_configuration:
+            flat_rhsm_configuration = flatten_rhsm_nested_dict(nested_rhsm_configuration)
+        self._config_backup_callback(flat_rhsm_configuration)
+        # also store a copy in this task, in case we encounter an error and need to roll-back
+        # when this task is still running
+        self._rhsm_configuration = flat_rhsm_configuration
 
         # now run the Satellite provisioning script we just downloaded, so that the installation
         # environment can talk to the Satellite instance the user has specified via custom
@@ -909,9 +920,19 @@ def _provision_system_for_satellite(self):
             # which is an unrecoverable error, so we end there.
             raise e
 
+    def _roll_back_satellite_provisioning(self):
+        """Something failed after we did Satellite provisioning - roll it back."""
+        log.debug("registration attempt: rolling back Satellite provisioning")
+        rollback_task = RollBackSatelliteProvisioningTask(
+            rhsm_config_proxy=self._rhsm_observer.get_proxy(RHSM_CONFIG),
+            rhsm_configuration=self._rhsm_configuration
+        )
+        rollback_task.run()
+        log.debug("registration attempt: Satellite provisioning rolled back")
+
     def run(self):
         """Try to register and subscribe the installation environment."""
-
+        provisioned_for_satellite = False
         # check authentication method has been set and credentials seem to be
         # sufficient (though not necessarily valid)
         register_task = None
@@ -950,6 +971,7 @@ def run(self):
                 # environment for Satellite
                 log.debug("registration attempt: provisioning system for Satellite")
                 self._provision_system_for_satellite()
+                provisioned_for_satellite = True
                 # if we got there without an exception being raised, it was a success!
                 log.debug("registration attempt: system provisioned for Satellite")
 
@@ -959,12 +981,16 @@ def run(self):
             except (RegistrationError, MultipleOrganizationsError) as e:
                 log.debug("registration attempt: registration attempt failed: %s", e)
                 log.debug("registration attempt: skipping auto attach due to registration error")
+                if provisioned_for_satellite:
+                    self._roll_back_satellite_provisioning()
                 raise e
             log.debug("registration attempt: registration succeeded")
         else:
             log.debug(
                 "registration attempt: credentials insufficient, skipping registration attempt"
             )
+            if provisioned_for_satellite:
+                self._roll_back_satellite_provisioning()
             raise RegistrationError(_("Registration failed due to insufficient credentials."))
 
         # try to attach subscription
@@ -975,7 +1001,14 @@ def run(self):
             rhsm_attach_proxy=rhsm_attach_proxy,
             sla=sla
         )
-        subscription_task.run()
+        try:
+            subscription_task.run()
+        except SubscriptionError as e:
+            # also roll back Satellite provisioning if registration was successfull
+            # but auto-attach failed
+            if provisioned_for_satellite:
+                self._roll_back_satellite_provisioning()
+            raise e
         # if we got this far without an exception then subscriptions have been attached
         self._subscription_attached_callback(True)
 

pyanaconda/modules/subscription/subscription.py

@@ -50,6 +50,7 @@
     RegisterAndSubscribeTask, UnregisterTask, SystemPurposeConfigurationTask, \
     RetrieveOrganizationsTask
 from pyanaconda.modules.subscription.rhsm_observer import RHSMObserver
+from pyanaconda.modules.subscription.utils import flatten_rhsm_nested_dict
 
 
 from pykickstart.errors import KickstartParseWarning
@@ -102,7 +103,7 @@ def __init__(self):
         self._satellite_provisioning_script = None
         self.registered_to_satellite_changed = Signal()
         self._registered_to_satellite = False
-        self._rhsm_conf_before_satellite_provisioning = None
+        self._rhsm_conf_before_satellite_provisioning = {}
 
         # registration status
         self.registered_changed = Signal()
@@ -587,26 +588,6 @@ def rhsm_observer(self):
         """
         return self._rhsm_observer
 
-    def _flatten_rhsm_nested_dict(self, nested_dict):
-        """Convert the GetAll() returned nested dict into a flat one.
-
-        RHSM returns a nested dict with categories on top
-        and category keys & values inside. This is not convenient
-        for setting keys based on original values, so
-        let's normalize the dict to the flat key based
-        structure similar to what's used by SetAll().
-
-        :param dict nested_dict: the nested dict returned by GetAll()
-        :return: flat key/value dictionary, similar to format used by SetAll()
-        :rtype: dict
-        """
-        flat_dict = {}
-        for category_key, category_dict in nested_dict.items():
-            for key, value in category_dict.items():
-                flat_key = "{}.{}".format(category_key, key)
-                flat_dict[flat_key] = value
-        return flat_dict
-
     def get_rhsm_config_defaults(self):
         """Return RHSM config default values.
 
@@ -636,7 +617,7 @@ def get_rhsm_config_defaults(self):
             # turn the variant into a dict with get_native()
             nested_dict = get_native(proxy.GetAll(""))
             # flatten the nested dict
-            flat_dict = self._flatten_rhsm_nested_dict(nested_dict)
+            flat_dict = flatten_rhsm_nested_dict(nested_dict)
             self._rhsm_config_defaults = flat_dict
         return self._rhsm_config_defaults
 
@@ -659,16 +640,11 @@ def unregister_with_task(self):
 
         :return: a DBus path of an installation task
         """
-        # if we have a configuration backup, flatten the clean RHSM config so that the task
-        # can feed it to SetAll()
-        flat_rhsm_configuration = {}
-        if self._rhsm_conf_before_satellite_provisioning:
-            flat_rhsm_configuration = self._flatten_rhsm_nested_dict(
-                self._rhsm_conf_before_satellite_provisioning
-            )
+        # the configuration backup is already flattened by the task that fetched it,
+        # we can directly feed it to SetAll()
         task = UnregisterTask(rhsm_observer=self.rhsm_observer,
                               registered_to_satellite=self.registered_to_satellite,
-                              rhsm_configuration=flat_rhsm_configuration)
+                              rhsm_configuration=self._rhsm_conf_before_satellite_provisioning)
         # apply state changes on success
         task.succeeded_signal.connect(self._system_unregistered_callback)
         return task

pyanaconda/modules/subscription/utils.py

@@ -0,0 +1,39 @@
+#
+# Utility functions for network module
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions of
+# the GNU General Public License v.2, or (at your option) any later version.
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY expressed or implied, including the implied warranties of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General
+# Public License for more details.  You should have received a copy of the
+# GNU General Public License along with this program; if not, write to the
+# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.  Any Red Hat trademarks that are incorporated in the
+# source code or documentation are not subject to the GNU General Public
+# License and may only be used or replicated with the express permission of
+# Red Hat, Inc.
+#
+
+def flatten_rhsm_nested_dict(nested_dict):
+    """Convert the GetAll() returned nested dict into a flat one.
+
+    RHSM returns a nested dict with categories on top
+    and category keys & values inside. This is not convenient
+    for setting keys based on original values, so
+    let's normalize the dict to the flat key based
+    structure similar to what's used by SetAll().
+
+    :param dict nested_dict: the nested dict returned by GetAll()
+    :return: flat key/value dictionary, similar to format used by SetAll()
+    :rtype: dict
+    """
+    flat_dict = {}
+    for category_key, category_dict in nested_dict.items():
+        for key, value in category_dict.items():
+            flat_key = "{}.{}".format(category_key, key)
+            flat_dict[flat_key] = value
+    return flat_dict

tests/nosetests/pyanaconda_tests/modules/subscription/subscription_tasks_test.py

@@ -1325,7 +1325,9 @@ def provision_system_for_satellite_download_error_test(self, download_task):
     @patch("pyanaconda.core.util.restart_service")
     @patch("pyanaconda.modules.subscription.runtime.RunSatelliteProvisioningScriptTask")
     @patch("pyanaconda.modules.subscription.runtime.DownloadSatelliteProvisioningScriptTask")
-    def provision_sat_run_error_test(self, download_task, run_script_task, restart_service):
+    @patch("pyanaconda.modules.subscription.runtime.BackupRHSMConfBeforeSatelliteProvisioningTask")
+    def provision_sat_run_error_test(self, backup_task, download_task, run_script_task, \
+                                     restart_service):
         """Test Satellite provisioning in RegisterAndSubscribeTask - script run failed."""
         # create the task and related bits
         subscription_request = SubscriptionRequest()
@@ -1346,6 +1348,8 @@ def provision_sat_run_error_test(self, download_task, run_script_task, restart_s
         download_task.return_value.run.return_value = "foo bar script"
         # make the mock run task fail
         run_script_task.side_effect = SatelliteProvisioningError()
+        # make the mock backup task return mock RHSM config dict
+        backup_task.return_value.run.return_value = {"foo": {"bar": "baz"}}
         # run the provisioning method, check correct exception is raised
         with self.assertRaises(SatelliteProvisioningError):
             task._provision_system_for_satellite()
@@ -1393,10 +1397,12 @@ def provision_success_test(self, download_task, backup_task, run_script_task, re
             satellite_script_callback=satellite_script_callback,
             config_backup_callback=config_backup_callback
         )
+        # mock the roll back method
+        task._roll_back_satellite_provisioning = Mock()
         # make the mock download task return the script from its run() method
         download_task.return_value.run.return_value = "foo bar script"
         # make the mock backup task return mock RHSM config dict
-        backup_task.return_value.run.return_value = {"foo": "bar"}
+        backup_task.return_value.run.return_value = {"foo": {"bar": "baz"}}
         # run the provisioning method
         task._provision_system_for_satellite()
         # download task should have been instantiated
@@ -1410,11 +1416,13 @@ def provision_success_test(self, download_task, backup_task, run_script_task, re
         # registration attempt
         rhsm_observer.get_proxy.assert_called_once_with(RHSM_CONFIG)
         backup_task.assert_called_once_with(rhsm_config_proxy=rhsm_observer.get_proxy.return_value)
-        config_backup_callback.assert_called_once_with({"foo": "bar"})
+        config_backup_callback.assert_called_once_with({"foo.bar": "baz"})
         # then the run script task should have been instantiated
         run_script_task.assert_called_once_with(provisioning_script="foo bar script")
         # then the RHSM service restart should happen
         restart_service.assert_called_once_with(RHSM_SERVICE_NAME)
+        # make sure the rollback method was not called
+        task._roll_back_satellite_provisioning.assert_not_called()
 
     @patch("pyanaconda.modules.subscription.runtime.RegisterWithUsernamePasswordTask")
     def registration_error_username_password_test(self, register_username_task):
@@ -1609,6 +1617,134 @@ def registration_and_subscribe_satellite_test(self, register_task, attach_task,
         )
         parse_task.return_value.run_with_signals.assert_called_once()
 
+    @patch("pyanaconda.modules.subscription.runtime.ParseAttachedSubscriptionsTask")
+    @patch("pyanaconda.modules.subscription.runtime.AttachSubscriptionTask")
+    @patch("pyanaconda.modules.subscription.runtime.RegisterWithOrganizationKeyTask")
+    def registration_failure_satellite_test(self, register_task, attach_task, parse_task):
+        """Test RegisterAndSubscribeTask - registration failure with satellite provisioning."""
+        # create the task and related bits
+        rhsm_observer = Mock()
+        rhsm_register_server = Mock()
+        rhsm_attach = Mock()
+        rhsm_entitlement = Mock()
+        rhsm_syspurpose = Mock()
+        rhsm_observer.get_proxy.side_effect = [
+            rhsm_register_server, rhsm_attach, rhsm_entitlement, rhsm_syspurpose
+        ]
+        subscription_request = SubscriptionRequest()
+        subscription_request.type = SUBSCRIPTION_REQUEST_TYPE_ORG_KEY
+        subscription_request.organization = "foo_org"
+        subscription_request.activation_keys.set_secret(["key1", "key2", "key3"])
+        subscription_request.server_hostname = "satellite.example.com"
+        system_purpose_data = SystemPurposeData()
+        system_purpose_data.sla = "foo_sla"
+        subscription_attached_callback = Mock()
+        task = RegisterAndSubscribeTask(
+            rhsm_observer=rhsm_observer,
+            subscription_request=subscription_request,
+            system_purpose_data=system_purpose_data,
+            registered_callback=Mock(),
+            registered_to_satellite_callback=Mock(),
+            subscription_attached_callback=subscription_attached_callback,
+            subscription_data_callback=Mock(),
+            satellite_script_callback=Mock(),
+            config_backup_callback=Mock()
+        )
+        # mock the Satellite provisioning method
+        task._provision_system_for_satellite = Mock()
+        # mock the Satellite rollback method
+        task._roll_back_satellite_provisioning = Mock()
+        # make the register task throw an exception
+        register_task.return_value.run_with_signals.side_effect = RegistrationError()
+        # run the main task, epxect registration error
+        with self.assertRaises(RegistrationError):
+            task.run()
+        # check satellite provisioning was attempted
+        task._provision_system_for_satellite.assert_called_once_with()
+        # check the register task was properly instantiated and run
+        register_task.assert_called_once_with(
+            rhsm_register_server_proxy=rhsm_register_server,
+            organization='foo_org',
+            activation_keys=['key1', 'key2', 'key3']
+
+        )
+        register_task.return_value.run_with_signals.assert_called_once()
+        # check the subscription attach task has not been instantiated and run
+        attach_task.assert_not_called()
+        # also check the callback was not called
+        subscription_attached_callback.assert_not_called()
+        # check the subscription parsing task has not been instantiated and run
+        parse_task.assert_not_called()
+        parse_task.return_value.run_with_signals.assert_not_called()
+        # the Satellite provisioning rollback should have been called due to the failure
+        task._roll_back_satellite_provisioning.assert_called_once()
+
+    @patch("pyanaconda.modules.subscription.runtime.ParseAttachedSubscriptionsTask")
+    @patch("pyanaconda.modules.subscription.runtime.AttachSubscriptionTask")
+    @patch("pyanaconda.modules.subscription.runtime.RegisterWithOrganizationKeyTask")
+    def subscription_failure_satellite_test(self, register_task, attach_task, parse_task):
+        """Test RegisterAndSubscribeTask - subscription failure with satellite provisioning."""
+        # create the task and related bits
+        rhsm_observer = Mock()
+        rhsm_register_server = Mock()
+        rhsm_attach = Mock()
+        rhsm_entitlement = Mock()
+        rhsm_syspurpose = Mock()
+        rhsm_observer.get_proxy.side_effect = [
+            rhsm_register_server, rhsm_attach, rhsm_entitlement, rhsm_syspurpose
+        ]
+        subscription_request = SubscriptionRequest()
+        subscription_request.type = SUBSCRIPTION_REQUEST_TYPE_ORG_KEY
+        subscription_request.organization = "foo_org"
+        subscription_request.activation_keys.set_secret(["key1", "key2", "key3"])
+        subscription_request.server_hostname = "satellite.example.com"
+        system_purpose_data = SystemPurposeData()
+        system_purpose_data.sla = "foo_sla"
+        subscription_attached_callback = Mock()
+        task = RegisterAndSubscribeTask(
+            rhsm_observer=rhsm_observer,
+            subscription_request=subscription_request,
+            system_purpose_data=system_purpose_data,
+            registered_callback=Mock(),
+            registered_to_satellite_callback=Mock(),
+            subscription_attached_callback=subscription_attached_callback,
+            subscription_data_callback=Mock(),
+            satellite_script_callback=Mock(),
+            config_backup_callback=Mock()
+        )
+        # mock the Satellite provisioning method
+        task._provision_system_for_satellite = Mock()
+        # mock the Satellite rollback method
+        task._roll_back_satellite_provisioning = Mock()
+        # make the subscription task throw an exception
+        attach_task.return_value.run.side_effect = SubscriptionError()
+        # run the main task, epxect registration error
+        with self.assertRaises(SubscriptionError):
+            task.run()
+        # check satellite provisioning was attempted
+        task._provision_system_for_satellite.assert_called_once_with()
+        # check the register task was properly instantiated and run
+        register_task.assert_called_once_with(
+            rhsm_register_server_proxy=rhsm_register_server,
+            organization='foo_org',
+            activation_keys=['key1', 'key2', 'key3']
+
+        )
+        register_task.return_value.run_with_signals.assert_called_once()
+        # check the subscription attach task has been properly instantiated and run
+        attach_task.assert_called_once_with(
+            rhsm_attach_proxy=rhsm_attach,
+            sla="foo_sla",
+        )
+        attach_task.return_value.run.assert_called_once()
+        # also check the callback was not called
+        subscription_attached_callback.assert_not_called()
+        # check the subscription parsing task has not been instantiated and run
+        parse_task.assert_not_called()
+        parse_task.return_value.run_with_signals.assert_not_called()
+        # the Satellite provisioning rollback should have been called due to the failure
+        task._roll_back_satellite_provisioning.assert_called_once()
+
 
 class RetrieveOrganizationsTaskTestCase(unittest.TestCase):
     """Test the organization data parsing task."""