Skip to content

refactor(oidc): inject Azure Key Vault client factory - #1469

Merged
cristim merged 1 commit into
mainfrom
chore/1464-oidc-azure-seam
Jul 21, 2026
Merged

cristim merged 1 commit into
mainfrom
chore/1464-oidc-azure-seam

Conversation

@cristim

@cristim cristim commented Jul 19, 2026

Copy link
Copy Markdown
Member

Closes #1464

Problem

NewAzureKeyVaultSigner in internal/oidc/azure_signer.go called
azidentity.NewDefaultAzureCredential(nil) and azkeys.NewClient(...)
inline, with no injection seam. Existing tests only exercised
NewAzureKeyVaultSignerFromClient with a fake AzureKeyVaultClient. In
internal/oidc/factory.go, the Azure branch of NewSignerFromEnv calls
NewAzureKeyVaultSigner when both CUDLY_SIGNING_KEY_VAULT_URL and
CUDLY_SIGNING_KEY_NAME are set, and that branch was untested: any test
reaching it would build the live DefaultAzureCredential chain, risking
az/pwsh process spawns and macOS keychain prompts.

Fix

  • Added a package-level newAzureKeyVaultClient factory hook in
    azure_signer.go that holds the credential + client construction.
    NewAzureKeyVaultSigner now calls the hook instead of inlining the
    construction. Zero production behavior change (same error wrapping,
    same call sequence).
  • This mirrors the AzureSPProber.NewClient injected-factory pattern in
    internal/commitmentopts/probe_azure.go, adapted to this package's
    package-level-var style since AzureKeyVaultSigner construction is a
    free function rather than a method on a struct with an overridable
    field.
  • Added TestNewSignerFromEnv_AzureFullyConfigured in
    internal/oidc/azure_factory_test.go, which overrides the hook with
    the existing fakeAzureKeyVaultClient, calls NewSignerFromEnv with
    both Azure env vars set, and asserts a non-nil signer, no error, and
    that the hook received the configured vault URL.
  • Checked the other test in the file
    (TestNewSignerFromEnv_AzureHalfConfigured) - it only exercises the
    half-configured and both-empty cases, which return before reaching the
    hook, so no further protection was needed.

Verification

  • gofmt -l internal/ - clean
  • go vet ./internal/oidc/ - clean
  • go test ./internal/oidc/ -count=1 - all tests pass (28 tests,
    including the new one)
  • go build ./... - clean

🤖 Generated with Claude Code

Extract the azidentity.NewDefaultAzureCredential + azkeys.NewClient
construction out of NewAzureKeyVaultSigner into a package-level
newAzureKeyVaultClient hook, mirroring the AzureSPProber.NewClient
seam in internal/commitmentopts/probe_azure.go. This lets tests
override the hook with a fake and exercise the fully-configured
Azure branch of NewSignerFromEnv without spawning the live
credential chain (az/pwsh, macOS keychain prompts).

Adds TestNewSignerFromEnv_AzureFullyConfigured, which was previously
impossible to cover.
@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/low Minor harm urgency/this-quarter Within the quarter impact/internal Team-internal only effort/s Hours type/chore Maintenance / non-user-visible labels Jul 19, 2026
@coderabbitai

coderabbitai Bot commented Jul 19, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a55f973d-8c85-4886-bde8-1bc9af468ea5

📥 Commits

Reviewing files that changed from the base of the PR and between c848a07 and 3a123f4.

📒 Files selected for processing (2)
  • internal/oidc/azure_factory_test.go
  • internal/oidc/azure_signer.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/1464-oidc-azure-seam

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Jul 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit f10f9ea into main Jul 21, 2026
19 checks passed
@cristim
cristim deleted the chore/1464-oidc-azure-seam branch July 27, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p2 Backlog-worthy severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(oidc): add injection seam to NewAzureKeyVaultSigner and cover fully-configured factory branch

1 participant