From 8ddc812cd5f9ad98c94d4d8973b48bebc5fd6db8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 20:52:06 +0100 Subject: [PATCH 01/17] feat(aws): add shared reconcile window and fail-closed decision core One Decide rule for EC2 and Redshift reconcile: exactly one match and no uncertain candidate is Found, everything else is Unknown with a distinct Reason phrase. Refs #298 Co-Authored-By: Claude Opus 5.5 --- .../aws/internal/purchasecfg/reconcile.go | 66 +++++++++ .../internal/purchasecfg/reconcile_test.go | 139 ++++++++++++++++++ 2 files changed, 205 insertions(+) create mode 100644 providers/aws/internal/purchasecfg/reconcile.go create mode 100644 providers/aws/internal/purchasecfg/reconcile_test.go diff --git a/providers/aws/internal/purchasecfg/reconcile.go b/providers/aws/internal/purchasecfg/reconcile.go new file mode 100644 index 00000000..912d7962 --- /dev/null +++ b/providers/aws/internal/purchasecfg/reconcile.go @@ -0,0 +1,66 @@ +package purchasecfg + +import ( + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// ClockSkew pads both ends of the reconcile match window (equals the Redshift guard's skew). +const ClockSkew = 5 * time.Minute + +// One fixed Reason phrase per Unknown branch; callers never invent their own. +const ( + ReasonInvalidWindow = "invalid-window" + ReasonInvalidRequest = "invalid-request" + ReasonLookupFailed = "lookup-failed" + ReasonLookupIncomplete = "lookup-incomplete" + ReasonOfferingUnresolved = "offering-unresolved" + ReasonNoCandidate = "no-candidate" + ReasonAmbiguous = "ambiguous" + ReasonUncertainCandidate = "uncertain-candidate" +) + +// ReconcileWindow returns the padded interval a candidate's start time must fall in. +// ok is false for a zero time or an end before start. +func ReconcileWindow(start, end time.Time) (lo, hi time.Time, ok bool) { + if start.IsZero() || end.IsZero() || end.Before(start) { + return time.Time{}, time.Time{}, false + } + return start.Add(-ClockSkew), end.Add(ClockSkew), true +} + +// InWindow reports whether t lies in [lo, hi], both ends inclusive. +func InWindow(t, lo, hi time.Time) bool { + return !t.Before(lo) && !t.After(hi) +} + +// UnknownResult is the verdict for early exits and failures. +func UnknownResult(reason string) common.ReconcileResult { + return common.ReconcileResult{Verdict: common.ReconcileUnknown, Reason: reason} +} + +// Decide is the single fail-closed rule for both services: Found only for exactly one match and no +// uncertain candidate. +func Decide(matches, uncertain, criteria []string, tagged bool) common.ReconcileResult { + if len(uncertain) > 0 { + res := UnknownResult(ReasonUncertainCandidate) + res.CandidateIDs = append(append([]string{}, matches...), uncertain...) + return res + } + switch len(matches) { + case 0: + return UnknownResult(ReasonNoCandidate) + case 1: + return common.ReconcileResult{ + Verdict: common.ReconcileFound, + CommitmentID: matches[0], + Criteria: criteria, + Tagged: tagged, + } + default: + res := UnknownResult(ReasonAmbiguous) + res.CandidateIDs = append([]string{}, matches...) + return res + } +} diff --git a/providers/aws/internal/purchasecfg/reconcile_test.go b/providers/aws/internal/purchasecfg/reconcile_test.go new file mode 100644 index 00000000..45b3c472 --- /dev/null +++ b/providers/aws/internal/purchasecfg/reconcile_test.go @@ -0,0 +1,139 @@ +package purchasecfg + +import ( + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReconcileWindow(t *testing.T) { + t.Parallel() + start := time.Date(2026, 11, 6, 12, 0, 0, 0, time.UTC) + end := start.Add(time.Minute) + wantLo, wantHi := start.Add(-ClockSkew), end.Add(ClockSkew) + + tests := []struct { + name string + start, end time.Time + ok bool + }{ + {"valid", start, end, true}, + {"equal start and end", start, start, true}, + {"zero start", time.Time{}, end, false}, + {"zero end", start, time.Time{}, false}, + {"both zero", time.Time{}, time.Time{}, false}, + {"end before start", end, start, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + lo, hi, ok := ReconcileWindow(tt.start, tt.end) + assert.Equal(t, tt.ok, ok) + if !tt.ok { + assert.True(t, lo.IsZero() && hi.IsZero()) + } + }) + } + + lo, hi, ok := ReconcileWindow(start, end) + require.True(t, ok) + assert.Equal(t, wantLo, lo) + assert.Equal(t, wantHi, hi) +} + +func TestInWindow(t *testing.T) { + t.Parallel() + start := time.Date(2026, 11, 6, 12, 0, 0, 0, time.UTC) + end := start.Add(time.Minute) + lo, hi, ok := ReconcileWindow(start, end) + require.True(t, ok) + + tests := []struct { + name string + at time.Time + want bool + }{ + {"exactly lo", lo, true}, + {"one ns before lo", lo.Add(-time.Nanosecond), false}, + {"inside", start, true}, + {"exactly hi", hi, true}, + {"one ns after hi", hi.Add(time.Nanosecond), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, InWindow(tt.at, lo, hi)) + }) + } +} + +func TestDecide(t *testing.T) { + t.Parallel() + crit := []string{"instance-type", "count"} + + tests := []struct { + name string + matches []string + uncertain []string + tagged bool + verdict common.ReconcileVerdict + id, reason string + candidates []string + }{ + {"0 match 0 uncertain", nil, nil, false, common.ReconcileUnknown, "", ReasonNoCandidate, nil}, + {"0 match 1 uncertain", nil, []string{"u1"}, false, common.ReconcileUnknown, "", ReasonUncertainCandidate, []string{"u1"}}, + {"1 match 0 uncertain", []string{"m1"}, nil, false, common.ReconcileFound, "m1", "", nil}, + {"1 match 0 uncertain tagged", []string{"m1"}, nil, true, common.ReconcileFound, "m1", "", nil}, + {"1 match 1 uncertain", []string{"m1"}, []string{"u1"}, false, common.ReconcileUnknown, "", ReasonUncertainCandidate, []string{"m1", "u1"}}, + {"2 match 0 uncertain", []string{"m1", "m2"}, nil, false, common.ReconcileUnknown, "", ReasonAmbiguous, []string{"m1", "m2"}}, + {"2 match 1 uncertain", []string{"m1", "m2"}, []string{"u1"}, false, common.ReconcileUnknown, "", ReasonUncertainCandidate, []string{"m1", "m2", "u1"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := Decide(tt.matches, tt.uncertain, crit, tt.tagged) + assert.Equal(t, tt.verdict, got.Verdict) + assert.Equal(t, tt.id, got.CommitmentID) + assert.Equal(t, tt.reason, got.Reason) + assert.Equal(t, tt.candidates, got.CandidateIDs) + if tt.verdict == common.ReconcileFound { + assert.True(t, got.IsFound()) + assert.Equal(t, crit, got.Criteria) + assert.Equal(t, tt.tagged, got.Tagged) + assert.Empty(t, got.Reason) + } else { + assert.False(t, got.IsFound()) + assert.Empty(t, got.CommitmentID) + assert.Empty(t, got.Criteria) + assert.False(t, got.Tagged) + } + }) + } +} + +func TestUnknownResult(t *testing.T) { + t.Parallel() + got := UnknownResult(ReasonLookupFailed) + assert.Equal(t, common.ReconcileUnknown, got.Verdict) + assert.Equal(t, ReasonLookupFailed, got.Reason) + assert.Empty(t, got.CommitmentID) + assert.False(t, got.IsFound()) +} + +func TestReasonPhrasesAreDistinct(t *testing.T) { + t.Parallel() + phrases := []string{ + ReasonInvalidWindow, ReasonInvalidRequest, ReasonLookupFailed, ReasonLookupIncomplete, + ReasonOfferingUnresolved, ReasonNoCandidate, ReasonAmbiguous, ReasonUncertainCandidate, + } + seen := map[string]bool{} + for _, p := range phrases { + assert.NotEmpty(t, p) + assert.False(t, seen[p], "duplicate reason phrase %q", p) + seen[p] = true + } + assert.Len(t, seen, 8) +} From f278ad54b52ac70d9222233ced67d97b01c0d3ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:06:33 +0100 Subject: [PATCH 02/17] fix(aws): treat a match without an ID as uncertain in reconcile Decide A match with an empty ID can never be Found with a usable CommitmentID. It now yields Unknown uncertain-candidate. Refs #298 Co-Authored-By: Claude Opus 5.5 --- providers/aws/internal/purchasecfg/reconcile.go | 14 ++++++++++++-- .../aws/internal/purchasecfg/reconcile_test.go | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/providers/aws/internal/purchasecfg/reconcile.go b/providers/aws/internal/purchasecfg/reconcile.go index 912d7962..ed991b90 100644 --- a/providers/aws/internal/purchasecfg/reconcile.go +++ b/providers/aws/internal/purchasecfg/reconcile.go @@ -43,9 +43,19 @@ func UnknownResult(reason string) common.ReconcileResult { // Decide is the single fail-closed rule for both services: Found only for exactly one match and no // uncertain candidate. func Decide(matches, uncertain, criteria []string, tagged bool) common.ReconcileResult { - if len(uncertain) > 0 { + // A match without an ID cannot be reported as Found, so it counts as uncertain. + var ids []string + blank := false + for _, id := range append(append([]string{}, matches...), uncertain...) { + if id == "" { + blank = true + continue + } + ids = append(ids, id) + } + if blank || len(uncertain) > 0 { res := UnknownResult(ReasonUncertainCandidate) - res.CandidateIDs = append(append([]string{}, matches...), uncertain...) + res.CandidateIDs = ids return res } switch len(matches) { diff --git a/providers/aws/internal/purchasecfg/reconcile_test.go b/providers/aws/internal/purchasecfg/reconcile_test.go index 45b3c472..8d9ca21b 100644 --- a/providers/aws/internal/purchasecfg/reconcile_test.go +++ b/providers/aws/internal/purchasecfg/reconcile_test.go @@ -88,6 +88,8 @@ func TestDecide(t *testing.T) { {"1 match 0 uncertain", []string{"m1"}, nil, false, common.ReconcileFound, "m1", "", nil}, {"1 match 0 uncertain tagged", []string{"m1"}, nil, true, common.ReconcileFound, "m1", "", nil}, {"1 match 1 uncertain", []string{"m1"}, []string{"u1"}, false, common.ReconcileUnknown, "", ReasonUncertainCandidate, []string{"m1", "u1"}}, + {"1 empty-id match", []string{""}, nil, false, common.ReconcileUnknown, "", ReasonUncertainCandidate, nil}, + {"2 match one empty id", []string{"m1", ""}, nil, false, common.ReconcileUnknown, "", ReasonUncertainCandidate, []string{"m1"}}, {"2 match 0 uncertain", []string{"m1", "m2"}, nil, false, common.ReconcileUnknown, "", ReasonAmbiguous, []string{"m1", "m2"}}, {"2 match 1 uncertain", []string{"m1", "m2"}, []string{"u1"}, false, common.ReconcileUnknown, "", ReasonUncertainCandidate, []string{"m1", "m2", "u1"}}, } From b79a3243515b12ed2ceb63bfd6b4c8cf2d869957 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:09:54 +0100 Subject: [PATCH 03/17] refactor(ec2): extract productMatches from offering checks Behaviour-preserving; reconcile matching reuses it. Refs #298 Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/ec2/offering_match.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/providers/aws/services/ec2/offering_match.go b/providers/aws/services/ec2/offering_match.go index bf20a51d..e2cc7001 100644 --- a/providers/aws/services/ec2/offering_match.go +++ b/providers/aws/services/ec2/offering_match.go @@ -73,10 +73,15 @@ var offeringChecks = []struct { if got == "" || want == "" { return false } - return strings.TrimSuffix(got, vpcProductSuffix) != strings.TrimSuffix(want, vpcProductSuffix) + return !productMatches(got, want) }}, } +// productMatches compares product descriptions ignoring the " (Amazon VPC)" suffix. +func productMatches(got, want string) bool { + return strings.TrimSuffix(got, vpcProductSuffix) == strings.TrimSuffix(want, vpcProductSuffix) +} + // offeringMismatch returns the reason o does not satisfy w, or "" on a match. func offeringMismatch(o types.ReservedInstancesOffering, w offeringWant) string { for _, c := range offeringChecks { From 82933bc10c19b37111ad3e004d5a6e878106b1cd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:10:59 +0100 Subject: [PATCH 04/17] feat(ec2): classify described reserved instances for purchase reconcile riFit applies the section 4.1 tri-state rules, tag handling and live-state check; reconcile (next commit) folds the results through purchasecfg.Decide. Refs #298 Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/ec2/reconcile_match.go | 140 ++++++++++++++++++ .../aws/services/ec2/reconcile_match_test.go | 132 +++++++++++++++++ 2 files changed, 272 insertions(+) create mode 100644 providers/aws/services/ec2/reconcile_match.go create mode 100644 providers/aws/services/ec2/reconcile_match_test.go diff --git a/providers/aws/services/ec2/reconcile_match.go b/providers/aws/services/ec2/reconcile_match.go new file mode 100644 index 00000000..ec603db6 --- /dev/null +++ b/providers/aws/services/ec2/reconcile_match.go @@ -0,0 +1,140 @@ +package ec2 + +import ( + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2/types" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" +) + +// fit is how one described reservation relates to the purchase being reconciled. +type fit int + +const ( + fitNo fit = iota // a field is present and different: not this purchase + fitUncertain // cannot be ruled in or out + fitMatch // every required field is present and equal +) + +// worse combines two fits: a definite mismatch beats doubt, doubt beats a match. +func worse(a, b fit) fit { + if a < b { + return a + } + return b +} + +func eq[T comparable](got, want, zero T) fit { + switch got { + case zero: + return fitUncertain + case want: + return fitMatch + } + return fitNo +} + +// riFit classifies one reserved instance against the rec's query, section 4.1 of the plan. +// tagged is true only for a match that carries the request's idempotency token. +func riFit(ri types.ReservedInstances, q ec2OfferingQuery, class types.OfferingClassType, count int, token string, lo, hi time.Time) (f fit, tagged bool) { + owned, other, dup := tagState(ri.Tags, token) + if dup { + return fitUncertain, false + } + if other { + return fitNo, false + } + f = attrFit(ri, q, class, count, lo, hi) + if owned { + if f == fitMatch { + return fitMatch, true + } + return fitUncertain, false + } + return f, false +} + +// tagState reads the idempotency tag. Distinct values under the key are dup (uncertain); a lone value +// is owned when it equals a non-empty request token and other otherwise. +func tagState(tags []types.Tag, token string) (owned, other, dup bool) { + var val *string + for _, t := range tags { + if aws.ToString(t.Key) != common.IdempotencyTagKey { + continue + } + v := aws.ToString(t.Value) + if val != nil && *val != v { + return false, false, true + } + val = &v + } + if val == nil { + return false, false, false + } + if token != "" && *val == token { + return true, false, false + } + return false, true, false +} + +func attrFit(ri types.ReservedInstances, q ec2OfferingQuery, class types.OfferingClassType, count int, lo, hi time.Time) fit { + fits := []fit{ + eq(ri.InstanceType, q.instanceType, ""), + eq(ri.InstanceTenancy, q.tenancy, ""), + eq(ri.Scope, q.scope, ""), + eq(ri.OfferingType, q.wantOfferingType, ""), + eq(ri.OfferingClass, class, ""), + zoneFit(ri, q), + presentFit(ri.InstanceCount, func(n int32) bool { return int(n) == count }), + presentFit(ri.Duration, func(d int64) bool { return d == q.duration }), + presentFit(ri.Start, func(s time.Time) bool { return purchasecfg.InWindow(s, lo, hi) }), + stateFit(ri.State), + } + if ri.ProductDescription == "" { + fits = append(fits, fitUncertain) + } else if !productMatches(string(ri.ProductDescription), string(q.productDesc)) { + fits = append(fits, fitNo) + } + if aws.ToString(ri.ReservedInstancesId) == "" { + fits = append(fits, fitUncertain) + } + f := fitMatch + for _, x := range fits { + f = worse(f, x) + } + return f +} + +// zoneFit: a zonal rec needs the same AZ; a regional rec must carry none. +func zoneFit(ri types.ReservedInstances, q ec2OfferingQuery) fit { + az := aws.ToString(ri.AvailabilityZone) + if q.availabilityZone == "" { + if az != "" { + return fitNo + } + return fitMatch + } + return eq(az, q.availabilityZone, "") +} + +func presentFit[T any](p *T, ok func(T) bool) fit { + switch { + case p == nil: + return fitUncertain + case ok(*p): + return fitMatch + } + return fitNo +} + +// stateFit: only live states can be reported Found; any other state with matching attributes is doubt. +func stateFit(s types.ReservedInstanceState) fit { + switch s { + case types.ReservedInstanceStateActive, types.ReservedInstanceStatePaymentPending, types.ReservedInstanceStateQueued: + return fitMatch + } + return fitUncertain +} diff --git a/providers/aws/services/ec2/reconcile_match_test.go b/providers/aws/services/ec2/reconcile_match_test.go new file mode 100644 index 00000000..9ef9e5cc --- /dev/null +++ b/providers/aws/services/ec2/reconcile_match_test.go @@ -0,0 +1,132 @@ +package ec2 + +import ( + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/stretchr/testify/assert" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func TestRIFit(t *testing.T) { + t.Parallel() + start := time.Date(2026, 11, 6, 12, 0, 0, 0, time.UTC) + lo, hi := start.Add(-time.Minute), start.Add(time.Minute) + const token = "tok-1" + const count = 3 + + regional := ec2OfferingQuery{ + instanceType: types.InstanceTypeM5Large, productDesc: types.RIProductDescriptionLinuxUnix, + tenancy: types.TenancyDefault, scope: types.ScopeRegional, + wantOfferingType: types.OfferingTypeValuesNoUpfront, + offeringClass: types.OfferingClassTypeStandard, duration: OneYearSeconds, + } + zonal := regional + zonal.scope = types.ScopeAvailabilityZone + zonal.availabilityZone = "us-east-1a" + class := types.OfferingClassTypeStandard + + baseRI := func(mut ...func(*types.ReservedInstances)) types.ReservedInstances { + ri := types.ReservedInstances{ + ReservedInstancesId: aws.String("ri-1"), InstanceType: types.InstanceTypeM5Large, + InstanceCount: aws.Int32(count), ProductDescription: types.RIProductDescriptionLinuxUnix, + InstanceTenancy: types.TenancyDefault, Scope: types.ScopeRegional, + Duration: aws.Int64(OneYearSeconds), OfferingType: types.OfferingTypeValuesNoUpfront, + OfferingClass: types.OfferingClassTypeStandard, Start: aws.Time(start), + State: types.ReservedInstanceStateActive, + } + for _, m := range mut { + m(&ri) + } + return ri + } + tag := func(vals ...string) func(*types.ReservedInstances) { + return func(ri *types.ReservedInstances) { + for _, v := range vals { + ri.Tags = append(ri.Tags, types.Tag{Key: aws.String(common.IdempotencyTagKey), Value: aws.String(v)}) + } + } + } + zonalRI := func(mut ...func(*types.ReservedInstances)) types.ReservedInstances { + return baseRI(append([]func(*types.ReservedInstances){func(ri *types.ReservedInstances) { + ri.Scope = types.ScopeAvailabilityZone + ri.AvailabilityZone = aws.String("us-east-1a") + }}, mut...)...) + } + set := func(f func(*types.ReservedInstances)) func(*types.ReservedInstances) { return f } + + tests := []struct { + name string + ri types.ReservedInstances + q ec2OfferingQuery + token string + want fit + wantTagged bool + }{ + {"all equal untagged", baseRI(), regional, token, fitMatch, false}, + {"zonal all equal", zonalRI(), zonal, token, fitMatch, false}, + {"vpc suffix ignored", baseRI(set(func(ri *types.ReservedInstances) { ri.ProductDescription = "Linux/UNIX (Amazon VPC)" })), regional, token, fitMatch, false}, + + {"instance type differs", baseRI(set(func(ri *types.ReservedInstances) { ri.InstanceType = types.InstanceTypeC5Large })), regional, token, fitNo, false}, + {"count differs", baseRI(set(func(ri *types.ReservedInstances) { ri.InstanceCount = aws.Int32(count + 1) })), regional, token, fitNo, false}, + {"product differs", baseRI(set(func(ri *types.ReservedInstances) { ri.ProductDescription = types.RIProductDescriptionWindows })), regional, token, fitNo, false}, + {"tenancy differs", baseRI(set(func(ri *types.ReservedInstances) { ri.InstanceTenancy = types.TenancyDedicated })), regional, token, fitNo, false}, + {"scope differs", baseRI(set(func(ri *types.ReservedInstances) { ri.Scope = types.ScopeAvailabilityZone })), regional, token, fitNo, false}, + {"az differs", zonalRI(set(func(ri *types.ReservedInstances) { ri.AvailabilityZone = aws.String("us-east-1b") })), zonal, token, fitNo, false}, + {"regional rec with AZ set", baseRI(set(func(ri *types.ReservedInstances) { ri.AvailabilityZone = aws.String("us-east-1a") })), regional, token, fitNo, false}, + {"duration differs", baseRI(set(func(ri *types.ReservedInstances) { ri.Duration = aws.Int64(3 * OneYearSeconds) })), regional, token, fitNo, false}, + {"offering type differs", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingType = types.OfferingTypeValuesAllUpfront })), regional, token, fitNo, false}, + {"class differs", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingClass = types.OfferingClassTypeConvertible })), regional, token, fitNo, false}, + + {"instance type missing", baseRI(set(func(ri *types.ReservedInstances) { ri.InstanceType = "" })), regional, token, fitUncertain, false}, + {"count missing", baseRI(set(func(ri *types.ReservedInstances) { ri.InstanceCount = nil })), regional, token, fitUncertain, false}, + {"product missing", baseRI(set(func(ri *types.ReservedInstances) { ri.ProductDescription = "" })), regional, token, fitUncertain, false}, + {"tenancy missing", baseRI(set(func(ri *types.ReservedInstances) { ri.InstanceTenancy = "" })), regional, token, fitUncertain, false}, + {"scope missing", baseRI(set(func(ri *types.ReservedInstances) { ri.Scope = "" })), regional, token, fitUncertain, false}, + {"zonal rec with AZ missing", zonalRI(set(func(ri *types.ReservedInstances) { ri.AvailabilityZone = nil })), zonal, token, fitUncertain, false}, + {"duration missing", baseRI(set(func(ri *types.ReservedInstances) { ri.Duration = nil })), regional, token, fitUncertain, false}, + {"offering type missing", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingType = "" })), regional, token, fitUncertain, false}, + {"class missing", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingClass = "" })), regional, token, fitUncertain, false}, + {"id nil", baseRI(set(func(ri *types.ReservedInstances) { ri.ReservedInstancesId = nil })), regional, token, fitUncertain, false}, + {"id empty", baseRI(set(func(ri *types.ReservedInstances) { ri.ReservedInstancesId = aws.String("") })), regional, token, fitUncertain, false}, + {"id empty but count differs", baseRI(set(func(ri *types.ReservedInstances) { + ri.ReservedInstancesId = nil + ri.InstanceCount = aws.Int32(count + 1) + })), regional, token, fitNo, false}, + + {"start at lo", baseRI(set(func(ri *types.ReservedInstances) { ri.Start = aws.Time(lo) })), regional, token, fitMatch, false}, + {"start at hi", baseRI(set(func(ri *types.ReservedInstances) { ri.Start = aws.Time(hi) })), regional, token, fitMatch, false}, + {"start before lo", baseRI(set(func(ri *types.ReservedInstances) { ri.Start = aws.Time(lo.Add(-time.Nanosecond)) })), regional, token, fitNo, false}, + {"start after hi", baseRI(set(func(ri *types.ReservedInstances) { ri.Start = aws.Time(hi.Add(time.Nanosecond)) })), regional, token, fitNo, false}, + {"start nil", baseRI(set(func(ri *types.ReservedInstances) { ri.Start = nil })), regional, token, fitUncertain, false}, + + {"state active", baseRI(), regional, token, fitMatch, false}, + {"state payment-pending", baseRI(set(func(ri *types.ReservedInstances) { ri.State = types.ReservedInstanceStatePaymentPending })), regional, token, fitMatch, false}, + {"state queued", baseRI(set(func(ri *types.ReservedInstances) { ri.State = types.ReservedInstanceStateQueued })), regional, token, fitMatch, false}, + {"state retired", baseRI(set(func(ri *types.ReservedInstances) { ri.State = types.ReservedInstanceStateRetired })), regional, token, fitUncertain, false}, + {"state payment-failed", baseRI(set(func(ri *types.ReservedInstances) { ri.State = types.ReservedInstanceStatePaymentFailed })), regional, token, fitUncertain, false}, + {"state queued-deleted", baseRI(set(func(ri *types.ReservedInstances) { ri.State = types.ReservedInstanceStateQueuedDeleted })), regional, token, fitUncertain, false}, + {"state empty", baseRI(set(func(ri *types.ReservedInstances) { ri.State = "" })), regional, token, fitUncertain, false}, + + {"tag other value", baseRI(tag("other")), regional, token, fitNo, false}, + {"tag same value attributes equal", baseRI(tag(token)), regional, token, fitMatch, true}, + {"same-token tag count differs", baseRI(tag(token), set(func(ri *types.ReservedInstances) { ri.InstanceCount = aws.Int32(count + 1) })), regional, token, fitUncertain, false}, + {"same-token tag outside window", baseRI(tag(token), set(func(ri *types.ReservedInstances) { ri.Start = aws.Time(hi.Add(time.Hour)) })), regional, token, fitUncertain, false}, + {"duplicate tag key two values", baseRI(tag(token, "other")), regional, token, fitUncertain, false}, + {"duplicate tag key same value", baseRI(tag(token, token)), regional, token, fitMatch, true}, + {"empty request token with token tag", baseRI(tag("other")), regional, "", fitNo, false}, + {"empty request token with empty tag value", baseRI(tag("")), regional, "", fitNo, false}, + {"empty request token untagged", baseRI(), regional, "", fitMatch, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, tagged := riFit(tt.ri, tt.q, class, count, tt.token, lo, hi) + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.wantTagged, tagged) + }) + } +} From f4b96e2472c7d9d613eba8c78472aecc8c992c35 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:20:47 +0100 Subject: [PATCH 05/17] fix(ec2): compare reserved instance class against the query class Drop the redundant class parameter from riFit; the query already carries it. Refs #298 Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/ec2/reconcile_match.go | 8 ++++---- providers/aws/services/ec2/reconcile_match_test.go | 7 +++++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/providers/aws/services/ec2/reconcile_match.go b/providers/aws/services/ec2/reconcile_match.go index ec603db6..5e68800a 100644 --- a/providers/aws/services/ec2/reconcile_match.go +++ b/providers/aws/services/ec2/reconcile_match.go @@ -39,7 +39,7 @@ func eq[T comparable](got, want, zero T) fit { // riFit classifies one reserved instance against the rec's query, section 4.1 of the plan. // tagged is true only for a match that carries the request's idempotency token. -func riFit(ri types.ReservedInstances, q ec2OfferingQuery, class types.OfferingClassType, count int, token string, lo, hi time.Time) (f fit, tagged bool) { +func riFit(ri types.ReservedInstances, q ec2OfferingQuery, count int, token string, lo, hi time.Time) (f fit, tagged bool) { owned, other, dup := tagState(ri.Tags, token) if dup { return fitUncertain, false @@ -47,7 +47,7 @@ func riFit(ri types.ReservedInstances, q ec2OfferingQuery, class types.OfferingC if other { return fitNo, false } - f = attrFit(ri, q, class, count, lo, hi) + f = attrFit(ri, q, count, lo, hi) if owned { if f == fitMatch { return fitMatch, true @@ -80,13 +80,13 @@ func tagState(tags []types.Tag, token string) (owned, other, dup bool) { return false, true, false } -func attrFit(ri types.ReservedInstances, q ec2OfferingQuery, class types.OfferingClassType, count int, lo, hi time.Time) fit { +func attrFit(ri types.ReservedInstances, q ec2OfferingQuery, count int, lo, hi time.Time) fit { fits := []fit{ eq(ri.InstanceType, q.instanceType, ""), eq(ri.InstanceTenancy, q.tenancy, ""), eq(ri.Scope, q.scope, ""), eq(ri.OfferingType, q.wantOfferingType, ""), - eq(ri.OfferingClass, class, ""), + eq(ri.OfferingClass, q.offeringClass, ""), zoneFit(ri, q), presentFit(ri.InstanceCount, func(n int32) bool { return int(n) == count }), presentFit(ri.Duration, func(d int64) bool { return d == q.duration }), diff --git a/providers/aws/services/ec2/reconcile_match_test.go b/providers/aws/services/ec2/reconcile_match_test.go index 9ef9e5cc..170c06e2 100644 --- a/providers/aws/services/ec2/reconcile_match_test.go +++ b/providers/aws/services/ec2/reconcile_match_test.go @@ -24,10 +24,11 @@ func TestRIFit(t *testing.T) { wantOfferingType: types.OfferingTypeValuesNoUpfront, offeringClass: types.OfferingClassTypeStandard, duration: OneYearSeconds, } + convertibleQ := regional + convertibleQ.offeringClass = types.OfferingClassTypeConvertible zonal := regional zonal.scope = types.ScopeAvailabilityZone zonal.availabilityZone = "us-east-1a" - class := types.OfferingClassTypeStandard baseRI := func(mut ...func(*types.ReservedInstances)) types.ReservedInstances { ri := types.ReservedInstances{ @@ -79,6 +80,8 @@ func TestRIFit(t *testing.T) { {"regional rec with AZ set", baseRI(set(func(ri *types.ReservedInstances) { ri.AvailabilityZone = aws.String("us-east-1a") })), regional, token, fitNo, false}, {"duration differs", baseRI(set(func(ri *types.ReservedInstances) { ri.Duration = aws.Int64(3 * OneYearSeconds) })), regional, token, fitNo, false}, {"offering type differs", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingType = types.OfferingTypeValuesAllUpfront })), regional, token, fitNo, false}, + {"convertible RI vs standard query", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingClass = types.OfferingClassTypeConvertible })), regional, token, fitNo, false}, + {"standard RI vs convertible query", baseRI(), convertibleQ, token, fitNo, false}, {"class differs", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingClass = types.OfferingClassTypeConvertible })), regional, token, fitNo, false}, {"instance type missing", baseRI(set(func(ri *types.ReservedInstances) { ri.InstanceType = "" })), regional, token, fitUncertain, false}, @@ -124,7 +127,7 @@ func TestRIFit(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, tagged := riFit(tt.ri, tt.q, class, count, tt.token, lo, hi) + got, tagged := riFit(tt.ri, tt.q, count, tt.token, lo, hi) assert.Equal(t, tt.want, got) assert.Equal(t, tt.wantTagged, tagged) }) From c5d186f9f8f804f9435dd5d39c86394447cdc2e9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:49:58 +0100 Subject: [PATCH 06/17] test(ec2): drop duplicate class-mismatch row from TestRIFit Refs #298 Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/ec2/reconcile_match_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/providers/aws/services/ec2/reconcile_match_test.go b/providers/aws/services/ec2/reconcile_match_test.go index 170c06e2..44fc384a 100644 --- a/providers/aws/services/ec2/reconcile_match_test.go +++ b/providers/aws/services/ec2/reconcile_match_test.go @@ -80,7 +80,6 @@ func TestRIFit(t *testing.T) { {"regional rec with AZ set", baseRI(set(func(ri *types.ReservedInstances) { ri.AvailabilityZone = aws.String("us-east-1a") })), regional, token, fitNo, false}, {"duration differs", baseRI(set(func(ri *types.ReservedInstances) { ri.Duration = aws.Int64(3 * OneYearSeconds) })), regional, token, fitNo, false}, {"offering type differs", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingType = types.OfferingTypeValuesAllUpfront })), regional, token, fitNo, false}, - {"convertible RI vs standard query", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingClass = types.OfferingClassTypeConvertible })), regional, token, fitNo, false}, {"standard RI vs convertible query", baseRI(), convertibleQ, token, fitNo, false}, {"class differs", baseRI(set(func(ri *types.ReservedInstances) { ri.OfferingClass = types.OfferingClassTypeConvertible })), regional, token, fitNo, false}, From f7cd7a026af21ecd007f506c544dcae2da80fa21 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:53:08 +0100 Subject: [PATCH 07/17] feat(ec2): add read-only ReconcilePurchase After an unknown purchase outcome, describe all reserved instances once and fold riFit results through purchasecfg.Decide. Nothing is purchased or tagged. Refs #298 Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/ec2/reconcile.go | 74 +++++++ providers/aws/services/ec2/reconcile_test.go | 196 +++++++++++++++++++ 2 files changed, 270 insertions(+) create mode 100644 providers/aws/services/ec2/reconcile.go create mode 100644 providers/aws/services/ec2/reconcile_test.go diff --git a/providers/aws/services/ec2/reconcile.go b/providers/aws/services/ec2/reconcile.go new file mode 100644 index 00000000..ac6bccaa --- /dev/null +++ b/providers/aws/services/ec2/reconcile.go @@ -0,0 +1,74 @@ +package ec2 + +import ( + "context" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" +) + +var reconcileCriteria = []string{ + "instance-type", "count", "product", "tenancy", "scope", "zone", + "duration", "offering-type", "offering-class", "start-window", +} + +// reconcileQuery validates the request and builds the lookup query; reason is non-empty on refusal. +func (c *Client) reconcileQuery(req common.ReconcileRequest) (q ec2OfferingQuery, count int, lo, hi time.Time, reason string) { + lo, hi, ok := purchasecfg.ReconcileWindow(req.Options.RequestedAt, req.AttemptEndedAt) + if !ok { + return q, 0, lo, hi, purchasecfg.ReasonInvalidWindow + } + rec := req.Recommendation + n, err := offeringprice.PurchaseCount(rec.Count) + if rec.Region != c.region || err != nil { + return q, 0, lo, hi, purchasecfg.ReasonInvalidRequest + } + if q, err = c.buildEC2QueryFromRec(rec); err != nil { + return q, 0, lo, hi, purchasecfg.ReasonInvalidRequest + } + // Same class resolution as findOffering, so reconcile matches what the purchase asked for. + if q.offeringClass, err = resolveOfferingClassType(req.Options.OfferingClass); err != nil { + return q, 0, lo, hi, purchasecfg.ReasonInvalidRequest + } + return q, int(n), lo, hi, "" +} + +// ReconcilePurchase reports, read-only, whether a purchase that returned ErrOutcomeUnknown landed. +// Any doubt or failure is Unknown; see purchasecfg.Decide. +func (c *Client) ReconcilePurchase(ctx context.Context, req common.ReconcileRequest) common.ReconcileResult { + q, count, lo, hi, reason := c.reconcileQuery(req) + if reason != "" { + return purchasecfg.UnknownResult(reason) + } + + if ctx.Err() != nil { + return purchasecfg.UnknownResult(purchasecfg.ReasonLookupFailed) + } + out, err := c.client.DescribeReservedInstances(ctx, &ec2.DescribeReservedInstancesInput{}) + if err != nil || out == nil { + return purchasecfg.UnknownResult(purchasecfg.ReasonLookupFailed) + } + + var matches, uncertain []string + tagged := false + for i := range out.ReservedInstances { + ri := out.ReservedInstances[i] + id := aws.ToString(ri.ReservedInstancesId) + f, isTagged := riFit(ri, q, count, req.Options.IdempotencyToken, lo, hi) + switch f { + case fitMatch: + matches = append(matches, id) + tagged = isTagged + case fitUncertain: + uncertain = append(uncertain, id) + case fitNo: + } + } + // Tagged is EC2-only: Redshift nodes cannot carry the idempotency tag. + return purchasecfg.Decide(matches, uncertain, reconcileCriteria, tagged) +} diff --git a/providers/aws/services/ec2/reconcile_test.go b/providers/aws/services/ec2/reconcile_test.go new file mode 100644 index 00000000..e0bbb0c7 --- /dev/null +++ b/providers/aws/services/ec2/reconcile_test.go @@ -0,0 +1,196 @@ +package ec2 + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2" + "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" +) + +var reconcileT0 = time.Date(2026, 11, 6, 12, 0, 0, 0, time.UTC) + +func reconcileRec() common.Recommendation { + return common.Recommendation{ + Region: "us-east-1", ResourceType: "m5.large", Count: 3, PaymentOption: "no-upfront", Term: "1yr", + Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}, + } +} + +func reconcileReq(mut ...func(*common.ReconcileRequest)) common.ReconcileRequest { + req := common.ReconcileRequest{ + Recommendation: reconcileRec(), + Options: common.PurchaseOptions{RequestedAt: reconcileT0, IdempotencyToken: "tok-1", OfferingClass: "standard"}, + AttemptEndedAt: reconcileT0.Add(time.Minute), + } + for _, m := range mut { + m(&req) + } + return req +} + +func reconcileRI(id string, mut ...func(*types.ReservedInstances)) types.ReservedInstances { + ri := types.ReservedInstances{ + ReservedInstancesId: aws.String(id), InstanceType: types.InstanceTypeM5Large, InstanceCount: aws.Int32(3), + ProductDescription: types.RIProductDescriptionLinuxUnix, InstanceTenancy: types.TenancyDefault, + Scope: types.ScopeRegional, Duration: aws.Int64(OneYearSeconds), OfferingType: types.OfferingTypeValuesNoUpfront, + OfferingClass: types.OfferingClassTypeStandard, Start: aws.Time(reconcileT0.Add(30 * time.Second)), + State: types.ReservedInstanceStateActive, + } + for _, m := range mut { + m(&ri) + } + return ri +} + +func riTag(v string) func(*types.ReservedInstances) { + return func(ri *types.ReservedInstances) { + ri.Tags = append(ri.Tags, types.Tag{Key: aws.String(common.IdempotencyTagKey), Value: aws.String(v)}) + } +} + +func reconcileClient(ris []types.ReservedInstances, descErr error) (*Client, *MockEC2Client) { + m := &MockEC2Client{} + if ris != nil || descErr != nil { + call := m.On("DescribeReservedInstances", mock.Anything, mock.Anything) + if descErr != nil { + call.Return(nil, descErr) + } else { + call.Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: ris}, nil) + } + } + return &Client{client: m, region: "us-east-1"}, m +} + +func TestReconcilePurchase(t *testing.T) { + t.Parallel() + canceled, cancel := context.WithCancel(context.Background()) + cancel() + future := reconcileRI("ri-q", func(ri *types.ReservedInstances) { + ri.State = types.ReservedInstanceStateQueued + ri.Start = aws.Time(reconcileT0.Add(24 * time.Hour)) + }) + + tests := []struct { + name string + ctx context.Context + ris []types.ReservedInstances + descErr error + req common.ReconcileRequest + wantDescribe bool + verdict common.ReconcileVerdict + id, reason string + tagged bool + candidates []string + }{ + {name: "one untagged match", ris: []types.ReservedInstances{reconcileRI("ri-1")}, req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileFound, id: "ri-1"}, + {name: "one same-token tagged match", ris: []types.ReservedInstances{reconcileRI("ri-1", riTag("tok-1"))}, req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileFound, id: "ri-1", tagged: true}, + {name: "default class is convertible", ris: []types.ReservedInstances{reconcileRI("ri-1", func(ri *types.ReservedInstances) { ri.OfferingClass = types.OfferingClassTypeConvertible })}, + req: reconcileReq(func(r *common.ReconcileRequest) { r.Options.OfferingClass = "" }), + wantDescribe: true, verdict: common.ReconcileFound, id: "ri-1"}, + {name: "convertible RI does not match standard request", ris: []types.ReservedInstances{reconcileRI("ri-1", func(ri *types.ReservedInstances) { ri.OfferingClass = types.OfferingClassTypeConvertible })}, + req: reconcileReq(), wantDescribe: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "two untagged matches", ris: []types.ReservedInstances{reconcileRI("ri-1"), reconcileRI("ri-2")}, req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonAmbiguous, candidates: []string{"ri-1", "ri-2"}}, + {name: "match plus other-lineage tagged same-spec RI", ris: []types.ReservedInstances{reconcileRI("ri-1"), reconcileRI("ri-2", riTag("other"))}, req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileFound, id: "ri-1"}, + {name: "match plus retired same-spec RI", ris: []types.ReservedInstances{reconcileRI("ri-1"), reconcileRI("ri-2", func(ri *types.ReservedInstances) { ri.State = types.ReservedInstanceStateRetired })}, req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonUncertainCandidate, candidates: []string{"ri-1", "ri-2"}}, + {name: "empty list", ris: []types.ReservedInstances{}, req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "queued RI with future start", ris: []types.ReservedInstances{future}, req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "describe error", descErr: errors.New("boom"), req: reconcileReq(), + wantDescribe: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonLookupFailed}, + {name: "canceled context", ctx: canceled, ris: []types.ReservedInstances{reconcileRI("ri-1")}, req: reconcileReq(), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonLookupFailed}, + {name: "bad offering class", req: reconcileReq(func(r *common.ReconcileRequest) { r.Options.OfferingClass = "bogus" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "region mismatch", req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Region = "eu-west-1" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "zero count", req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Count = 0 }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "unparsable payment option", req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.PaymentOption = "bogus" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "zero RequestedAt", req: reconcileReq(func(r *common.ReconcileRequest) { r.Options.RequestedAt = time.Time{} }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidWindow}, + {name: "zero AttemptEndedAt", req: reconcileReq(func(r *common.ReconcileRequest) { r.AttemptEndedAt = time.Time{} }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidWindow}, + {name: "end before start", req: reconcileReq(func(r *common.ReconcileRequest) { r.AttemptEndedAt = reconcileT0.Add(-time.Hour) }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidWindow}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + c, m := reconcileClient(tt.ris, tt.descErr) + ctx := tt.ctx + if ctx == nil { + ctx = context.Background() + } + got := c.ReconcilePurchase(ctx, tt.req) + + assert.Equal(t, tt.verdict, got.Verdict) + assert.Equal(t, tt.id, got.CommitmentID) + assert.Equal(t, tt.reason, got.Reason) + assert.Equal(t, tt.tagged, got.Tagged) + assert.Equal(t, tt.candidates, got.CandidateIDs) + if tt.verdict == common.ReconcileFound { + assert.NotEmpty(t, got.Criteria) + } + if tt.wantDescribe { + m.AssertNumberOfCalls(t, "DescribeReservedInstances", 1) + } else { + m.AssertNotCalled(t, "DescribeReservedInstances", mock.Anything, mock.Anything) + } + m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) + m.AssertNotCalled(t, "CreateTags", mock.Anything, mock.Anything) + }) + } +} + +// The purchase returns the empty-200 outcome-unknown and the RI was never tagged: the token lookup +// cannot see it, reconcile can. +func TestReconcilePurchase_LostResponseUntaggedRI(t *testing.T) { + t.Parallel() + m := &MockEC2Client{} + c := &Client{client: m, region: "us-east-1"} + untagged := reconcileRI("ri-lost") + m.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ReservedInstancesOfferings: []types.ReservedInstancesOffering{{ + ReservedInstancesOfferingId: aws.String("offering-1"), InstanceType: types.InstanceTypeM5Large, + Duration: aws.Int64(OneYearSeconds), OfferingType: types.OfferingTypeValuesNoUpfront, + ProductDescription: types.RIProductDescriptionLinuxUnix, InstanceTenancy: types.TenancyDefault, + OfferingClass: types.OfferingClassTypeStandard, + }}}, nil) + m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.Anything). + Return(&ec2.PurchaseReservedInstancesOfferingOutput{}, nil) + byToken := func(in *ec2.DescribeReservedInstancesInput) bool { return len(in.Filters) > 0 } + m.On("DescribeReservedInstances", mock.Anything, mock.MatchedBy(byToken)). + Return(&ec2.DescribeReservedInstancesOutput{}, nil) + m.On("DescribeReservedInstances", mock.Anything, mock.MatchedBy(func(in *ec2.DescribeReservedInstancesInput) bool { return !byToken(in) })). + Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstances{untagged}}, nil) + + req := reconcileReq() + _, err := c.PurchaseCommitment(context.Background(), req.Recommendation, req.Options) + require.ErrorIs(t, err, common.ErrOutcomeUnknown) + + _, found, err := c.findRIByIdempotencyToken(context.Background(), req.Options.IdempotencyToken) + require.NoError(t, err) + assert.False(t, found, "the token lookup alone misses an untagged RI") + + got := c.ReconcilePurchase(context.Background(), req) + assert.True(t, got.IsFound()) + assert.Equal(t, "ri-lost", got.CommitmentID) + assert.False(t, got.Tagged) +} From 70eb84a218a1a6ba897ff7995cac0fd8f3b9d12f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:59:06 +0100 Subject: [PATCH 08/17] fix(aws): copy criteria in purchasecfg.Decide Found results shared the caller backing array, so a consumer writing an element corrupted later results. Clone on the Found path. Co-Authored-By: Claude Opus 5.5 --- providers/aws/internal/purchasecfg/reconcile.go | 3 ++- providers/aws/internal/purchasecfg/reconcile_test.go | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/providers/aws/internal/purchasecfg/reconcile.go b/providers/aws/internal/purchasecfg/reconcile.go index ed991b90..1c0706c6 100644 --- a/providers/aws/internal/purchasecfg/reconcile.go +++ b/providers/aws/internal/purchasecfg/reconcile.go @@ -1,6 +1,7 @@ package purchasecfg import ( + "slices" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -65,7 +66,7 @@ func Decide(matches, uncertain, criteria []string, tagged bool) common.Reconcile return common.ReconcileResult{ Verdict: common.ReconcileFound, CommitmentID: matches[0], - Criteria: criteria, + Criteria: slices.Clone(criteria), Tagged: tagged, } default: diff --git a/providers/aws/internal/purchasecfg/reconcile_test.go b/providers/aws/internal/purchasecfg/reconcile_test.go index 8d9ca21b..3a8acdf0 100644 --- a/providers/aws/internal/purchasecfg/reconcile_test.go +++ b/providers/aws/internal/purchasecfg/reconcile_test.go @@ -139,3 +139,15 @@ func TestReasonPhrasesAreDistinct(t *testing.T) { } assert.Len(t, seen, 8) } + +func TestDecideCopiesCriteria(t *testing.T) { + t.Parallel() + crit := []string{"instance-type", "count"} + r1 := Decide([]string{"m1"}, nil, crit, false) + r2 := Decide([]string{"m1"}, nil, crit, false) + + r1.Criteria[0] = "mutated" + + assert.Equal(t, []string{"instance-type", "count"}, r2.Criteria) + assert.Equal(t, []string{"instance-type", "count"}, crit) +} From 15eb7043788767145e46fc453e2029542695b165 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:01:11 +0100 Subject: [PATCH 09/17] refactor(aws): extract Redshift describeAllNodes paging helper findMatchingNodes now filters the full scan; behaviour is unchanged and pinned by a characterization test. redshiftClockSkew reuses purchasecfg.ClockSkew. Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/redshift/dedupe.go | 26 +++++-- .../redshift/find_matching_nodes_test.go | 76 +++++++++++++++++++ 2 files changed, 95 insertions(+), 7 deletions(-) create mode 100644 providers/aws/services/redshift/find_matching_nodes_test.go diff --git a/providers/aws/services/redshift/dedupe.go b/providers/aws/services/redshift/dedupe.go index de3a0008..ee2379d5 100644 --- a/providers/aws/services/redshift/dedupe.go +++ b/providers/aws/services/redshift/dedupe.go @@ -24,7 +24,7 @@ const ( // redshiftClockSkew is the slack between the caller's RequestedAt and AWS's // node StartTime. The window deliberately has no upper bound: a late // StartTime must never cause a double buy. - redshiftClockSkew = 5 * time.Minute + redshiftClockSkew = purchasecfg.ClockSkew // recoverLookupTimeout bounds the post-error lookup, which runs on a // context detached from the caller's cancellation. recoverLookupTimeout = 30 * time.Second @@ -47,8 +47,24 @@ var errRequestedAtRequired = errors.New("RequestedAt is required with an idempot // DescribeReservedNodes to list a node that attempt bought; an AttemptJournal // (go#107) records the attempts but cannot close that listing lag. func (c *Client) findMatchingNodes(ctx context.Context, offeringID string, count int, requestedAt time.Time) ([]string, error) { + nodes, err := c.describeAllNodes(ctx) + if err != nil { + return nil, err + } since := requestedAt.Add(-redshiftClockSkew) var ids []string + for i := range nodes { + if nodeMatches(&nodes[i], offeringID, count, since) { + ids = append(ids, aws.ToString(nodes[i].ReservedNodeId)) + } + } + return ids, nil +} + +// describeAllNodes pages through every reserved node, failing if the scan is incomplete +// (API error, cancelled context or page cap) so callers never treat a partial list as complete. +func (c *Client) describeAllNodes(ctx context.Context) ([]redshifttypes.ReservedNode, error) { + var nodes []redshifttypes.ReservedNode var marker *string for page := 0; page < maxReservedNodePages; page++ { if err := ctx.Err(); err != nil { @@ -61,13 +77,9 @@ func (c *Client) findMatchingNodes(ctx context.Context, offeringID string, count if err != nil { return nil, fmt.Errorf("failed to describe reserved nodes for dedupe: %w", err) } - for i := range response.ReservedNodes { - if nodeMatches(&response.ReservedNodes[i], offeringID, count, since) { - ids = append(ids, aws.ToString(response.ReservedNodes[i].ReservedNodeId)) - } - } + nodes = append(nodes, response.ReservedNodes...) if aws.ToString(response.Marker) == "" { - return ids, nil + return nodes, nil } marker = response.Marker } diff --git a/providers/aws/services/redshift/find_matching_nodes_test.go b/providers/aws/services/redshift/find_matching_nodes_test.go new file mode 100644 index 00000000..4e20f660 --- /dev/null +++ b/providers/aws/services/redshift/find_matching_nodes_test.go @@ -0,0 +1,76 @@ +package redshift + +import ( + "context" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/redshift" + "github.com/aws/aws-sdk-go-v2/service/redshift/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Characterization of findMatchingNodes: pins the paging, cap, ctx and field-matching behaviour. +func TestFindMatchingNodes_Characterization(t *testing.T) { + t.Parallel() + active := "active" + match := func(id string) types.ReservedNode { return dnode(id, active, dedupeReqAt) } + old := dnode("old", active, dedupeReqAt.Add(-time.Hour)) + otherOffering := dnode("other", active, dedupeReqAt, func(n *types.ReservedNode) { n.ReservedNodeOfferingId = aws.String("x") }) + otherCount := dnode("cnt", active, dedupeReqAt, func(n *types.ReservedNode) { n.NodeCount = aws.Int32(9) }) + noStart := dnode("nostart", active, dedupeReqAt, func(n *types.ReservedNode) { n.StartTime = nil }) + noOffering := dnode("nooff", active, dedupeReqAt, func(n *types.ReservedNode) { n.ReservedNodeOfferingId = nil }) + noCount := dnode("nocnt", active, dedupeReqAt, func(n *types.ReservedNode) { n.NodeCount = nil }) + retired := dnode("ret", "retired", dedupeReqAt) + + tests := []struct { + name string + pages [][]types.ReservedNode + want []string + }{ + {"one page", [][]types.ReservedNode{{match("a"), old, otherOffering, otherCount, retired}}, []string{"a"}}, + {"two pages", [][]types.ReservedNode{{match("a")}, {match("b")}}, []string{"a", "b"}}, + {"missing fields still match", [][]types.ReservedNode{{noStart, noOffering, noCount}}, []string{"nostart", "nooff", "nocnt"}}, + {"empty", [][]types.ReservedNode{{}}, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + m := &MockRedshiftClient{} + m.Test(t) + expectNodePages(m, tt.pages...) + c := &Client{client: m, region: "eu-west-1"} + got, err := c.findMatchingNodes(context.Background(), "offering-1", 1, dedupeReqAt) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } + + t.Run("cap reached", func(t *testing.T) { + t.Parallel() + m := &MockRedshiftClient{} + m.Test(t) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodesOutput{Marker: aws.String("more")}, nil) + c := &Client{client: m, region: "eu-west-1"} + _, err := c.findMatchingNodes(context.Background(), "offering-1", 1, dedupeReqAt) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap reached") + m.AssertNumberOfCalls(t, "DescribeReservedNodes", maxReservedNodePages) + }) + + t.Run("ctx cancelled", func(t *testing.T) { + t.Parallel() + m := &MockRedshiftClient{} + m.Test(t) + c := &Client{client: m, region: "eu-west-1"} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err := c.findMatchingNodes(ctx, "offering-1", 1, dedupeReqAt) + assert.ErrorIs(t, err, context.Canceled) + m.AssertNotCalled(t, "DescribeReservedNodes", mock.Anything, mock.Anything) + }) +} From ee28d00fe6a0cee0bc81e30fb1622ccba8258127 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:01:36 +0100 Subject: [PATCH 10/17] style(aws): fix misspell lint in Redshift dedupe Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/redshift/dedupe.go | 2 +- providers/aws/services/redshift/find_matching_nodes_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/providers/aws/services/redshift/dedupe.go b/providers/aws/services/redshift/dedupe.go index ee2379d5..58c4e7e7 100644 --- a/providers/aws/services/redshift/dedupe.go +++ b/providers/aws/services/redshift/dedupe.go @@ -62,7 +62,7 @@ func (c *Client) findMatchingNodes(ctx context.Context, offeringID string, count } // describeAllNodes pages through every reserved node, failing if the scan is incomplete -// (API error, cancelled context or page cap) so callers never treat a partial list as complete. +// (API error, canceled context or page cap) so callers never treat a partial list as complete. func (c *Client) describeAllNodes(ctx context.Context) ([]redshifttypes.ReservedNode, error) { var nodes []redshifttypes.ReservedNode var marker *string diff --git a/providers/aws/services/redshift/find_matching_nodes_test.go b/providers/aws/services/redshift/find_matching_nodes_test.go index 4e20f660..595b81df 100644 --- a/providers/aws/services/redshift/find_matching_nodes_test.go +++ b/providers/aws/services/redshift/find_matching_nodes_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/require" ) -// Characterization of findMatchingNodes: pins the paging, cap, ctx and field-matching behaviour. +// Characterization of findMatchingNodes: pins the paging, cap, ctx and field-matching behavior. func TestFindMatchingNodes_Characterization(t *testing.T) { t.Parallel() active := "active" @@ -62,7 +62,7 @@ func TestFindMatchingNodes_Characterization(t *testing.T) { m.AssertNumberOfCalls(t, "DescribeReservedNodes", maxReservedNodePages) }) - t.Run("ctx cancelled", func(t *testing.T) { + t.Run("ctx canceled", func(t *testing.T) { t.Parallel() m := &MockRedshiftClient{} m.Test(t) From ee7021c198dd6f77b2a3624e8cd12ecbe85c608f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:04:05 +0100 Subject: [PATCH 11/17] test(aws): pin mid-scan ctx cancel in findMatchingNodes The pre-canceled case duplicated an existing test; cancel during page 1 instead so the per-page ctx re-check is covered. Co-Authored-By: Claude Opus 5.5 --- .../services/redshift/find_matching_nodes_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/providers/aws/services/redshift/find_matching_nodes_test.go b/providers/aws/services/redshift/find_matching_nodes_test.go index 595b81df..7a7450fd 100644 --- a/providers/aws/services/redshift/find_matching_nodes_test.go +++ b/providers/aws/services/redshift/find_matching_nodes_test.go @@ -62,15 +62,19 @@ func TestFindMatchingNodes_Characterization(t *testing.T) { m.AssertNumberOfCalls(t, "DescribeReservedNodes", maxReservedNodePages) }) - t.Run("ctx canceled", func(t *testing.T) { + t.Run("ctx canceled mid-scan", func(t *testing.T) { t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() m := &MockRedshiftClient{} m.Test(t) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything). + Run(func(mock.Arguments) { cancel() }). + Return(&redshift.DescribeReservedNodesOutput{ReservedNodes: []types.ReservedNode{match("a")}, Marker: aws.String("p1")}, nil) c := &Client{client: m, region: "eu-west-1"} - ctx, cancel := context.WithCancel(context.Background()) - cancel() - _, err := c.findMatchingNodes(ctx, "offering-1", 1, dedupeReqAt) + got, err := c.findMatchingNodes(ctx, "offering-1", 1, dedupeReqAt) assert.ErrorIs(t, err, context.Canceled) - m.AssertNotCalled(t, "DescribeReservedNodes", mock.Anything, mock.Anything) + assert.Nil(t, got) + m.AssertNumberOfCalls(t, "DescribeReservedNodes", 1) }) } From 6a956f0e31c7a729387ccb0f0310f53b2b3e6d69 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:08:39 +0100 Subject: [PATCH 12/17] feat(aws): add Redshift reserved-node reconcile classifier Pure nodeFit classifies a described node against the purchase as no, uncertain or match; only live nodes with every field present and equal match. Co-Authored-By: Claude Opus 5.5 --- .../aws/services/redshift/reconcile_match.go | 89 +++++++++++++++++++ .../services/redshift/reconcile_match_test.go | 74 +++++++++++++++ 2 files changed, 163 insertions(+) create mode 100644 providers/aws/services/redshift/reconcile_match.go create mode 100644 providers/aws/services/redshift/reconcile_match_test.go diff --git a/providers/aws/services/redshift/reconcile_match.go b/providers/aws/services/redshift/reconcile_match.go new file mode 100644 index 00000000..1d43cdfc --- /dev/null +++ b/providers/aws/services/redshift/reconcile_match.go @@ -0,0 +1,89 @@ +package redshift + +import ( + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + redshifttypes "github.com/aws/aws-sdk-go-v2/service/redshift/types" + + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" +) + +// fit is how one reserved node relates to the purchase being reconciled. +type fit int + +const ( + fitNo fit = iota // a field is present and different: not this purchase + fitUncertain // cannot be ruled in or out + fitMatch // every required field is present and equal +) + +// secondsPerMonth is the arithmetic matchesDuration uses for offerings. +const secondsPerMonth = 2592000 + +// presentFit: nil is doubt, an equal value matches, a different one rules the node out. +func presentFit[T comparable](got *T, want T) fit { + switch { + case got == nil: + return fitUncertain + case *got == want: + return fitMatch + } + return fitNo +} + +// nodeFit classifies one reserved node against the purchase, section 4.2 of the plan. A definite +// mismatch beats doubt, doubt beats a match; only a live node with every field present and equal +// is fitMatch. +func nodeFit(node *redshifttypes.ReservedNode, offeringID string, count int, nodeType string, months int, lo, hi time.Time) fit { + fits := []fit{ + presentFit(node.ReservedNodeOfferingId, offeringID), + presentFit(node.NodeType, nodeType), + nodeCountFit(node.NodeCount, count), + durationFit(node.Duration, months), + startFit(node.StartTime, lo, hi), + stateFit(node.State), + } + if aws.ToString(node.ReservedNodeId) == "" { + fits = append(fits, fitUncertain) + } + f := fitMatch + for _, x := range fits { + f = min(f, x) + } + return f +} + +func nodeCountFit(got *int32, want int) fit { + if got == nil { + return fitUncertain + } + return presentFit(aws.Int(int(*got)), want) +} + +func durationFit(got *int32, months int) fit { + if got == nil { + return fitUncertain + } + return presentFit(aws.Int(int(*got/secondsPerMonth)), months) +} + +func startFit(got *time.Time, lo, hi time.Time) fit { + switch { + case got == nil: + return fitUncertain + case purchasecfg.InWindow(*got, lo, hi): + return fitMatch + } + return fitNo +} + +// stateFit: only live states can be reported Found; any other state (retired, payment-failed, +// exchanging, empty) with matching attributes is doubt. The SDK defines no enum for the field. +func stateFit(state *string) fit { + switch aws.ToString(state) { + case "active", redshiftStatePendingPayment, "payment-pending": + return fitMatch + } + return fitUncertain +} diff --git a/providers/aws/services/redshift/reconcile_match_test.go b/providers/aws/services/redshift/reconcile_match_test.go new file mode 100644 index 00000000..f9c2f2a2 --- /dev/null +++ b/providers/aws/services/redshift/reconcile_match_test.go @@ -0,0 +1,74 @@ +package redshift + +import ( + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/redshift/types" + "github.com/stretchr/testify/assert" +) + +var matchT0 = time.Date(2026, 11, 6, 12, 0, 0, 0, time.UTC) + +const oneYearNodeSeconds = int32(12 * 2592000) + +func baseNode(mut ...func(*types.ReservedNode)) types.ReservedNode { + n := types.ReservedNode{ + ReservedNodeId: aws.String("rn-1"), ReservedNodeOfferingId: aws.String("offering-1"), + NodeCount: aws.Int32(2), NodeType: aws.String("ra3.xlplus"), Duration: aws.Int32(oneYearNodeSeconds), + StartTime: aws.Time(matchT0.Add(time.Minute)), State: aws.String("active"), + } + for _, m := range mut { + m(&n) + } + return n +} + +func TestNodeFit(t *testing.T) { + t.Parallel() + lo, hi := matchT0.Add(-5*time.Minute), matchT0.Add(10*time.Minute) + set := func(f func(*types.ReservedNode)) []func(*types.ReservedNode) { return []func(*types.ReservedNode){f} } + + tests := []struct { + name string + mut []func(*types.ReservedNode) + want fit + }{ + {"all equal", nil, fitMatch}, + {"state pending-payment", set(func(n *types.ReservedNode) { n.State = aws.String("pending-payment") }), fitMatch}, + {"state payment-pending", set(func(n *types.ReservedNode) { n.State = aws.String("payment-pending") }), fitMatch}, + {"start at lo", set(func(n *types.ReservedNode) { n.StartTime = aws.Time(lo) }), fitMatch}, + {"start at hi", set(func(n *types.ReservedNode) { n.StartTime = aws.Time(hi) }), fitMatch}, + {"start before lo", set(func(n *types.ReservedNode) { n.StartTime = aws.Time(lo.Add(-time.Second)) }), fitNo}, + {"start after hi", set(func(n *types.ReservedNode) { n.StartTime = aws.Time(hi.Add(time.Second)) }), fitNo}, + {"future start", set(func(n *types.ReservedNode) { n.StartTime = aws.Time(matchT0.Add(24 * time.Hour)) }), fitNo}, + {"nil start", set(func(n *types.ReservedNode) { n.StartTime = nil }), fitUncertain}, + {"other offering", set(func(n *types.ReservedNode) { n.ReservedNodeOfferingId = aws.String("x") }), fitNo}, + {"nil offering", set(func(n *types.ReservedNode) { n.ReservedNodeOfferingId = nil }), fitUncertain}, + {"other count", set(func(n *types.ReservedNode) { n.NodeCount = aws.Int32(3) }), fitNo}, + {"nil count", set(func(n *types.ReservedNode) { n.NodeCount = nil }), fitUncertain}, + {"other node type", set(func(n *types.ReservedNode) { n.NodeType = aws.String("dc2.large") }), fitNo}, + {"nil node type", set(func(n *types.ReservedNode) { n.NodeType = nil }), fitUncertain}, + {"other duration", set(func(n *types.ReservedNode) { n.Duration = aws.Int32(3 * oneYearNodeSeconds) }), fitNo}, + {"nil duration", set(func(n *types.ReservedNode) { n.Duration = nil }), fitUncertain}, + {"state retired", set(func(n *types.ReservedNode) { n.State = aws.String("retired") }), fitUncertain}, + {"state payment-failed", set(func(n *types.ReservedNode) { n.State = aws.String("payment-failed") }), fitUncertain}, + {"state exchanging", set(func(n *types.ReservedNode) { n.State = aws.String("exchanging") }), fitUncertain}, + {"state empty", set(func(n *types.ReservedNode) { n.State = aws.String("") }), fitUncertain}, + {"state nil", set(func(n *types.ReservedNode) { n.State = nil }), fitUncertain}, + {"nil id", set(func(n *types.ReservedNode) { n.ReservedNodeId = nil }), fitUncertain}, + {"empty id", set(func(n *types.ReservedNode) { n.ReservedNodeId = aws.String("") }), fitUncertain}, + {"retired and other count is no", []func(*types.ReservedNode){ + func(n *types.ReservedNode) { n.State = aws.String("retired") }, + func(n *types.ReservedNode) { n.NodeCount = aws.Int32(3) }, + }, fitNo}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + n := baseNode(tt.mut...) + assert.Equal(t, tt.want, nodeFit(&n, "offering-1", 2, "ra3.xlplus", 12, lo, hi)) + }) + } +} From 05f4e057a276c9b0750a08e80fe3f7a1f2196a5f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:12:30 +0100 Subject: [PATCH 13/17] feat(aws): add Redshift ReconcilePurchase Read-only lookup after an unknown purchase outcome: resolves the offering, scans every reserved node and folds nodeFit results into purchasecfg.Decide. An incomplete scan is Unknown (cap is lookup-incomplete, distinct from lookup-failed). Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/redshift/dedupe.go | 5 +- providers/aws/services/redshift/reconcile.go | 74 +++++++ .../aws/services/redshift/reconcile_test.go | 187 ++++++++++++++++++ 3 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 providers/aws/services/redshift/reconcile.go create mode 100644 providers/aws/services/redshift/reconcile_test.go diff --git a/providers/aws/services/redshift/dedupe.go b/providers/aws/services/redshift/dedupe.go index 58c4e7e7..07c72252 100644 --- a/providers/aws/services/redshift/dedupe.go +++ b/providers/aws/services/redshift/dedupe.go @@ -30,6 +30,9 @@ const ( recoverLookupTimeout = 30 * time.Second ) +// errScanCapReached marks an incomplete describeAllNodes scan, so callers can tell it from an API error. +var errScanCapReached = errors.New("pagination cap reached") + // errRequestedAtRequired refuses a tokenized purchase that cannot be matched. var errRequestedAtRequired = errors.New("RequestedAt is required with an idempotency token: Redshift reserved nodes cannot be tagged, so an earlier purchase is found by start time; refusing to purchase to avoid a possible double-buy") @@ -83,7 +86,7 @@ func (c *Client) describeAllNodes(ctx context.Context) ([]redshifttypes.Reserved } marker = response.Marker } - return nil, fmt.Errorf("pagination cap reached after %d pages for Redshift reserved-node dedupe lookup", maxReservedNodePages) + return nil, fmt.Errorf("%w after %d pages for Redshift reserved-node dedupe lookup", errScanCapReached, maxReservedNodePages) } func nodeMatches(node *redshifttypes.ReservedNode, offeringID string, count int, since time.Time) bool { diff --git a/providers/aws/services/redshift/reconcile.go b/providers/aws/services/redshift/reconcile.go new file mode 100644 index 00000000..bdd87d4f --- /dev/null +++ b/providers/aws/services/redshift/reconcile.go @@ -0,0 +1,74 @@ +package redshift + +import ( + "context" + "errors" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" +) + +var reconcileCriteria = []string{"offering-id", "count", "node-type", "duration", "start-window"} + +// reconcileParams validates the request; reason is non-empty on refusal. +func (c *Client) reconcileParams(req common.ReconcileRequest) (count int32, months int, lo, hi time.Time, reason string) { + lo, hi, ok := purchasecfg.ReconcileWindow(req.Options.RequestedAt, req.AttemptEndedAt) + if !ok { + return 0, 0, lo, hi, purchasecfg.ReasonInvalidWindow + } + rec := req.Recommendation + count, countErr := offeringprice.PurchaseCount(rec.Count) + months, termErr := requiredMonthsForTerm(rec.Term) + if rec.Region != c.region || countErr != nil || termErr != nil { + return 0, 0, lo, hi, purchasecfg.ReasonInvalidRequest + } + return count, months, lo, hi, "" +} + +// scanFailure maps an incomplete describeAllNodes scan to its Reason. +func scanFailure(err error) string { + if errors.Is(err, errScanCapReached) { + return purchasecfg.ReasonLookupIncomplete + } + return purchasecfg.ReasonLookupFailed +} + +// ReconcilePurchase reports, read-only, whether a purchase that returned ErrOutcomeUnknown landed. +// Any doubt or failure is Unknown; see purchasecfg.Decide. +func (c *Client) ReconcilePurchase(ctx context.Context, req common.ReconcileRequest) common.ReconcileResult { + count, months, lo, hi, reason := c.reconcileParams(req) + if reason != "" { + return purchasecfg.UnknownResult(reason) + } + if ctx.Err() != nil { + return purchasecfg.UnknownResult(purchasecfg.ReasonLookupFailed) + } + + rec := req.Recommendation + offeringID, err := c.findOfferingID(ctx, rec, req.Options.ExecutionID) + if err != nil || offeringID == "" { + return purchasecfg.UnknownResult(purchasecfg.ReasonOfferingUnresolved) + } + nodes, err := c.describeAllNodes(ctx) + if err != nil { + return purchasecfg.UnknownResult(scanFailure(err)) + } + + var matches, uncertain []string + for i := range nodes { + id := aws.ToString(nodes[i].ReservedNodeId) + switch nodeFit(&nodes[i], offeringID, int(count), rec.ResourceType, months, lo, hi) { + case fitMatch: + matches = append(matches, id) + case fitUncertain: + uncertain = append(uncertain, id) + case fitNo: + } + } + // Tagged is EC2-only: Redshift reserved nodes cannot carry the idempotency tag, so it is always false. + return purchasecfg.Decide(matches, uncertain, reconcileCriteria, false) +} diff --git a/providers/aws/services/redshift/reconcile_test.go b/providers/aws/services/redshift/reconcile_test.go new file mode 100644 index 00000000..07e6a99a --- /dev/null +++ b/providers/aws/services/redshift/reconcile_test.go @@ -0,0 +1,187 @@ +package redshift + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/redshift" + "github.com/aws/aws-sdk-go-v2/service/redshift/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" +) + +func reconcileReq(mut ...func(*common.ReconcileRequest)) common.ReconcileRequest { + req := common.ReconcileRequest{ + Recommendation: rsIdemRec(), + Options: common.PurchaseOptions{RequestedAt: matchT0, ExecutionID: "exec-1"}, + AttemptEndedAt: matchT0.Add(time.Minute), + } + for _, m := range mut { + m(&req) + } + return req +} + +// reconcileNode builds a node that matches rsIdemRec (offering-1, 1 x ra3.xlplus, 1yr) in the window. +func reconcileNode(id string, mut ...func(*types.ReservedNode)) types.ReservedNode { + n := baseNode(func(n *types.ReservedNode) { + n.ReservedNodeId = aws.String(id) + n.NodeCount = aws.Int32(1) + }) + for _, m := range mut { + m(&n) + } + return n +} + +func nodeState(s string) func(*types.ReservedNode) { + return func(n *types.ReservedNode) { n.State = aws.String(s) } +} + +type reconcileFake struct { + pages [][]types.ReservedNode + describeErr error + offeringErr error + alwaysMarker bool +} + +func (f reconcileFake) client(t *testing.T) (*Client, *MockRedshiftClient) { + t.Helper() + m := &MockRedshiftClient{} + m.Test(t) + if f.offeringErr != nil { + m.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything).Return(nil, f.offeringErr).Maybe() + } else { + expectRSOffering(m) + } + switch { + case f.describeErr != nil: + m.On("DescribeReservedNodes", mock.Anything, mock.Anything).Return(nil, f.describeErr).Maybe() + case f.alwaysMarker: + m.On("DescribeReservedNodes", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodesOutput{Marker: aws.String("more")}, nil).Maybe() + default: + expectNodePages(m, f.pages...) + } + c := &Client{region: "eu-west-1"} + c.SetRedshiftAPI(m) + return c, m +} + +func TestReconcilePurchase(t *testing.T) { + t.Parallel() + canceled, cancel := context.WithCancel(context.Background()) + cancel() + one := func(ns ...types.ReservedNode) reconcileFake { return reconcileFake{pages: [][]types.ReservedNode{ns}} } + + tests := []struct { + name string + ctx context.Context + fake reconcileFake + req common.ReconcileRequest + wantAPICalls bool + verdict common.ReconcileVerdict + id, reason string + candidates []string + }{ + {name: "one matching active node", fake: one(reconcileNode("rn-1")), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileFound, id: "rn-1"}, + {name: "match on page 2", fake: reconcileFake{pages: [][]types.ReservedNode{{reconcileNode("rn-x", func(n *types.ReservedNode) { n.NodeCount = aws.Int32(9) })}, {reconcileNode("rn-2")}}}, + req: reconcileReq(), wantAPICalls: true, verdict: common.ReconcileFound, id: "rn-2"}, + {name: "pending-payment node matches", fake: one(reconcileNode("rn-1", nodeState("pending-payment"))), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileFound, id: "rn-1"}, + {name: "no nodes", fake: one(), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "two matches", fake: one(reconcileNode("rn-1"), reconcileNode("rn-2")), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonAmbiguous, candidates: []string{"rn-1", "rn-2"}}, + {name: "count differs", fake: one(reconcileNode("rn-1", func(n *types.ReservedNode) { n.NodeCount = aws.Int32(5) })), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "other offering", fake: one(reconcileNode("rn-1", func(n *types.ReservedNode) { n.ReservedNodeOfferingId = aws.String("other") })), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "started before lo", fake: one(reconcileNode("rn-1", func(n *types.ReservedNode) { n.StartTime = aws.Time(matchT0.Add(-time.Hour)) })), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "started after hi", fake: one(reconcileNode("rn-1", func(n *types.ReservedNode) { n.StartTime = aws.Time(matchT0.Add(time.Hour)) })), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "future start", fake: one(reconcileNode("rn-1", func(n *types.ReservedNode) { n.StartTime = aws.Time(matchT0.Add(24 * time.Hour)) })), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonNoCandidate}, + {name: "nil start", fake: one(reconcileNode("rn-1", func(n *types.ReservedNode) { n.StartTime = nil })), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonUncertainCandidate, candidates: []string{"rn-1"}}, + {name: "retired node", fake: one(reconcileNode("rn-1", nodeState("retired"))), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonUncertainCandidate, candidates: []string{"rn-1"}}, + {name: "payment-failed node", fake: one(reconcileNode("rn-1", nodeState("payment-failed"))), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonUncertainCandidate, candidates: []string{"rn-1"}}, + {name: "match plus retired", fake: one(reconcileNode("rn-1"), reconcileNode("rn-2", nodeState("retired"))), req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonUncertainCandidate, candidates: []string{"rn-1", "rn-2"}}, + {name: "describe error", fake: reconcileFake{describeErr: errors.New("boom")}, req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonLookupFailed}, + {name: "page cap", fake: reconcileFake{alwaysMarker: true}, req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonLookupIncomplete}, + {name: "canceled context", ctx: canceled, fake: one(reconcileNode("rn-1")), req: reconcileReq(), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonLookupFailed}, + {name: "offering error", fake: reconcileFake{offeringErr: errors.New("boom")}, req: reconcileReq(), + wantAPICalls: true, verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonOfferingUnresolved}, + {name: "region mismatch", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Region = "us-east-1" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "empty region", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Region = "" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "zero count", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Count = 0 }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "bad term", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Term = "5yr" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "zero RequestedAt", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Options.RequestedAt = time.Time{} }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidWindow}, + {name: "zero AttemptEndedAt", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.AttemptEndedAt = time.Time{} }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidWindow}, + {name: "end before start", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.AttemptEndedAt = matchT0.Add(-time.Hour) }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidWindow}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + c, m := tt.fake.client(t) + ctx := tt.ctx + if ctx == nil { + ctx = context.Background() + } + got := c.ReconcilePurchase(ctx, tt.req) + + assert.Equal(t, tt.verdict, got.Verdict) + assert.Equal(t, tt.id, got.CommitmentID) + assert.Equal(t, tt.reason, got.Reason) + assert.Equal(t, tt.candidates, got.CandidateIDs) + assert.False(t, got.Tagged, "Tagged is EC2-only") + if tt.verdict == common.ReconcileFound { + assert.NotEmpty(t, got.Criteria) + } else { + assert.Empty(t, got.Criteria) + } + if !tt.wantAPICalls { + m.AssertNotCalled(t, "DescribeReservedNodes", mock.Anything, mock.Anything) + m.AssertNotCalled(t, "DescribeReservedNodeOfferings", mock.Anything, mock.Anything) + } + m.AssertNotCalled(t, "PurchaseReservedNodeOffering", mock.Anything, mock.Anything) + }) + } +} + +func TestReconcilePurchase_CapDistinctFromLookupFailed(t *testing.T) { + t.Parallel() + _, err := (&Client{client: capClient(t)}).describeAllNodes(context.Background()) + require.ErrorIs(t, err, errScanCapReached) +} + +func capClient(t *testing.T) *MockRedshiftClient { + t.Helper() + m := &MockRedshiftClient{} + m.Test(t) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodesOutput{Marker: aws.String("more")}, nil) + return m +} From b31b9ad70472e01a1592ca02f13ef6d82810688e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:18:43 +0100 Subject: [PATCH 14/17] test(aws): pin PurchaseReconciler wiring for EC2 and Redshift Assert the ec2 and redshift clients, directly and via Provider.GetServiceClient, implement provider.PurchaseReconciler, and that rds, elasticache, memorydb, opensearch and savingsplans do not. Add the CHANGELOG entry for the AWS reconcilers (#298). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + providers/aws/reconciler_test.go | 67 ++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 providers/aws/reconciler_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4caacb67..729e2d0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **`common`:** `AttemptJournal`, `AttemptRecord` and an optional `PurchaseOptions.AttemptJournal`, a caller-backed durable record of purchase attempts per idempotency token (#107). Nothing reads it yet. - **`pkg`:** `provider.PurchaseReconciler`, an optional read-only `ServiceClient` interface with `common.ReconcileRequest`, `common.ReconcileResult` and `common.ReconcileVerdict` (`ReconcileFound`/`ReconcileUnknown`, never NotFound), so a purchase that returned `ErrOutcomeUnknown` can later be checked against the provider. Types only; AWS EC2 and Redshift implementations follow (#298). +- **`aws`:** the `ec2` and `redshift` service clients now implement `provider.PurchaseReconciler`: a read-only check, after a purchase returned `ErrOutcomeUnknown`, of whether the purchase landed. The verdict is `Found` or `Unknown`, never `NotFound`. It reports `Found` only for exactly one full attribute match inside the attempt window, and `Unknown` otherwise. The other AWS clients (rds, elasticache, memorydb, opensearch, savingsplans) have an idempotency token and do not implement it. The platform consumes it in a follow-up (#298). - **`ladder`/`aws`:** `ladder.ObservedCommitmentRateCapability` and its AWS implementation report commitment dollars per covered on-demand dollar measured on the account's existing Savings Plans and convertible RIs (account-scoped Cost Explorer utilization). Layers with no usable observation are absent. diff --git a/providers/aws/reconciler_test.go b/providers/aws/reconciler_test.go new file mode 100644 index 00000000..5b6c7a00 --- /dev/null +++ b/providers/aws/reconciler_test.go @@ -0,0 +1,67 @@ +package aws + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + sptypes "github.com/aws/aws-sdk-go-v2/service/savingsplans/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" +) + +// Only the EC2 and Redshift purchase APIs lack an idempotency token. A silent Found from any +// other client would be wrong, and a wrapper hiding the reconciler would degrade to Unknown. +func TestServiceClients_PurchaseReconcilerWiring(t *testing.T) { + t.Parallel() + cfg := aws.Config{Region: "us-east-1"} + + constructors := map[string]struct { + client provider.ServiceClient + want bool + }{ + "ec2": {NewEC2Client(cfg), true}, + "redshift": {NewRedshiftClient(cfg), true}, + "rds": {NewRDSClient(cfg), false}, + "elasticache": {NewElastiCacheClient(cfg), false}, + "memorydb": {NewMemoryDBClient(cfg), false}, + "opensearch": {NewOpenSearchClient(cfg), false}, + "savingsplans": {NewSavingsPlansClient(cfg, sptypes.SavingsPlanTypeCompute), false}, + } + for name, tc := range constructors { + t.Run(name, func(t *testing.T) { + t.Parallel() + _, ok := tc.client.(provider.PurchaseReconciler) + assert.Equal(t, tc.want, ok) + }) + } +} + +func TestProvider_GetServiceClient_PurchaseReconciler(t *testing.T) { + t.Parallel() + p := &Provider{cfg: aws.Config{Region: "us-east-1"}} + + cases := map[common.ServiceType]bool{ + common.ServiceEC2: true, + common.ServiceCompute: true, + common.ServiceRedshift: true, + common.ServiceDataWarehouse: true, + common.ServiceRDS: false, + common.ServiceElastiCache: false, + common.ServiceMemoryDB: false, + common.ServiceOpenSearch: false, + common.ServiceSavingsPlansCompute: false, + } + for service, want := range cases { + t.Run(string(service), func(t *testing.T) { + t.Parallel() + client, err := p.GetServiceClient(context.Background(), service, "us-east-1") + require.NoError(t, err) + _, ok := client.(provider.PurchaseReconciler) + assert.Equal(t, want, ok) + }) + } +} From b4e909df41fee747028e13338785aaaf9a903196 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:20:30 +0100 Subject: [PATCH 15/17] test(aws): cover every GetServiceClient slug in reconciler wiring test Add the seven remaining slugs from the switch, all expecting no reconciler. Co-Authored-By: Claude Opus 5.5 --- providers/aws/reconciler_test.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/providers/aws/reconciler_test.go b/providers/aws/reconciler_test.go index 5b6c7a00..27895e9d 100644 --- a/providers/aws/reconciler_test.go +++ b/providers/aws/reconciler_test.go @@ -45,15 +45,22 @@ func TestProvider_GetServiceClient_PurchaseReconciler(t *testing.T) { p := &Provider{cfg: aws.Config{Region: "us-east-1"}} cases := map[common.ServiceType]bool{ - common.ServiceEC2: true, - common.ServiceCompute: true, - common.ServiceRedshift: true, - common.ServiceDataWarehouse: true, - common.ServiceRDS: false, - common.ServiceElastiCache: false, - common.ServiceMemoryDB: false, - common.ServiceOpenSearch: false, - common.ServiceSavingsPlansCompute: false, + common.ServiceEC2: true, + common.ServiceCompute: true, + common.ServiceRedshift: true, + common.ServiceDataWarehouse: true, + common.ServiceRDS: false, + common.ServiceElastiCache: false, + common.ServiceMemoryDB: false, + common.ServiceOpenSearch: false, + common.ServiceSavingsPlansCompute: false, + common.ServiceRelationalDB: false, + common.ServiceCache: false, + common.ServiceSearch: false, + common.ServiceSavingsPlansEC2Instance: false, + common.ServiceSavingsPlansSageMaker: false, + common.ServiceSavingsPlansDatabase: false, + common.ServiceSavingsPlansAll: false, } for service, want := range cases { t.Run(string(service), func(t *testing.T) { From 195661182455ecc472f3f1af3dae411ca33984c3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:32:07 +0100 Subject: [PATCH 16/17] test(aws): pin EC2 empty-region refusal and the reconcile ClockSkew value Add the empty-region row to the EC2 reconcile table and assert ClockSkew is 5 minutes, so neither can change without a test failing. Co-Authored-By: Claude Opus 5.5 --- providers/aws/internal/purchasecfg/reconcile_test.go | 1 + providers/aws/services/ec2/reconcile_test.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/providers/aws/internal/purchasecfg/reconcile_test.go b/providers/aws/internal/purchasecfg/reconcile_test.go index 3a8acdf0..449b7496 100644 --- a/providers/aws/internal/purchasecfg/reconcile_test.go +++ b/providers/aws/internal/purchasecfg/reconcile_test.go @@ -13,6 +13,7 @@ func TestReconcileWindow(t *testing.T) { t.Parallel() start := time.Date(2026, 11, 6, 12, 0, 0, 0, time.UTC) end := start.Add(time.Minute) + assert.Equal(t, 5*time.Minute, ClockSkew, "window padding is part of the reconcile contract") wantLo, wantHi := start.Add(-ClockSkew), end.Add(ClockSkew) tests := []struct { diff --git a/providers/aws/services/ec2/reconcile_test.go b/providers/aws/services/ec2/reconcile_test.go index e0bbb0c7..da01df3c 100644 --- a/providers/aws/services/ec2/reconcile_test.go +++ b/providers/aws/services/ec2/reconcile_test.go @@ -119,6 +119,8 @@ func TestReconcilePurchase(t *testing.T) { verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, {name: "region mismatch", req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Region = "eu-west-1" }), verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "empty region", req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Region = "" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, {name: "zero count", req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Count = 0 }), verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, {name: "unparsable payment option", req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.PaymentOption = "bogus" }), From 9b7ef42bda4285bc00fb625559e13c64b4ad5b71 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:44:52 +0100 Subject: [PATCH 17/17] fix(aws): reject empty node type and bad payment option in Redshift reconcile Validate both in reconcileParams so they return invalid-request before any Describe call instead of offering-unresolved. The supported payment options now come from one table shared with matchesPaymentOption (#298). Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/redshift/client.go | 26 ++++++++++++------- providers/aws/services/redshift/reconcile.go | 3 ++- .../aws/services/redshift/reconcile_test.go | 6 +++++ 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index 556c81a1..944fac26 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -403,16 +403,22 @@ func matchesPaymentOption(offering redshifttypes.ReservedNodeOffering, paymentOp 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 - } + match, ok := paymentShapes[paymentOption] + return ok && match(hasUpfront, hasRecurring) +} + +// paymentShapes maps each supported payment option to its upfront/recurring price shape. +// Its key set is the supported-option list (see supportedPaymentOption). +var paymentShapes = map[string]func(hasUpfront, hasRecurring bool) bool{ + "all-upfront": func(u, r bool) bool { return u && !r }, + "no-upfront": func(u, r bool) bool { return !u && r }, + "partial-upfront": func(u, r bool) bool { return u && r }, +} + +// supportedPaymentOption reports whether matchesPaymentOption can ever match the option. +func supportedPaymentOption(paymentOption string) bool { + _, ok := paymentShapes[paymentOption] + return ok } // requiredMonthsForTerm converts a reservation term string to the offering diff --git a/providers/aws/services/redshift/reconcile.go b/providers/aws/services/redshift/reconcile.go index bdd87d4f..fca6c1fd 100644 --- a/providers/aws/services/redshift/reconcile.go +++ b/providers/aws/services/redshift/reconcile.go @@ -23,7 +23,8 @@ func (c *Client) reconcileParams(req common.ReconcileRequest) (count int32, mont rec := req.Recommendation count, countErr := offeringprice.PurchaseCount(rec.Count) months, termErr := requiredMonthsForTerm(rec.Term) - if rec.Region != c.region || countErr != nil || termErr != nil { + if rec.Region != c.region || countErr != nil || termErr != nil || + rec.ResourceType == "" || !supportedPaymentOption(rec.PaymentOption) { return 0, 0, lo, hi, purchasecfg.ReasonInvalidRequest } return count, months, lo, hi, "" diff --git a/providers/aws/services/redshift/reconcile_test.go b/providers/aws/services/redshift/reconcile_test.go index 07e6a99a..1a929224 100644 --- a/providers/aws/services/redshift/reconcile_test.go +++ b/providers/aws/services/redshift/reconcile_test.go @@ -131,6 +131,12 @@ func TestReconcilePurchase(t *testing.T) { verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, {name: "empty region", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Region = "" }), verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "empty node type", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.ResourceType = "" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "unsupported payment option", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.PaymentOption = "bogus" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, + {name: "empty payment option", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.PaymentOption = "" }), + verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, {name: "zero count", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Count = 0 }), verdict: common.ReconcileUnknown, reason: purchasecfg.ReasonInvalidRequest}, {name: "bad term", fake: one(reconcileNode("rn-1")), req: reconcileReq(func(r *common.ReconcileRequest) { r.Recommendation.Term = "5yr" }),