sec(azure): harden HTTP clients + fix fabricated reservation prices in managedredis/savingsplans and 5 pricing clients

pkg/exchange/reshape_crossfamily_test.go

@@ -465,15 +465,15 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) {
 	}
 	// m6i.xlarge: same "m" prefix (gen jump bonus), exact NF, high confidence.
 	nearPerfect := OfferingOption{
-		InstanceType:        "m6i.xlarge",
+		InstanceType:         "m6i.xlarge",
 		EffectiveMonthlyCost: 85, // slightly more expensive
 		NormalizationFactor:  8,
 		SavingsAbs:           floatPtr(220),
 		RecommendationCount:  5,
 	}
 	// r5.xlarge: different prefix (no family gen bonus), same NF, no confidence.
 	crossFamily := OfferingOption{
-		InstanceType:        "r5.xlarge",
+		InstanceType:         "r5.xlarge",
 		EffectiveMonthlyCost: 80, // same cost as source -- better raw price
 		NormalizationFactor:  8,
 	}
@@ -497,13 +497,13 @@ func TestCompositeScore_SameArchOutranksCrossArch(t *testing.T) {
 	}
 	// c6i: same "c" prefix (family gen bonus), Intel x86 (same arch).
 	sameArch := OfferingOption{
-		InstanceType:        "c6i.xlarge",
+		InstanceType:         "c6i.xlarge",
 		EffectiveMonthlyCost: 90,
 		NormalizationFactor:  8,
 	}
 	// c6g: same "c" prefix (family gen bonus), Graviton ARM (cross arch).
 	crossArch := OfferingOption{
-		InstanceType:        "c6g.xlarge",
+		InstanceType:         "c6g.xlarge",
 		EffectiveMonthlyCost: 90,
 		NormalizationFactor:  8,
 	}
@@ -522,14 +522,14 @@ func TestCompositeScore_HighConfidenceOutranksLow(t *testing.T) {
 		MonthlyCost:         70,
 	}
 	highConf := OfferingOption{
-		InstanceType:        "r5.xlarge",
+		InstanceType:         "r5.xlarge",
 		EffectiveMonthlyCost: 70,
 		NormalizationFactor:  8,
 		SavingsAbs:           floatPtr(250),
 		RecommendationCount:  4,
 	}
 	lowConf := OfferingOption{
-		InstanceType:        "r5.xlarge",
+		InstanceType:         "r5.xlarge",
 		EffectiveMonthlyCost: 70,
 		NormalizationFactor:  8,
 		SavingsAbs:           floatPtr(10),
@@ -550,13 +550,13 @@ func TestCompositeScore_AbsentSavingsIsNeutral(t *testing.T) {
 		MonthlyCost:         70,
 	}
 	absentSavings := OfferingOption{
-		InstanceType:        "r6i.xlarge",
+		InstanceType:         "r6i.xlarge",
 		EffectiveMonthlyCost: 70,
 		NormalizationFactor:  8,
 		SavingsAbs:           nil, // not supplied
 	}
 	lowConf := OfferingOption{
-		InstanceType:        "r6i.xlarge",
+		InstanceType:         "r6i.xlarge",
 		EffectiveMonthlyCost: 70,
 		NormalizationFactor:  8,
 		SavingsAbs:           floatPtr(5), // explicitly low confidence

providers/azure/internal/recommendations/converter.go

@@ -40,7 +40,15 @@ type ExtractedFields struct {
 	CommitmentCost   float64
 	EstimatedSavings float64
 	Term             string
-	Scope            string
+	// Scope is populated from the API response ("Shared" or a subscription ID)
+	// but is not yet threaded into the purchase body. All service clients
+	// currently hardcode "appliedScopeType": "Shared", which is correct because
+	// the recommendation filter in recommendations.go already asserts
+	// "properties/scope eq 'Shared'" — only Shared-scope recommendations are
+	// ever requested. This field is retained for future wiring if single-
+	// subscription scoped recommendations are ever requested. See finding M1/M2
+	// in docs/code-review/09-provider-azure.md.
+	Scope string
 	// RecurringMonthlyCost is the monthly recurring charge for this
 	// commitment. Azure Reservation recommendations are all all-upfront
 	// (a single payment, no recurring monthly charge), so this is always

providers/azure/mocks/azure_mocks.go

@@ -206,7 +206,9 @@ func CreateSampleReservationDetails(subscriptionID, region string) []*armconsump
 	}
 }
 
-// CreateSampleVMPricingResponse creates a sample VM pricing response for testing
+// CreateSampleVMPricingResponse creates a sample VM pricing response for testing.
+// Includes both 1-year and 3-year reservation entries so 3YearTerm tests exercise
+// real pricing rather than a fabricated fallback.
 func CreateSampleVMPricingResponse() string {
 	return `{
 		"Items": [
@@ -218,7 +220,18 @@ func CreateSampleVMPricingResponse() string {
 				"productName": "Virtual Machines D Series",
 				"serviceName": "Virtual Machines",
 				"armSkuName": "Standard_D2s_v3",
-				"reservationTerm": "1 Years",
+				"reservationTerm": "1 Year",
+				"type": "Reservation"
+			},
+			{
+				"currencyCode": "USD",
+				"retailPrice": 1200.0,
+				"unitPrice": 0.096,
+				"armRegionName": "eastus",
+				"productName": "Virtual Machines D Series",
+				"serviceName": "Virtual Machines",
+				"armSkuName": "Standard_D2s_v3",
+				"reservationTerm": "3 Years",
 				"type": "Reservation"
 			},
 			{

providers/azure/provider.go

@@ -165,6 +165,15 @@ func (p *AzureProvider) DisplayName() string {
 }
 
 // IsConfigured checks if Azure credentials are available. Thread-safe via sync.Once.
+//
+// Sticky-failure contract: when ambient credential resolution fails (e.g. a
+// transient IMDS timeout during startup), the error is memoised under
+// sync.Once. Subsequent calls return false for the process lifetime with no
+// retry. For long-lived server deployments this means a transient auth failure
+// at boot requires a process restart to recover. This is acceptable for the
+// current Lambda/container deployment model where restarts are cheap; if a
+// long-lived daemon pattern is introduced, replace the sync.Once with a
+// time-bounded cache or single-flight retry.
 func (p *AzureProvider) IsConfigured() bool {
 	// If credential was injected via SetCredential, skip the Once path.
 	if p.cred != nil {
@@ -191,7 +200,7 @@ func (p *AzureProvider) IsConfigured() bool {
 // GetCredentials returns Azure credentials
 func (p *AzureProvider) GetCredentials() (provider.Credentials, error) {
 	if !p.IsConfigured() {
-		return nil, fmt.Errorf("Azure is not configured")
+		return nil, fmt.Errorf("azure provider is not configured")
 	}
 
 	// DefaultAzureCredential can use multiple sources
@@ -206,7 +215,7 @@ func (p *AzureProvider) GetCredentials() (provider.Credentials, error) {
 // ValidateCredentials validates that Azure credentials are working
 func (p *AzureProvider) ValidateCredentials(ctx context.Context) error {
 	if !p.IsConfigured() {
-		return fmt.Errorf("Azure is not configured")
+		return fmt.Errorf("azure provider is not configured")
 	}
 
 	// Use injected client if available (for testing)
@@ -227,7 +236,7 @@ func (p *AzureProvider) ValidateCredentials(ctx context.Context) error {
 	_, err := pager.NextPage(ctx)
 	if err != nil {
 		logging.Errorf("purchase[Azure]: subscriptions.NextPage failed after %s: %v", time.Since(t0), err)
-		return fmt.Errorf("Azure credentials validation failed: %w", err)
+		return fmt.Errorf("azure credentials validation failed: %w", err)
 	}
 	logging.Infof("purchase[Azure]: subscriptions.NextPage returned in %s", time.Since(t0))
 	return nil
@@ -242,7 +251,7 @@ func (p *AzureProvider) ValidateCredentials(ctx context.Context) error {
 //     where the STS-identified account is always the default).
 func (p *AzureProvider) GetAccounts(ctx context.Context) ([]common.Account, error) {
 	if !p.IsConfigured() {
-		return nil, fmt.Errorf("Azure is not configured")
+		return nil, fmt.Errorf("azure provider is not configured")
 	}
 
 	// Use injected client if available (for testing)
@@ -437,7 +446,7 @@ func (p *AzureProvider) GetSupportedServices() []common.ServiceType {
 // an extra GetAccounts round-trip per iteration.
 func (p *AzureProvider) GetServiceClient(ctx context.Context, service common.ServiceType, region string) (provider.ServiceClient, error) {
 	if !p.IsConfigured() {
-		return nil, fmt.Errorf("Azure is not configured")
+		return nil, fmt.Errorf("azure provider is not configured")
 	}
 
 	// Use explicit subscription ID if configured; otherwise resolve from accounts.
@@ -458,7 +467,7 @@ func (p *AzureProvider) GetServiceClient(ctx context.Context, service common.Ser
 // returned by GetAccounts to avoid O(n) redundant API calls.
 func (p *AzureProvider) GetServiceClientForAccount(ctx context.Context, service common.ServiceType, region, subscriptionID string) (provider.ServiceClient, error) {
 	if !p.IsConfigured() {
-		return nil, fmt.Errorf("Azure is not configured")
+		return nil, fmt.Errorf("azure provider is not configured")
 	}
 	if subscriptionID == "" {
 		return nil, fmt.Errorf("subscriptionID must not be empty")
@@ -499,7 +508,7 @@ func (p *AzureProvider) newServiceClientForSubscription(service common.ServiceTy
 // GetRecommendationsClientForAccount.
 func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider.RecommendationsClient, error) {
 	if !p.IsConfigured() {
-		return nil, fmt.Errorf("Azure is not configured")
+		return nil, fmt.Errorf("azure provider is not configured")
 	}
 
 	// Use explicit subscription ID if configured; otherwise resolve from accounts.
@@ -520,7 +529,7 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider.
 // returned by GetAccounts to avoid O(n) redundant API calls.
 func (p *AzureProvider) GetRecommendationsClientForAccount(ctx context.Context, subscriptionID string) (provider.RecommendationsClient, error) {
 	if !p.IsConfigured() {
-		return nil, fmt.Errorf("Azure is not configured")
+		return nil, fmt.Errorf("azure provider is not configured")
 	}
 	if subscriptionID == "" {
 		return nil, fmt.Errorf("subscriptionID must not be empty")

providers/azure/provider_test.go

@@ -306,7 +306,7 @@ func TestAzureProvider_GetCredentials_NotConfigured(t *testing.T) {
 	if !p.IsConfigured() {
 		_, err := p.GetCredentials()
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure is not configured")
+		assert.Contains(t, err.Error(), "azure provider is not configured")
 	}
 }
 
@@ -319,7 +319,7 @@ func TestAzureProvider_ValidateCredentials(t *testing.T) {
 		})
 		err := p.ValidateCredentials(context.Background())
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure is not configured")
+		assert.Contains(t, err.Error(), "azure provider is not configured")
 	})
 
 	t.Run("success with mock subscriptions client", func(t *testing.T) {
@@ -370,7 +370,7 @@ func TestAzureProvider_ValidateCredentials(t *testing.T) {
 
 		err := p.ValidateCredentials(context.Background())
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure credentials validation failed")
+		assert.Contains(t, err.Error(), "azure credentials validation failed")
 	})
 }
 
@@ -380,7 +380,7 @@ func TestAzureProvider_GetServiceClient_NotConfigured(t *testing.T) {
 	if !p.IsConfigured() {
 		_, err := p.GetServiceClient(context.Background(), common.ServiceCompute, "eastus")
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure is not configured")
+		assert.Contains(t, err.Error(), "azure provider is not configured")
 	}
 }
 
@@ -434,7 +434,7 @@ func TestAzureProvider_GetRecommendationsClient_NotConfigured(t *testing.T) {
 	if !p.IsConfigured() {
 		_, err := p.GetRecommendationsClient(context.Background())
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure is not configured")
+		assert.Contains(t, err.Error(), "azure provider is not configured")
 	}
 }
 
@@ -811,7 +811,7 @@ func TestAzureProvider_GetCredentials(t *testing.T) {
 		})
 		_, err := p.GetCredentials()
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure is not configured")
+		assert.Contains(t, err.Error(), "azure provider is not configured")
 	})
 
 	t.Run("success returns credentials info", func(t *testing.T) {
@@ -1194,7 +1194,7 @@ func TestAzureProvider_GetServiceClientForAccount(t *testing.T) {
 		p.SetCredentialProvider(&mockCredentialProvider{err: errors.New("no cred")})
 		_, err := p.GetServiceClientForAccount(context.Background(), common.ServiceCompute, "eastus", "sub-1")
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure is not configured")
+		assert.Contains(t, err.Error(), "azure provider is not configured")
 	})
 }
 
@@ -1218,6 +1218,6 @@ func TestAzureProvider_GetRecommendationsClientForAccount(t *testing.T) {
 		p.SetCredentialProvider(&mockCredentialProvider{err: errors.New("no cred")})
 		_, err := p.GetRecommendationsClientForAccount(context.Background(), "sub-1")
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "Azure is not configured")
+		assert.Contains(t, err.Error(), "azure provider is not configured")
 	})
 }

providers/azure/recommendations.go

@@ -236,7 +236,11 @@ func (r *RecommendationsClientAdapter) getAdvisorRecommendations(ctx context.Con
 	for pager.More() {
 		page, err := pager.NextPage(ctx)
 		if err != nil {
-			logging.Warnf("Azure Advisor pagination error (partial results may be returned): %v", err)
+			// Advisor partial-OK is intentional: per-service reservation results
+			// (compute, database, etc.) remain useful on their own when Advisor
+			// pagination fails mid-stream. Warnf with the accumulated count so
+			// operators can assess the completeness of the Advisor slice.
+			logging.Warnf("Azure Advisor pagination error after %d recommendations (partial results returned): %v", len(recommendations), err)
 			break
 		}
 		recommendations = r.appendAdvisorPageRecs(params, page, recommendations)
@@ -378,13 +382,13 @@ func serviceFromImpactedField(field *string) string {
 	}
 	f := *field
 	switch {
-	case contains(f, "Microsoft.Compute"):
+	case strings.Contains(f, "Microsoft.Compute"):
 		return string(common.ServiceCompute)
-	case contains(f, "Microsoft.Sql"):
+	case strings.Contains(f, "Microsoft.Sql"):
 		return string(common.ServiceRelationalDB)
-	case contains(f, "Microsoft.Cache"):
+	case strings.Contains(f, "Microsoft.Cache"):
 		return string(common.ServiceCache)
-	case contains(f, "Microsoft.DBforMySQL"), contains(f, "Microsoft.DBforPostgreSQL"):
+	case strings.Contains(f, "Microsoft.DBforMySQL"), strings.Contains(f, "Microsoft.DBforPostgreSQL"):
 		return string(common.ServiceRelationalDB)
 	}
 	return ""
@@ -458,8 +462,3 @@ func shouldIncludeService(params common.RecommendationParams, service common.Ser
 	// Check if this is the requested service
 	return params.Service == service
 }
-
-// contains checks if a string contains a substring
-func contains(s, substr string) bool {
-	return strings.Contains(s, substr)
-}

providers/azure/recommendations_test.go

@@ -2,6 +2,7 @@ package azure
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
@@ -126,7 +127,7 @@ func TestContains(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := contains(tt.s, tt.substr)
+			result := strings.Contains(tt.s, tt.substr)
 			assert.Equal(t, tt.expected, result)
 		})
 	}
@@ -414,3 +415,40 @@ func TestConvertAdvisorRecommendation_UnknownService(t *testing.T) {
 func strPtr(s string) *string {
 	return &s
 }
+
+// TestMergeServiceResults_OrderIsStable is a regression test for L3:
+// the merged output of mergeServiceResults must preserve the canonical
+// compute -> database -> cache -> cosmosdb -> savingsplans -> advisor order
+// regardless of the order arguments are passed in. This test pins the actual
+// argument order used in GetRecommendations so a reordering of the serviceResult
+// literals will be caught immediately.
+func TestMergeServiceResults_OrderIsStable(t *testing.T) {
+	mkRec := func(svc common.ServiceType, marker string) common.Recommendation {
+		return common.Recommendation{Service: svc, Provider: common.ProviderAzure, ResourceType: marker}
+	}
+
+	computeRec := mkRec(common.ServiceCompute, "compute")
+	dbRec := mkRec(common.ServiceRelationalDB, "database")
+	cacheRec := mkRec(common.ServiceCache, "cache")
+	cosmosRec := mkRec(common.ServiceNoSQL, "cosmosdb")
+	spRec := mkRec(common.ServiceSavingsPlans, "savingsplans")
+	advisorRec := mkRec(common.ServiceCompute, "advisor") // Advisor produces Compute recs
+
+	// Replicate the exact call order from GetRecommendations.
+	result := mergeServiceResults(
+		serviceResult{"compute", []common.Recommendation{computeRec}, nil},
+		serviceResult{"database", []common.Recommendation{dbRec}, nil},
+		serviceResult{"cache", []common.Recommendation{cacheRec}, nil},
+		serviceResult{"cosmosdb", []common.Recommendation{cosmosRec}, nil},
+		serviceResult{"savingsplans", []common.Recommendation{spRec}, nil},
+		serviceResult{"advisor", []common.Recommendation{advisorRec}, nil},
+	)
+
+	require.Len(t, result, 6, "all six services must be represented")
+	assert.Equal(t, "compute", result[0].ResourceType, "first must be compute")
+	assert.Equal(t, "database", result[1].ResourceType, "second must be database")
+	assert.Equal(t, "cache", result[2].ResourceType, "third must be cache")
+	assert.Equal(t, "cosmosdb", result[3].ResourceType, "fourth must be cosmosdb")
+	assert.Equal(t, "savingsplans", result[4].ResourceType, "fifth must be savingsplans")
+	assert.Equal(t, "advisor", result[5].ResourceType, "sixth must be advisor (compute-type)")
+}