Skip to content

Commit 47dc38c

Browse files
carlotaarvelacarlotaarvela
committed
PR comment - refactoring
1 parent 2fbe63f commit 47dc38c

File tree

2 files changed

+118
-94
lines changed

2 files changed

+118
-94
lines changed

‎src/aks-preview/azext_aks_preview/managed_cluster_decorator.py‎

Lines changed: 100 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,21 @@
167167
ResourceReference = TypeVar("ResourceReference")
168168

169169

170+
def _get_monitoring_addon_key(cluster, addon_consts):
171+
"""Resolve the monitoring addon key from the cluster's addon_profiles.
172+
173+
The API response may return the addon key as either "omsagent" or "omsAgent".
174+
Returns the key present in addon_profiles, or the default constant if neither is found.
175+
"""
176+
const_monitoring = addon_consts.get("CONST_MONITORING_ADDON_NAME")
177+
if cluster.addon_profiles:
178+
if const_monitoring in cluster.addon_profiles:
179+
return const_monitoring
180+
if CONST_MONITORING_ADDON_NAME_CAMELCASE in cluster.addon_profiles:
181+
return CONST_MONITORING_ADDON_NAME_CAMELCASE
182+
return const_monitoring
183+
184+
170185
# pylint: disable=too-few-public-methods
171186
class AKSPreviewManagedClusterModels(AKSManagedClusterModels):
172187
"""Store the models used in aks series of commands.
@@ -5102,113 +5117,111 @@ def _should_create_dcra(self) -> bool:
51025117
params = self.context.raw_param
51035118
return (
51045119
params.get("enable_addons") is not None or
5105-
params.get("enable-azure-monitor-logs") is not None or
5120+
params.get("enable_azure_monitor_logs") is not None or
5121+
self._is_cnl_or_hlsm_changing()
5122+
)
5123+
5124+
def _is_cnl_or_hlsm_changing(self) -> bool:
5125+
"""Return True if any CNL or High Log Scale Mode flag was provided."""
5126+
params = self.context.raw_param
5127+
return (
51065128
params.get("enable_container_network_logs") is not None or
51075129
params.get("enable_retina_flow_logs") is not None or
51085130
params.get("disable_container_network_logs") is not None or
51095131
params.get("disable_retina_flow_logs") is not None or
51105132
params.get("enable_high_log_scale_mode") is not None
51115133
)
51125134

5113-
# pylint: disable=too-many-locals,too-many-branches
5114-
def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None:
5115-
"""Postprocessing performed after the cluster is created.
5135+
def _postprocess_monitoring_enable(self, cluster: ManagedCluster) -> None:
5136+
"""Handle monitoring addon postprocessing for the enable case."""
5137+
enable_msi_auth_for_monitoring = self.context.get_enable_msi_auth_for_monitoring()
5138+
if not enable_msi_auth_for_monitoring:
5139+
# add cluster spn/msi Monitoring Metrics Publisher role assignment to publish metrics to MDM
5140+
# mdm metrics is supported only in azure public cloud, so add the role assignment only in this cloud
5141+
cloud_name = self.cmd.cli_ctx.cloud.name
5142+
if cloud_name.lower() == "azurecloud":
5143+
cluster_resource_id = resource_id(
5144+
subscription=self.context.get_subscription_id(),
5145+
resource_group=self.context.get_resource_group_name(),
5146+
namespace="Microsoft.ContainerService",
5147+
type="managedClusters",
5148+
name=self.context.get_name(),
5149+
)
5150+
self.context.external_functions.add_monitoring_role_assignment(
5151+
cluster, cluster_resource_id, self.cmd
5152+
)
5153+
elif self._should_create_dcra():
5154+
addon_consts = self.context.get_addon_consts()
5155+
monitoring_addon_key = _get_monitoring_addon_key(cluster, addon_consts)
5156+
self.context.external_functions.ensure_container_insights_for_monitoring(
5157+
self.cmd,
5158+
cluster.addon_profiles[monitoring_addon_key],
5159+
self.context.get_subscription_id(),
5160+
self.context.get_resource_group_name(),
5161+
self.context.get_name(),
5162+
self.context.get_location(),
5163+
remove_monitoring=False,
5164+
aad_route=self.context.get_enable_msi_auth_for_monitoring(),
5165+
create_dcr=self._is_cnl_or_hlsm_changing(),
5166+
create_dcra=True,
5167+
enable_syslog=self.context.get_enable_syslog(),
5168+
data_collection_settings=self.context.get_data_collection_settings(),
5169+
is_private_cluster=self.context.get_enable_private_cluster(),
5170+
ampls_resource_id=self.context.get_ampls_resource_id(),
5171+
enable_high_log_scale_mode=self.context.get_enable_high_log_scale_mode(),
5172+
)
51165173

5117-
:return: None
5118-
"""
5119-
# monitoring addon
5120-
monitoring_addon_enabled = self.context.get_intermediate("monitoring_addon_enabled", default_value=False)
5121-
if monitoring_addon_enabled:
5122-
enable_msi_auth_for_monitoring = self.context.get_enable_msi_auth_for_monitoring()
5123-
if not enable_msi_auth_for_monitoring:
5124-
# add cluster spn/msi Monitoring Metrics Publisher role assignment to publish metrics to MDM
5125-
# mdm metrics is supported only in azure public cloud, so add the role assignment only in this cloud
5126-
cloud_name = self.cmd.cli_ctx.cloud.name
5127-
if cloud_name.lower() == "azurecloud":
5128-
cluster_resource_id = resource_id(
5129-
subscription=self.context.get_subscription_id(),
5130-
resource_group=self.context.get_resource_group_name(),
5131-
namespace="Microsoft.ContainerService",
5132-
type="managedClusters",
5133-
name=self.context.get_name(),
5134-
)
5135-
self.context.external_functions.add_monitoring_role_assignment(
5136-
cluster, cluster_resource_id, self.cmd
5137-
)
5138-
elif self._should_create_dcra():
5139-
# Create/update the DCR when CNL or HLSM flags change so that the DCR streams
5140-
# (e.g. Microsoft-ContainerLogV2-HighScale) are kept in sync.
5141-
cnl_or_hlsm_changing = (
5142-
self.context.raw_param.get("enable_container_network_logs") is not None or
5143-
self.context.raw_param.get("enable_retina_flow_logs") is not None or
5144-
self.context.raw_param.get("disable_container_network_logs") is not None or
5145-
self.context.raw_param.get("disable_retina_flow_logs") is not None or
5146-
self.context.raw_param.get("enable_high_log_scale_mode") is not None
5147-
)
5148-
addon_consts = self.context.get_addon_consts()
5149-
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
5150-
# The API response may return the addon key as either "omsagent" or "omsAgent"
5151-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME
5152-
if CONST_MONITORING_ADDON_NAME not in cluster.addon_profiles and \
5153-
CONST_MONITORING_ADDON_NAME_CAMELCASE in cluster.addon_profiles:
5154-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME_CAMELCASE
5174+
def _postprocess_monitoring_disable(self) -> None:
5175+
"""Handle monitoring addon postprocessing for the disable case."""
5176+
addon_consts = self.context.get_addon_consts()
5177+
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
5178+
5179+
# Get the current cluster state to check config before it was disabled
5180+
current_cluster = self.client.get(self.context.get_resource_group_name(), self.context.get_name())
5181+
5182+
if (current_cluster.addon_profiles and
5183+
CONST_MONITORING_ADDON_NAME in current_cluster.addon_profiles):
5184+
5185+
addon_profile = current_cluster.addon_profiles[CONST_MONITORING_ADDON_NAME]
5186+
5187+
try:
51555188
self.context.external_functions.ensure_container_insights_for_monitoring(
51565189
self.cmd,
5157-
cluster.addon_profiles[monitoring_addon_key],
5190+
addon_profile,
51585191
self.context.get_subscription_id(),
51595192
self.context.get_resource_group_name(),
51605193
self.context.get_name(),
51615194
self.context.get_location(),
5162-
remove_monitoring=False,
5163-
aad_route=self.context.get_enable_msi_auth_for_monitoring(),
5164-
create_dcr=cnl_or_hlsm_changing,
5195+
remove_monitoring=True,
5196+
aad_route=True,
5197+
create_dcr=False,
51655198
create_dcra=True,
5166-
enable_syslog=self.context.get_enable_syslog(),
5167-
data_collection_settings=self.context.get_data_collection_settings(),
5168-
is_private_cluster=self.context.get_enable_private_cluster(),
5169-
ampls_resource_id=self.context.get_ampls_resource_id(),
5170-
enable_high_log_scale_mode=self.context.get_enable_high_log_scale_mode(),
5199+
enable_syslog=False,
5200+
data_collection_settings=None,
5201+
ampls_resource_id=None,
5202+
enable_high_log_scale_mode=False
51715203
)
5204+
except TypeError:
5205+
pass
5206+
5207+
# pylint: disable=too-many-locals,too-many-branches
5208+
def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None:
5209+
"""Postprocessing performed after the cluster is created.
5210+
5211+
:return: None
5212+
"""
5213+
# monitoring addon
5214+
monitoring_addon_enabled = self.context.get_intermediate("monitoring_addon_enabled", default_value=False)
5215+
if monitoring_addon_enabled:
5216+
self._postprocess_monitoring_enable(cluster)
51725217

51735218
# Handle monitoring addon postprocessing (disable case) - same logic as aks_disable_addons
51745219
monitoring_addon_disable_postprocessing_required = self.context.get_intermediate(
51755220
"monitoring_addon_disable_postprocessing_required", default_value=False
51765221
)
51775222

51785223
if monitoring_addon_disable_postprocessing_required:
5179-
addon_consts = self.context.get_addon_consts()
5180-
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
5181-
5182-
# Get the current cluster state to check config before it was disabled
5183-
current_cluster = self.client.get(self.context.get_resource_group_name(), self.context.get_name())
5184-
5185-
if (current_cluster.addon_profiles and
5186-
CONST_MONITORING_ADDON_NAME in current_cluster.addon_profiles):
5187-
5188-
# Use the current cluster addon profile for cleanup
5189-
addon_profile = current_cluster.addon_profiles[CONST_MONITORING_ADDON_NAME]
5190-
5191-
# Call ensure_container_insights_for_monitoring with remove_monitoring=True (same as aks_disable_addons)
5192-
try:
5193-
self.context.external_functions.ensure_container_insights_for_monitoring(
5194-
self.cmd,
5195-
addon_profile,
5196-
self.context.get_subscription_id(),
5197-
self.context.get_resource_group_name(),
5198-
self.context.get_name(),
5199-
self.context.get_location(),
5200-
remove_monitoring=True,
5201-
aad_route=True,
5202-
create_dcr=False,
5203-
create_dcra=True,
5204-
enable_syslog=False,
5205-
data_collection_settings=None,
5206-
ampls_resource_id=None,
5207-
enable_high_log_scale_mode=False
5208-
)
5209-
except TypeError:
5210-
# Ignore TypeError just like aks_disable_addons does
5211-
pass
5224+
self._postprocess_monitoring_disable()
52125225

52135226
# ingress appgw addon
52145227
ingress_appgw_addon_enabled = self.context.get_intermediate("ingress_appgw_addon_enabled", default_value=False)
@@ -7804,18 +7817,12 @@ def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None:
78047817
)
78057818
if monitoring_addon_postprocessing_required:
78067819
addon_consts = self.context.get_addon_consts()
7807-
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
78087820
CONST_MONITORING_USING_AAD_MSI_AUTH = addon_consts.get("CONST_MONITORING_USING_AAD_MSI_AUTH")
78097821

7810-
# The API response may return the addon key as either "omsagent" or "omsAgent"
7811-
monitoring_addon_key = None
7812-
if cluster.addon_profiles:
7813-
if CONST_MONITORING_ADDON_NAME in cluster.addon_profiles:
7814-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME
7815-
elif CONST_MONITORING_ADDON_NAME_CAMELCASE in cluster.addon_profiles:
7816-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME_CAMELCASE
7822+
monitoring_addon_key = _get_monitoring_addon_key(cluster, addon_consts)
78177823

7818-
if (monitoring_addon_key and
7824+
if (cluster.addon_profiles and
7825+
monitoring_addon_key in cluster.addon_profiles and
78197826
cluster.addon_profiles[monitoring_addon_key].enabled):
78207827

78217828
# Check if MSI auth is enabled

‎src/aks-preview/azext_aks_preview/tests/latest/test_managed_cluster_decorator.py‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7914,7 +7914,7 @@ def test_postprocessing_create_dcr_false_when_only_enable_addons(self):
79147914
def test_postprocessing_ensure_container_insights_not_called_without_relevant_flags(self):
79157915
"""ensure_container_insights_for_monitoring is NOT called when no relevant flags are set.
79167916

7917-
When neither enable_addons, enable-azure-monitor-logs, nor any CNL/HLSM flag is
7917+
When neither enable_addons, enable_azure_monitor_logs, nor any CNL/HLSM flag is
79187918
present in raw_params, the outer elif is not entered.
79197919
"""
79207920
dec = self._make_postprocessing_decorator({})
@@ -8076,6 +8076,23 @@ def test_postprocessing_create_dcr_true_with_camelcase_addon_key(self):
80768076
_, kwargs = mock_ecifm.call_args
80778077
self.assertTrue(kwargs["create_dcr"])
80788078

8079+
def test_postprocessing_create_dcr_false_when_enable_azure_monitor_logs(self):
8080+
"""create_dcr=False when enable_azure_monitor_logs triggers ensure_container_insights_for_monitoring.
8081+
8082+
enable_azure_monitor_logs is in the outer _should_create_dcra condition but NOT in
8083+
_is_cnl_or_hlsm_changing, so create_dcr must be False.
8084+
"""
8085+
dec = self._make_postprocessing_decorator({"enable_azure_monitor_logs": True})
8086+
cluster = self._make_cluster_with_monitoring()
8087+
external_functions = dec.context.external_functions
8088+
with patch.object(
8089+
external_functions, "ensure_container_insights_for_monitoring", return_value=None
8090+
) as mock_ecifm:
8091+
dec.postprocessing_after_mc_created(cluster)
8092+
mock_ecifm.assert_called_once()
8093+
_, kwargs = mock_ecifm.call_args
8094+
self.assertFalse(kwargs["create_dcr"])
8095+
80798096

80808097
def test_set_up_health_monitor_profile(self):
80818098
# no flag - no change

0 commit comments

Comments
 (0)