Repository: Azure/WALinuxAgent
Pull request #1970 (2020-07-31 22:16:17)

Uninstall unregistered extensions
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description
Currently the Agent doesn't uninstall extensions if the extension was unregistered from PIR. This PR handles that scenario - Issue #1449
Additionally did some code cleanup in the PR.

<!--
Please add an informative description that covers that changes made by the pull request. 
This checklist is used to make sure that common issues in a pull request are addressed.
This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
-->

---

### PR information
- [ ] The title of the PR is clear and informative.
- [ ] There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, [see this page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [ ] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [ ] If applicable, the PR references the bug/issue that it fixes in the description.
- [ ] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [ ] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970?src=pr&el=h1) Report
> Merging [#1970](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970?src=pr&el=desc) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/ac671841edf49bd432311e140cffc2b159c12254&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `57.14%`.

[![Impacted file tree graph](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970/graphs/tree.svg?width=650&height=150&src=pr&token=ae90E2KYCw)](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           develop    #1970      +/-   ##
===========================================
+ Coverage    69.60%   69.66%   +0.05%     
===========================================
  Files           85       85              
  Lines        11998    11998              
  Branches      1677     1677              
===========================================
+ Hits          8351     8358       +7     
+ Misses        3273     3268       -5     
+ Partials       374      372       -2     
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/ga/exthandlers.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2dhL2V4dGhhbmRsZXJzLnB5) | `87.04% <57.14%> (+0.74%)` | :arrow_up: |
| [azurelinuxagent/common/utils/restutil.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi91dGlscy9yZXN0dXRpbC5weQ==) | `90.58% <0.00%> (-0.19%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970?src=pr&el=footer). Last update [ac67184...381046f](https://codecov.io/gh/Azure/WALinuxAgent/pull/1970?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/azurelinuxagent/ga/exthandlers.py b/azurelinuxagent/ga/exthandlers.py
index fa87054d..a8099dd1 100644
--- a/azurelinuxagent/ga/exthandlers.py
+++ b/azurelinuxagent/ga/exthandlers.py
@@ -228,6 +228,17 @@ class ExtHandlerState(object):
     FailedUpgrade = "FailedUpgrade"
 
 
+class ExtensionRequestedState(object):
+    """
+    This is the state of the Extension as requested by the Goal State.
+    CRP only supports 2 states as of now - Enabled and Uninstall
+    Disabled was used for older XML extensions and we keep it to support backward compatibility.
+    """
+    Enabled = u"enabled"
+    Disabled = u"disabled"
+    Uninstall = u"uninstall"
+
+
 def get_exthandlers_handler(protocol):
     return ExtHandlersHandler(protocol)
 
@@ -416,7 +427,11 @@ class ExtHandlersHandler(object):
 
             state = ext_handler.properties.state
 
-            if ext_handler_i.decide_version(target_state=state) is None:
+            # The Guest Agent currently only supports 1 installed version per extension on the VM.
+            # If the extension version is unregistered and the customers wants to uninstall the extension,
+            # we should let it go through even if the installed version doesnt exist in Handler manifest (PIR) anymore.
+            # If target state is enabled and version not found in manifest, do not process the extension.
+            if ext_handler_i.decide_version(target_state=state) is None and state == ExtensionRequestedState.Enabled:
                 version = ext_handler_i.ext_handler.properties.version
                 name = ext_handler_i.ext_handler.name
                 err_msg = "Unable to find version {0} in manifest for extension {1}".format(version, name)
@@ -428,11 +443,11 @@ class ExtHandlersHandler(object):
             self.log_etag = True
 
             ext_handler_i.logger.info("Target handler state: {0} [incarnation {1}]", state, etag)
-            if state == u"enabled":
+            if state == ExtensionRequestedState.Enabled:
                 self.handle_enable(ext_handler_i)
-            elif state == u"disabled":
+            elif state == ExtensionRequestedState.Disabled:
                 self.handle_disable(ext_handler_i)
-            elif state == u"uninstall":
+            elif state == ExtensionRequestedState.Uninstall:
                 self.handle_uninstall(ext_handler_i)
             else:
                 message = u"Unknown ext handler state:{0}".format(state)
@@ -723,10 +738,10 @@ class ExtHandlerInstance(object):
         # Note:
         #  - A downgrade, which will be bound to the same major version,
         #    is allowed if the installed version is no longer available
-        if target_state == u"uninstall" or target_state == u"disabled":
+        if target_state in (ExtensionRequestedState.Uninstall, ExtensionRequestedState.Disabled):
             if installed_pkg is None:
-                msg = "Failed to find installed version of {0} " \
-                      "to uninstall".format(self.ext_handler.name)
+                msg = "Failed to find installed version: {0} of Handler: {1}  in handler manifest to uninstall.".format(
+                    installed_version, self.ext_handler.name)
                 self.logger.warn(msg)
             self.pkg = installed_pkg
             self.ext_handler.properties.version = str(installed_version) \


Test patch:
diff --git a/tests/ga/test_extension.py b/tests/ga/test_extension.py
index 430af614..a8d7d0fa 100644
--- a/tests/ga/test_extension.py
+++ b/tests/ga/test_extension.py
@@ -40,7 +40,7 @@ from azurelinuxagent.common.version import PY_VERSION_MAJOR, PY_VERSION_MINOR, P
     GOAL_STATE_AGENT_VERSION, CURRENT_VERSION, DISTRO_NAME, DISTRO_VERSION
 from azurelinuxagent.ga.exthandlers import ExtHandlerState, ExtHandlersHandler, ExtHandlerInstance, HANDLER_PKG_EXT, \
     migrate_handler_state, get_exthandlers_handler, AGENT_STATUS_FILE, ExtCommandEnvVariable, \
-    HandlerManifest, NOT_RUN, ValidHandlerStatus, HANDLER_NAME_PATTERN
+    HandlerManifest, NOT_RUN, ValidHandlerStatus, HANDLER_NAME_PATTERN, ExtensionRequestedState
 
 from azurelinuxagent.ga.monitor import get_monitor_handler
 from nose.plugins.attrib import attr
@@ -153,7 +153,7 @@ class TestExtensionCleanup(AgentTestCase):
 
             # Update incarnation and extension config
             protocol.mock_wire_data.set_incarnation(2)
-            protocol.mock_wire_data.set_extensions_config_state("uninstall")
+            protocol.mock_wire_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
 
             protocol.client.update_goal_state()
             exthandlers_handler.run()
@@ -200,7 +200,7 @@ class TestExtensionCleanup(AgentTestCase):
 
             # Update incarnation and extension config to uninstall the extension, this should delete the extension
             protocol.mock_wire_data.set_incarnation(2)
-            protocol.mock_wire_data.set_extensions_config_state("uninstall")
+            protocol.mock_wire_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
 
             protocol.client.update_goal_state()
             exthandlers_handler.run()
@@ -499,7 +499,7 @@ class TestExtension(ExtensionTestCase):
 
         # Test disable
         test_data.set_incarnation(5)
-        test_data.set_extensions_config_state("disabled")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Disabled)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -508,7 +508,7 @@ class TestExtension(ExtensionTestCase):
 
         # Test uninstall
         test_data.set_incarnation(6)
-        test_data.set_extensions_config_state("uninstall")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -603,7 +603,7 @@ class TestExtension(ExtensionTestCase):
 
         # Test uninstall
         test_data.set_incarnation(2)
-        test_data.set_extensions_config_state("uninstall")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -650,7 +650,7 @@ class TestExtension(ExtensionTestCase):
 
         # Test uninstall
         test_data.set_incarnation(4)
-        test_data.set_extensions_config_state("uninstall")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -725,7 +725,7 @@ class TestExtension(ExtensionTestCase):
         # the first extension disabled. The first extension enabled should be
         # the last one disabled.
         test_data.set_incarnation(3)
-        test_data.set_extensions_config_state("disabled")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Disabled)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -741,7 +741,7 @@ class TestExtension(ExtensionTestCase):
         # the first extension uninstalled. The first extension installed
         # should be the last one uninstalled.
         test_data.set_incarnation(4)
-        test_data.set_extensions_config_state("uninstall")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
 
         # Swap the dependency ordering AGAIN
         test_data.ext_conf = test_data.ext_conf.replace("dependencyLevel=\"3\"", "dependencyLevel=\"6\"")
@@ -873,7 +873,7 @@ class TestExtension(ExtensionTestCase):
 
         # Test disable
         test_data.set_incarnation(5)
-        test_data.set_extensions_config_state("disabled")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Disabled)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -882,7 +882,7 @@ class TestExtension(ExtensionTestCase):
 
         # Test uninstall
         test_data.set_incarnation(6)
-        test_data.set_extensions_config_state("uninstall")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -899,7 +899,7 @@ class TestExtension(ExtensionTestCase):
 
         # Test re-install
         test_data.set_incarnation(8)
-        test_data.set_extensions_config_state("enabled")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Enabled)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -988,12 +988,32 @@ class TestExtension(ExtensionTestCase):
             self.assertTrue(os.path.exists(ehi.get_extension_events_dir()), "Events directory should exist")
 
             # Uninstall extensions now
-            test_data.set_extensions_config_state("uninstall")
+            test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
             test_data.set_incarnation(2)
             protocol.update_goal_state()
             exthandlers_handler.run()
             self.assertTrue(os.path.exists(ehi.get_extension_events_dir()), "Events directory should still exist")
 
+    def test_it_should_uninstall_unregistered_extensions_properly(self, *args):
+        test_data = mockwiredata.WireProtocolData(mockwiredata.DATA_FILE)
+        exthandlers_handler, protocol = self._create_mock(test_data, *args)
+        exthandlers_handler.run()
+        self._assert_handler_status(protocol.report_vm_status, "Ready", 1, "1.0.0")
+
+        # Update version and set it to uninstall. That is how it would be propagated by CRP if a version 1.0.0 is
+        # unregistered in PIR and a new version 1.0.1 is published.
+        test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
+        test_data.set_extensions_config_version("1.0.1")
+        # Since the installed version is not in PIR anymore, we need to also remove it from manifest file
+        test_data.manifest = test_data.manifest.replace("1.0.0", "9.9.9")
+        test_data.set_incarnation(2)
+        protocol.update_goal_state()
+        exthandlers_handler.run()
+        args, _ = protocol.report_vm_status.call_args
+        vm_status = args[0]
+        self.assertEqual(0, len(vm_status.vmAgent.extensionHandlers),
+                         "The extension should not be reported as it is uninstalled")
+
     @patch('azurelinuxagent.common.errorstate.ErrorState.is_triggered')
     @patch('azurelinuxagent.ga.exthandlers.add_event')
     def test_ext_handler_report_status_permanent(self, mock_add_event, mock_error_state, *args):
@@ -1552,7 +1572,7 @@ class TestExtension(ExtensionTestCase):
 
         # Next incarnation, disable extension
         test_data.set_incarnation(2)
-        test_data.set_extensions_config_state("disabled")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Disabled)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -1590,7 +1610,7 @@ class TestExtension(ExtensionTestCase):
 
         # Next incarnation, disable extension
         test_data.set_incarnation(2)
-        test_data.set_extensions_config_state("disabled")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Disabled)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -1619,7 +1639,7 @@ class TestExtension(ExtensionTestCase):
 
         # Next incarnation, disable extension
         test_data.set_incarnation(2)
-        test_data.set_extensions_config_state("uninstall")
+        test_data.set_extensions_config_state(ExtensionRequestedState.Uninstall)
         protocol.update_goal_state()
 
         exthandlers_handler.run()
@@ -2159,7 +2179,7 @@ class TestExtensionSequencing(AgentTestCase):
             if handler_map.get(h) is None:
                 handler = ExtHandler(name=h)
                 extension = Extension(name=h)
-                handler.properties.state = "enabled"
+                handler.properties.state = ExtensionRequestedState.Enabled
                 handler.properties.extensions.append(extension)
                 handler_map[h] = handler
                 all_handlers.append(handler)