Skip to content

{AKS} az aks update: Add --network-policy to support updating the mode of a network policy #6809

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
zhoxing-ms merged 5 commits into from
Oct 25, 2023
Merged

{AKS} az aks update: Add --network-policy to support updating the mode of a network policy #6809

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14cb218
add --network-policy none to az aks update
Sep 26, 2023
896d8f9
add recording file
Oct 17, 2023
8a5b7fb
describe changes in HISTORY.rst and bump extension version to 0.5.161
Oct 20, 2023
522a456
Merge branch 'main' into aks-preview/network-policy
Oct 20, 2023
80aa27a
Merge branch 'main' into aks-preview/network-policy
Oct 24, 2023
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
6 changes: 6 additions & 0 deletions src/aks-preview/azext_aks_preview/_consts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@
CONST_NETWORK_DATAPLANE_AZURE = "azure"
CONST_NETWORK_DATAPLANE_CILIUM = "cilium"

# network policy
CONST_NETWORK_POLICY_AZURE = "azure"
CONST_NETWORK_POLICY_CALICO = "calico"
CONST_NETWORK_POLICY_CILIUM = "cilium"
CONST_NETWORK_POLICY_NONE = "none"

# disk driver versions
CONST_DISK_DRIVER_V1 = "v1"
CONST_DISK_DRIVER_V2 = "v2"
Expand Down
8 changes: 7 additions & 1 deletion src/aks-preview/azext_aks_preview/_help.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@
short-summary: (PREVIEW) The Kubernetes network policy to use.
long-summary: |
Using together with "azure" network plugin.
Specify "azure" for Azure network policy manager and "calico" for calico network policy controller.
Specify "azure" for Azure network policy manager, "calico" for calico network policy controller, "cilium" for Azure CNI Overlay powered by Cilium.
Defaults to "" (network policy disabled).
- name: --network-dataplane
type: string
Expand Down Expand Up @@ -854,6 +854,12 @@
long-summary: |
Used to control the mode the network plugin should operate in. For example, "overlay" used with
--network-plugin=azure will use an overlay network (non-VNET IPs) for pods in the cluster.
- name: --network-policy
type: string
short-summary: Update the mode of a network policy.
long-summary: |
Specify "azure" for Azure network policy manager, "cilium" for Azure CNI Overlay powered by Cilium.
Comment thread
FumingZhang marked this conversation as resolved.
Defaults to "" (network policy disabled).
Comment thread
robogatikov marked this conversation as resolved.
Outdated
- name: --network-dataplane
type: string
short-summary: The network dataplane to use.
Expand Down
5 changes: 5 additions & 0 deletions src/aks-preview/azext_aks_preview/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
CONST_NETWORK_PLUGIN_KUBENET,
CONST_NETWORK_PLUGIN_MODE_OVERLAY,
CONST_NETWORK_PLUGIN_NONE,
CONST_NETWORK_POLICY_AZURE,
CONST_NETWORK_POLICY_CALICO,
CONST_NETWORK_POLICY_CILIUM,
CONST_NETWORK_POLICY_NONE,
CONST_NODE_IMAGE_UPGRADE_CHANNEL,
CONST_NODE_OS_CHANNEL_NODE_IMAGE,
CONST_NODE_OS_CHANNEL_NONE,
Expand Down Expand Up @@ -466,6 +470,7 @@ def load_arguments(self, _):
c.argument('nat_gateway_managed_outbound_ip_count', type=int, validator=validate_nat_gateway_managed_outbound_ip_count)
c.argument('nat_gateway_idle_timeout', type=int, validator=validate_nat_gateway_idle_timeout)
c.argument('network_dataplane', arg_type=get_enum_type(network_dataplanes))
c.argument('network_policy')
c.argument('kube_proxy_config')
c.argument('auto_upgrade_channel', arg_type=get_enum_type(auto_upgrade_channels))
c.argument('node_os_upgrade_channel', arg_type=get_enum_type(node_os_upgrade_channels))
Expand Down
1 change: 1 addition & 0 deletions src/aks-preview/azext_aks_preview/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,7 @@ def aks_update(
load_balancer_managed_outbound_ipv6_count=None,
outbound_type=None,
network_plugin_mode=None,
network_policy=None,
network_dataplane=None,
pod_cidr=None,
enable_pod_security_policy=False,
Expand Down
12 changes: 12 additions & 0 deletions src/aks-preview/azext_aks_preview/managed_cluster_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,13 @@ def get_network_plugin_mode(self) -> Union[str, None]:
# this parameter does not need validation
return network_plugin_mode

def get_network_policy(self) -> Union[str, None]:
"""Get the value of network_dataplane.

:return: str or None
"""
return self.raw_param.get("network_policy")

def get_network_dataplane(self) -> Union[str, None]:
"""Get the value of network_dataplane.

Expand Down Expand Up @@ -3151,6 +3158,11 @@ def update_network_plugin_settings(self, mc: ManagedCluster) -> ManagedCluster:
pod_cidr = self.context.get_pod_cidr()
if pod_cidr:
mc.network_profile.pod_cidr = pod_cidr

network_policy = self.context.get_network_policy()
if network_policy:
mc.network_profile.network_policy = network_policy

@wedaly wedaly Sep 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in addition to the live test you added, should we have a unit test in test_managed_cluster_decorator.py? Maybe add something to test_update_network_plugin_settings?

@robogatikov robogatikov Sep 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a test case to update network-policy from empty string to "azure"

@FumingZhang FumingZhang Sep 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Python implicitly converts "" to False, if user specifies --network-policy "", the code will not update the property value to "", is this expected?


return mc

def update_enable_network_observability_in_network_profile(self, mc: ManagedCluster) -> ManagedCluster:
Expand Down
3 changes: 2 additions & 1 deletion src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4460,6 +4460,7 @@ def test_aks_migrate_cluster_to_cilium_dataplane(self, resource_group, resource_
create_cmd = 'aks create --resource-group={resource_group} --name={name} --location={location} ' \
'--network-plugin azure --ssh-key-value={ssh_key_value} --kubernetes-version {k8s_version} ' \
'--network-plugin-mode=overlay'

self.cmd(create_cmd, checks=[
self.check('provisioningState', 'Succeeded'),
self.check('networkProfile.networkPlugin', 'azure'),
Expand All @@ -4468,7 +4469,7 @@ def test_aks_migrate_cluster_to_cilium_dataplane(self, resource_group, resource_
])

# update to enable cilium dataplane
update_cmd = 'aks update -g {resource_group} -n {name} --network-dataplane=cilium'
update_cmd = 'aks update -g {resource_group} -n {name} --network-dataplane=cilium --network-policy=cilium'

@wedaly wedaly Sep 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need to update the test fixtures after running the live test? I would have expected this to change the PUT MC request, but don't see any changes in recordings/test_aks_migrate_cluster_to_cilium_dataplane.yaml

@robogatikov robogatikov Sep 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about test fixtures, probably @FumingZhang or @zhoxing-ms can help clarify

@FumingZhang FumingZhang Sep 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Queued live test to validate the change. If test passed, the recording file would be published as pipeline artifact, could download and commit it.

@FumingZhang FumingZhang Sep 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test failed due to

E azure.core.exceptions.HttpResponseError: (PropertyChangeNotAllowed) Changing property 'networkProfile.networkPolicy' is not allowed.
E Code: PropertyChangeNotAllowed
E Message: Changing property 'networkProfile.networkPolicy' is not allowed.
E Target: networkProfile.networkPolicy

To test the feature, need a feature flag or specific sub?

@robogatikov robogatikov Sep 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @FumingZhang. The test failed because the change that would allow updating network policy to "cilium" has not been rolled out to eastus yet. We should wait a few days for it to reach eastus I suppose unless there is some other option.

self.cmd(update_cmd, checks=[
self.check('provisioningState', 'Succeeded'),
Expand Down
33 changes: 33 additions & 0 deletions src/aks-preview/azext_aks_preview/tests/latest/test_managed_cluster_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5111,6 +5111,39 @@ def test_update_network_plugin_settings(self):

self.assertEqual(dec_mc_5, ground_truth_mc_5)

# test update network policy
dec_6 = AKSPreviewManagedClusterUpdateDecorator(
self.cmd,
self.client,
{
"network_policy": "azure",
},
CUSTOM_MGMT_AKS_PREVIEW,
)
mc_6 = self.models.ManagedCluster(
location="test_location",
network_profile=self.models.ContainerServiceNetworkProfile(
network_plugin="azure",
network_policy="",
),
)

dec_6.context.attach_mc(mc_6)
# fail on passing the wrong mc object
with self.assertRaises(CLIInternalError):
dec_6.update_network_plugin_settings(None)
dec_mc_6 = dec_6.update_network_plugin_settings(mc_6)

ground_truth_mc_6 = self.models.ManagedCluster(
location="test_location",
network_profile=self.models.ContainerServiceNetworkProfile(
network_plugin="azure",
network_policy="azure",
),
)

self.assertEqual(dec_mc_6, ground_truth_mc_6)

def test_update_api_server_access_profile(self):
dec_1 = AKSPreviewManagedClusterUpdateDecorator(
self.cmd,
Expand Down