Skip to content

Commit 837ccac

Browse files
carlotaarvelacarlotaarvela
committed
PR comment - refactoring
1 parent 706af4d commit 837ccac

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
@@ -171,6 +171,21 @@
171171
ResourceReference = TypeVar("ResourceReference")
172172

173173

174+
def _get_monitoring_addon_key(cluster, addon_consts):
175+
"""Resolve the monitoring addon key from the cluster's addon_profiles.
176+
177+
The API response may return the addon key as either "omsagent" or "omsAgent".
178+
Returns the key present in addon_profiles, or the default constant if neither is found.
179+
"""
180+
const_monitoring = addon_consts.get("CONST_MONITORING_ADDON_NAME")
181+
if cluster.addon_profiles:
182+
if const_monitoring in cluster.addon_profiles:
183+
return const_monitoring
184+
if CONST_MONITORING_ADDON_NAME_CAMELCASE in cluster.addon_profiles:
185+
return CONST_MONITORING_ADDON_NAME_CAMELCASE
186+
return const_monitoring
187+
188+
174189
# pylint: disable=too-few-public-methods
175190
class AKSPreviewManagedClusterModels(AKSManagedClusterModels):
176191
"""Store the models used in aks series of commands.
@@ -5191,113 +5206,111 @@ def _should_create_dcra(self) -> bool:
51915206
params = self.context.raw_param
51925207
return (
51935208
params.get("enable_addons") is not None or
5194-
params.get("enable-azure-monitor-logs") is not None or
5209+
params.get("enable_azure_monitor_logs") is not None or
5210+
self._is_cnl_or_hlsm_changing()
5211+
)
5212+
5213+
def _is_cnl_or_hlsm_changing(self) -> bool:
5214+
"""Return True if any CNL or High Log Scale Mode flag was provided."""
5215+
params = self.context.raw_param
5216+
return (
51955217
params.get("enable_container_network_logs") is not None or
51965218
params.get("enable_retina_flow_logs") is not None or
51975219
params.get("disable_container_network_logs") is not None or
51985220
params.get("disable_retina_flow_logs") is not None or
51995221
params.get("enable_high_log_scale_mode") is not None
52005222
)
52015223

5202-
# pylint: disable=too-many-locals,too-many-branches
5203-
def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None:
5204-
"""Postprocessing performed after the cluster is created.
5224+
def _postprocess_monitoring_enable(self, cluster: ManagedCluster) -> None:
5225+
"""Handle monitoring addon postprocessing for the enable case."""
5226+
enable_msi_auth_for_monitoring = self.context.get_enable_msi_auth_for_monitoring()
5227+
if not enable_msi_auth_for_monitoring:
5228+
# add cluster spn/msi Monitoring Metrics Publisher role assignment to publish metrics to MDM
5229+
# mdm metrics is supported only in azure public cloud, so add the role assignment only in this cloud
5230+
cloud_name = self.cmd.cli_ctx.cloud.name
5231+
if cloud_name.lower() == "azurecloud":
5232+
cluster_resource_id = resource_id(
5233+
subscription=self.context.get_subscription_id(),
5234+
resource_group=self.context.get_resource_group_name(),
5235+
namespace="Microsoft.ContainerService",
5236+
type="managedClusters",
5237+
name=self.context.get_name(),
5238+
)
5239+
self.context.external_functions.add_monitoring_role_assignment(
5240+
cluster, cluster_resource_id, self.cmd
5241+
)
5242+
elif self._should_create_dcra():
5243+
addon_consts = self.context.get_addon_consts()
5244+
monitoring_addon_key = _get_monitoring_addon_key(cluster, addon_consts)
5245+
self.context.external_functions.ensure_container_insights_for_monitoring(
5246+
self.cmd,
5247+
cluster.addon_profiles[monitoring_addon_key],
5248+
self.context.get_subscription_id(),
5249+
self.context.get_resource_group_name(),
5250+
self.context.get_name(),
5251+
self.context.get_location(),
5252+
remove_monitoring=False,
5253+
aad_route=self.context.get_enable_msi_auth_for_monitoring(),
5254+
create_dcr=self._is_cnl_or_hlsm_changing(),
5255+
create_dcra=True,
5256+
enable_syslog=self.context.get_enable_syslog(),
5257+
data_collection_settings=self.context.get_data_collection_settings(),
5258+
is_private_cluster=self.context.get_enable_private_cluster(),
5259+
ampls_resource_id=self.context.get_ampls_resource_id(),
5260+
enable_high_log_scale_mode=self.context.get_enable_high_log_scale_mode(),
5261+
)
52055262

5206-
:return: None
5207-
"""
5208-
# monitoring addon
5209-
monitoring_addon_enabled = self.context.get_intermediate("monitoring_addon_enabled", default_value=False)
5210-
if monitoring_addon_enabled:
5211-
enable_msi_auth_for_monitoring = self.context.get_enable_msi_auth_for_monitoring()
5212-
if not enable_msi_auth_for_monitoring:
5213-
# add cluster spn/msi Monitoring Metrics Publisher role assignment to publish metrics to MDM
5214-
# mdm metrics is supported only in azure public cloud, so add the role assignment only in this cloud
5215-
cloud_name = self.cmd.cli_ctx.cloud.name
5216-
if cloud_name.lower() == "azurecloud":
5217-
cluster_resource_id = resource_id(
5218-
subscription=self.context.get_subscription_id(),
5219-
resource_group=self.context.get_resource_group_name(),
5220-
namespace="Microsoft.ContainerService",
5221-
type="managedClusters",
5222-
name=self.context.get_name(),
5223-
)
5224-
self.context.external_functions.add_monitoring_role_assignment(
5225-
cluster, cluster_resource_id, self.cmd
5226-
)
5227-
elif self._should_create_dcra():
5228-
# Create/update the DCR when CNL or HLSM flags change so that the DCR streams
5229-
# (e.g. Microsoft-ContainerLogV2-HighScale) are kept in sync.
5230-
cnl_or_hlsm_changing = (
5231-
self.context.raw_param.get("enable_container_network_logs") is not None or
5232-
self.context.raw_param.get("enable_retina_flow_logs") is not None or
5233-
self.context.raw_param.get("disable_container_network_logs") is not None or
5234-
self.context.raw_param.get("disable_retina_flow_logs") is not None or
5235-
self.context.raw_param.get("enable_high_log_scale_mode") is not None
5236-
)
5237-
addon_consts = self.context.get_addon_consts()
5238-
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
5239-
# The API response may return the addon key as either "omsagent" or "omsAgent"
5240-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME
5241-
if CONST_MONITORING_ADDON_NAME not in cluster.addon_profiles and \
5242-
CONST_MONITORING_ADDON_NAME_CAMELCASE in cluster.addon_profiles:
5243-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME_CAMELCASE
5263+
def _postprocess_monitoring_disable(self) -> None:
5264+
"""Handle monitoring addon postprocessing for the disable case."""
5265+
addon_consts = self.context.get_addon_consts()
5266+
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
5267+
5268+
# Get the current cluster state to check config before it was disabled
5269+
current_cluster = self.client.get(self.context.get_resource_group_name(), self.context.get_name())
5270+
5271+
if (current_cluster.addon_profiles and
5272+
CONST_MONITORING_ADDON_NAME in current_cluster.addon_profiles):
5273+
5274+
addon_profile = current_cluster.addon_profiles[CONST_MONITORING_ADDON_NAME]
5275+
5276+
try:
52445277
self.context.external_functions.ensure_container_insights_for_monitoring(
52455278
self.cmd,
5246-
cluster.addon_profiles[monitoring_addon_key],
5279+
addon_profile,
52475280
self.context.get_subscription_id(),
52485281
self.context.get_resource_group_name(),
52495282
self.context.get_name(),
52505283
self.context.get_location(),
5251-
remove_monitoring=False,
5252-
aad_route=self.context.get_enable_msi_auth_for_monitoring(),
5253-
create_dcr=cnl_or_hlsm_changing,
5284+
remove_monitoring=True,
5285+
aad_route=True,
5286+
create_dcr=False,
52545287
create_dcra=True,
5255-
enable_syslog=self.context.get_enable_syslog(),
5256-
data_collection_settings=self.context.get_data_collection_settings(),
5257-
is_private_cluster=self.context.get_enable_private_cluster(),
5258-
ampls_resource_id=self.context.get_ampls_resource_id(),
5259-
enable_high_log_scale_mode=self.context.get_enable_high_log_scale_mode(),
5288+
enable_syslog=False,
5289+
data_collection_settings=None,
5290+
ampls_resource_id=None,
5291+
enable_high_log_scale_mode=False
52605292
)
5293+
except TypeError:
5294+
pass
5295+
5296+
# pylint: disable=too-many-locals,too-many-branches
5297+
def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None:
5298+
"""Postprocessing performed after the cluster is created.
5299+
5300+
:return: None
5301+
"""
5302+
# monitoring addon
5303+
monitoring_addon_enabled = self.context.get_intermediate("monitoring_addon_enabled", default_value=False)
5304+
if monitoring_addon_enabled:
5305+
self._postprocess_monitoring_enable(cluster)
52615306

52625307
# Handle monitoring addon postprocessing (disable case) - same logic as aks_disable_addons
52635308
monitoring_addon_disable_postprocessing_required = self.context.get_intermediate(
52645309
"monitoring_addon_disable_postprocessing_required", default_value=False
52655310
)
52665311

52675312
if monitoring_addon_disable_postprocessing_required:
5268-
addon_consts = self.context.get_addon_consts()
5269-
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
5270-
5271-
# Get the current cluster state to check config before it was disabled
5272-
current_cluster = self.client.get(self.context.get_resource_group_name(), self.context.get_name())
5273-
5274-
if (current_cluster.addon_profiles and
5275-
CONST_MONITORING_ADDON_NAME in current_cluster.addon_profiles):
5276-
5277-
# Use the current cluster addon profile for cleanup
5278-
addon_profile = current_cluster.addon_profiles[CONST_MONITORING_ADDON_NAME]
5279-
5280-
# Call ensure_container_insights_for_monitoring with remove_monitoring=True (same as aks_disable_addons)
5281-
try:
5282-
self.context.external_functions.ensure_container_insights_for_monitoring(
5283-
self.cmd,
5284-
addon_profile,
5285-
self.context.get_subscription_id(),
5286-
self.context.get_resource_group_name(),
5287-
self.context.get_name(),
5288-
self.context.get_location(),
5289-
remove_monitoring=True,
5290-
aad_route=True,
5291-
create_dcr=False,
5292-
create_dcra=True,
5293-
enable_syslog=False,
5294-
data_collection_settings=None,
5295-
ampls_resource_id=None,
5296-
enable_high_log_scale_mode=False
5297-
)
5298-
except TypeError:
5299-
# Ignore TypeError just like aks_disable_addons does
5300-
pass
5313+
self._postprocess_monitoring_disable()
53015314

53025315
# ingress appgw addon
53035316
ingress_appgw_addon_enabled = self.context.get_intermediate("ingress_appgw_addon_enabled", default_value=False)
@@ -7965,18 +7978,12 @@ def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None:
79657978
)
79667979
if monitoring_addon_postprocessing_required:
79677980
addon_consts = self.context.get_addon_consts()
7968-
CONST_MONITORING_ADDON_NAME = addon_consts.get("CONST_MONITORING_ADDON_NAME")
79697981
CONST_MONITORING_USING_AAD_MSI_AUTH = addon_consts.get("CONST_MONITORING_USING_AAD_MSI_AUTH")
79707982

7971-
# The API response may return the addon key as either "omsagent" or "omsAgent"
7972-
monitoring_addon_key = None
7973-
if cluster.addon_profiles:
7974-
if CONST_MONITORING_ADDON_NAME in cluster.addon_profiles:
7975-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME
7976-
elif CONST_MONITORING_ADDON_NAME_CAMELCASE in cluster.addon_profiles:
7977-
monitoring_addon_key = CONST_MONITORING_ADDON_NAME_CAMELCASE
7983+
monitoring_addon_key = _get_monitoring_addon_key(cluster, addon_consts)
79787984

7979-
if (monitoring_addon_key and
7985+
if (cluster.addon_profiles and
7986+
monitoring_addon_key in cluster.addon_profiles and
79807987
cluster.addon_profiles[monitoring_addon_key].enabled):
79817988

79827989
# 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
@@ -8125,7 +8125,7 @@ def test_postprocessing_create_dcr_false_when_only_enable_addons(self):
81258125
def test_postprocessing_ensure_container_insights_not_called_without_relevant_flags(self):
81268126
"""ensure_container_insights_for_monitoring is NOT called when no relevant flags are set.
81278127

8128-
When neither enable_addons, enable-azure-monitor-logs, nor any CNL/HLSM flag is
8128+
When neither enable_addons, enable_azure_monitor_logs, nor any CNL/HLSM flag is
81298129
present in raw_params, the outer elif is not entered.
81308130
"""
81318131
dec = self._make_postprocessing_decorator({})
@@ -8287,6 +8287,23 @@ def test_postprocessing_create_dcr_true_with_camelcase_addon_key(self):
82878287
_, kwargs = mock_ecifm.call_args
82888288
self.assertTrue(kwargs["create_dcr"])
82898289

8290+
def test_postprocessing_create_dcr_false_when_enable_azure_monitor_logs(self):
8291+
"""create_dcr=False when enable_azure_monitor_logs triggers ensure_container_insights_for_monitoring.
8292+
8293+
enable_azure_monitor_logs is in the outer _should_create_dcra condition but NOT in
8294+
_is_cnl_or_hlsm_changing, so create_dcr must be False.
8295+
"""
8296+
dec = self._make_postprocessing_decorator({"enable_azure_monitor_logs": True})
8297+
cluster = self._make_cluster_with_monitoring()
8298+
external_functions = dec.context.external_functions
8299+
with patch.object(
8300+
external_functions, "ensure_container_insights_for_monitoring", return_value=None
8301+
) as mock_ecifm:
8302+
dec.postprocessing_after_mc_created(cluster)
8303+
mock_ecifm.assert_called_once()
8304+
_, kwargs = mock_ecifm.call_args
8305+
self.assertFalse(kwargs["create_dcr"])
8306+
82908307

82918308
def test_set_up_health_monitor_profile(self):
82928309
# no flag - no change

0 commit comments

Comments
 (0)