Skip to content

[AKS] Honor addon names defined in Azure CLI #15376

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.

Sign up for GitHub

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

Merged
yungezz merged 2 commits into from
Oct 9, 2020
Merged

[AKS] Honor addon names defined in Azure CLI #15376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
301def2
Honor addon names defined in Azure CLI
Oct 1, 2020
71fc907
Added unit test case
Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/azure-cli/azure/cli/command_modules/acs/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2429,8 +2429,12 @@ def _update_addons(cmd, instance, subscription_id, resource_group_name, addons,
if addon == 'aciConnector':
# only linux is supported for now, in the future this will be a user flag
addon += os_type
# addon name is case insensitive
addon = next((x for x in addon_profiles.keys() if x.lower() == addon.lower()), addon)

# honor addon names defined in Azure CLI
for key in list(addon_profiles):
if key.lower() == addon.lower() and key != addon:
addon_profiles[addon] = addon_profiles.pop(key)

if enable:
# add new addons or update existing ones and enable them
addon_profile = addon_profiles.get(addon, ManagedClusterAddonProfile(enabled=False))
Expand Down Expand Up @@ -2740,8 +2744,9 @@ def _ensure_default_log_analytics_workspace_for_monitoring(cmd, subscription_id,

def _ensure_container_insights_for_monitoring(cmd, addon):
# Workaround for this addon key which has been seen lowercased in the wild.
if 'loganalyticsworkspaceresourceid' in addon.config:
addon.config['logAnalyticsWorkspaceResourceID'] = addon.config.pop('loganalyticsworkspaceresourceid')
for key in list(addon.config):
if key.lower() == 'loganalyticsworkspaceresourceid' and key != 'logAnalyticsWorkspaceResourceID':
Copy link
Member

@yungezz yungezz Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for != part, better to lower() before compare

Copy link
Contributor Author

@kaiqzhan kaiqzhan Oct 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here my intention is to change the keys back to 'logAnalyticsWorkspaceResourceID'. The intention of key != 'logAnalyticsWorkspaceResourceID' is to skip if the key is already be 'logAnalyticsWorkspaceResourceID'.
For example, if the key is 'loganalyticsworkspaceresourceid' or 'LogAnalyticsWorkspaceResourceID', the value will be pop out and write back with key 'logAnalyticsWorkspaceResourceID'. But if the key is already 'logAnalyticsWorkspaceResourceID', it skips and nothing changed.

Copy link
Member

@fengzhou-msft fengzhou-msft Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you assign 'logAnalyticsWorkspaceResourceID' to a variable and use the variable instead of using the hard-coded string in multiple places?

Copy link
Contributor Author

@kaiqzhan kaiqzhan Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fengzhou-msft We're already doing that pattern in azure-cli-extension. I'll change that in another pr.

addon.config['logAnalyticsWorkspaceResourceID'] = addon.config.pop(key)

workspace_resource_id = addon.config['logAnalyticsWorkspaceResourceID']

Expand Down
13 changes: 11 additions & 2 deletions src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ def test_update_addons(self, rg_def, cf_resource_groups, cf_resources):
instance.addon_profiles['kubedashboard'] = ManagedClusterAddonProfile(enabled=True)
instance = _update_addons(cmd, instance, '00000000-0000-0000-0000-000000000000',
'clitest000001', 'kube-dashboard', enable=False)
dashboard_addon_profile = instance.addon_profiles['kubedashboard']
dashboard_addon_profile = instance.addon_profiles['kubeDashboard']
self.assertFalse(dashboard_addon_profile.enabled)

# kube-dashboard enabled, there's existing dashboard addon profile
Expand All @@ -668,7 +668,7 @@ def test_update_addons(self, rg_def, cf_resource_groups, cf_resources):
instance.addon_profiles['kubedashboard'] = ManagedClusterAddonProfile(enabled=False)
instance = _update_addons(cmd, instance, '00000000-0000-0000-0000-000000000000',
'clitest000001', 'kube-dashboard', enable=True)
dashboard_addon_profile = instance.addon_profiles['kubedashboard']
dashboard_addon_profile = instance.addon_profiles['kubeDashboard']
self.assertTrue(dashboard_addon_profile.enabled)

# monitoring enabled and then enabled again should error
Expand Down Expand Up @@ -710,6 +710,15 @@ def test_ensure_container_insights_for_monitoring(self, invoke_def, cf_resources
self.assertEqual(args[3]['resources'][0]['type'], "Microsoft.Resources/deployments")
self.assertEqual(args[4]['workspaceResourceId']['value'], wsID)

# when addon config key is lower cased
addon.config = {
'loganalyticsworkspaceresourceid': wsID
}
self.assertTrue(_ensure_container_insights_for_monitoring(cmd, addon))
args, kwargs = invoke_def.call_args
self.assertEqual(args[3]['resources'][0]['type'], "Microsoft.Resources/deployments")
self.assertEqual(args[4]['workspaceResourceId']['value'], wsID)

@mock.patch('azure.cli.command_modules.acs.custom._urlretrieve')
@mock.patch('azure.cli.command_modules.acs.custom.logger')
def test_k8s_install_kubectl_emit_warnings(self, logger_mock, mock_url_retrieve):
Expand Down