diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 5bd968af0..01257deb5 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -365,7 +365,7 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda serviceResult{name: "OpenSearch", recs: osRecs, err: osErr}, serviceResult{name: "Redshift", recs: redshiftRecs, err: redshiftErr}, serviceResult{name: "SavingsPlans", recs: spRecs, err: spErr}, - ), nil + ) } // serviceResult bundles a per-service collection outcome for the deterministic @@ -382,10 +382,26 @@ type serviceResult struct { // results in the order the slice is passed — callers must preserve the // canonical EC2 → RDS → ElastiCache → OpenSearch → Redshift → SavingsPlans // order so that order-sensitive consumers stay stable. -func mergeServiceResults(results ...serviceResult) []common.Recommendation { +// +// Partial failure is tolerated: as long as at least one service succeeded, the +// successful services' recommendations are returned with a nil error and the +// failures are logged at WARN. But when EVERY service errored (e.g. a sustained +// Cost Explorer throttle that exhausts each service's per-combo retries), the +// merge returns a wrapped error instead of an empty-but-nil-error result +// (08-H4). Returning (recs, nil) on a total failure makes a throttled run +// indistinguishable from "no savings available", which an operator can misread +// as "nothing to buy": the same hazard the per-service all-combos-failed guard +// in GetRecommendationsForService prevents one level down. +func mergeServiceResults(results ...serviceResult) ([]common.Recommendation, error) { total := 0 + failures := 0 + var lastErr error for _, r := range results { total += len(r.recs) + if r.err != nil { + failures++ + lastErr = r.err + } } out := make([]common.Recommendation, 0, total) for _, r := range results { @@ -395,5 +411,8 @@ func mergeServiceResults(results ...serviceResult) []common.Recommendation { } out = append(out, r.recs...) } - return out + if failures == len(results) && failures > 0 { + return nil, fmt.Errorf("all %d AWS recommendation services failed: %w", failures, lastErr) + } + return out, nil } diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index cc700c7a9..63c45779c 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -819,3 +819,53 @@ func TestGetRecommendations_RI_PaginationCapError(t *testing.T) { assert.Equal(t, maxRecommendationPages, mock.calls, "must stop exactly at the cap, not one page later") } + +// TestMergeServiceResults_AllFailIsError is the 08-H4 regression test at the +// locus of the fix. When EVERY per-service collection errored (e.g. a sustained +// Cost Explorer throttle that exhausts each service's per-combo retries), +// mergeServiceResults must return a non-nil error so GetAllRecommendations +// surfaces "the whole run failed" rather than (emptyRecs, nil) -- which an +// operator reads as "no savings available". Tested directly because exercising +// it through the six concurrent goroutines of GetAllRecommendations would race +// on the shared *RateLimiter (a separate, out-of-scope issue, 08-C1). +// +// Pre-fix mergeServiceResults returned only []common.Recommendation and dropped +// every error to a WARN log; this test asserts the new (recs, error) contract. +func TestMergeServiceResults_AllFailIsError(t *testing.T) { + throttle := newThrottleError() + + // All services failed -> error, nil recs. + recs, err := mergeServiceResults( + serviceResult{name: "EC2", err: throttle}, + serviceResult{name: "RDS", err: throttle}, + serviceResult{name: "ElastiCache", err: throttle}, + serviceResult{name: "OpenSearch", err: throttle}, + serviceResult{name: "Redshift", err: throttle}, + serviceResult{name: "SavingsPlans", err: throttle}, + ) + require.Error(t, err, "all-services-failed must surface an error, not look like 'no recs'") + assert.Contains(t, err.Error(), "all", "error should signal that every service failed") + assert.Nil(t, recs) + + // Partial failure is still tolerated: surviving service's recs returned, nil error. + ec2Rec := common.Recommendation{Service: common.ServiceEC2} + recs, err = mergeServiceResults( + serviceResult{name: "EC2", recs: []common.Recommendation{ec2Rec}}, + serviceResult{name: "RDS", err: throttle}, + serviceResult{name: "ElastiCache", err: throttle}, + serviceResult{name: "OpenSearch", err: throttle}, + serviceResult{name: "Redshift", err: throttle}, + serviceResult{name: "SavingsPlans", err: throttle}, + ) + require.NoError(t, err, "a single surviving service must keep the run successful") + assert.Len(t, recs, 1) + assert.Equal(t, common.ServiceEC2, recs[0].Service) + + // No failures at all: empty-but-successful run stays nil error. + recs, err = mergeServiceResults( + serviceResult{name: "EC2"}, + serviceResult{name: "RDS"}, + ) + require.NoError(t, err) + assert.Empty(t, recs) +} diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index 9dc945439..a7b49b864 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -473,6 +473,13 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, // Returns an error when an offering matches on node type and duration but carries an // unrecognised ReservedNodeOfferingType -- this surfaces unexpected enum values rather // than silently skipping them and potentially committing to the wrong offering. +// +// In addition to node type and duration, the requested payment option is matched +// against the offering's price shape (08-H2). Redshift does not encode the +// payment option in the Regular/Upgradable ReservedNodeOfferingType enum; it is +// expressed through the offering's FixedPrice (upfront) and recurring charge, +// so an offering whose price shape does not match the operator's chosen payment +// option is skipped rather than purchased on the wrong terms. func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNodeOffering, rec common.Recommendation) (string, error) { for _, offering := range offerings { if offering.NodeType == nil || *offering.NodeType != rec.ResourceType { @@ -482,15 +489,62 @@ func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNode continue } offeringTypeStr := string(offering.ReservedNodeOfferingType) - if offeringTypeStr != "Regular" && offeringTypeStr != "Upgradable" { + if !c.matchesOfferingType(offeringTypeStr) { return "", fmt.Errorf("Redshift offering %s has unexpected type %q (rec: %s)", aws.ToString(offering.ReservedNodeOfferingId), offeringTypeStr, rec.ResourceType) } + if !matchesPaymentOption(offering, rec.PaymentOption) { + continue + } return aws.ToString(offering.ReservedNodeOfferingId), nil } return "", nil } +// offeringRecurringRate returns the offering's recurring charge: the hourly +// RecurringCharges entry when present, otherwise the UsagePrice. Redshift +// no-upfront/partial-upfront offerings carry a recurring charge while +// all-upfront offerings do not. +func offeringRecurringRate(offering redshifttypes.ReservedNodeOffering) float64 { + for _, charge := range offering.RecurringCharges { + if charge.RecurringChargeAmount != nil && aws.ToString(charge.RecurringChargeFrequency) == "Hourly" { + return *charge.RecurringChargeAmount + } + } + return aws.ToFloat64(offering.UsagePrice) +} + +// matchesPaymentOption reports whether a Redshift reserved-node offering's price +// shape matches the requested payment option (08-H2). The payment option is not +// carried in the Regular/Upgradable offering-type enum, so it is derived from +// the upfront (FixedPrice) and recurring components: +// +// - all-upfront: upfront > 0, recurring == 0 +// - no-upfront: upfront == 0, recurring > 0 +// - partial-upfront: upfront > 0, recurring > 0 +// +// An empty/unknown requested option matches nothing (the caller skips the +// offering and ultimately errors with "no offerings found") so a malformed +// recommendation never buys on an arbitrarily-chosen payment option. AWS RI +// pricing has no fractional cents below this threshold, so a strict > 0 test is +// safe against float noise. +func matchesPaymentOption(offering redshifttypes.ReservedNodeOffering, paymentOption string) bool { + upfront := aws.ToFloat64(offering.FixedPrice) + recurring := offeringRecurringRate(offering) + hasUpfront := upfront > 0 + hasRecurring := recurring > 0 + switch paymentOption { + case "all-upfront": + return hasUpfront && !hasRecurring + case "no-upfront": + return !hasUpfront && hasRecurring + case "partial-upfront": + return hasUpfront && hasRecurring + default: + return false + } +} + // matchesDuration checks if the offering duration matches func (c *Client) matchesDuration(offeringDuration *int32, term string) bool { if offeringDuration == nil { @@ -505,10 +559,11 @@ func (c *Client) matchesDuration(offeringDuration *int32, term string) bool { return int(offeringMonths) == requiredMonths } -// matchesOfferingType checks if the offering type is a valid Redshift reserved node offering type. -// Redshift uses "Regular" and "Upgradable" as offering type identifiers — not payment-option strings -// like other AWS services. Payment flexibility is encoded differently in the Redshift API. -func (c *Client) matchesOfferingType(offeringType string, _ string) bool { +// matchesOfferingType checks if the offering type is a valid Redshift reserved +// node offering type. Redshift uses "Regular" and "Upgradable" as offering-type +// identifiers; this is orthogonal to the payment option, which is matched +// separately by matchesPaymentOption from the offering's price shape (08-H2). +func (c *Client) matchesOfferingType(offeringType string) bool { return offeringType == "Regular" || offeringType == "Upgradable" } @@ -542,26 +597,40 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati offering := result.ReservedNodeOfferings[0] details := &common.OfferingDetails{ - OfferingID: aws.ToString(offering.ReservedNodeOfferingId), - ResourceType: aws.ToString(offering.NodeType), - Term: fmt.Sprintf("%d", aws.ToInt32(offering.Duration)), - PaymentOption: string(offering.ReservedNodeOfferingType), + OfferingID: aws.ToString(offering.ReservedNodeOfferingId), + ResourceType: aws.ToString(offering.NodeType), + Term: fmt.Sprintf("%d", aws.ToInt32(offering.Duration)), + // Report the derived payment option (08-H2): the offering's price shape, + // not the Regular/Upgradable ReservedNodeOfferingType enum, which is not + // a payment option. Lets the caller reconcile the bought terms. + PaymentOption: derivePaymentOption(offering), UpfrontCost: aws.ToFloat64(offering.FixedPrice), - RecurringCost: aws.ToFloat64(offering.UsagePrice), + RecurringCost: offeringRecurringRate(offering), Currency: aws.ToString(offering.CurrencyCode), } - for _, charge := range offering.RecurringCharges { - if charge.RecurringChargeAmount != nil && charge.RecurringChargeFrequency != nil { - if *charge.RecurringChargeFrequency == "Hourly" { - details.RecurringCost = *charge.RecurringChargeAmount - } - } - } - return details, nil } +// derivePaymentOption infers the offering's payment option from its price shape +// (08-H2), returning the canonical CUDly payment-option string. Returns "unknown" +// when the shape matches no known option (e.g. an offering with neither upfront +// nor recurring charge) so callers never mistake it for a deliberate choice. +func derivePaymentOption(offering redshifttypes.ReservedNodeOffering) string { + hasUpfront := aws.ToFloat64(offering.FixedPrice) > 0 + hasRecurring := offeringRecurringRate(offering) > 0 + switch { + case hasUpfront && !hasRecurring: + return "all-upfront" + case !hasUpfront && hasRecurring: + return "no-upfront" + case hasUpfront && hasRecurring: + return "partial-upfront" + default: + return "unknown" + } +} + // GetValidResourceTypes returns valid Redshift node types by querying the API func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { nodeTypes := make(map[string]bool) diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 09764e59c..9920f575b 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -259,6 +259,12 @@ func TestClient_ValidateOffering(t *testing.T) { NodeType: aws.String("dc2.large"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // partial-upfront price shape: upfront + recurring (08-H2). + FixedPrice: aws.Float64(500.0), + RecurringCharges: []types.RecurringCharge{{ + RecurringChargeAmount: aws.Float64(0.10), + RecurringChargeFrequency: aws.String("Hourly"), + }}, }, }, }, nil) @@ -411,7 +417,12 @@ func TestClient_PurchaseCommitment_TagsCarryRichDescriptors(t *testing.T) { NodeType: aws.String("ra3.xlplus"), Duration: aws.Int32(94608000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), - FixedPrice: aws.Float64(500.0), + // partial-upfront price shape: upfront + recurring (08-H2). + FixedPrice: aws.Float64(500.0), + RecurringCharges: []types.RecurringCharge{{ + RecurringChargeAmount: aws.Float64(0.10), + RecurringChargeFrequency: aws.String("Hourly"), + }}, }}, }, nil) @@ -473,24 +484,23 @@ func TestClient_MatchesDuration(t *testing.T) { func TestClient_MatchesOfferingType(t *testing.T) { client := &Client{} - // Redshift uses "Regular" and "Upgradable" offering types, not payment options - // The function returns true for valid offering types regardless of payment option + // Redshift uses "Regular" and "Upgradable" offering types; this is a + // validity check orthogonal to the payment option (matched separately by + // matchesPaymentOption from the offering's price shape, 08-H2). tests := []struct { - name string - offeringType string - paymentOption string - expected bool + name string + offeringType string + expected bool }{ - {"Regular offering type accepts any payment", "Regular", "all-upfront", true}, - {"Regular offering type with partial", "Regular", "partial-upfront", true}, - {"Upgradable offering type", "Upgradable", "all-upfront", true}, - {"Unknown offering type rejected", "Unknown", "all-upfront", false}, - {"Empty offering type rejected", "", "partial-upfront", false}, + {"Regular offering type accepted", "Regular", true}, + {"Upgradable offering type accepted", "Upgradable", true}, + {"Unknown offering type rejected", "Unknown", false}, + {"Empty offering type rejected", "", false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := client.matchesOfferingType(tt.offeringType, tt.paymentOption) + result := client.matchesOfferingType(tt.offeringType) assert.Equal(t, tt.expected, result) }) } @@ -611,6 +621,8 @@ func TestClient_PurchaseCommitment_PurchaseAPIError(t *testing.T) { NodeType: aws.String("dc2.large"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // all-upfront price shape: upfront only, no recurring (08-H2). + FixedPrice: aws.Float64(1000.0), }, }, }, nil).Once() @@ -653,6 +665,8 @@ func TestClient_PurchaseCommitment_EmptyResponse(t *testing.T) { NodeType: aws.String("dc2.large"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // all-upfront price shape: upfront only, no recurring (08-H2). + FixedPrice: aws.Float64(1000.0), }, }, }, nil).Once() @@ -698,6 +712,8 @@ func TestClient_GetOfferingDetails_Success(t *testing.T) { NodeType: aws.String("dc2.large"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // all-upfront price shape: upfront only, no recurring (08-H2). + FixedPrice: aws.Float64(500.0), }, }, }, nil).Once() @@ -825,6 +841,8 @@ func TestClient_GetOfferingDetails_EmptyResponseAfterFind(t *testing.T) { NodeType: aws.String("dc2.large"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // all-upfront price shape: upfront only, no recurring (08-H2). + FixedPrice: aws.Float64(500.0), }, }, }, nil).Once() @@ -870,6 +888,8 @@ func TestClient_GetOfferingDetails_EmptyOfferingsAfterFind(t *testing.T) { NodeType: aws.String("dc2.large"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // all-upfront price shape: upfront only, no recurring (08-H2). + FixedPrice: aws.Float64(500.0), }, }, }, nil).Once() @@ -1035,6 +1055,8 @@ func expectRSOffering(m *MockRedshiftClient) { NodeType: aws.String("ra3.xlplus"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // all-upfront price shape matching rsIdemRec (08-H2). + FixedPrice: aws.Float64(1000.0), }, }, }, nil) @@ -1200,6 +1222,8 @@ func TestFindOfferingID_HappyPath(t *testing.T) { NodeType: aws.String("ra3.xlplus"), Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + // all-upfront price shape matching rsIdemRec (08-H2). + FixedPrice: aws.Float64(1000.0), }, }, }, nil).Once() @@ -1264,3 +1288,147 @@ func TestClient_tagReservedNode_NameTagPresent(t *testing.T) { mockRS.AssertExpectations(t) mockSTS.AssertExpectations(t) } + +// rsPaymentRec builds a Redshift recommendation with the given payment option. +func rsPaymentRec(paymentOption string) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceDataWarehouse, + ResourceType: "ra3.xlplus", + Count: 1, + PaymentOption: paymentOption, + Term: "1yr", + Region: "us-east-1", + Details: common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 1}, + } +} + +// rsOffering builds a single matching-node/duration offering with an explicit +// price shape so payment-option matching (08-H2) can be exercised. +func rsOffering(id string, fixedPrice float64, recurringHourly float64) types.ReservedNodeOffering { + o := types.ReservedNodeOffering{ + ReservedNodeOfferingId: aws.String(id), + NodeType: aws.String("ra3.xlplus"), + Duration: aws.Int32(31536000), + ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + FixedPrice: aws.Float64(fixedPrice), + } + if recurringHourly > 0 { + o.RecurringCharges = []types.RecurringCharge{{ + RecurringChargeAmount: aws.Float64(recurringHourly), + RecurringChargeFrequency: aws.String("Hourly"), + }} + } + return o +} + +// TestFindOfferingID_PaymentOptionMustMatch is the 08-H2 regression test: the +// requested payment option must be matched against the offering's price shape, +// not silently ignored. Before the fix, matchesOfferingType discarded the +// payment option entirely, so a no-upfront request was filled with the first +// node-type/duration offering regardless of its actual payment terms. This test +// would FAIL pre-fix (it would return the all-upfront offering for a no-upfront +// request) and passes after. +func TestFindOfferingID_PaymentOptionMustMatch(t *testing.T) { + // Price shapes: all-upfront (upfront only), no-upfront (recurring only), + // partial-upfront (both). + allUpfront := rsOffering("off-all", 1000.0, 0) + noUpfront := rsOffering("off-no", 0, 0.10) + partialUpfront := rsOffering("off-partial", 500.0, 0.05) + + tests := []struct { + name string + paymentOption string + offerings []types.ReservedNodeOffering + wantID string + wantErr bool + }{ + { + name: "no-upfront request picks the no-upfront offering, not all-upfront listed first", + paymentOption: "no-upfront", + offerings: []types.ReservedNodeOffering{allUpfront, noUpfront}, + wantID: "off-no", + }, + { + name: "all-upfront request picks the all-upfront offering, not no-upfront listed first", + paymentOption: "all-upfront", + offerings: []types.ReservedNodeOffering{noUpfront, allUpfront}, + wantID: "off-all", + }, + { + name: "partial-upfront request picks the partial offering", + paymentOption: "partial-upfront", + offerings: []types.ReservedNodeOffering{allUpfront, noUpfront, partialUpfront}, + wantID: "off-partial", + }, + { + name: "no-upfront request errors when only an all-upfront offering exists", + paymentOption: "no-upfront", + offerings: []types.ReservedNodeOffering{allUpfront}, + wantErr: true, + }, + { + name: "empty payment option matches nothing and errors", + paymentOption: "", + offerings: []types.ReservedNodeOffering{allUpfront, noUpfront, partialUpfront}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockRS := &MockRedshiftClient{} + t.Cleanup(func() { mockRS.AssertExpectations(t) }) + client := &Client{client: mockRS, region: "us-east-1"} + + mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ + ReservedNodeOfferings: tt.offerings, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), rsPaymentRec(tt.paymentOption), "") + + if tt.wantErr { + assert.Error(t, err) + assert.Empty(t, id) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.wantID, id) + }) + } +} + +// TestMatchesPaymentOption directly exercises the price-shape classifier (08-H2). +func TestMatchesPaymentOption(t *testing.T) { + tests := []struct { + name string + fixedPrice float64 + recurring float64 + paymentOption string + want bool + }{ + {"all-upfront matches upfront-only", 1000, 0, "all-upfront", true}, + {"all-upfront rejects offering with recurring", 1000, 0.1, "all-upfront", false}, + {"no-upfront matches recurring-only", 0, 0.1, "no-upfront", true}, + {"no-upfront rejects upfront offering", 1000, 0, "no-upfront", false}, + {"partial-upfront matches both", 500, 0.05, "partial-upfront", true}, + {"partial-upfront rejects upfront-only", 500, 0, "partial-upfront", false}, + {"unknown payment option matches nothing", 500, 0.05, "weird", false}, + {"empty payment option matches nothing", 1000, 0, "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := matchesPaymentOption(rsOffering("x", tt.fixedPrice, tt.recurring), tt.paymentOption) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestDerivePaymentOption exercises the payment-option derivation used by +// GetOfferingDetails to surface the actual terms (08-H2). +func TestDerivePaymentOption(t *testing.T) { + assert.Equal(t, "all-upfront", derivePaymentOption(rsOffering("x", 1000, 0))) + assert.Equal(t, "no-upfront", derivePaymentOption(rsOffering("x", 0, 0.1))) + assert.Equal(t, "partial-upfront", derivePaymentOption(rsOffering("x", 500, 0.05))) + assert.Equal(t, "unknown", derivePaymentOption(rsOffering("x", 0, 0))) +}