[Mgmt][Core] new policy for policy token #42726
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new policy token header mechanism for Azure management operations, enabling the acquisition and application of Azure Policy tokens for external policy evaluation during resource operations.
Key changes:
- Adds
PolicyTokenHeaderPolicyandAsyncPolicyTokenHeaderPolicyclasses to handle policy token acquisition - Implements comprehensive test coverage for both sync and async policy implementations
- Updates package version and documentation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
_policy_token_header.py |
Core policy implementation for acquiring and applying policy tokens to requests |
_policy_token_header_async.py |
Async version of the policy token header policy |
test_policy_token_header.py |
Comprehensive test suite for sync policy implementation |
test_policy_token_header_async.py |
Comprehensive test suite for async policy implementation |
__init__.py |
Exports the new policy classes |
tools.py |
Minor syntax fix for trailing comma |
_version.py |
Version bump to 1.7.0 |
CHANGELOG.md |
Documents the new policy feature |
| if content and isinstance(content, str): | ||
| try: | ||
| content = json.loads(content) | ||
| except Exception: # pylint: disable=broad-except |
There was a problem hiding this comment.
Using a bare Exception catch is too broad. Consider catching specific exceptions like json.JSONDecodeError, ValueError, or TypeError that could be raised during JSON parsing.
| except Exception: # pylint: disable=broad-except | |
| except (json.JSONDecodeError, ValueError, TypeError): |
Copilot uses AI. Check for mistakes.
| except Exception as e: | ||
| request.context.options["acquire_policy_token"] = True | ||
| raise e |
There was a problem hiding this comment.
Using a bare Exception catch is too broad and the re-raise pattern 'raise e' loses the original traceback. Use 'raise' without the variable to preserve the traceback, and consider catching more specific exceptions.
| except Exception as e: | |
| request.context.options["acquire_policy_token"] = True | |
| raise e | |
| except Exception: | |
| request.context.options["acquire_policy_token"] = True | |
| raise |
Copilot uses AI. Check for mistakes.
| except Exception as e: | ||
| request.context.options["acquire_policy_token"] = True | ||
| raise e |
There was a problem hiding this comment.
Using a bare Exception catch is too broad and the re-raise pattern 'raise e' loses the original traceback. Use 'raise' without the variable to preserve the traceback, and consider catching more specific exceptions.
| except Exception as e: | |
| request.context.options["acquire_policy_token"] = True | |
| raise e | |
| raise |
Copilot uses AI. Check for mistakes.
| :raises ~azure.core.exceptions.HttpResponseError: If subscription ID cannot be extracted from request URL | ||
| """ | ||
| # try to get subscriptionId from request.http_request.url | ||
| subscription_id = ( |
There was a problem hiding this comment.
Can we use parse_resource_id method?
| method="POST", | ||
| url=acquire_policy_url.format(subscriptionId=subscription_id), | ||
| params={"api-version": "2025-03-01"}, | ||
| headers={"Accept": "application/json", "Content-Type": "application/json", "x-ms-force-sync": "true"}, |
There was a problem hiding this comment.
I believe we also need the auth header, which can be obtained from the main request.
for https://github.com/Azure/azure-sdk-for-python-pr/issues/968
Here is SDK client change 39108c6 for this new policy. I also run live test locally and all tests work as expected.