[Change Safety] Az.Accounts: AutoRest AcquirePolicyToken pipeline step (Stage A)#29840
[Change Safety] Az.Accounts: AutoRest AcquirePolicyToken pipeline step (Stage A)#29840YangAn-microsoft wants to merge 8 commits into
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull request overview
This PR extends the Az.Accounts “Change Safety” infrastructure to AutoRest-generated cmdlets by wiring new runtime hooks into the generated cmdlet VTable and HTTP pipeline, and adding unit tests plus release notes to document the groundwork.
Changes:
- Adds a conditional HTTP pipeline step that stamps an Azure Policy token onto outgoing write requests when Change Safety parameters are bound.
- Introduces a new VTable delegate for supplying Change Safety dynamic parameters and wires it through
Register-AzModule. - Adds unit tests for pipeline-step gating and dynamic-parameter gating, and updates the Accounts changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Accounts/Accounts/CommonModule/VTable.cs | Adds a new GetDynamicParameters delegate contract for Stage C generated modules. |
| src/Accounts/Accounts/CommonModule/RegisterAzModule.cs | Wires the new GetDynamicParameters delegate from ContextAdapter into the exported VTable. |
| src/Accounts/Accounts/CommonModule/ContextAdapter.cs | Appends the conditional AcquirePolicyToken pipeline step and implements Change Safety dynamic parameter building/gating. |
| src/Accounts/Accounts/ChangeLog.md | Documents upcoming Change Safety plumbing for AutoRest cmdlets. |
| src/Accounts/Accounts.Test/UnitTest/AcquirePolicyTokenHandlerTests.cs | Adds unit coverage for step-append gating based on bound Change Safety parameters. |
| src/Accounts/Accounts.Test/ChangeSafetyDynamicParametersTests.cs | Adds tests for dynamic parameter exposure on write verbs and exclusion on read verbs. |
…angelog; doc grammar
…lue (match GetParameterValue convention)
…neric changelog, doc fix - ContextAdapter: use SwitchParameter.ToBool() instead of IsPresent - ContextAdapter: remove WhatIf handling from the AutoRest step; generated write cmdlets gate the HTTP call via ShouldProcess, so the write request (and this step) never runs under -WhatIf - Accounts ChangeLog: rephrase to a generic entry - VTable: fix missing space in XML doc comments
VeryEarly
left a comment
VeryEarly
left a comment
There was a problem hiding this comment.
you can keep trying this direction, but I have a concern about this design:
Essentially, we will get delegates at runtime to:
- function to add dynamic parameters
- http pipeline step to add dynamic parameter values to header
But for generated modules, in order to expose dynamic parameters, the public object GetDynamicParameters() need to be implemented in each cmdlet instance, which will be injected in generator logic. And in order to get that value, the cmdlet needs to call the call the getparametervalue delegate explicitly by name which will also be injected in generator logic. That will make codegen logic depends on a runtime wired delegate. Which is not generally the best practice. And I'll argue whether this increased or decreased the complexity.
Keeping the http pipeline step is fine, because it append/prepend the step into pipeline in module level, no cmdlet level changes required.
Please consider my concern unless you plan to do it differently.
I will explore and compare with changing generator to generate self-contained static parameters on write cmdlets |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
…p Stage A The AutoRest generator now emits -AcquirePolicyToken/-ChangeReference as static parameters, so the runtime-wired GetDynamicParametersValue delegate is no longer needed. Removed it from ContextAdapter, VTable, and RegisterAzModule; the Stage A pipeline step (which reads the values from BoundParameters) is unchanged. Replaced the Stage B dynamic-parameter tests with a contract pin test asserting the parameter names and help text stay in sync with the generator's hardcoded literals.
YangAn-microsoft
changed the title
…ntry - ContextAdapter: add ConfigureAwait(false) to the next(...) await for consistency with the preceding StampPolicyTokenAsync await.\n- ChangeLog: drop the 'refined internal APIs' phrasing per reviewer feedback.
…e ChangeReference as empty - Use named arguments on StampPolicyTokenAsync for readability/robustness against signature changes.\n- Normalize a whitespace-only -ChangeReference to null so the step stays a no-op unless a real value is provided; add a unit test.
| { | ||
| await acquirer.StampPolicyTokenAsync( | ||
| request, | ||
| shouldAcquire: true, |
|
|
||
| // Generated write cmdlets gate the HTTP call behind ShouldProcess, so under -WhatIf the write | ||
| // request (and therefore this pipeline step) is never sent. No WhatIf handling is needed here. | ||
| var acquirer = new Microsoft.WindowsAzure.Commands.Common.PolicyTokenAcquirer(); |
| } | ||
|
|
||
| [Fact] | ||
| public void AcquirePolicyTokenSwitchNotPresent_DoesNotAppendStep() |
Summary
Brings the Change Safety feature (
-AcquirePolicyToken/-ChangeReference) to AutoRest-generated cmdlets — the Az.Accounts (processing) half.ContextAdapter.OnNewRequestappends anAcquirePolicyTokenSendAsyncStep(beside the auth step) that calls the sharedPolicyTokenAcquireron write requests. It reads-AcquirePolicyToken/-ChangeReferencefrom the cmdlet'sBoundParametersby name and stamps thex-ms-policy-external-evaluationsheader. Guarded no-op: when neither parameter is bound, the step isn't even added (zero per-request cost). Fail-closed if acquisition fails.Why / dependencies
AutoRest cmdlets don't inherit
AzurePSCmdlet, so they can't reuse the SDK Change Safety handler by inheritance — the equivalent is injected centrally via theRegister-AzModuleVTable /ContextAdapter.OnNewRequest, mirroring how auth, telemetry, and completers already work. The generator declares the two parameters statically; their values flow to this pipeline step throughBoundParameters.PolicyTokenAcquirer+ChangeSafetyParameters). This PR consumes those types.@autorest/powershell(replaces the earlier VTable prototype PR), followed by a pilot regeneration, per the design doc.Draft status / CI note
Kept as a draft: CI will not restore/compile until the common package containing
PolicyTokenAcquirer/ChangeSafetyParameters(from #454) is released and this repo's common version is bumped. Local development validated against a locally-built common package.Tests
10 unit tests pass locally:
AcquirePolicyTokenHandlerTests— Stage A step gating (no-op when off,-WhatIf, dedup, write-verb behavior).ChangeSafetyParameterContractTests— pins the parameter names and help text to the values the generator emits.