-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block extensions disallowed by policy #3259
base: develop
Are you sure you want to change the base?
Conversation
b440696
to
a37508f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an initial review not including tests
""" | ||
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the error message as discussed above.
azurelinuxagent/ga/exthandlers.py
Outdated
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this create .status files for single config extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new function __handle_and_report_policy_error() - this should create a status file for any extension with settings.
azurelinuxagent/ga/exthandlers.py
Outdated
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question here about .status file for single config extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new function __handle_and_report_policy_error() - this should create a status file for any extension with settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. I'm going to spend tomorrow going through each e2e scenario and unit test, sorry for the slow review :/
azurelinuxagent/ga/exthandlers.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
azurelinuxagent/ga/exthandlers.py
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
|
||
# 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create_status_file_if_not_exist() will not overwrite existing status file (for the current sequence number). Is this behavior acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should overwrite the existing file with the policy error
azurelinuxagent/ga/exthandlers.py
Outdated
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a test case for extension is allowed by policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
tests_e2e/test_suites/ext_policy.yml
Outdated
name: "ExtensionPolicy" | ||
tests: | ||
- "ext_policy/ext_policy.py" | ||
images: "random(endorsed)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should run this on more distros so we can get better coverage before releasing the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will running on all endorsed distros add too much overhead to the daily runs?
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)) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also check for [ExtensionPolicyError] in the message to confirm the failure was due to policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also please add a case for multi config ext allowed by policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
name: "ExtPolicyWithDependencies" | ||
tests: | ||
- "ext_policy/ext_policy_with_dependencies.py" | ||
images: "random(endorsed)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to all endorsed, but I can change to 5-10 if this adds too much overhead.
e909568
to
86de0c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting comments for Agent code.
Will post comments for test code separately.
""" | ||
Error raised during agent extension policy enforcement. | ||
""" | ||
def __init__(self, msg, code, inner=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'code' and 'inner' parameters are not in the same order as in the base class, which can lead to subtle coding errors.
# Invoke policy engine to determine if extension is allowed. If not, block extension and report error on | ||
# behalf of the extension. | ||
policy_err_map = { | ||
ExtensionRequestedState.Enabled: ('enable', ExtensionErrorCodes.PluginEnableProcessingFailed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a comment describing the elements in the tuple?
for extension, ext_handler in all_extensions: | ||
|
||
handler_i = ExtHandlerInstance(ext_handler, self.protocol, extension=extension) | ||
|
||
# Invoke policy engine to determine if extension is allowed. If not, block extension and report error on | ||
# behalf of the extension. | ||
policy_err_map = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this is a constant... define it at the class level?
# Invoke policy engine to determine if extension is allowed. If not, block extension and report error on | ||
# behalf of the extension. | ||
policy_err_map = { | ||
ExtensionRequestedState.Enabled: ('enable', ExtensionErrorCodes.PluginEnableProcessingFailed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'enable' and 'disable' are internal CRP/Agent operations; users are not aware of them. They should not be propagated to error messages displayed to the user
} | ||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the intention of creating an exception object here? seems like it is only used to pass the error code, but it is never raised
err = ExtensionPolicyError(msg, code=policy_err_code) | ||
self.__handle_and_report_policy_error(handler_i, err, report_op=handler_i.operation, message=ustr(err), | ||
extension=extension, report=True) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we are missing a continue statement here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think continue statement would break the dependency logic.
It's ok to use continue in the other condition because we know all extensions will fail (dependencies don't matter)
@@ -482,10 +483,50 @@ def handle_ext_handlers(self, goal_state_id): | |||
depends_on_err_msg = None | |||
extensions_enabled = conf.get_extensions_enabled() | |||
|
|||
# Instantiate policy engine, and use same engine to handle all extension handlers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check for 'if not extensions_enabled:' should be done before the checks for policy
|
||
# 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should overwrite the existing file with the policy error
ext_handler_i.create_status_file_if_not_exist(extension, status=ExtensionStatusValue.error, code=error.code, | ||
operation=report_op, message=message) | ||
|
||
if report: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would report be False?
@@ -990,7 +1061,10 @@ def report_ext_handler_status(self, vm_status, ext_handler, goal_state_changed): | |||
# extension even if HandlerState == NotInstalled (Sample scenario: ExtensionsGoalStateError, DecideVersionError, etc) | |||
# We also need to report extension status for an uninstalled handler if extensions are disabled because CRP | |||
# waits for extension runtime status before failing the extension operation. | |||
if handler_state != ExtHandlerState.NotInstalled or ext_handler.supports_multi_config or not conf.get_extensions_enabled(): | |||
# In the case of policy failures, we want to report extension status with a terminal code so CRP fails fast. If |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this
# We also need to report extension status for an uninstalled handler if extensions are disabled because CRP
# waits for extension runtime status before failing the extension operation.
# In the case of policy failures, we want to report extension status with a terminal code so CRP fails fast. If
# extension status is not present, collect_ext_status() will set a default transitioning status, and CRP will
# wait for timeout.
to
# We also need to report extension status for an uninstalled handler if extensions are disabled, or if the extension
# failed due to policy, because CRP waits for extension runtime status before failing the extension operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention of the change is to enter this condition when the extension fails due to policy, but this change means that we enter the condition whenever policy is enabled.
Is there any negative effect to calling ext_handler_i.get_extension_handler_statuses...
whenever policy is enabled? Why is this behind the if condition in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comments mainly for e2e tests. I'll review unit tests once the comments in exthandlers.py are resolved
|
||
# Only allowlisted extensions should be processed. | ||
# We only allowlist CustomScript: CustomScript should be enabled, RunCommand and AzureMonitor should fail. | ||
# (Note that CustomScript blocked by policy is tested in a later test case.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding comments to the review so I can follow the scenarios easier. Consider adding these as comments in the code, but ultimately up to you:
This policy tests the following scenarios:
- single config ext (CSE) enable operation succeeds when allowed by policy
- no-config ext (AzureMonitor) enable operation fails fast when disallowed by policy
- single multi-config instance (RunCommandHandler) enable operation fails fast when disallowed by policy
|
||
# When allowlist is turned off, all extensions should be processed. | ||
# RunCommand and AzureMonitorLinuxAgent should be successfully enabled and then deleted. | ||
policy = \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This policy tests the following scenarios:
- single multi-config instance (RunCommandHandler) enable operation succeeds when allowed by policy
- single multi-config instance (RunCommandHandler) delete operation succeeds when allowed by policy
- no-config ext (AzureMonitor) enable operation succeeds when allowed by policy
- no-config ext (AzureMonitor) delete operation succeeds when allowed by policy
self._operation_should_succeed("delete", azure_monitor) | ||
|
||
# Should not uninstall disallowed extensions. | ||
# CustomScript is removed from the allowlist: delete operation should fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This policy tests the following scenarios:
- a delete operation on a previously enabled single-config ext (CSE) which is now disallowed by policy fails fast
- multiple multi-config instances (RunCommandHandler and RunCommandHandler2) enable operations fail fast when disallowed by policy
- single-config ext (CSE) enable operation fails fast when disallowed by policy
# If single-config extension is initially blocked, and policy is updated to allow it, extension should be | ||
# successfully enabled and report status correctly. | ||
self._operation_should_fail("enable", custom_script) | ||
policy = \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This policy tests the following scenarios:
- enable operation on a previously disallowed single-config ext (CSE) succeeds after allowing ext by policy
log.info("CRP returned an error for deletion operation, may be a false error. Checking agent log to determine if operation succeeded. Exception: {0}".format(crp_err)) | ||
try: | ||
for ssh_client in ssh_clients.values(): | ||
msg = ("Remove the extension slice: {0}".format(str(ext_to_delete))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message is related to cgroup. Right now it's logged even when cgroup isn't enabled (which might and probably should change in the future).
Instead, we should check that the handler was successfully uninstalled. i.e. the last ext status reported by the agent shouldn't include the handler:
2024-11-26T23:54:04.306568Z INFO ExtHandler ExtHandler Extension status: [('Microsoft.Azure.Monitor.AzureMonitorLinuxAgent', 'Ready')]
You might also consider doing this by checking the instance view
_test_cases = [ | ||
_should_fail_single_config_depends_on_disallowed_no_config, | ||
_should_fail_single_config_depends_on_disallowed_single_config, | ||
# TODO: RunCommand is unable to be installed properly, so these tests are currently disabled. Investigate the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's specify 'RunCommandHandler' since RunCommand is a different extension (confusing, I know :)
Also is it that RunCommandHandler is unable to be "installed properly" or uninstalled?
_should_fail_single_config_depends_on_disallowed_single_config, | ||
# TODO: RunCommand is unable to be installed properly, so these tests are currently disabled. Investigate the | ||
# issue and enable these 3 tests. | ||
# _should_fail_single_config_depends_on_disallowed_multi_config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about _should_fail_multi_config_depends_on_disallowed_multi_config?
RunCommandHandler1 depends on RunCommandHandler2 for example
return policy, template, expected_errors, deletion_order | ||
|
||
|
||
def _should_no_dependencies(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this may be leftover code, I don't see it referenced
from pathlib import Path | ||
from tests_e2e.tests.lib.agent_log import AgentLog | ||
|
||
|
||
def main(): | ||
parser = argparse.ArgumentParser() | ||
parser.add_argument("--data", dest='data', required=True) | ||
parser.add_argument("--after-timestamp", dest='after_timestamp', required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :) thanks for adding this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for test code
|
||
def _test_policy_case(self, policy, op, expected_status_code, expected_handler_status, expected_ext_count=1, | ||
expected_status_msg=None): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add some comments explaining the setup done by this function? (e.g. why incarnation 2?) thanks
@@ -630,6 +630,98 @@ 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_extensions_disallowed_by_policy_properly(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "properly" mean? (what is the expected behavior?)
def test_it_should_handle_and_report_extensions_disallowed_by_policy_properly(self): | ||
"""If multiconfig extension is disallowed by policy, all instances should be blocked.""" | ||
policy_path = os.path.join(self.tmp_dir, "waagent_policy.json") | ||
patch('azurelinuxagent.common.conf.get_policy_file_path', return_value=str(policy_path)).start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be done using the 'with' statement
- "ext_policy/ext_policy_with_dependencies.py" | ||
images: "endorsed" | ||
executes_on_scale_set: true | ||
# This test should run on its own VMSS, because other tests may leave behind extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should handle this in the test and allow it to share the vm
self._ssh_client.copy_to_node(local_path=local_path, remote_path=remote_path) | ||
policy_file_final_dest = "/etc/waagent_policy.json" | ||
log.info("Copying policy file to test VM [%s]", self._context.vm.name) | ||
self._ssh_client.run_command(f"mv {remote_path} {policy_file_final_dest}", use_sudo=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why create and then rename?
|
||
# RunCommandHandler is a multi-config extension, so we set up two instances (configurations) here and test both. | ||
run_command = ExtPolicy.TestCase( | ||
VirtualMachineExtensionClient(self._context.vm, VmExtensionIds.RunCommandHandler, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use VirtualMachineRunCommand instead of VirtualMachineExtensionClient
self._create_policy_file(policy) | ||
self._operation_should_succeed("enable", custom_script) | ||
self._operation_should_fail("enable", run_command) | ||
if VmExtensionIds.AzureMonitorLinuxAgent.supports_distro((self._ssh_client.run_command("get_distro.py").rstrip())): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how much coverage are we getting for this case?
if VmExtensionIds.AzureMonitorLinuxAgent.supports_distro((self._ssh_client.run_command("get_distro.py").rstrip())): | ||
self._operation_should_fail("enable", azure_monitor) | ||
|
||
# When allowlist is turned off, all extensions should be processed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about CustomScript?
self._operation_should_succeed("enable", azure_monitor) | ||
self._operation_should_succeed("delete", azure_monitor) | ||
|
||
# Should not uninstall disallowed extensions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like
# Only allowlisted extensions should be processed.
and
# Should not uninstall disallowed extensions.
or are we trying to test something different?
} | ||
} | ||
self._create_policy_file(policy) | ||
# # Known CRP issue - delete/uninstall operation times out instead of reporting an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a CRP issue, but rather a design issue. Uninstall is best effort and never fails. You should consider checking the agent log for the error and then the instance view to confirm the extension was not uninstalled
Description
Issue #
PR #2 for the policy engine allowlist feature:
PR information
Quality of Code and Contribution Guidelines