Block extensions disallowed by policy #3259

mgunnala · 2024-11-11T20:43:38Z

Description

Issue #

PR #2 for the policy engine allowlist feature:

invoke policy engine from exthandlers.py
block all extensions and report status if any errors are thrown during engine initialization
block any extensions that are disallowed by policy
add unit and e2e tests

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.
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.

azurelinuxagent/ga/policy/policy_engine.py

codecov · 2024-11-13T00:57:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.77%. Comparing base (3aebcdd) to head (a37508f).
Report is 328 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3259      +/-   ##
===========================================
+ Coverage    71.97%   72.77%   +0.79%     
===========================================
  Files          103      114      +11     
  Lines        15692    17081    +1389     
  Branches      2486     2277     -209     
===========================================
+ Hits         11295    12431    +1136     
- Misses        3881     4107     +226     
- Partials       516      543      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

maddieford

Did an initial review not including tests

azurelinuxagent/ga/policy/policy_engine.py

maddieford · 2024-11-14T22:12:25Z

azurelinuxagent/ga/policy/policy_engine.py

+    """
+    # TODO: when CRP adds terminal error code for policy-related extension failures, set that as the default code.
+    def __init__(self, msg, inner=None, code=-1):
+        msg = "Extension is disallowed by agent policy and will not be processed: {0}".format(msg)


In case where agent failed to parse policy, I'm not sure we should say 'Extension is disallowed by policy'. In this case, extension is disallowed because there's some issue reading or parsing the policy.

I also am hesitant about 'agent policy' since policy is provided by customer

I could change this to "Extension will not be processed: "

Parsing errors (InvalidPolicyError) would look like "Extension will not be processed: customer-provided policy file (path) is invalid, please correct the following error..."

Extension disallowed errors (ExtensionPolicyError) would look like "Extension will not be processed: failed to enable extension CustomScript because extension is not specified in policy allowlist. To enable, add extension to the allowed list in policy file (path)."

I've updated the error message as discussed above.

maddieford · 2024-11-14T22:32:44Z

azurelinuxagent/ga/exthandlers.py

+            policy_op, policy_err_code = policy_err_map.get(ext_handler.state)
+            if policy_error is not None:
+                err = ExtensionPolicyError(msg="", inner=policy_error, code=policy_err_code)
+                self.__handle_and_report_ext_handler_errors(handler_i, err,


Does this create .status files for single config extensions?

I added a new function __handle_and_report_policy_error() - this should create a status file for any extension with settings.

maddieford · 2024-11-14T22:36:07Z

azurelinuxagent/ga/exthandlers.py

+                                                                                             ext_handler.name,
+                                                                                             conf.get_policy_file_path())
+                err = ExtensionPolicyError(msg, code=policy_err_code)
+                self.__handle_and_report_ext_handler_errors(handler_i, err,


same question here about .status file for single config extensions

I added a new function __handle_and_report_policy_error() - this should create a status file for any extension with settings.

maddieford

Changes look good. I'm going to spend tomorrow going through each e2e scenario and unit test, sorry for the slow review :/

maddieford · 2024-11-20T00:15:39Z

azurelinuxagent/ga/exthandlers.py

+                ExtensionRequestedState.Enabled: ('enable', ExtensionErrorCodes.PluginEnableProcessingFailed),
+                # TODO: CRP does not currently have a terminal error code for uninstall. Once CRP adds
+                # an error code for uninstall or for policy, use this code instead of PluginDisableProcessingFailed
+                # Note that currently, CRP waits for 90 minutes to time out for a failed uninstall operation, instead of


Could we add some more detail to this comment?
Something like:

Note that currently, CRP will poll until the agent does not report a status for an extension that should be uninstalled. In the case of a policy error, the agent will report a failed status on behalf of the extension, which will cause CRP to poll for the full timeout period, instead of failing fast.

maddieford · 2024-11-20T00:21:21Z

azurelinuxagent/ga/exthandlers.py

@@ -692,6 +734,26 @@ def __handle_and_report_ext_handler_errors(ext_handler_i, error, report_op, mess
            add_event(name=name, version=handler_version, op=report_op, is_success=False, log_event=True,
                      message=message)

+    @staticmethod
+    def __handle_and_report_policy_error(ext_handler_i, error, report_op, message, report=True, extension=None):
+        # TODO: Consider merging this function with __handle_and_report_ext_handler_errors() above.


Could you please leave some comment explaining why we broke this into a separate function? For policy related failures, we want to fail extensions fast. CRP will continue to poll for single-config ext status until timeout, so agent should write a status for single-config extensions. The other function does not create that status and we didn't want to touch the other function without investigating the impact of that change further

mgunnala · 2024-11-20T17:54:19Z

azurelinuxagent/ga/exthandlers.py

+
+        # Create status file for extensions with settings (single and multi config).
+        if extension is not None:
+            ext_handler_i.create_status_file_if_not_exist(extension, status=ExtensionStatusValue.error, code=error.code,


create_status_file_if_not_exist() will not overwrite existing status file (for the current sequence number). Is this behavior acceptable?

maddieford · 2024-11-21T02:26:07Z

azurelinuxagent/ga/exthandlers.py

+                ExtensionRequestedState.Enabled: ('enable', ExtensionErrorCodes.PluginEnableProcessingFailed),
+                # Note: currently, when uninstall is requested for an extension, CRP polls until the agent does not
+                # report status for that extension, or until timeout is reached. In the case of a policy error, the
+                # agent reports failed status on behalf of the extension, which will cause CRP to  for the full timeout,


Suggested change

# agent reports failed status on behalf of the extension, which will cause CRP to for the full timeout,

# agent reports failed status on behalf of the extension, which will cause CRP to poll for the full timeout,

nit

Fixed, thanks!

maddieford · 2024-11-21T02:55:54Z

tests/ga/test_extension.py

@@ -3507,5 +3510,144 @@ def test_report_msg_if_handler_manifest_contains_invalid_values(self):
                self.assertIn("'supportsMultipleExtensions' has a non-boolean value", kw_messages[2]['message'])


+class TestExtensionPolicy(TestExtensionBase):


could we add a test case for extension is allowed by policy

maddieford · 2024-11-21T17:51:30Z

tests_e2e/test_suites/ext_policy.yml

+name: "ExtensionPolicy"
+tests:
+  - "ext_policy/ext_policy.py"
+images: "random(endorsed)"


I think we should run this on more distros so we can get better coverage before releasing the changes

will running on all endorsed distros add too much overhead to the daily runs?

maddieford · 2024-11-21T17:58:59Z

tests_e2e/tests/ext_policy/ext_policy.py

+        # Prepare extensions to test
+        unique = str(uuid.uuid4())
+        test_file = f"waagent-test.{unique}"
+        custom_script = ExtPolicy.TestCase(


Could you please add a comment explaining why you chose each of these extensions (single-config, multi-config, etc), so that it is obvious to anyone who reviews this test case in the future

maddieford · 2024-11-21T18:38:34Z

tests_e2e/tests/ext_policy/ext_policy.py

+            fail(f"The agent should have reported an error trying to {operation} {extension_case.extension.__str__()} "
+                 f"because the extension is disallowed by policy.")
+        except Exception as error:
+            assert_that("Extension will not be processed" in str(error)) \


Could we also check for [ExtensionPolicyError] in the message to confirm the failure was due to policy

azurelinuxagent/ga/exthandlers.py

maddieford · 2024-11-21T18:58:32Z

tests/ga/test_multi_config_extension.py

@@ -630,6 +630,70 @@ def test_it_should_handle_and_report_enable_errors_properly(self):
                }
                self._assert_extension_status(sc_handler, expected_extensions)

+    def test_it_should_handle_and_report_disallowed_extensions_properly(self):


Could you also please add a case for multi config ext allowed by policy

maddieford · 2024-11-21T18:58:56Z

tests_e2e/test_suites/ext_policy_with_dependencies.yml

+name: "ExtPolicyWithDependencies"
+tests:
+  - "ext_policy/ext_policy_with_dependencies.py"
+images: "random(endorsed)"


same comment here, we should get more coverage than 1 run per day, maybe consider running on all endorsed, or 5-10 endorsed images per day

Updated to all endorsed, but I can change to 5-10 if this adds too much overhead.

maddieford · 2024-11-21T18:59:37Z

tests_e2e/tests/ext_policy/ext_policy.py

+                }
+            }
+        self._create_policy_file(policy)
+        self._operation_should_succeed("enable", custom_script)


Could we also add the test case where single-config ext should fail due to policy

Is the test case on line 189 sufficient, or did you mean something else?
(I will also add comments to make it clearer that this case is being tested)

mgunnala added 3 commits November 8, 2024 16:22

Block disallowed extension processing

c2cc2c6

Enable policy e2e tests

151081d

Pylint

edec2af

mgunnala requested review from narrieta, ZhidongPeng, nagworld9 and maddieford as code owners November 11, 2024 20:43

mgunnala commented Nov 12, 2024

View reviewed changes

azurelinuxagent/ga/policy/policy_engine.py Outdated Show resolved Hide resolved

Fix e2e test failures

a37508f

mgunnala force-pushed the allowlist_2 branch from b440696 to a37508f Compare November 12, 2024 18:52

maddieford reviewed Nov 14, 2024

View reviewed changes

mgunnala and others added 2 commits November 18, 2024 12:19

Address review comments

b0da554

Merge branch 'develop' into allowlist_2

a4f5cab

maddieford reviewed Nov 20, 2024

View reviewed changes

Address review comments

699b9ba

mgunnala commented Nov 20, 2024

View reviewed changes

maddieford reviewed Nov 21, 2024

View reviewed changes

Address test review comments

86de0c5

mgunnala force-pushed the allowlist_2 branch from e909568 to 86de0c5 Compare November 21, 2024 21:41

mgunnala added 4 commits November 22, 2024 13:08

Remove status file for single-config

c3e9b89

Add back status file for single-config

65d7034

Run e2e tests on all endorsed

95f247a

Fix UT failures

3b18519

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block extensions disallowed by policy #3259

Block extensions disallowed by policy #3259

mgunnala commented Nov 11, 2024

codecov bot commented Nov 13, 2024

maddieford left a comment

maddieford Nov 14, 2024

mgunnala Nov 15, 2024 •

edited

Loading

mgunnala Nov 18, 2024

maddieford Nov 14, 2024

mgunnala Nov 18, 2024

maddieford Nov 14, 2024

mgunnala Nov 18, 2024

maddieford left a comment

maddieford Nov 20, 2024

mgunnala Nov 20, 2024

maddieford Nov 20, 2024

mgunnala Nov 20, 2024

mgunnala Nov 20, 2024

maddieford Nov 21, 2024

mgunnala Nov 22, 2024

maddieford Nov 21, 2024

mgunnala Nov 23, 2024

maddieford Nov 21, 2024

mgunnala Nov 21, 2024

maddieford Nov 21, 2024

mgunnala Nov 22, 2024

maddieford Nov 21, 2024

mgunnala Nov 22, 2024

maddieford Nov 21, 2024

mgunnala Nov 22, 2024

maddieford Nov 21, 2024

mgunnala Nov 22, 2024

maddieford Nov 21, 2024

mgunnala Nov 21, 2024 •

edited

Loading

	# agent reports failed status on behalf of the extension, which will cause CRP to for the full timeout,
	# agent reports failed status on behalf of the extension, which will cause CRP to poll for the full timeout,

		@@ -3507,5 +3510,144 @@ def test_report_msg_if_handler_manifest_contains_invalid_values(self):
		self.assertIn("'supportsMultipleExtensions' has a non-boolean value", kw_messages[2]['message'])


		class TestExtensionPolicy(TestExtensionBase):

Block extensions disallowed by policy #3259

Are you sure you want to change the base?

Block extensions disallowed by policy #3259

Conversation

mgunnala commented Nov 11, 2024

Description

PR information

Quality of Code and Contribution Guidelines

codecov bot commented Nov 13, 2024

Codecov Report

maddieford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgunnala Nov 15, 2024 • 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

maddieford left a 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

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 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

mgunnala Nov 15, 2024 •

edited

Loading

mgunnala Nov 21, 2024 •

edited

Loading