Application Gateway for Containers addon commands#9387
Application Gateway for Containers addon commands#9387zhoxing-ms merged 25 commits intoAzure:mainfrom
Conversation
JackStromberg
requested review from
FumingZhang and
andyliuliming
as code owners
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| aks applicationloadbalancer | sub group aks applicationloadbalancer added |
||
| aks create | cmd aks create added parameter enable_application_load_balancer |
||
| aks update | cmd aks update added parameter disable_application_load_balancer |
||
| aks update | cmd aks update added parameter enable_application_load_balancer |
|
Hi @JackStromberg, |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
yanzhudd,
yonzhan and
zhoxing-ms
Release SuggestionsModule: aks-preview
Notes
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the Application Load Balancer (Application Gateway for Containers) addon to AKS clusters. The implementation follows the pattern established for other special addons like web app routing.
Key changes:
- Adds new CLI commands
az aks applicationloadbalancer enable/disable/updatefor managing the addon - Integrates the addon into existing addon management infrastructure (
aks addon enable/disable) - Stores configuration in the cluster's ingress profile rather than the standard addon profile
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| managed_cluster_decorator.py | Adds getter method for the parameter and methods to set up and update the application load balancer profile in the ingress profile |
| custom.py | Implements the enable/disable/update commands and integrates with existing addon list/show/enable/disable functionality |
| commands.py | Registers the new aks applicationloadbalancer command group with enable, disable, and update commands |
| addonconfiguration.py | Adds handling for the applicationloadbalancer addon in the update_addons function |
| _help.py | Adds documentation for the new commands and updates addon descriptions; fixes typo in app routing help |
| _consts.py | Defines constants for the new addon name and adds it to the ADDONS dictionary and descriptions |
| HISTORY.rst | Documents the new feature in the pending release notes |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
FumingZhang
left a comment
FumingZhang
left a comment
There was a problem hiding this comment.
Could you please add a new scenario test case (may refer to existing examples from test_aks_commands.py) for the change?
| name_prefix="clitest", | ||
| location="westus2", | ||
| ) | ||
| def test_aks_applicationloadbalancer_enable_disable( |
CodeGen Tools Feedback CollectionThank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey |
|
/azp run |
| not mc.ingress_profile and | ||
| not mc.ingress_profile.application_load_balancer and |
There was a problem hiding this comment.
The logic in this condition uses and instead of or, which will cause it to always evaluate to False. The condition should check if any of these are falsy (None or False), not if all of them are falsy. This should be:
if (
not mc.ingress_profile or
not mc.ingress_profile.application_load_balancer or
not mc.ingress_profile.application_load_balancer.enabled
):This matches the pattern used for web_application_routing above at lines 2700-2704.
| not mc.ingress_profile and | |
| not mc.ingress_profile.application_load_balancer and | |
| not mc.ingress_profile or | |
| not mc.ingress_profile.application_load_balancer or |
| def aks_applicationloadbalancer_disable( | ||
| cmd, | ||
| client, | ||
| resource_group_name, | ||
| name | ||
| ): | ||
| return _aks_applicationloadbalancer_update( | ||
| cmd, | ||
| client, | ||
| resource_group_name, | ||
| name, | ||
| enable_application_load_balancer=False) |
There was a problem hiding this comment.
The aks_applicationloadbalancer_disable function should pass disable_application_load_balancer=True instead of enable_application_load_balancer=False. The decorator's update_application_load_balancer_profile method checks for both enable_application_load_balancer and disable_application_load_balancer parameters separately (lines 6654-6655 in managed_cluster_decorator.py). Setting enable_application_load_balancer=False would result in no operation being performed. This is also related to the missing disable_application_load_balancer parameter in the _aks_applicationloadbalancer_update function signature.
| def test_aks_applicationloadbalancer_enable_disable( | ||
| self, resource_group, resource_group_location | ||
| ): | ||
| """This test case exercises enabling and disabling application load balancer (Application Gateway for Containers) in an AKS cluster.""" | ||
|
|
||
| # reset the count so in replay mode the random names will start with 0 | ||
| self.test_resources_count = 0 | ||
|
|
||
| # create cluster without application load balancer | ||
| aks_name = self.create_random_name("cliakstest", 16) | ||
| node_vm_size = "standard_d4_v5" | ||
| self.kwargs.update( | ||
| { | ||
| "resource_group": resource_group, | ||
| "aks_name": aks_name, | ||
| "location": resource_group_location, | ||
| "ssh_key_value": self.generate_ssh_keys(), | ||
| } | ||
| ) | ||
| create_cmd = ( | ||
| "aks create --resource-group={resource_group} --name={aks_name} --location={location} " | ||
| "--enable-oidc-issuer " | ||
| "--enable-workload-identity " | ||
| "--enable-gateway-api " | ||
| "--enable-application-load-balancer " | ||
| "--ssh-key-value={ssh_key_value}" | ||
| ) | ||
| self.cmd( | ||
| create_cmd, | ||
| checks=[ | ||
| self.check("provisioningState", "Succeeded"), | ||
| self.check("ingressProfile.applicationLoadBalancer.enabled", True) | ||
| ], | ||
| ) | ||
|
|
||
| # disable application load balancer | ||
| disable_applicationloadbalancer_cmd = "aks update --resource-group={resource_group} --name={aks_name} --disable-application-load-balancer --disable-gateway-api" | ||
| self.cmd( | ||
| disable_applicationloadbalancer_cmd, | ||
| checks=[ | ||
| self.check("provisioningState", "Succeeded"), | ||
| self.check("ingressProfile.applicationLoadBalancer.enabled", False), | ||
| ], | ||
| ) | ||
|
|
||
| # delete cluster | ||
| delete_cmd = "aks delete --resource-group={resource_group} --name={aks_name} --yes --no-wait" | ||
| self.cmd(delete_cmd, checks=[self.is_empty()]) |
There was a problem hiding this comment.
The test named test_aks_applicationloadbalancer_enable_disable does not test the dedicated az aks applicationloadbalancer enable and az aks applicationloadbalancer disable commands. It only tests aks update with the --disable-application-load-balancer flag. Consider adding test coverage for the dedicated commands introduced in this PR (aks applicationloadbalancer enable and aks applicationloadbalancer disable).
…ds.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ds.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ds.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ds.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ds.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Please fix failed CI checks, you'll need to update the recording file as the code uses a new API version 2025-10-02-preview instead of 2025-09-02-preview now
|
|
/azp run |
|
Commenter does not have sufficient privileges for PR 9387 in repo Azure/azure-cli-extensions |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
az aks applicationloadbalancer enable: Enable Application Load Balancer add-on for an existing cluster.az aks applicationloadbalancer disable: Disable Application Load Balancer add-on for an existing cluster.az aks applicationloadbalancer update: Update Application Load Balancer add-on for an existing cluster.General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)