From 323412662b2a685873f474f014c494005a9accc8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 21:05:56 +0200 Subject: [PATCH] fix(aws/opensearch): validate RI count before idempotency short-circuit (follow-up to #831) PurchaseCommitment ran safeInt32Count after the idempotency guard, so a re-drive carrying an invalid instance count (negative / int32-overflow / zero) against an already-existing reservation would short-circuit to Success and never surface the bad input. Move the count validation to the boundary -- immediately after result init, before findOfferingID and the idempotency guard -- so invalid counts fail loud regardless of idempotency state (addresses the unresolved CodeRabbit Major thread on #831). Also harden the tag-failure structured-log test to assert the error= field carries the underlying cause, not just the OPENSEARCH_TAG_FAILED sentinel and commitment_id (unresolved CodeRabbit Minor thread on #831). Tests: - New TestClient_PurchaseCommitment_InvalidCount_FailsBeforeIdempotencyGuard: verified it FAILS on the pre-fix ordering (returns Success via short-circuit) and PASSES after the reorder; asserts the offering lookup and reservation describe are never reached. - TestClient_PurchaseCommitment_OfferingNotFound now sets a valid Count=1 (it previously relied on Count=0 being tolerated up to the offering lookup). --- providers/aws/services/opensearch/client.go | 16 ++++++--- .../aws/services/opensearch/client_test.go | 35 +++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index f4c9e8ff0..a5a08032e 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -150,6 +150,17 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati Timestamp: time.Now(), } + // Validate the instance count at the boundary, before any lookups or the + // idempotency short-circuit. Previously this ran after idempotencyGuard, so + // a re-drive carrying an invalid count (negative / int32-overflow) against + // an already-existing reservation would short-circuit to Success and never + // surface the bad input. Fail loud up front instead. + instanceCount, countErr := safeInt32Count(rec.Count) + if countErr != nil { + result.Error = countErr + return result, result.Error + } + offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID) if err != nil { result.Error = fmt.Errorf("failed to find offering: %w", err) @@ -188,11 +199,6 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } - instanceCount, countErr := safeInt32Count(rec.Count) - if countErr != nil { - result.Error = countErr - return result, result.Error - } input := &opensearch.PurchaseReservedInstanceOfferingInput{ ReservedInstanceOfferingId: aws.String(offeringID), ReservationName: aws.String(reservationName), diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index a51011f45..0028d9797 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -574,6 +574,7 @@ func TestClient_PurchaseCommitment_OfferingNotFound(t *testing.T) { rec := common.Recommendation{ Service: common.ServiceSearch, ResourceType: "m5.large.search", + Count: 1, // valid count: this test exercises the offering-lookup path PaymentOption: "partial-upfront", Term: "1yr", Details: common.SearchDetails{ @@ -791,6 +792,32 @@ func TestClient_PurchaseCommitment_Idempotent_GuardShortCircuits(t *testing.T) { mockOS.AssertNotCalled(t, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) } +// TestClient_PurchaseCommitment_InvalidCount_FailsBeforeIdempotencyGuard is the +// regression guard for the count-validation ordering fix (follow-up to #831). +// An invalid instance count must fail loud up front, even when an idempotency +// token would otherwise short-circuit against an existing reservation. Before +// the fix, safeInt32Count ran after idempotencyGuard, so this call returned +// Success and silently ignored the bad count. The mocks assert the offering +// lookup and reservation describe are never reached. +func TestClient_PurchaseCommitment_InvalidCount_FailsBeforeIdempotencyGuard(t *testing.T) { + mockOS := &MockOpenSearchClient{} + client := &Client{client: mockOS, region: "eu-west-1"} + token := common.DeriveIdempotencyToken("exec-badcount", 0) + + rec := osIdemRec() + rec.Count = 0 // invalid: safeInt32Count rejects n < 1 + + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) + assert.Error(t, err) + assert.False(t, result.Success, "invalid count must not report Success via the idempotency short-circuit") + if err != nil { + assert.Contains(t, err.Error(), "instance count 0 is out of valid range") + } + mockOS.AssertNotCalled(t, "DescribeReservedInstanceOfferings", mock.Anything, mock.Anything) + mockOS.AssertNotCalled(t, "DescribeReservedInstances", mock.Anything, mock.Anything) + mockOS.AssertNotCalled(t, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) +} + func TestClient_PurchaseCommitment_Idempotent_NotFoundProceeds(t *testing.T) { mockOS := &MockOpenSearchClient{} client := &Client{client: mockOS, region: "eu-west-1"} @@ -1070,6 +1097,14 @@ func TestPurchaseCommitment_TagFailure_StructuredLog(t *testing.T) { logOut := buf.String() assert.Contains(t, logOut, "OPENSEARCH_TAG_FAILED", "structured sentinel must appear in log") assert.Contains(t, logOut, "commitment_id="+riID, "commitment ID must be present for operator lookup") + // The error field must carry the underlying cause so operators can triage + // the tag failure without re-running; asserting only the sentinel would let + // a line missing the error text pass. The error may be wrapped (retry + // helper), so assert the field marker and the root cause independently + // rather than pinning the exact wrapper text. + assert.Contains(t, logOut, "error=", "structured line must include an error= field") + assert.Contains(t, logOut, "ValidationException: invalid resource type", + "structured line must surface the underlying tag-failure cause") // The account ID (000000000000) must not be logged -- it is not PII but // the structured line should stay minimal and scoped to the commitment. assert.NotContains(t, logOut, "000000000000", "account ID must not appear in the tag-failure log line")