diff --git a/CHANGELOG.md b/CHANGELOG.md index 492cfd2a..603abd6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -218,6 +218,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **AWS/OpenSearch:** the STS account-ID lookup used for post-purchase tagging caches only a successful result, so one throttled or timed-out `GetCallerIdentity` no longer disables tagging for the client's lifetime (#53). - **AWS/recommendations:** `normalizeRegionName` maps the current `Europe (Ireland|Frankfurt|London|Paris|Stockholm)` Cost Explorer display names to region codes, so coverage lookups keyed by region code match those recommendations (#53). - **AWS/Savings Plans:** `GetExistingCommitments` returns an error naming the plan ID and value when a plan has an unparseable start or end date, instead of silently leaving the date zero (#53). +- **AWS/Savings Plans:** `GetExistingCommitments` returns an error naming the plan ID when a plan has no start or end date; a zero start date was never treated as recent by the purchase dedupe, so a duplicate purchase could slip through (#53). - **AWS/RDS:** `PurchaseCommitment` refuses an offering whose upfront (`FixedPrice` x count) exceeds the approved recommendation upfront by more than 5%, or that is priced in a non-USD currency; a zero approved upfront is not checked (#53). - Bump Go to 1.26.10 to pick up the fix for GO-2026-6809 (crypto/x509), which `govulncheck` now reports against 1.26.9. - **AWS/MemoryDB, OpenSearch, Redshift:** `PurchaseCommitment` refuses an offering whose upfront (`FixedPrice` x count) exceeds the approved recommendation upfront by more than 5%, or that is priced in a non-USD currency (MemoryDB offerings carry no currency, so only the recommendation's currency is checked there), or, for MemoryDB, an upfront offering with a zero price or one bought in a China region (cn-north-1, cn-northwest-1), where the assumed USD price is wrong; a zero approved upfront is not checked. All AWS RI price guards (EC2, RDS, MemoryDB, OpenSearch, Redshift) also refuse a NaN or infinite approved upfront (#53). diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index dea260c0..88d022b0 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -185,8 +185,9 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // common.Commitment, returning ok=false for entries that should be skipped // (missing ID, or a plan type that does not match this client's scope). It is // split out of GetExistingCommitments to keep that function under the -// cyclomatic limit. A start or end date that is present but not RFC3339 is an -// error naming the plan ID and the bad value. +// cyclomatic limit. A start or end date that is missing or not RFC3339 is an +// error naming the plan ID (and the bad value); a zero StartDate would never +// count as recent in the purchase dedupe. func (c *Client) toCommitment(sp types.SavingsPlan, service common.ServiceType) (common.Commitment, bool, error) { if sp.SavingsPlanId == nil { return common.Commitment{}, false, nil @@ -206,20 +207,22 @@ func (c *Client) toCommitment(sp types.SavingsPlan, service common.ServiceType) State: common.CommitmentState(sp.State), } - if sp.Start != nil { - startTime, err := time.Parse(time.RFC3339, *sp.Start) - if err != nil { - return common.Commitment{}, false, fmt.Errorf("savings plan %s: invalid start date %q: %w", *sp.SavingsPlanId, *sp.Start, err) - } - commitment.StartDate = startTime + if sp.Start == nil { + return common.Commitment{}, false, fmt.Errorf("savings plan %s: missing start date", *sp.SavingsPlanId) } - if sp.End != nil { - endTime, err := time.Parse(time.RFC3339, *sp.End) - if err != nil { - return common.Commitment{}, false, fmt.Errorf("savings plan %s: invalid end date %q: %w", *sp.SavingsPlanId, *sp.End, err) - } - commitment.EndDate = endTime + startTime, err := time.Parse(time.RFC3339, *sp.Start) + if err != nil { + return common.Commitment{}, false, fmt.Errorf("savings plan %s: invalid start date %q: %w", *sp.SavingsPlanId, *sp.Start, err) + } + commitment.StartDate = startTime + if sp.End == nil { + return common.Commitment{}, false, fmt.Errorf("savings plan %s: missing end date", *sp.SavingsPlanId) + } + endTime, err := time.Parse(time.RFC3339, *sp.End) + if err != nil { + return common.Commitment{}, false, fmt.Errorf("savings plan %s: invalid end date %q: %w", *sp.SavingsPlanId, *sp.End, err) } + commitment.EndDate = endTime return commitment, true, nil } diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index c42f2273..834428bf 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -156,6 +156,8 @@ func TestClient_GetExistingCommitments(t *testing.T) { SavingsPlanId: aws.String("sp-123"), SavingsPlanType: types.SavingsPlanTypeCompute, State: types.SavingsPlanStateActive, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), }, { // No SavingsPlanId - should be skipped @@ -230,8 +232,8 @@ func TestClient_GetExistingCommitments(t *testing.T) { func TestClient_GetExistingCommitments_InvalidDateNamesPlanAndValue(t *testing.T) { t.Parallel() for field, plan := range map[string]types.SavingsPlan{ - "start": {SavingsPlanId: aws.String("sp-bad"), SavingsPlanType: types.SavingsPlanTypeCompute, Start: aws.String("not-a-date")}, - "end": {SavingsPlanId: aws.String("sp-bad"), SavingsPlanType: types.SavingsPlanTypeCompute, End: aws.String("not-a-date")}, + "start": {SavingsPlanId: aws.String("sp-bad"), SavingsPlanType: types.SavingsPlanTypeCompute, Start: aws.String("not-a-date"), End: aws.String("2027-01-01T00:00:00Z")}, + "end": {SavingsPlanId: aws.String("sp-bad"), SavingsPlanType: types.SavingsPlanTypeCompute, Start: aws.String("2024-01-01T00:00:00Z"), End: aws.String("not-a-date")}, } { t.Run(field, func(t *testing.T) { m := &MockSavingsPlansClient{} @@ -247,6 +249,28 @@ func TestClient_GetExistingCommitments_InvalidDateNamesPlanAndValue(t *testing.T } } +// TestClient_GetExistingCommitments_MissingDateNamesPlan pins that a plan with +// no start or end date is an error, not a zero date that the purchase dedupe +// would never treat as recent. +func TestClient_GetExistingCommitments_MissingDateNamesPlan(t *testing.T) { + t.Parallel() + for field, plan := range map[string]types.SavingsPlan{ + "start": {SavingsPlanId: aws.String("sp-nodate"), SavingsPlanType: types.SavingsPlanTypeCompute, End: aws.String("2027-01-01T00:00:00Z")}, + "end": {SavingsPlanId: aws.String("sp-nodate"), SavingsPlanType: types.SavingsPlanTypeCompute, Start: aws.String("2024-01-01T00:00:00Z")}, + } { + t.Run(field, func(t *testing.T) { + m := &MockSavingsPlansClient{} + m.On("DescribeSavingsPlans", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOutput{SavingsPlans: []types.SavingsPlan{plan}}, nil).Once() + c := &Client{client: m, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "sp-nodate") + assert.Contains(t, err.Error(), "missing "+field+" date") + }) + } +} + // TestClient_GetExistingCommitments_UmbrellaMode verifies that an SP client // constructed with an empty plan type (legacy umbrella mode, used by the // AWS provider's `case common.ServiceSavingsPlansAll` branch in GetServiceClient) @@ -263,16 +287,22 @@ func TestClient_GetExistingCommitments_UmbrellaMode(t *testing.T) { SavingsPlanId: aws.String("sp-compute"), SavingsPlanType: types.SavingsPlanTypeCompute, State: types.SavingsPlanStateActive, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), }, { SavingsPlanId: aws.String("sp-sagemaker"), SavingsPlanType: types.SavingsPlanTypeSagemaker, State: types.SavingsPlanStateActive, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), }, { SavingsPlanId: aws.String("sp-database"), SavingsPlanType: types.SavingsPlanTypeDatabase, State: types.SavingsPlanStateActive, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), }, }, }, nil).Once() @@ -309,6 +339,8 @@ func TestClient_GetExistingCommitments_Pagination(t *testing.T) { SavingsPlanId: aws.String("sp-page1"), SavingsPlanType: types.SavingsPlanTypeCompute, State: types.SavingsPlanStateActive, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), Region: aws.String("us-east-1"), }, }, @@ -324,6 +356,8 @@ func TestClient_GetExistingCommitments_Pagination(t *testing.T) { SavingsPlanId: aws.String("sp-page2"), SavingsPlanType: types.SavingsPlanTypeCompute, State: types.SavingsPlanStateActive, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), Region: aws.String("us-west-2"), }, }, @@ -366,6 +400,8 @@ func TestClient_GetExistingCommitments_CtxCancellation(t *testing.T) { SavingsPlanId: aws.String("sp-1"), SavingsPlanType: types.SavingsPlanTypeCompute, State: types.SavingsPlanStateActive, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), }, }, NextToken: aws.String("token-page2"), @@ -1728,6 +1764,8 @@ func TestClient_GetExistingCommitments_RequestsPaymentPending(t *testing.T) { SavingsPlanId: aws.String("sp-pending"), SavingsPlanType: types.SavingsPlanTypeCompute, State: types.SavingsPlanStatePaymentPending, + Start: aws.String("2024-01-01T00:00:00Z"), + End: aws.String("2027-01-01T00:00:00Z"), }}, }, nil).Once() t.Cleanup(func() { mockClient.AssertExpectations(t) })