You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(azure): dedupe subscriptions in the multi-subscription fan-out to prevent double-counted savings #59
NewMultiSubscriptionRecommendationsClient (providers/azure/recommendations_multi_subscription.go) builds one subscriptionClient per entry in the accounts slice it is handed, without deduplicating by subscription ID. If that slice ever contains the same subscription twice, the fan-out queries it twice and mergeSubscriptionResults concatenates both responses — double-counting that subscription's recommendations, and therefore its reported savings.
What is and is not verified
Verified: the constructor does not dedupe, and the merge step is a plain concatenation, so a duplicate entry would double-count. Attempted in PartialSubscriptionFailureError would also count the subscription twice.
Not verified: whether ARM's subscriptions.List can actually return the same subscriptionID on more than one page. The review that surfaced this could not establish that it can, and could not establish that it cannot. It is stated openly here rather than asserted either way. Pagination overlap under concurrent tenant mutation is the plausible mechanism, but that is a hypothesis, not an observation.
Given the uncertainty, this is filed as cheap defensive hardening on a money-affecting aggregate rather than as a confirmed bug.
Suggested direction
Dedupe by subscription ID in NewMultiSubscriptionRecommendationsClient, keeping first occurrence, and add a unit test that passes a duplicated account and asserts the subscription is queried once and its recommendations appear once. If a duplicate is dropped, consider a logging.Warnf so a genuinely duplicate-emitting API surface becomes visible rather than silently absorbed.
An alternative placement is fetchAccounts in providers/azure/accounts_cache.go, which would protect every consumer of GetAccounts rather than just the fan-out. That is the broader fix and may be the better one.
Summary
NewMultiSubscriptionRecommendationsClient(providers/azure/recommendations_multi_subscription.go) builds onesubscriptionClientper entry in theaccountsslice it is handed, without deduplicating by subscription ID. If that slice ever contains the same subscription twice, the fan-out queries it twice andmergeSubscriptionResultsconcatenates both responses — double-counting that subscription's recommendations, and therefore its reported savings.What is and is not verified
AttemptedinPartialSubscriptionFailureErrorwould also count the subscription twice.AccountFilterpath is deduplicated (added in feat(azure): org-wide multi-subscription recommendation collection (closes #553) cloud-commitments-cli#1520 while fixing the partially-matching-filter gap), so a duplicated filter entry is safe. The unguarded path is theaccountsslice itself, which reaches the constructor fromgetOrFetchAccounts/ ARMsubscriptions.List.subscriptions.Listcan actually return the samesubscriptionIDon more than one page. The review that surfaced this could not establish that it can, and could not establish that it cannot. It is stated openly here rather than asserted either way. Pagination overlap under concurrent tenant mutation is the plausible mechanism, but that is a hypothesis, not an observation.Given the uncertainty, this is filed as cheap defensive hardening on a money-affecting aggregate rather than as a confirmed bug.
Suggested direction
Dedupe by subscription ID in
NewMultiSubscriptionRecommendationsClient, keeping first occurrence, and add a unit test that passes a duplicated account and asserts the subscription is queried once and its recommendations appear once. If a duplicate is dropped, consider alogging.Warnfso a genuinely duplicate-emitting API surface becomes visible rather than silently absorbed.An alternative placement is
fetchAccountsinproviders/azure/accounts_cache.go, which would protect every consumer ofGetAccountsrather than just the fan-out. That is the broader fix and may be the better one.Provenance
Found during the adversarial review of LeanerCloud/cloud-commitments-cli#1520; not fixed there to keep that PR's diff scoped to the reported findings.