fix(purchases): resolve per-account credentials in single-account path (closes #602) - #603
Conversation
closes #602) The single-account execution path in executePurchase was passing nil as provCfg to processPurchaseRecommendations. For Azure and GCP this caused CreateAndValidateProvider to fall back to DefaultAzureCredential / ADC, which then exhausted its entire chain (Environment, WorkloadIdentity, ManagedIdentity, CLI) and failed in the Lambda runtime where none of those sources are available. The multi-account fan-out path (executeForAccount) already called resolveAccountProvider correctly; this fix brings the single-account path into parity. Changes: - In executePurchase (single-account path), when selected recs exist, derive the cloud account ID from exec.CloudAccountID (set for plan-with-single-account) or, failing that, from the shared cloud_account_id on the recommendations (direct-execute path where PlanID is empty and exec.CloudAccountID is nil). - Call GetCloudAccount + resolveAccountProvider to build a provCfg with explicit credentials instead of nil. - Surface credential-resolution errors loudly (matching the multi-account pattern) rather than silently falling back to ambient. - Skip credential resolution entirely when no recs are selected to avoid spurious failures on approval-only flows (recs present for contact- email gating but not for purchase). - Add singleCloudAccountIDFromRecs helper and three new tests: - TestExecutePurchase_SingleAccount_AzureUsesResolvedCreds: asserts the factory receives a non-nil provCfg for a direct-execute Azure purchase (regression for #602). - TestExecutePurchase_SingleAccount_CredResolutionError: asserts the error surfaces with the account ID when GetCloudAccount fails. - TestSingleCloudAccountIDFromRecs: table-driven unit test for the new helper covering empty slice, all-nil, single, shared, mixed, and multi-account cases. Fixes QA execution bf7cb732-52c9-4d46-86e2-b1dc208cbe3c. Refs #453, #597.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR fixes P0 credential resolution failures in single-account purchase execution by resolving per-account provider credentials instead of passing nil. Two new helpers extract a single cloud account ID from recommendations and resolve the corresponding provider config; the single-account branch now calls these helpers and returns errors when credential resolution fails, enabling Azure and GCP purchases that previously fell back to broken ambient credentials in Lambda. ChangesSingle-account credential resolution
Sequence DiagramsequenceDiagram
participant executePurchase
participant resolveSingleAccountProvider
participant singleCloudAccountIDFromRecs
participant config as m.config
participant resolveAccountProvider
participant processPurchaseRecommendations
executePurchase->>resolveSingleAccountProvider: resolve per-account credentials
resolveSingleAccountProvider->>singleCloudAccountIDFromRecs: extract account ID from recs
singleCloudAccountIDFromRecs-->>resolveSingleAccountProvider: single account ID or nil
resolveSingleAccountProvider->>config: GetCloudAccount(cloudAccountID)
config-->>resolveSingleAccountProvider: account record or nil/error
resolveSingleAccountProvider->>resolveAccountProvider: resolve credentials for account
resolveAccountProvider-->>resolveSingleAccountProvider: provider config or error
resolveSingleAccountProvider-->>executePurchase: (provCfg, error)
executePurchase->>processPurchaseRecommendations: process with resolved provider config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
DefaultAzureCredentialchain exhaustion error becauseexecutePurchasepassednilasprovCfgon the single-account path, triggering ambient credential discovery that has no valid source in a Lambda runtime.resolveAccountProvidercorrectly; this PR brings the single-account path into parity.resolveSingleAccountProviderhelper (extracted to keepexecutePurchaseunder gocyclo:10) andsingleCloudAccountIDFromRecshelper that derives a shared account ID from recommendation records for the direct-execute case whereexec.CloudAccountIDis nil.What changed
internal/purchase/execution.go:executePurchasenow callsm.resolveSingleAccountProviderbeforeprocessPurchaseRecommendations; resolution is skipped when no recs are selected (avoids DB round-trips and spurious failures on approval-only flows).internal/purchase/execution.go: NewresolveSingleAccountProvidermethod resolves credentials fromexec.CloudAccountIDor, when nil, from the single sharedcloud_account_idon the recommendations. Errors surface loudly; nil is returned only when no account is identifiable (ambient/AWS-role path).internal/purchase/execution.go: NewsingleCloudAccountIDFromRecshelper returns the single distinct account ID across all recs, or nil if zero or more than one distinct ID is present.internal/purchase/execution_test.go: Three new tests:TestExecutePurchase_SingleAccount_AzureUsesResolvedCreds(regression for fix(purchases): single-account path passes nil provCfg — Azure/GCP purchases fail with DefaultAzureCredential chain errors (P0) #602 -- asserts non-nilprovCfgreaches the factory).TestExecutePurchase_SingleAccount_CredResolutionError(error path -- asserts DB error surfaces with account ID).TestSingleCloudAccountIDFromRecs(table-driven unit test for the helper).AWS / Azure / GCP impact
ManagedIdentityCredential/service_principal/workload_identity_federationcredentials stored in the credential store, instead of falling back to the DefaultAzureCredential chain.resolveSingleAccountProviderreturns(nil, nil)and the ambient Lambda role is used exactly as before.Test plan
go test ./internal/purchase/... -count=1 -short-- 132 passed (131 pre-existing + 1 new test function with 9 sub-tests across 3 test functions)go test ./internal/api/... -count=1 -short-- 1153 passed (regression)go build ./...-- successDefaultAzureCredentialerror in Lambda logsCloses #602
Refs #453, #597
Summary by CodeRabbit
Bug Fixes
Tests