[ChangeSafety] Phase 1: AcquirePolicyToken Support#449
[ChangeSafety] Phase 1: AcquirePolicyToken Support#449
Conversation
|
Related to this change: Azure/azure-powershell#28711 |
There was a problem hiding this comment.
Pull Request Overview
This pull request implements Phase 1 of the ChangeSafety feature, adding support for acquiring Azure Policy tokens to validate resource operations. The implementation introduces a new -AcquirePolicyToken dynamic parameter and handler that intercepts write operations to automatically obtain policy tokens from the Azure Authorization API.
Key changes:
- Introduces
AcquirePolicyTokenHandlerto intercept HTTP requests and acquire policy tokens for write operations - Adds
-AcquirePolicyTokendynamic parameter to Azure cmdlets (excludes read-only operations like Get-/List-/Show-*) - Implements feature flag support via environment variable
AZ_ENABLE_POLICY_TOKENand config keyEnablePolicyToken
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Common/AcquirePolicyTokenHandler.cs | New handler that acquires policy tokens from Azure Authorization API and attaches them to outgoing requests |
| src/Common/AzurePSCmdlet.cs | Adds dynamic parameter support, feature flag logic, and handler registration for policy token acquisition |
| src/ResourceManager/Version2016_09_01/AzureRMCmdlet.cs | Updates GetDynamicParameters to call base implementation to support inherited dynamic parameters |
| src/Common/Utilities/GeneralUtilities.cs | Adds policy token header to authorization header sanitization list |
| src/Authentication.Abstractions/Models/ConfigKeysForCommon.cs | Defines new config key for EnablePolicyToken feature flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| || commandName.EndsWith("List", StringComparison.OrdinalIgnoreCase) | ||
| || commandName.EndsWith("Show", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
The logic for excluding read-only cmdlets checks if the command name ends with 'List' or 'Show', but cmdlets typically follow the pattern 'Get-', 'Remove-', 'Set-*' where the verb comes first. The check should use StartsWith for 'List' and 'Show' as well, not EndsWith. For example, 'List-AzResource' would be caught correctly, but a cmdlet named 'Get-ResourceList' would incorrectly be excluded.
| || commandName.EndsWith("List", StringComparison.OrdinalIgnoreCase) | |
| || commandName.EndsWith("Show", StringComparison.OrdinalIgnoreCase)) | |
| || commandName.StartsWith("List", StringComparison.OrdinalIgnoreCase) | |
| || commandName.StartsWith("Show", StringComparison.OrdinalIgnoreCase)) |
|
|
||
| if (!(_cmdlet?.IsPolicyTokenFeatureEnabled() ?? false)) | ||
| { | ||
| EnqueueDebug("Skip: feature disabled (EnableAcquirePolicyToken config set to false)."); |
There was a problem hiding this comment.
The debug message incorrectly references 'EnableAcquirePolicyToken' config, but the actual config key is 'EnablePolicyToken' as defined in ConfigKeysForCommon.cs. This could confuse users troubleshooting feature flag issues.
| EnqueueDebug("Skip: feature disabled (EnableAcquirePolicyToken config set to false)."); | |
| EnqueueDebug("Skip: feature disabled (EnablePolicyToken config set to false)."); |
| using (var http = new HttpClient()) | ||
| { | ||
| EnqueueDebug($"POST acquirePolicyToken {tokenUri}"); | ||
| var response = await http.SendAsync(tokenRequest, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Creating a new HttpClient instance for each token request is inefficient and can lead to socket exhaustion under load. HttpClient should be reused or managed via a static instance or IHttpClientFactory. Consider using a shared static HttpClient instance for this handler.
| if (string.IsNullOrEmpty(token)) | ||
| { | ||
| EnqueueDebug("Response OK but token missing."); | ||
| throw new InvalidOperationException($"No token returned. Response:{responseContent}"); |
There was a problem hiding this comment.
Missing space after colon in error message. Should be 'Response: {responseContent}' for consistency with other error messages in the file.
| throw new InvalidOperationException($"No token returned. Response:{responseContent}"); | |
| throw new InvalidOperationException($"No token returned. Response: {responseContent}"); |
| catch { } | ||
| } | ||
| } | ||
| catch { } | ||
|
|
There was a problem hiding this comment.
Empty catch blocks silently swallow exceptions without logging, making debugging configuration issues difficult. Consider logging the exception or at minimum adding a comment explaining why the exception is intentionally ignored.
| catch { } | |
| } | |
| } | |
| catch { } | |
| catch (Exception ex) | |
| { | |
| Debug.WriteLine($"Exception in configManager.GetConfigValue: {ex}"); | |
| } | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Debug.WriteLine($"Exception in IsPolicyTokenFeatureEnabled: {ex}"); | |
| } |
1 participant
No description provided.