Decision log of Identity - #47466
Conversation
pvaneck
left a comment
There was a problem hiding this comment.
Overall, looks pretty good. Left some comments.
|
It could be useful to have copilot make a pass at categorizing these a bit and maybe adding a ToC for that (this would avoid reordering the entire document, which I assume you want to be chronological) |
There was a problem hiding this comment.
Pull request overview
This PR adds a new design-decisions.md "Architecture Decision Log" to the azure-identity package — a chronological, non-obvious record of key design/architecture decisions with citations to the PRs (and review discussions) that introduced them. It is positioned as an experiment to give new developers and agents deeper "why" context that isn't covered by the existing user-facing docs. The README is updated to link to it, and cspell.json gets two new allowed words for the new file. The change is documentation-only and introduces no code or behavioral changes.
I spot-checked a representative sample of the historical claims against the current source (e.g., service_fabric.py's connection_verify=False pylint disable, AZURE_IDENTITY_DISABLE_MULTITENANTAUTH, FailedDACCredential, the resolve_tenant "organizations" carve-out, and the AZURE_TOKEN_CREDENTIALS/Kubernetes env vars) and found them accurate. The README link target, TOC anchors, and cspell additions are all correct.
Changes:
- Add
sdk/identity/azure-identity/design-decisions.md(551 lines) documenting design decisions by year with PR citations and maintenance guidance. - Add a link to the decision log from the README's Contributing section.
- Add
nocaeandROPCtocspell.jsonfor the new file.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/identity/azure-identity/design-decisions.md | New architecture decision log; content verified against codebase on a sample basis; two entries deviate from the doc's own year-month date convention. |
| sdk/identity/azure-identity/README.md | Adds a link to the new decision log, consistent with the existing absolute-URL link style. |
| .vscode/cspell.json | Adds nocae and ROPC as allowed words for the new file (other identity jargon is already tolerated). |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (9)
sdk/identity/azure-identity/design-decisions.md:540
- This summary repeats the overly broad constructor rule and even cites
FailedDACCredential, whose purpose is to handle credentials that intentionally fail construction. Narrow the guidance so agents do not remove required constructor validation.
- **Constructors must be side-effect-free and cheap**, because DAC probes and discards
credentials (#12172, #8294, #31745, `FailedDACCredential`).
sdk/identity/azure-identity/design-decisions.md:502
- PR #42145 merged on 2025-07-23, not in August. This should use
(2025-07)under the document's merge-date convention.
PR [#42145](https://github.com/Azure/azure-sdk-for-python/pull/42145) (2025-08)
sdk/identity/azure-identity/design-decisions.md:354
- The cited PR explains that MSAL caches are agnostic to token claims, not scopes. Cache entries are searched with a
targetcontaining the scopes, so calling them scope-agnostic gives agents the wrong model for why CAE needs separate caches.
consequence: because MSAL token caches are scope-agnostic and would otherwise overwrite a
CAE token with a non-CAE one (or vice versa), each credential now keeps **two** MSAL
sdk/identity/azure-identity/design-decisions.md:371
within_credential_chaindoes not reclassify authentication errors; current uses control IMDS probing, proxy behavior, and logging. DAC-specific error reclassification is keyed bywithin_dac, so conflating the two context variables could cause future changes to apply the wrong chain behavior.
signal throughout the codebase, and why credentials inside DAC re-classify
`ClientAuthenticationError` into `CredentialUnavailableError` (via the `within_dac` /
`within_credential_chain` context vars) to avoid terminating the chain.
sdk/identity/azure-identity/design-decisions.md:422
- This describes
allow_brokeras a current package option, but neither the current broker implementation nor its generated API surface contains that parameter. Preserve the historical decision while making clear that the switch was subsequently removed, otherwise readers may try to configure a nonexistent option.
extensions). In that package, `allow_broker` defaults to `True` because installing the package
*is* the opt-in.
sdk/identity/azure-identity/design-decisions.md:449
AccessTokenInfois a regular extensible class, not a dataclass, and only some credentials useGetTokenMixin; CLI, chained, and other credentials implement or delegate these methods separately. The current wording incorrectly presents one implementation strategy as universal.
solution adds a parallel `get_token_info(*scopes, options: TokenRequestOptions) ->
AccessTokenInfo`, where `TokenRequestOptions` is a `TypedDict` options bag and `AccessTokenInfo`
is an extensible dataclass. Crucially, **`get_token` is kept for backward compatibility** and
both methods funnel into the same `GetTokenMixin._get_token_base`, so cache/retry logic is
shared and `get_token` simply drops the extra `AccessTokenInfo` fields. This is why the
sdk/identity/azure-identity/design-decisions.md:198
- This absolute rule conflicts with credentials such as
WorkloadIdentityCredential, which intentionally validate configuration and raise during construction (as this log later documents). PR #12172 supports avoiding unnecessary cache loading and user-visible logging, not a blanket ban on every observable constructor effect.
This issue also appears on line 539 of the same file.
### Credential constructors must not have observable side effects (lazy cache load)
sdk/identity/azure-identity/design-decisions.md:463
- These do not follow the stated merge-date convention: PR #39785 merged on 2025-02-20 and PR #40613 merged on 2025-04-21. Using release months here makes the supposedly auditable timeline inaccurate.
This issue also appears on line 502 of the same file.
PRs [#39785](https://github.com/Azure/azure-sdk-for-python/pull/39785) (2025-03),
[#40613](https://github.com/Azure/azure-sdk-for-python/pull/40613) (2025-05),
[#41822](https://github.com/Azure/azure-sdk-for-python/pull/41822) (2025-07)
sdk/identity/azure-identity/design-decisions.md:488
- This entry is out of the strict merge-date order established above: introducing PR #41709 merged on 2025-07-10, before broker PR #40335 merged on 2025-07-17. Move the complete
AZURE_TOKEN_CREDENTIALSentry ahead of the broker entry so readers can trust the chronology.
### `AZURE_TOKEN_CREDENTIALS` can select a single credential; explicit args win
PRs [#41709](https://github.com/Azure/azure-sdk-for-python/pull/41709) (2025-07),
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d72d0497-7d50-4f07-bfc9-aa869b906d52
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
sdk/identity/azure-identity/design-decisions.md:400
- PR #32200 merged on 2023-09-25, not in October. This should use the actual merge month so the timeline follows the convention stated at the top of the file.
PRs [#32200](https://github.com/Azure/azure-sdk-for-python/pull/32200) (2023-10),
sdk/identity/azure-identity/design-decisions.md:416
- These citations start with the removal from core, so they do not support the heading's “added to core” history or the package creation. WAM was added by #25944 in 2022, while #32360 created
azure-identity-brokerbefore #32369 removed the core support. Reframe this as the 2023 extraction and cite the earlier addition as context; otherwise the entry conflicts with the log's strict introducing-PR chronology.
### Broker support: added to core, then pulled out into `azure-identity-broker`
PRs [#32369](https://github.com/Azure/azure-sdk-for-python/pull/32369) (2023-10),
[#32442](https://github.com/Azure/azure-sdk-for-python/pull/32442) (2023-10)
sdk/identity/azure-identity/design-decisions.md:422
allow_brokeris no longer an option inazure-identity-broker; #34186 explicitly removed the unused kwarg in 2024. Present tense here teaches readers to look for a nonexistent setting, so record both the launch behavior and its removal.
extensions). In that package, `allow_broker` defaults to `True` because installing the package
*is* the opt-in.
sdk/identity/azure-identity/design-decisions.md:356
- The cache key limitation is about claims/capabilities, not scopes—#30777 describes the caches as claims-agnostic. This also overstates the MSAL application split: sync
MsalCredentialhas the two application maps, while async credentials useAadClientand only split their caches. Keeping those distinctions matters for agents modifying async CAE behavior.
consequence: because MSAL token caches are scope-agnostic and would otherwise overwrite a
CAE token with a non-CAE one (or vice versa), each credential now keeps **two** MSAL
applications and **two** caches (`_client_applications`/`_cae_client_applications`,
`_cache`/`_cae_cache`; persistent caches get `.nocae`/`.cae` suffixes). `AZURE_IDENTITY_DISABLE_CP1`
sdk/identity/azure-identity/design-decisions.md:432
- This migration applied only to synchronous managed-identity implementations. The
aioplatform credentials still inheritAsyncManagedIdentityBaseand useAsyncManagedIdentityClient(for example,aio/_credentials/app_service.py:7-16), consistent with this document's later note that MSAL has no async API. Qualify both the heading and body to avoid directing async changes toward the wrong architecture.
### All managed-identity credentials migrate onto `msal.ManagedIdentityClient`
PR [#36225](https://github.com/Azure/azure-sdk-for-python/pull/36225) (2024-07)
A new `MsalManagedIdentityClient` base wraps `msal.ManagedIdentityClient` (passing an
azure-core-backed `http_client`), and every platform credential (App Service, Arc, Azure ML,
sdk/identity/azure-identity/design-decisions.md:463
- The first two merge months are incorrect: #39785 merged on 2025-02-20 and #40613 merged on 2025-04-21. Update them to preserve the auditable merge-date chronology promised by this document.
PRs [#39785](https://github.com/Azure/azure-sdk-for-python/pull/39785) (2025-03),
[#40613](https://github.com/Azure/azure-sdk-for-python/pull/40613) (2025-05),
[#41822](https://github.com/Azure/azure-sdk-for-python/pull/41822) (2025-07)
sdk/identity/azure-identity/design-decisions.md:524
- The cited introducing PR #43287 used
use_token_proxy; the currentenable_azure_proxyname came from follow-up #44147. Without that citation, readers cannot reconcile the claimed current API with the cited decision, which is exactly the context this log is intended to preserve.
Entra limits Federated Identity Credentials (FICs) per managed identity, which large AKS
clusters can exhaust. Binding mode routes the FIC exchange through an AKS-side proxy
(`enable_azure_proxy=True`, configured via `AZURE_KUBERNETES_TOKEN_PROXY` and SNI/CA env vars)
so a single set of FICs serves many pods. The proxy path is intentionally skipped when running
[Pilot] PR Pipeline Failure AnalysisA CI pipeline failed on this pull request. Here is an automated analysis of what went wrong and how to get the build green. What failedThe Spell Check validation task failed on
Build: https://dev.azure.com/azure-sdk/public/_build/results?buildId=6629756 Recommended next steps
Raw pipeline analysis (azsdk ci analyze)
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
sdk/identity/azure-identity/design-decisions.md:502
- #42145 merged on 2025-07-23, so the stated August merge month is incorrect under this log's convention.
PR [#42145](https://github.com/Azure/azure-sdk-for-python/pull/42145) (2025-08)
sdk/identity/azure-identity/design-decisions.md:479
- This violates the file's strict chronological-order rule: #41709 merged on 2025-07-10, while #40335 merged on 2025-07-17. Move the
AZURE_TOKEN_CREDENTIALSentry before the broker entry so the timeline remains trustworthy.
### Brokered auth in the DAC chain — last, silent-only, conditional
PR [#40335](https://github.com/Azure/azure-sdk-for-python/pull/40335) (2025-07)
sdk/identity/azure-identity/design-decisions.md:342
- This entry attributes the current construction-error handling to the 2023 PRs, but neither PR added a
try/except ValueError: #28536 guarded DAC construction with an environment-variable check, and #29728 added the exclusion flag. TheFailedDACCredential/exception-catching behavior arrived in #42346 in 2025, so the current text gives agents an incorrect history of this code path.
### `WorkloadIdentityCredential` can crash DAC construction
PRs [#28536](https://github.com/Azure/azure-sdk-for-python/pull/28536) (2023-02),
[#29728](https://github.com/Azure/azure-sdk-for-python/pull/29728) (2023-04)
`WorkloadIdentityCredential.__init__` raises `ValueError` if its required env vars
(`AZURE_TENANT_ID`, `AZURE_CLIENT_ID`, `AZURE_FEDERATED_TOKEN_FILE`) are missing — which would
crash DAC on any non-AKS machine. DAC wraps construction in a `try/except ValueError` so the
chain can continue. (The `FailedDACCredential` placeholder that formalized this handling came
later — see the 2025 section.)
sdk/identity/azure-identity/design-decisions.md:400
- The log defines these labels as merge dates, but #32200 merged on 2023-09-25, not in October. Correcting the month keeps the historical record auditable.
This issue also appears on line 502 of the same file.
PRs [#32200](https://github.com/Azure/azure-sdk-for-python/pull/32200) (2023-10),
sdk/identity/azure-identity/design-decisions.md:463
- These merge months are incorrect: #39785 merged on 2025-02-20 and #40613 merged on 2025-04-21. Since this file explicitly uses merge dates to establish chronology, both labels need correction.
This issue also appears on line 477 of the same file.
PRs [#39785](https://github.com/Azure/azure-sdk-for-python/pull/39785) (2025-03),
[#40613](https://github.com/Azure/azure-sdk-for-python/pull/40613) (2025-05),
[#41822](https://github.com/Azure/azure-sdk-for-python/pull/41822) (2025-07)
Azure/azure-sdk-tools#16050
asked Copilot to analyze git history, PR descriptions, etc. to document a historical timeline of (non-obvious) decisions made when developing Identity
experiment to see if this is useful context to have in the repo for agents. would possibly require automated agentic workflows to maintain
Prompt: