From aeff01c7a816e26642df4dea72dbc7dbe93f6f40 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 03:37:15 +0200 Subject: [PATCH 1/2] fix(ec2): filter RI exchange offering lookups by availability zone FindConvertibleOffering and ListTargetOfferings ignored the AZ, so a zonal convertible RI could be matched to, or exchanged into, an offering in another zone. Both now take an AvailabilityZone, send it as the typed request field for zonal scope, keep only offerings in that zone, and refuse a zonal lookup without an AZ or a regional one with an AZ before any API call. The zone rule is shared with the purchase path from #305. Refs #310 Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/ec2/client.go | 83 ++++++-- providers/aws/services/ec2/client_test.go | 244 +++++++++++++++++++++- 2 files changed, 298 insertions(+), 29 deletions(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 3a2ed67..608b558 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -3,6 +3,7 @@ package ec2 import ( "context" + "errors" "fmt" "log" "math" @@ -422,20 +423,9 @@ func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDet if err != nil { return ec2OfferingQuery{}, fmt.Errorf("EC2 recommendation for %s: %w", rec.ResourceType, err) } - az := strings.TrimSpace(details.AvailabilityZone) - if scope == types.ScopeAvailabilityZone && az == "" { - return ec2OfferingQuery{}, fmt.Errorf( - "EC2 recommendation for %s has zonal scope but no AvailabilityZone: "+ - "refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", - rec.ResourceType, - ) - } - if scope == types.ScopeRegional && az != "" { - return ec2OfferingQuery{}, fmt.Errorf( - "EC2 recommendation for %s has regional scope but AvailabilityZone %q: "+ - "contradictory input; re-fetch the recommendation", - rec.ResourceType, az, - ) + az, err := resolveExchangeZone(scope, details.AvailabilityZone) + if err != nil { + return ec2OfferingQuery{}, fmt.Errorf("EC2 recommendation for %s: %w", rec.ResourceType, err) } return ec2OfferingQuery{ instanceType: types.InstanceType(rec.ResourceType), @@ -447,6 +437,27 @@ func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDet }, nil } +// resolveExchangeZone returns the trimmed AZ to send for scope ("" for +// regional). A zonal lookup without an AZ and a regional lookup with one are +// both refused rather than guessed. +func resolveExchangeZone(scope types.Scope, az string) (string, error) { + az = strings.TrimSpace(az) + if scope == types.ScopeAvailabilityZone && az == "" { + return "", errors.New("zonal RI offering needs an availability zone; re-fetch the recommendation") + } + if scope == types.ScopeRegional && az != "" { + return "", fmt.Errorf("regional RI offering must not name an availability zone (contradictory input); "+ + "re-fetch the recommendation: got %q", az) + } + return az, nil +} + +// offeringInZone reports whether o is in az; az == "" (regional) matches any +// offering. A nil offering AZ never matches a named AZ. +func offeringInZone(o types.ReservedInstancesOffering, az string) bool { + return az == "" || aws.ToString(o.AvailabilityZone) == az +} + // describeInputFromQuery builds the SDK request struct for one page of the // typed lookup. Typed fields land on AWS's primary indices; only scope has no // typed equivalent and stays in Filters[]. @@ -587,7 +598,7 @@ func scanEC2OfferingPage(offerings []types.ReservedInstancesOffering, q ec2Offer wantType := q.wantOfferingType for i := range offerings { o := &offerings[i] - if q.availabilityZone != "" && aws.ToString(o.AvailabilityZone) != q.availabilityZone { + if !offeringInZone(*o, q.availabilityZone) { log.Printf("EC2 findOffering skipping offering %s in AZ %q (want %q)", aws.ToString(o.ReservedInstancesOfferingId), aws.ToString(o.AvailabilityZone), q.availabilityZone) continue @@ -822,7 +833,10 @@ type FindConvertibleOfferingParams struct { ProductDescription string Tenancy string Scope string - Duration int64 + // AvailabilityZone is the source RI's AZ name: required for zonal scope and + // refused for regional scope. + AvailabilityZone string + Duration int64 } // FindConvertibleOffering finds a convertible RI offering ID for the given parameters. @@ -835,6 +849,12 @@ func (c *Client) FindConvertibleOffering(ctx context.Context, params FindConvert if err != nil { return "", err } + // Keeping the target in the source RI's AZ is policy from #310/#289 (no + // silent AZ drift), not an AWS requirement of exchanges. + az, err := resolveExchangeZone(scope, params.AvailabilityZone) + if err != nil { + return "", err + } duration := params.Duration if duration == 0 { duration = OneYearSeconds @@ -858,17 +878,25 @@ func (c *Client) FindConvertibleOffering(ctx context.Context, params FindConvert IncludeMarketplace: aws.Bool(false), MaxResults: aws.Int32(20), } + if az != "" { + input.AvailabilityZone = aws.String(az) + } result, err := c.client.DescribeReservedInstancesOfferings(ctx, input) if err != nil { return "", fmt.Errorf("failed to describe convertible offerings: %w", err) } - if len(result.ReservedInstancesOfferings) == 0 { - return "", fmt.Errorf("no convertible offering found for %s (%s, %s, %s)", params.InstanceType, productDesc, tenancy, scope) + for i := range result.ReservedInstancesOfferings { + o := result.ReservedInstancesOfferings[i] + if offeringInZone(o, az) { + return aws.ToString(o.ReservedInstancesOfferingId), nil + } + log.Printf("FindConvertibleOffering skipping offering %s in AZ %q (want %q)", + aws.ToString(o.ReservedInstancesOfferingId), aws.ToString(o.AvailabilityZone), az) } - - return aws.ToString(result.ReservedInstancesOfferings[0].ReservedInstancesOfferingId), nil + return "", fmt.Errorf("no convertible offering found for %s (%s, %s, %s, AZ %q)", + params.InstanceType, productDesc, tenancy, scope, az) } // TargetOffering is one valid target offering for a convertible RI exchange. @@ -891,6 +919,7 @@ type ListTargetOfferingsParams struct { ProductDescription string Tenancy string Scope string + AvailabilityZone string Duration int64 OfferingType string } @@ -927,9 +956,12 @@ func normalizeTargetOfferingsParams(p ListTargetOfferingsParams) (productDesc st // appendTargetOfferings maps the SDK result page into TargetOffering values // and appends them to out. Extracted from ListTargetOfferings to keep it // under the gocyclo threshold. -func appendTargetOfferings(out []TargetOffering, offerings []types.ReservedInstancesOffering, scope string) []TargetOffering { +func appendTargetOfferings(out []TargetOffering, offerings []types.ReservedInstancesOffering, scope, az string) []TargetOffering { for i := range offerings { o := &offerings[i] + if !offeringInZone(*o, az) { + continue + } id := aws.ToString(o.ReservedInstancesOfferingId) if id == "" { continue @@ -975,6 +1007,10 @@ func (c *Client) ListTargetOfferings(ctx context.Context, params ListTargetOffer if err != nil { return nil, err } + az, err := resolveExchangeZone(scope, params.AvailabilityZone) + if err != nil { + return nil, err + } productDesc, duration, offeringType := normalizeTargetOfferingsParams(params) var out []TargetOffering @@ -995,11 +1031,14 @@ func (c *Client) ListTargetOfferings(ctx context.Context, params ListTargetOffer NextToken: nextToken, Filters: []types.Filter{{Name: aws.String("scope"), Values: []string{string(scope)}}}, } + if az != "" { + input.AvailabilityZone = aws.String(az) + } result, err := c.client.DescribeReservedInstancesOfferings(ctx, input) if err != nil { return nil, fmt.Errorf("describe target offerings: %w", err) } - out = appendTargetOfferings(out, result.ReservedInstancesOfferings, string(scope)) + out = appendTargetOfferings(out, result.ReservedInstancesOfferings, string(scope), az) if isLastEC2Page(result.NextToken) { break } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 94350e3..7a54b6c 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -1532,18 +1532,23 @@ var validTenancyScopeCases = []struct { name string tenancy string scope string + az string wantTenancy types.Tenancy wantScope string }{ - {"default_region", "default", "Region", types.TenancyDefault, "Region"}, - {"dedicated_az", "dedicated", "Availability Zone", types.TenancyDedicated, "Availability Zone"}, - {"legacy_shared_region", "shared", "region", types.TenancyDefault, "Region"}, - {"legacy_az_hyphenated", "dedicated", "availability-zone", types.TenancyDedicated, "Availability Zone"}, + {"default_region", "default", "Region", "", types.TenancyDefault, "Region"}, + {"dedicated_az", "dedicated", "Availability Zone", "us-east-1a", types.TenancyDedicated, "Availability Zone"}, + {"legacy_shared_region", "shared", "region", "", types.TenancyDefault, "Region"}, + {"legacy_az_hyphenated", "dedicated", "availability-zone", "us-east-1a", types.TenancyDedicated, "Availability Zone"}, } // captureOfferingsInput returns a mock that records the request it receives // and answers with a single offering. -func captureOfferingsInput(offeringID string, got **ec2.DescribeReservedInstancesOfferingsInput) *MockEC2Client { +func captureOfferingsInput(offeringID, az string, got **ec2.DescribeReservedInstancesOfferingsInput) *MockEC2Client { + var offeringAZ *string + if az != "" { + offeringAZ = aws.String(az) + } mockEC2 := &MockEC2Client{} mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). Run(func(args mock.Arguments) { @@ -1553,6 +1558,7 @@ func captureOfferingsInput(offeringID string, got **ec2.DescribeReservedInstance ReservedInstancesOfferings: []types.ReservedInstancesOffering{{ ReservedInstancesOfferingId: aws.String(offeringID), InstanceType: types.InstanceTypeM5Large, + AvailabilityZone: offeringAZ, }}, }, nil).Once() return mockEC2 @@ -1573,19 +1579,22 @@ func TestFindConvertibleOffering_TenancyScopeVariants_SendEnumValues(t *testing. t.Run(tc.name, func(t *testing.T) { t.Parallel() var got *ec2.DescribeReservedInstancesOfferingsInput - client := &Client{client: captureOfferingsInput("offering-"+tc.name, &got), region: "us-east-1"} + client := &Client{client: captureOfferingsInput("offering-"+tc.name, tc.az, &got), region: "us-east-1"} id, err := client.FindConvertibleOffering(context.Background(), FindConvertibleOfferingParams{ InstanceType: "m5.large", ProductDescription: "Linux/UNIX", Tenancy: tc.tenancy, Scope: tc.scope, + AvailabilityZone: tc.az, Duration: OneYearSeconds, }) require.NoError(t, err) assert.Equal(t, "offering-"+tc.name, id) require.NotNil(t, got) + assert.Equal(t, tc.az, aws.ToString(got.AvailabilityZone)) + assert.Nil(t, got.AvailabilityZoneId, "never send the zone id alongside the name") assert.Equal(t, tc.wantTenancy, got.InstanceTenancy) assert.Nil(t, filterValues(got.Filters, "instance-tenancy"), "instance-tenancy is not a documented filter") assert.Equal(t, []string{tc.wantScope}, filterValues(got.Filters, "scope")) @@ -1602,12 +1611,13 @@ func TestListTargetOfferings_TenancyScopeVariants_SendEnumValues(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() var got *ec2.DescribeReservedInstancesOfferingsInput - client := &Client{client: captureOfferingsInput("offering-"+tc.name, &got), region: "us-east-1"} + client := &Client{client: captureOfferingsInput("offering-"+tc.name, tc.az, &got), region: "us-east-1"} offerings, err := client.ListTargetOfferings(context.Background(), ListTargetOfferingsParams{ ProductDescription: "Linux/UNIX", Tenancy: tc.tenancy, Scope: tc.scope, + AvailabilityZone: tc.az, Duration: OneYearSeconds, }) @@ -1616,6 +1626,8 @@ func TestListTargetOfferings_TenancyScopeVariants_SendEnumValues(t *testing.T) { assert.Equal(t, "offering-"+tc.name, offerings[0].OfferingID) assert.Equal(t, tc.wantScope, offerings[0].Scope) require.NotNil(t, got) + assert.Equal(t, tc.az, aws.ToString(got.AvailabilityZone)) + assert.Nil(t, got.AvailabilityZoneId, "never send the zone id alongside the name") assert.Equal(t, tc.wantTenancy, got.InstanceTenancy) assert.Equal(t, []string{tc.wantScope}, filterValues(got.Filters, "scope")) assert.Equal(t, types.OfferingClassTypeConvertible, got.OfferingClass) @@ -1837,3 +1849,221 @@ func TestFindOffering_ZonalRecWithoutAZRefusedBeforeAPICall(t *testing.T) { require.Error(t, err) assert.Nil(t, cap.LastDescribeOfferingsInput) } + +func TestResolveExchangeZone(t *testing.T) { + t.Parallel() + tests := []struct { + name string + scope types.Scope + az string + want string + wantErr string + }{ + {"zonal with AZ", types.ScopeAvailabilityZone, "us-east-1b", "us-east-1b", ""}, + {"zonal trims whitespace", types.ScopeAvailabilityZone, " us-east-1b ", "us-east-1b", ""}, + {"zonal blank errors", types.ScopeAvailabilityZone, "", "", "zonal RI offering needs an availability zone"}, + {"zonal whitespace-only errors", types.ScopeAvailabilityZone, " ", "", "zonal RI offering needs an availability zone"}, + {"regional with AZ errors", types.ScopeRegional, "us-east-1b", "", "regional RI offering must not name an availability zone"}, + {"regional blank ok", types.ScopeRegional, "", "", ""}, + {"regional whitespace-only ok", types.ScopeRegional, " ", "", ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := resolveExchangeZone(tc.scope, tc.az) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + assert.Contains(t, err.Error(), "re-fetch") + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestOfferingInZone(t *testing.T) { + t.Parallel() + o1b := types.ReservedInstancesOffering{AvailabilityZone: aws.String("us-east-1b")} + assert.True(t, offeringInZone(o1b, "us-east-1b")) + assert.False(t, offeringInZone(o1b, "us-east-1a")) + assert.False(t, offeringInZone(types.ReservedInstancesOffering{}, "us-east-1b"), "nil AZ is a mismatch") + assert.True(t, offeringInZone(types.ReservedInstancesOffering{}, ""), "regional matches any offering") + assert.True(t, offeringInZone(o1b, "")) +} + +func zonedOffering(id, az string) types.ReservedInstancesOffering { + o := types.ReservedInstancesOffering{ + ReservedInstancesOfferingId: aws.String(id), + InstanceType: types.InstanceTypeM5Large, + } + if az != "" { + o.AvailabilityZone = aws.String(az) + } + return o +} + +// pagedOfferingsMock records every request and serves one page per call. +func pagedOfferingsMock(got *[]*ec2.DescribeReservedInstancesOfferingsInput, pages ...[]types.ReservedInstancesOffering) *MockEC2Client { + m := &MockEC2Client{} + for i, p := range pages { + out := &ec2.DescribeReservedInstancesOfferingsOutput{ReservedInstancesOfferings: p} + if i < len(pages)-1 { + out.NextToken = aws.String("next") + } + m.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + *got = append(*got, args.Get(1).(*ec2.DescribeReservedInstancesOfferingsInput)) + }).Return(out, nil).Once() + } + return m +} + +func zonalFindParams(az string) FindConvertibleOfferingParams { + return FindConvertibleOfferingParams{ + InstanceType: "m5.large", ProductDescription: "Linux/UNIX", Tenancy: "default", + Scope: "Availability Zone", AvailabilityZone: az, Duration: OneYearSeconds, + } +} + +func zonalListParams(az string) ListTargetOfferingsParams { + return ListTargetOfferingsParams{ + ProductDescription: "Linux/UNIX", Tenancy: "default", + Scope: "Availability Zone", AvailabilityZone: az, Duration: OneYearSeconds, + } +} + +func TestFindConvertibleOffering_Zonal_SendsAZAndPicksMatchingOffering(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + m := pagedOfferingsMock(&got, []types.ReservedInstancesOffering{ + zonedOffering("off-1a", "us-east-1a"), zonedOffering("off-nil", ""), zonedOffering("off-1b", "us-east-1b"), + }) + client := &Client{client: m, region: "us-east-1"} + + id, err := client.FindConvertibleOffering(context.Background(), zonalFindParams(" us-east-1b ")) + + require.NoError(t, err) + assert.Equal(t, "off-1b", id) + require.Len(t, got, 1) + assert.Equal(t, "us-east-1b", aws.ToString(got[0].AvailabilityZone)) + assert.Nil(t, got[0].AvailabilityZoneId) +} + +func TestFindConvertibleOffering_Zonal_NoMatchingAZ_Errors(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + m := pagedOfferingsMock(&got, []types.ReservedInstancesOffering{ + zonedOffering("off-1a", "us-east-1a"), zonedOffering("off-nil", ""), + }) + client := &Client{client: m, region: "us-east-1"} + + id, err := client.FindConvertibleOffering(context.Background(), zonalFindParams("us-east-1b")) + + require.Error(t, err) + assert.Empty(t, id) + assert.Contains(t, err.Error(), "no convertible offering found") + assert.Contains(t, err.Error(), "us-east-1b") +} + +func TestFindConvertibleOffering_ZoneMisuse_ErrorsBeforeAPICall(t *testing.T) { + t.Parallel() + zonalNoAZ := zonalFindParams("") + regionalWithAZ := zonalFindParams("us-east-1b") + regionalWithAZ.Scope = "Region" + for name, tc := range map[string]struct { + params FindConvertibleOfferingParams + wantErr string + }{ + "zonal without AZ": {zonalNoAZ, "zonal RI offering needs an availability zone"}, + "regional with AZ": {regionalWithAZ, "regional RI offering must not name an availability zone"}, + "zonal whitespace AZ": {zonalFindParams(" "), "zonal RI offering needs an availability zone"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + m := matchingOfferingMock() + client := &Client{client: m, region: "us-east-1"} + _, err := client.FindConvertibleOffering(context.Background(), tc.params) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + m.AssertNotCalled(t, "DescribeReservedInstancesOfferings", mock.Anything, mock.Anything) + }) + } +} + +func TestFindConvertibleOffering_Regional_DoesNotSendAZ(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + m := pagedOfferingsMock(&got, []types.ReservedInstancesOffering{zonedOffering("off-reg", "")}) + client := &Client{client: m, region: "us-east-1"} + p := zonalFindParams("") + p.Scope = "Region" + + id, err := client.FindConvertibleOffering(context.Background(), p) + + require.NoError(t, err) + assert.Equal(t, "off-reg", id) + require.Len(t, got, 1) + assert.Nil(t, got[0].AvailabilityZone) +} + +func TestListTargetOfferings_Zonal_SendsAZAndFiltersOfferings(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + m := pagedOfferingsMock(&got, + []types.ReservedInstancesOffering{zonedOffering("off-1a", "us-east-1a"), zonedOffering("off-nil", "")}, + []types.ReservedInstancesOffering{zonedOffering("off-1b", "us-east-1b")}, + ) + client := &Client{client: m, region: "us-east-1"} + + offerings, err := client.ListTargetOfferings(context.Background(), zonalListParams("us-east-1b")) + + require.NoError(t, err) + require.Len(t, offerings, 1) + assert.Equal(t, "off-1b", offerings[0].OfferingID) + require.Len(t, got, 2, "both pages walked") + for _, in := range got { + assert.Equal(t, "us-east-1b", aws.ToString(in.AvailabilityZone)) + assert.Nil(t, in.AvailabilityZoneId) + } +} + +func TestListTargetOfferings_ZoneMisuse_ErrorsBeforeAPICall(t *testing.T) { + t.Parallel() + regionalWithAZ := zonalListParams("us-east-1b") + regionalWithAZ.Scope = "Region" + for name, tc := range map[string]struct { + params ListTargetOfferingsParams + wantErr string + }{ + "zonal without AZ": {zonalListParams(""), "zonal RI offering needs an availability zone"}, + "regional with AZ": {regionalWithAZ, "regional RI offering must not name an availability zone"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + m := matchingOfferingMock() + client := &Client{client: m, region: "us-east-1"} + _, err := client.ListTargetOfferings(context.Background(), tc.params) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + m.AssertNotCalled(t, "DescribeReservedInstancesOfferings", mock.Anything, mock.Anything) + }) + } +} + +func TestListTargetOfferings_Regional_DoesNotSendAZ(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + m := pagedOfferingsMock(&got, []types.ReservedInstancesOffering{zonedOffering("off-reg", "")}) + client := &Client{client: m, region: "us-east-1"} + p := zonalListParams("") + p.Scope = "Region" + + offerings, err := client.ListTargetOfferings(context.Background(), p) + + require.NoError(t, err) + require.Len(t, offerings, 1) + require.Len(t, got, 1) + assert.Nil(t, got[0].AvailabilityZone) +} From 28bbcb853a801bd87aa45590dce69ccf4a4773f5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 16:26:26 +0200 Subject: [PATCH 2/2] fix(ec2): paginate convertible offering lookups before reporting no match FindConvertibleOffering read only the first DescribeReservedInstancesOfferings page, so a zonal match that lived on a later page was reported as missing. Follow NextToken up to maxTargetOfferingPages, applying the AZ filter on each page, and fail with a distinct pagination-cap error when the cap is hit. Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/ec2/client.go | 67 ++++++++++++++--------- providers/aws/services/ec2/client_test.go | 56 +++++++++++++++++++ 2 files changed, 98 insertions(+), 25 deletions(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 608b558..83b0cb4 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -855,24 +855,42 @@ func (c *Client) FindConvertibleOffering(ctx context.Context, params FindConvert if err != nil { return "", err } - duration := params.Duration - if duration == 0 { - duration = OneYearSeconds - } - productDesc := params.ProductDescription - if productDesc == "" { - productDesc = defaultEC2Platform - } - - filters := []types.Filter{ - {Name: aws.String("instance-type"), Values: []string{params.InstanceType}}, - {Name: aws.String("product-description"), Values: []string{productDesc}}, - {Name: aws.String("scope"), Values: []string{string(scope)}}, - {Name: aws.String("duration"), Values: []string{fmt.Sprintf("%d", duration)}}, + productDesc, duration, _ := normalizeTargetOfferingsParams(ListTargetOfferingsParams{ + ProductDescription: params.ProductDescription, + Duration: params.Duration, + }) + input := convertibleOfferingInput(params.InstanceType, productDesc, scope, tenancy, az, duration) + for page := 1; page <= maxTargetOfferingPages; page++ { + if err := ctx.Err(); err != nil { + return "", err + } + pageInput := *input + result, err := c.client.DescribeReservedInstancesOfferings(ctx, &pageInput) + if err != nil { + return "", fmt.Errorf("failed to describe convertible offerings: %w", err) + } + if id := firstOfferingInZone(result.ReservedInstancesOfferings, az); id != "" { + return id, nil + } + if isLastEC2Page(result.NextToken) { + return "", fmt.Errorf("no convertible offering found for %s (%s, %s, %s, AZ %q)", + params.InstanceType, productDesc, tenancy, scope, az) + } + input.NextToken = result.NextToken } + return "", fmt.Errorf("no convertible offering found for %s (%s, %s, %s, AZ %q) within %d pages: pagination cap reached", + params.InstanceType, productDesc, tenancy, scope, az, maxTargetOfferingPages) +} +// convertibleOfferingInput builds the first-page request for FindConvertibleOffering. +func convertibleOfferingInput(instanceType, productDesc string, scope types.Scope, tenancy types.Tenancy, az string, duration int64) *ec2.DescribeReservedInstancesOfferingsInput { input := &ec2.DescribeReservedInstancesOfferingsInput{ - Filters: filters, + Filters: []types.Filter{ + {Name: aws.String("instance-type"), Values: []string{instanceType}}, + {Name: aws.String("product-description"), Values: []string{productDesc}}, + {Name: aws.String("scope"), Values: []string{string(scope)}}, + {Name: aws.String("duration"), Values: []string{fmt.Sprintf("%d", duration)}}, + }, InstanceTenancy: tenancy, OfferingClass: types.OfferingClassTypeConvertible, IncludeMarketplace: aws.Bool(false), @@ -881,22 +899,21 @@ func (c *Client) FindConvertibleOffering(ctx context.Context, params FindConvert if az != "" { input.AvailabilityZone = aws.String(az) } + return input +} - result, err := c.client.DescribeReservedInstancesOfferings(ctx, input) - if err != nil { - return "", fmt.Errorf("failed to describe convertible offerings: %w", err) - } - - for i := range result.ReservedInstancesOfferings { - o := result.ReservedInstancesOfferings[i] +// firstOfferingInZone returns the ID of the first offering in az, or "" when +// none matches. +func firstOfferingInZone(offerings []types.ReservedInstancesOffering, az string) string { + for i := range offerings { + o := offerings[i] if offeringInZone(o, az) { - return aws.ToString(o.ReservedInstancesOfferingId), nil + return aws.ToString(o.ReservedInstancesOfferingId) } log.Printf("FindConvertibleOffering skipping offering %s in AZ %q (want %q)", aws.ToString(o.ReservedInstancesOfferingId), aws.ToString(o.AvailabilityZone), az) } - return "", fmt.Errorf("no convertible offering found for %s (%s, %s, %s, AZ %q)", - params.InstanceType, productDesc, tenancy, scope, az) + return "" } // TargetOffering is one valid target offering for a convertible RI exchange. diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 7a54b6c..d677092 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -1967,6 +1967,62 @@ func TestFindConvertibleOffering_Zonal_NoMatchingAZ_Errors(t *testing.T) { assert.Contains(t, err.Error(), "us-east-1b") } +func TestFindConvertibleOffering_Zonal_MatchOnSecondPage_FollowsNextToken(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + m := pagedOfferingsMock(&got, + []types.ReservedInstancesOffering{zonedOffering("off-1a", "us-east-1a")}, + []types.ReservedInstancesOffering{zonedOffering("off-1b", "us-east-1b")}, + ) + client := &Client{client: m, region: "us-east-1"} + + id, err := client.FindConvertibleOffering(context.Background(), zonalFindParams("us-east-1b")) + + require.NoError(t, err) + assert.Equal(t, "off-1b", id) + require.Len(t, got, 2) + assert.Nil(t, got[0].NextToken) + assert.Equal(t, "next", aws.ToString(got[1].NextToken)) + assert.Equal(t, "us-east-1b", aws.ToString(got[1].AvailabilityZone)) +} + +func TestFindConvertibleOffering_Zonal_NoMatchAcrossPages_Errors(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + m := pagedOfferingsMock(&got, + []types.ReservedInstancesOffering{zonedOffering("off-1a", "us-east-1a")}, + []types.ReservedInstancesOffering{zonedOffering("off-1c", "us-east-1c")}, + ) + client := &Client{client: m, region: "us-east-1"} + + id, err := client.FindConvertibleOffering(context.Background(), zonalFindParams("us-east-1b")) + + require.Error(t, err) + assert.Empty(t, id) + assert.Contains(t, err.Error(), "no convertible offering found") + assert.NotContains(t, err.Error(), "pagination cap") + assert.Len(t, got, 2) +} + +func TestFindConvertibleOffering_Zonal_PageCapReached_Errors(t *testing.T) { + t.Parallel() + var got []*ec2.DescribeReservedInstancesOfferingsInput + pages := make([][]types.ReservedInstancesOffering, maxTargetOfferingPages+1) + for i := range pages { + pages[i] = []types.ReservedInstancesOffering{zonedOffering("off-1a", "us-east-1a")} + } + m := pagedOfferingsMock(&got, pages...) + client := &Client{client: m, region: "us-east-1"} + + id, err := client.FindConvertibleOffering(context.Background(), zonalFindParams("us-east-1b")) + + require.Error(t, err) + assert.Empty(t, id) + assert.Contains(t, err.Error(), "pagination cap reached") + assert.Contains(t, err.Error(), fmt.Sprintf("%d pages", maxTargetOfferingPages)) + assert.Len(t, got, maxTargetOfferingPages) +} + func TestFindConvertibleOffering_ZoneMisuse_ErrorsBeforeAPICall(t *testing.T) { t.Parallel() zonalNoAZ := zonalFindParams("")