[PostgreSQL] Refactor PostgreSQL network commands and validation logic#33111
Conversation
- Removed unused imports and consolidated network command functions for better clarity and maintainability. - Updated the `prepare_private_network` function to streamline subnet and VNet validation, ensuring correct resource ID formats. - Enhanced error messages for better user guidance and consistency. - Renamed functions for clarity, such as `prepare_public_network` to `compute_firewall_rule_ip_ranges`. - Improved validation logic in `validators.py` for subnet and VNet parameters, ensuring correct usage and format. - Updated the `flexible_replica_create` command to utilize the new network validation function. - Enhanced private DNS zone validation to handle cases with and without subnet IDs more effectively.
nachoalonsoportillo
requested review from
Pan-Qi,
VeryEarly,
calvinhzy and
jsntcy
as code owners
|
Validation for Azure CLI Full Test Starting...
Thanks for your contribution! |
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
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>
|
There was a problem hiding this comment.
Pull request overview
Refactors PostgreSQL flexible server networking helpers/validators, including subnet/VNet/Private DNS zone handling and user-facing error messages, and updates replica/create/restore workflows to use the new validation entrypoints.
Changes:
- Replaced prior network “provisioning” helpers with
flexible_server_validate_networkand moved subnet/DNS validation intovalidators.py. - Updated command implementations (create/restore/georestore/reviveDropped/replica) to call the refactored network validator.
- Simplified/standardized various validation and error/help messages.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/postgresql/utils/validators.py |
Refactors subnet & private DNS zone validation; updates several error messages. |
src/azure-cli/azure/cli/command_modules/postgresql/utils/_flexible_server_util.py |
Tweaks an authentication error message. |
src/azure-cli/azure/cli/command_modules/postgresql/commands/replica_commands.py |
Switches replica create flow to the new network validation function. |
src/azure-cli/azure/cli/command_modules/postgresql/commands/network_commands.py |
Replaces prior network provisioning logic with compute_firewall_rule_ip_ranges + flexible_server_validate_network. |
src/azure-cli/azure/cli/command_modules/postgresql/commands/custom_commands.py |
Updates server create/restore/georestore/reviveDropped/update flows to use the refactored network/DNS validation. |
src/azure-cli/azure/cli/command_modules/postgresql/_params.py |
Removes address-prefix args and updates help strings for VNet/subnet/private DNS/restore-time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resource_group=resource_group_name, | ||
| subnet_id=instance.network.delegated_subnet_resource_id, | ||
| location=location, | ||
| yes=yes) |
There was a problem hiding this comment.
validate_private_dns_zone does not accept resource_group or yes keyword arguments (current signature is (..., private_dns_zone_suffix=None, subnet_id=None, location=None)), so this call will raise TypeError at runtime. Either update the function signature to support these parameters or remove them from the call site and handle RG/subscription selection & prompting elsewhere.
| resource_group=resource_group_name, | |
| subnet_id=instance.network.delegated_subnet_resource_id, | |
| location=location, | |
| yes=yes) | |
| subnet_id=instance.network.delegated_subnet_resource_id, | |
| location=location) |
| private_dns_zone_arguments=private_dns_zone_arguments, | ||
| public_access=public_access, | ||
| vnet=vnet, | ||
| subnet=subnet) |
There was a problem hiding this comment.
--yes is passed into flexible_server_create, but it is not forwarded to flexible_server_validate_network. This means the command may still prompt for confirmation (e.g., when auto-detecting IP) even in non-interactive/--yes scenarios. Pass yes=yes through so network/firewall prompting respects the CLI flag.
| subnet=subnet) | |
| subnet=subnet, | |
| yes=yes) |
| network, start_ip, end_ip = flexible_server_validate_network(cmd=cmd, | ||
| resource_group_name=resource_group_name, | ||
| server_name=server_name, | ||
| location=location, | ||
| db_context=db_context, | ||
| private_dns_zone_arguments=private_dns_zone_arguments, | ||
| public_access=public_access, | ||
| vnet=vnet, | ||
| subnet=subnet) |
There was a problem hiding this comment.
flexible_server_validate_network only takes the private-network path when subnet is set. As a result, specifying --vnet without --subnet here will silently ignore the VNet and proceed down the public-network path, which is inconsistent with the parameter contract. Either require --subnet whenever --vnet is provided, or reintroduce logic to derive/create a subnet when only --vnet is supplied.
| @@ -821,8 +814,6 @@ def flexible_server_revivedropped(cmd, client, resource_group_name, server_name, | |||
| public_access='Disabled', | |||
| vnet=vnet, | |||
| subnet=subnet, | |||
| vnet_address_prefix=vnet_address_prefix, | |||
| subnet_address_prefix=subnet_address_prefix, | |||
| yes=yes) | |||
There was a problem hiding this comment.
This condition triggers network validation when only vnet is provided, but flexible_server_validate_network ignores vnet unless subnet is also set and may return a network object without delegated_subnet_resource_id while public_access='Disabled'. This can lead to invalid server create payloads. Either require subnet when vnet is provided, or update flexible_server_validate_network to properly handle vnet-only inputs.
| elif subnet is None and vnet is None and private_dns_zone_arguments is not None: | ||
| raise RequiredArgumentMissingError("Private DNS zone can only be used with private access setting. Use vnet or/and subnet parameters.") | ||
| elif subnet is None and private_dns_zone_arguments is not None: | ||
| raise RequiredArgumentMissingError("Private DNS zone can only be used with private access setting. Use --subnet parameter.") |
There was a problem hiding this comment.
flexible_server_validate_network ignores vnet unless subnet is also provided. Consider adding an explicit validation like if vnet is not None and subnet is None: raise RequiredArgumentMissingError(...) (or implement vnet-only support). As-is, callers can pass --vnet and end up with a public-network configuration (or an invalid private config if public_access is forced to Disabled).
| raise RequiredArgumentMissingError("Private DNS zone can only be used with private access setting. Use --subnet parameter.") | |
| raise RequiredArgumentMissingError("Private DNS zone can only be used with private access setting. Use --subnet parameter.") | |
| elif vnet is not None and subnet is None: | |
| raise RequiredArgumentMissingError("Virtual network can only be used with private access setting. Use --subnet parameter together with --vnet.") |
| options_list=['--restore-time'], | ||
| default=get_current_time(), | ||
| help='The point in time in UTC to restore from (ISO8601 format), e.g., 2017-04-26T02:10:00+00:00' | ||
| help='The point in time in UTC to restore from (ISO8601 format), e.g., 2026-03-22T18:20:22+00:00' |
There was a problem hiding this comment.
The --restore-time help text is split across two adjacent string literals without a separating space/punctuation, so the rendered help will read like ...+00:00The default value.... Add a space (or period + space) at the end of the first literal or start of the second to keep the help readable.
| help='The point in time in UTC to restore from (ISO8601 format), e.g., 2026-03-22T18:20:22+00:00' | |
| help='The point in time in UTC to restore from (ISO8601 format), e.g., 2026-03-22T18:20:22+00:00 ' |
| if source_server_object.network.public_network_access == 'Disabled' and subnet: | ||
| parameters.network, _, _ = flexible_server_validate_network(cmd=cmd, | ||
| resource_group_name=resource_group_name, | ||
| server_name=replica_name, | ||
| location=location, | ||
| db_context=db_context, | ||
| private_dns_zone_arguments=private_dns_zone_arguments, | ||
| public_access='Disabled', | ||
| vnet=vnet, | ||
| subnet=subnet, | ||
| vnet_address_prefix=vnet_address_prefix, | ||
| subnet_address_prefix=subnet_address_prefix, | ||
| vnet=vnet, | ||
| yes=yes) | ||
| else: | ||
| parameters.network = source_server_object.network |
There was a problem hiding this comment.
This command accepts vnet and subnet parameters, but network customization is only applied when subnet is provided; if a user supplies --vnet without --subnet, it will be ignored and the replica will inherit the source server's network without any error. Consider validating that vnet is not provided unless subnet is also provided (or support vnet-only behavior).
| 'Note that the subnet will be delegated to flexibleServers. ' | ||
| 'After delegation, this subnet cannot be used for any other type of Azure resources.' |
There was a problem hiding this comment.
--subnet help says the subnet “will be delegated to flexibleServers”, but the refactored code path (flexible_server_validate_network + validate_subnet) only formats/returns a subnet resource ID and does not perform delegation. Either reintroduce delegation logic or update the help text so users know they must pre-delegate the subnet themselves.
| 'Note that the subnet will be delegated to flexibleServers. ' | |
| 'After delegation, this subnet cannot be used for any other type of Azure resources.' | |
| 'Note that the subnet must be delegated to flexibleServers before creating the server. ' | |
| 'A subnet delegated to flexibleServers cannot be used for any other type of Azure resources.' |
| 'The name or id of new or existing private DNS zone. ' | ||
| 'You can use the private DNS zone from same resource group, different resource group, or different subscription. ' | ||
| 'If you want to use a DNS zone from different resource group or subscription, provide its resource identifier. ' | ||
| 'CLI creates a new private DNS zone within the same resource group as virtual network if not provided by users.' |
There was a problem hiding this comment.
--private-dns-zone help claims “CLI creates a new private DNS zone … if not provided by users”, but the current refactor only computes/validates an ARM ID and no longer creates or links the zone. Either restore the create/link behavior or adjust the help text to avoid promising functionality that isn’t implemented.
| 'The name or id of new or existing private DNS zone. ' | |
| 'You can use the private DNS zone from same resource group, different resource group, or different subscription. ' | |
| 'If you want to use a DNS zone from different resource group or subscription, provide its resource identifier. ' | |
| 'CLI creates a new private DNS zone within the same resource group as virtual network if not provided by users.' | |
| 'The name or resource ID of an existing private DNS zone. ' | |
| 'You can use a private DNS zone from the same resource group, a different resource group, or a different subscription. ' | |
| 'If you want to use a DNS zone from a different resource group or subscription, provide its full resource ID. ' | |
| 'The CLI does not automatically create or link private DNS zones when this parameter is omitted.' |
| if subnet is not None: | ||
| subnet_id = validate_subnet(cmd, | ||
| resource_group_name, | ||
| server_name, | ||
| vnet=vnet, | ||
| subnet=subnet, | ||
| location=location, | ||
| delegation_service_name=DELEGATION_SERVICE_NAME, | ||
| vnet_address_pref=vnet_address_prefix, | ||
| subnet_address_pref=subnet_address_prefix, | ||
| yes=yes) | ||
| private_dns_zone_id = prepare_private_dns_zone(db_context, | ||
| resource_group_name, | ||
| server_name, | ||
| private_dns_zone=private_dns_zone_arguments, | ||
| subnet_id=subnet_id, | ||
| location=location, | ||
| yes=yes) | ||
| subnet=subnet) | ||
| private_dns_zone_id = validate_private_dns_zone(db_context, | ||
| server_name=server_name, | ||
| private_dns_zone=private_dns_zone_arguments, | ||
| subnet_id=subnet_id, | ||
| location=location) | ||
| network.delegated_subnet_resource_id = subnet_id | ||
| network.private_dns_zone_arm_resource_id = private_dns_zone_id |
There was a problem hiding this comment.
flexible_server_validate_network now sets delegated_subnet_resource_id / private_dns_zone_arm_resource_id purely by formatting IDs and no longer verifies existence, delegates the subnet, or creates/links the private DNS zone (the previous implementation did). This is a behavior change that can cause flexible-server create/restore/georestore to fail when users pass subnet/DNS names expecting the CLI to provision/configure the required network resources. Consider restoring the provisioning steps or explicitly validating existence/delegation and updating user-facing docs/errors accordingly.
nachoalonsoportillo
modified the milestones:
May 2026 (2026-05-05),
June 2026 (2026-06-02)
prepare_private_networkfunction to streamline subnet and VNet validation, ensuring correct resource ID formats.prepare_public_networktocompute_firewall_rule_ip_ranges.validators.pyfor subnet and VNet parameters, ensuring correct usage and format.flexible_replica_createcommand to utilize the new network validation function.Related command
Description
Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.