Overwrite extension heartbeat with policy error #3321

mgunnala · 2025-02-20T16:48:42Z

Description

Issue #

This PR addresses the following TODOs for the policy/allowlist feature:

in the case of an extension disallowed error, overwrite extension heartbeat with the error
instead of logging policy locally with each goal state, copy the policy file to the history folder

PR information

Ensure development PR is based on the develop branch.
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.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

narrieta · 2025-02-24T22:21:46Z

azurelinuxagent/ga/exthandlers.py

        policy_error = None
        try:
-            policy_engine = ExtensionPolicyEngine()
+            gs_history = self.protocol.get_goal_state().history
+            policy_engine = ExtensionPolicyEngine(gs_history)


you may want to have ExtensionPolicyEngine expose the path to the policy file and save it here, in order to avoid adding a dependency on GoalStateHistory

narrieta · 2025-02-24T22:24:02Z

azurelinuxagent/common/utils/archive.py

@@ -309,3 +310,7 @@ def save_shared_conf(self, text):

    def save_manifest(self, name, text):
        self.save(text, _MANIFEST_FILE_NAME.format(name))
+
+    def save_policy(self, text):


let's make this more generic, in case we need to add other files later... maybe add_file(self, path)?

Updated this to save_file(self, path)

narrieta · 2025-02-24T22:32:16Z

azurelinuxagent/ga/exthandlers.py

@@ -773,10 +775,27 @@ def __handle_ext_disallowed_error(self, ext_handler_i, error_code, report_op, me

        ext_handler_i.set_handler_status(status=ExtHandlerStatusValue.not_ready, message=message, code=error_code)

+        # If extension is reporting heartbeat, overwrite the heartbeat file with error message.
+        heartbeat_file = os.path.join(conf.get_lib_dir(), ext_handler_i.get_heartbeat_file())
+        if os.path.isfile(heartbeat_file):


we need to do this every time we report heartbeat, not only on when the error happens

narrieta · 2025-02-24T23:38:39Z

tests/ga/test_extension.py

@@ -3715,6 +3715,61 @@ def test_uninstall_should_succeed_if_extension_allowed(self):
                vm_status = args[0]
                self.assertEqual(0, len(vm_status.vmAgent.extensionHandlers))

+    def test_should_report_policy_failure_instead_of_heartbeat(self):


did we do any end-to-end testing?

I only added unit tests, since heartbeat isn't currently being e2e tested and none of the extensions we e2e test report heartbeat

If we want to add an e2e test for this, I could add AzureSecurityLinuxAgent to test heartbeat

Added an e2e test with AzureSecurityLinuxAgent

azurelinuxagent/common/utils/archive.py

narrieta · 2025-02-28T16:04:07Z

azurelinuxagent/common/utils/archive.py

+        """
+        Save contents of the specified file to the history folder. Output file will have the same name as the original file.
+        """
+        text = fileutil.read_file(path)


file copy would be better to maintain the permissions of the original file

narrieta · 2025-02-28T16:10:14Z

azurelinuxagent/ga/exthandlers.py

        policy_error = None
        try:
+            gs_history = self.protocol.get_goal_state().history
+            policy_file = conf.get_policy_file_path()


I'll move the responsibility of deciding how to get the path of the file (from conf currently) to the ExtensionPolicyEngine and then expose a method to get the file the engine is using (or None if no file)

Updated the implementation - ExtensionPolicyEngine exposes a property "policy_file_contents" that can be copied to history here

narrieta · 2025-02-28T16:32:45Z

azurelinuxagent/ga/exthandlers.py

                try:
                    heartbeat = ext_handler_i.collect_heartbeat()
                    if heartbeat is not None:
-                        handler_status.status = heartbeat.get('status')
+                        if not ext_disallowed:


if ext_disallowed: pass # The status already describes why the extension is disallowed else: handler_status.status = heartbeat.get('status')

narrieta · 2025-02-28T16:34:18Z

azurelinuxagent/ga/policy/policy_engine.py

+        self._policy = self._parse_policy(policy_contents)
+
+    @property
+    def policy(self):


where is this being used?

It's no longer being used currently, but I kept it here because we'll need to expose the policy for the extension-specific policy feature. I can remove it too.

I decided to remove this for now, I will add it back as needed in the future

maddieford · 2025-02-28T18:55:41Z

azurelinuxagent/ga/exthandlers.py

+            gs_history = self.protocol.get_goal_state().history
+            policy_file = conf.get_policy_file_path()
+            if gs_history is not None and os.path.isfile(policy_file):
+                gs_history.save_file(policy_file)


We may not be saving the same policy we parse in ExtensionPolicyEngine init if we do this here.

Could we dump the contents of the policy file to the history folder in ExtensionPolicyEngine.__read_policy?

I originally copied policy to history in PolicyEngine.__read_policy, but Norberto suggested moving it here to remove the PolicyEngine dependency on GoalStateHistory. The read time difference is minimal (< 0.01 ms), but if you think it's still an issue, I can move it back to PolicyEngine.

@narrieta @mgunnala what do you think of keeping the contents of the policy file when it's read as a property in ExtensionPolicyEngine. Then we can dump the contents to the history folder here in exthandlers.

If we keep it as is, it will be unlikely that we save a different policy to the history folder than what is being applied, but if it does happen it will be difficult to debug

Exposing the raw policy file contents as a property in ExtensionPolicyEngine makes sense to me

Updated the implementation - ExtensionPolicyEngine stores raw policy file contents in "policy_file_contents" property immediately after reading, it is copied to history here.

maddieford · 2025-02-28T19:02:06Z

azurelinuxagent/ga/exthandlers.py

-                            handler_status.message = parse_formatted_message(heartbeat.get('formattedMessage'))
+                            heartbeat_message = parse_formatted_message(heartbeat.get('formattedMessage'))
+                            if ext_disallowed:
+                                handler_status.message += " Extension was previously enabled and reported the following heartbeat:\n{0}".format(heartbeat_message)


handler_status.message is set to None in ExtHandlerStatus init. I don't think handler_status.message will be None since we write status on behalf of the Handler when it's disallowed, but we should probably add None check here before concatenating in case

If handler_status.message is None here, what should it be set to?

report_ext_handler_status() doesn't have the exact cause of the ext disallowed error (policy failure, ext disabled, etc.) - we could set the message to something like "Extension will not be processed because it is disallowed. Extension was previously enabled ... " but that might be confusing

I think we can update the check to 'if ext_disallowed and handler_status.message is not None'

If ext is disallowed and the handler_status.message is None then we've hit an unexpected case and should just write the heartbeat to the status imo.

We can add a comment that handler_status.message should not be None if the ext is disallowed (because the agent writes handler status on behalf of the ext in that case)

if ext_disallowed and handler_status.message is not None:
We should not just pass the heartbeat, but rather add a generic message explaining the extension was not execute. Also, since this is an unexpected condition, we should probably send telemetry to alert us of the issue. If sending telemetry, be careful not send on every single heartbeat

(feel free to skip sending telemetry, as long was we have a unit test asserting not None when disallowed; it may happen that a faulty extension sets the status to None)

We do have unit tests asserting that status is appropriately reported (not None) when extension is disallowed.

Is it sufficient to add a message like "Extension was not processed: failed to report status." ?

when not None, we use

Status + Extension was previously enabled and reported the following heartbeat:\n{0}

how about

Extension was not executed, but it was previously enabled and reported the following heartbeat:\n{0}

That sounds good to me

mgunnala added 3 commits February 19, 2025 12:16

Overwrite heartbeat and add UT

61be2d5

Add policy to history folder

b53cb9a

Save policy to history in policy_engine.py

03e3d04

mgunnala requested review from narrieta, ZhidongPeng, nagworld9 and maddieford as code owners February 20, 2025 16:48

mgunnala and others added 3 commits February 20, 2025 11:51

Merge branch 'develop' into overwrite_heartbeat

ab4a63d

Fix UT failures

5940f96

Add UT

e360e63

narrieta reviewed Feb 24, 2025

View reviewed changes

mgunnala added 2 commits February 27, 2025 12:24

Address review comments

0020d88

Skip AzureSecurityLinuxAgent on flatcar

ba7feb5

mgunnala commented Feb 27, 2025

View reviewed changes

azurelinuxagent/common/utils/archive.py Outdated Show resolved Hide resolved

narrieta reviewed Feb 28, 2025

View reviewed changes

maddieford reviewed Feb 28, 2025

View reviewed changes

mgunnala and others added 4 commits March 3, 2025 14:00

Address review comments 2

e48f6ae

Cleanup whitespace

ded0fbd

Update handler status message

6d781cf

Merge branch 'develop' into overwrite_heartbeat

91fd5e9

narrieta previously approved these changes Mar 4, 2025

View reviewed changes

Address comment

dc7e487

mgunnala dismissed narrieta’s stale review via dc7e487 March 4, 2025 18:06

maddieford approved these changes Mar 4, 2025

View reviewed changes

narrieta approved these changes Mar 4, 2025

View reviewed changes

narrieta merged commit e2caad2 into Azure:develop Mar 4, 2025
11 checks passed

mgunnala deleted the overwrite_heartbeat branch March 4, 2025 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overwrite extension heartbeat with policy error #3321

Overwrite extension heartbeat with policy error #3321

mgunnala commented Feb 20, 2025

narrieta Feb 24, 2025

mgunnala Feb 27, 2025

narrieta Feb 24, 2025

mgunnala Feb 27, 2025

narrieta Feb 24, 2025

mgunnala Feb 27, 2025

narrieta Feb 24, 2025

mgunnala Feb 25, 2025

mgunnala Feb 27, 2025

narrieta Feb 28, 2025

narrieta Feb 28, 2025

mgunnala Mar 3, 2025

narrieta Feb 28, 2025

mgunnala Mar 3, 2025

narrieta Feb 28, 2025

mgunnala Mar 3, 2025 •

edited

Loading

mgunnala Mar 3, 2025

maddieford Feb 28, 2025

mgunnala Mar 3, 2025

maddieford Mar 3, 2025 •

edited

Loading

mgunnala Mar 3, 2025

mgunnala Mar 3, 2025

maddieford Feb 28, 2025

mgunnala Mar 3, 2025 •

edited

Loading

maddieford Mar 3, 2025

mgunnala Mar 3, 2025

narrieta Mar 3, 2025

narrieta Mar 3, 2025

mgunnala Mar 3, 2025

narrieta Mar 3, 2025

mgunnala Mar 3, 2025

mgunnala Mar 3, 2025

Overwrite extension heartbeat with policy error #3321

Overwrite extension heartbeat with policy error #3321

Conversation

mgunnala commented Feb 20, 2025

Description

PR information

Quality of Code and Contribution Guidelines

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgunnala Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddieford Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgunnala Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgunnala Mar 3, 2025 •

edited

Loading

maddieford Mar 3, 2025 •

edited

Loading

mgunnala Mar 3, 2025 •

edited

Loading