-
Notifications
You must be signed in to change notification settings - Fork 3.3k
{Cloud} az cloud register: Compatible with more response formats from ARM endpoint API
#31566
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
base: dev
Are you sure you want to change the base?
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the az cloud register command to support multiple JSON response formats from the ARM metadata endpoint.
- Added handling for JSON response types: both dict and list formats
- Improved robustness for suffix retrieval by checking for the presence of the 'suffixes' key
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/cloud/custom.py | Handles JSON object or array responses from the metadata endpoint |
| src/azure-cli-core/azure/cli/core/cloud.py | Refines suffix handling by checking existence of the 'suffixes' key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is the client's responsibility to handle inconsistent API responses. If the API version is the same (2022-09-01), the response SHOULD be consistent.
It is a bad practice to use undocumented API or handle non-compliant API response.
Since there is no spec for this API, the service can change it at will without any responsibility. It is impossible to enumerate all possibilities for an it, so it can break Azure CLI all of a sudden, even causing security or privacy issues (if targeting the wrong endpoint).
We learnt several lessons from using undocumented API or handling non-compliant API response in the past:
| def _get_processed_arm_endpoint(name, add_dot=False, fallback_value=None): | ||
| if is_suffix: | ||
| return (_add_starting_dot(arm_dict['suffixes'][name]) if add_dot else arm_dict['suffixes'][name]) if name in arm_dict['suffixes'] else fallback_value | ||
| return (_add_starting_dot(arm_dict['suffixes'][name]) if add_dot else arm_dict['suffixes'][name]) if 'suffixes' in arm_dict and name in arm_dict['suffixes'] else fallback_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If arm_dict does contain suffixes but arm_dict['suffixes'] is None, name in arm_dict['suffixes'] will fail:
arm_dict = {'suffixes': None}
name = 'test'
print('suffixes' in arm_dict and name in arm_dict['suffixes'])Output:
Traceback (most recent call last):
File "D:\cli\testproj\main.py", line 3, in <module>
print('suffixes' in arm_dict and name in arm_dict['suffixes'])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: argument of type 'NoneType' is not iterable
It is hard to enumerate all possibilities for an undocumented API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Updated to use if name in arm_dict.get('suffixes', {}) now
…lyn-ys/azure-cli into fix_cloud_endpoint_discovery
Related command
Description
Fix #31547
ARM endpoint API
$ARM_HOST/metadata/endpointsmay return different formats for different API versions and different clouds. For example:What makes things worse, ARM doesn't have spec definition for this API. This PR aims for enhancing
az cloud registerto be able to handle both Json Object and Json Array response formats with/without suffixesTesting 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.