Skip to content
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

[AKS] BREAKING CHANGE: aks create: Change the default value of --enable-msi-auth-for-monitoring to true and add check for airgap clouds #26356

Merged
merged 12 commits into from
May 18, 2023
Merged
31 changes: 11 additions & 20 deletions src/azure-cli/azure/cli/command_modules/acs/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
from azure.cli.command_modules.acs._validators import extract_comma_separated_string
from azure.cli.command_modules.acs.addonconfiguration import (
add_ingress_appgw_addon_role_assignment,
add_monitoring_role_assignment,
add_virtual_node_role_assignment,
ensure_container_insights_for_monitoring,
ensure_default_log_analytics_workspace_for_monitoring,
Expand Down Expand Up @@ -452,7 +451,7 @@ def aks_create(
# addons
enable_addons=None,
workspace_resource_id=None,
enable_msi_auth_for_monitoring=False,
enable_msi_auth_for_monitoring=True,
wanlonghenry marked this conversation as resolved.
Show resolved Hide resolved
enable_syslog=False,
data_collection_settings=None,
aci_subnet_name=None,
Expand Down Expand Up @@ -869,7 +868,7 @@ def aks_enable_addons(cmd, client, resource_group_name, name, addons,
enable_secret_rotation=False,
rotation_poll_interval=None,
no_wait=False,
enable_msi_auth_for_monitoring=False,
enable_msi_auth_for_monitoring=True,
enable_syslog=False,
data_collection_settings=None,):
instance = client.get(resource_group_name, name)
Expand Down Expand Up @@ -940,21 +939,6 @@ def aks_enable_addons(cmd, client, resource_group_name, name, addons,
result = LongRunningOperation(cmd.cli_ctx)(
client.begin_create_or_update(resource_group_name, name, instance))

# For monitoring addon, Metrics role assignement doesnt require in case of MSI auth
if enable_monitoring and not enable_msi_auth_for_monitoring:
cloud_name = cmd.cli_ctx.cloud.name
# mdm metrics supported only in Azure Public cloud so add the role assignment only in this cloud
if cloud_name.lower() == 'azurecloud':
from msrestazure.tools import resource_id
cluster_resource_id = resource_id(
subscription=subscription_id,
resource_group=resource_group_name,
namespace='Microsoft.ContainerService', type='managedClusters',
name=name
)
add_monitoring_role_assignment(
result, cluster_resource_id, cmd)

if ingress_appgw_addon_enabled:
add_ingress_appgw_addon_role_assignment(result, cmd)

Expand All @@ -975,7 +959,7 @@ def aks_enable_addons(cmd, client, resource_group_name, name, addons,

def _update_addons(cmd, instance, subscription_id, resource_group_name, name, addons, enable,
workspace_resource_id=None,
enable_msi_auth_for_monitoring=False,
enable_msi_auth_for_monitoring=True,
wanlonghenry marked this conversation as resolved.
Show resolved Hide resolved
subnet_name=None,
appgw_name=None,
appgw_subnet_cidr=None,
Expand Down Expand Up @@ -1033,9 +1017,16 @@ def _update_addons(cmd, instance, subscription_id, resource_group_name, name, ad
workspace_resource_id = '/' + workspace_resource_id
if workspace_resource_id.endswith('/'):
workspace_resource_id = workspace_resource_id.rstrip('/')

cloud_name = cmd.cli_ctx.cloud.name
if enable_msi_auth_for_monitoring and (cloud_name.lower() == 'ussec' or cloud_name.lower() == 'usnat'):
if instance.identity is not None and instance.identity.type is not None and instance.identity.type == "userassigned":
logger.warning("--enable_msi_auth_for_monitoring is not supported in %s cloud and continuing monitoring enablement without this flag.", cloud_name)
enable_msi_auth_for_monitoring = False

addon_profile.config = {
CONST_MONITORING_LOG_ANALYTICS_WORKSPACE_RESOURCE_ID: workspace_resource_id}
addon_profile.config[CONST_MONITORING_USING_AAD_MSI_AUTH] = enable_msi_auth_for_monitoring
addon_profile.config[CONST_MONITORING_USING_AAD_MSI_AUTH] = "true" if enable_msi_auth_for_monitoring else "false"
elif addon == (CONST_VIRTUAL_NODE_ADDON_NAME + os_type):
if addon_profile.enabled:
raise CLIError('The virtual-node addon is already enabled for this managed cluster.\n'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1309,4 +1309,4 @@ interactions:
status:
code: 202
message: Accepted
version: 1
version: 1
Original file line number Diff line number Diff line change
Expand Up @@ -7323,7 +7323,6 @@ def create_new_cluster_with_monitoring_aad_auth(self, resource_group, resource_g
create_cmd = f'aks create --resource-group={resource_group} --name={aks_name} --location={resource_group_location} ' \
'--enable-managed-identity ' \
'--enable-addons monitoring ' \
'--enable-msi-auth-for-monitoring ' \
'--node-count 1 ' \
'--ssh-key-value={ssh_key_value} '
create_cmd += f'--assign-identity {identity_id} ' if user_assigned_identity else ''
Expand All @@ -7332,7 +7331,7 @@ def create_new_cluster_with_monitoring_aad_auth(self, resource_group, resource_g

response = self.cmd(create_cmd, checks=[
self.check('addonProfiles.omsagent.enabled', True),
self.check('addonProfiles.omsagent.config.useAADAuth', 'True')
self.check('addonProfiles.omsagent.config.useAADAuth', 'true')
]).get_output_in_json()

cluster_resource_id = response["id"]
Expand Down Expand Up @@ -7433,14 +7432,13 @@ def enable_monitoring_existing_cluster_aad_auth(self, resource_group, resource_g
create_cmd += f'--assign-identity {identity_id}' if user_assigned_identity else ''
self.cmd(create_cmd)

enable_monitoring_cmd = f'aks enable-addons -a monitoring --resource-group={resource_group} --name={aks_name} ' \
'--enable-msi-auth-for-monitoring '
enable_monitoring_cmd = f'aks enable-addons -a monitoring --resource-group={resource_group} --name={aks_name} '
if syslog_enabled:
enable_monitoring_cmd += f'--enable-syslog '

response = self.cmd(enable_monitoring_cmd, checks=[
self.check('addonProfiles.omsagent.enabled', True),
self.check('addonProfiles.omsagent.config.useAADAuth', 'True')
self.check('addonProfiles.omsagent.config.useAADAuth', 'true')
]).get_output_in_json()

cluster_resource_id = response["id"]
Expand Down Expand Up @@ -7497,7 +7495,7 @@ def test_aks_create_with_monitoring_legacy_auth(self, resource_group, resource_g
response = self.cmd(create_cmd, checks=[
self.check('addonProfiles.omsagent.enabled', True),
self.exists('addonProfiles.omsagent.config.logAnalyticsWorkspaceResourceID'),
self.check('addonProfiles.omsagent.config.useAADAuth', 'False')
self.check('addonProfiles.omsagent.config.useAADAuth', 'false')
]).get_output_in_json()

# make sure a DCR was not created
Expand Down