Skip to content

Commit 2f73e2b

Browse files
committed
fix(azure/managed-redis): fail closed on unknown terms and missing reservation price
- Add parseTermYears helper (mirrors synapse) to reject any term string outside the explicit "1yr"/"3yr" allowlist instead of silently defaulting to 1 year in PurchaseCommitment and GetOfferingDetails - Remove hardcoded 45% reservation-price fallback in getRedisPricing; return an error when the API returns no reservation price for the requested SKU - Add TestPurchaseCommitment_InvalidTerm and TestGetOfferingDetails_InvalidTerm to exercise the new error paths
1 parent 52edace commit 2f73e2b

2 files changed

Lines changed: 47 additions & 10 deletions

File tree

‎providers/azure/services/managedredis/client.go‎

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,20 @@ func (c *ManagedRedisClient) reservationDetailToCommitment(detail *armconsumptio
225225
return cm
226226
}
227227

228+
// parseTermYears maps a reservation term string to an integer year count.
229+
// Returns an error for any value outside the explicit allowlist so callers
230+
// fail closed rather than silently coercing to a 1-year purchase.
231+
func parseTermYears(term string) (int, error) {
232+
switch strings.ToLower(strings.TrimSpace(term)) {
233+
case "", "1", "1yr", "1y":
234+
return 1, nil
235+
case "3", "3yr", "3y":
236+
return 3, nil
237+
default:
238+
return 0, fmt.Errorf("unsupported reservation term: %s", term)
239+
}
240+
}
241+
228242
// PurchaseCommitment purchases Azure Cache for Redis reserved capacity via the Azure Reservations API.
229243
func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, _ common.PurchaseOptions) (common.PurchaseResult, error) {
230244
result := common.PurchaseResult{
@@ -234,16 +248,17 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common.
234248
Timestamp: time.Now(),
235249
}
236250

251+
termYears, termErr := parseTermYears(rec.Term)
252+
if termErr != nil {
253+
result.Error = termErr
254+
return result, result.Error
255+
}
256+
237257
reservationOrderID := uuid.New().String()
238258
apiVersion := "2022-11-01"
239259
purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s",
240260
reservationOrderID, apiVersion)
241261

242-
termYears := 1
243-
if rec.Term == "3yr" || rec.Term == "3" {
244-
termYears = 3
245-
}
246-
247262
requestBody := map[string]interface{}{
248263
"sku": map[string]string{
249264
"name": rec.ResourceType,
@@ -322,9 +337,9 @@ func (c *ManagedRedisClient) ValidateOffering(ctx context.Context, rec common.Re
322337

323338
// GetOfferingDetails retrieves reservation offering details from the Azure Retail Prices API.
324339
func (c *ManagedRedisClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) {
325-
termYears := 1
326-
if rec.Term == "3yr" || rec.Term == "3" {
327-
termYears = 3
340+
termYears, err := parseTermYears(rec.Term)
341+
if err != nil {
342+
return nil, err
328343
}
329344

330345
pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears)
@@ -506,8 +521,7 @@ func (c *ManagedRedisClient) getRedisPricing(ctx context.Context, sku, region st
506521

507522
hoursInTerm := 8760.0 * float64(termYears)
508523
if reservationPrice == 0 {
509-
// Azure Redis Cache reservations typically offer ~55% savings
510-
reservationPrice = onDemandPrice * hoursInTerm * 0.45
524+
return nil, fmt.Errorf("no reservation pricing found for Redis Cache SKU %s (%d year) in region %s", sku, termYears, region)
511525
}
512526

513527
savingsPct := ((onDemandPrice*hoursInTerm - reservationPrice) / (onDemandPrice * hoursInTerm)) * 100

‎providers/azure/services/managedredis/client_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,29 @@ func TestPurchaseCommitment_BadStatus(t *testing.T) {
585585
assert.Contains(t, err.Error(), "reservation purchase failed with status 400")
586586
}
587587

588+
func TestPurchaseCommitment_InvalidTerm(t *testing.T) {
589+
h := &mockHTTPClient{}
590+
t.Cleanup(func() { h.AssertExpectations(t) })
591+
c := NewClientWithHTTP(nil, "sub", "eastus", h)
592+
result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{
593+
ResourceType: "Premium_P1", Term: "5yr", Count: 1,
594+
}, common.PurchaseOptions{})
595+
require.Error(t, err)
596+
assert.False(t, result.Success)
597+
assert.Contains(t, err.Error(), "unsupported reservation term")
598+
}
599+
600+
func TestGetOfferingDetails_InvalidTerm(t *testing.T) {
601+
h := &mockHTTPClient{}
602+
t.Cleanup(func() { h.AssertExpectations(t) })
603+
c := NewClientWithHTTP(nil, "sub", "eastus", h)
604+
_, err := c.GetOfferingDetails(context.Background(), common.Recommendation{
605+
ResourceType: "Premium_P1", Term: "5yr",
606+
})
607+
require.Error(t, err)
608+
assert.Contains(t, err.Error(), "unsupported reservation term")
609+
}
610+
588611
// -- setter tests --
589612

590613
func TestSetterMethods(t *testing.T) {

0 commit comments

Comments
 (0)