From 38f4bc4e3b869803e295f4421efadd443fb0209b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:03:19 +0200 Subject: [PATCH 1/2] fix(purchases): mask idempotency token in EC2 RI re-drive log (closes #656) Wrap opts.IdempotencyToken with common.MaskToken in the EC2 RI idempotency-guard skip log, matching the treatment applied to RDS, ElastiCache, MemoryDB, OpenSearch, and Redshift in PR #652. Adds a test asserting the re-drive path emits the first-8-chars+"..." form. --- providers/aws/services/ec2/client.go | 2 +- providers/aws/services/ec2/client_test.go | 45 +++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 0d8bfa594..0e8166a77 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -134,7 +134,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } if found { - log.Printf("EC2 RI for idempotency token %s already exists (%s); skipping purchase (issue #636 re-drive)", opts.IdempotencyToken, existingID) + log.Printf("EC2 RI for idempotency token %s already exists (%s); skipping purchase (issue #636 re-drive)", common.MaskToken(opts.IdempotencyToken), existingID) result.Success = true result.CommitmentID = existingID return result, nil diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 44ecee07d..b57dcd0cf 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -820,3 +820,48 @@ func TestBuildEC2OfferingQuery_ValidPlatform(t *testing.T) { assert.Equal(t, types.RIProductDescription("Linux/UNIX"), q.productDesc) assert.Equal(t, types.Tenancy("default"), q.tenancy) } + +// TestPurchaseCommitment_IdempotencySkipLogMasked asserts that the re-drive +// skip-log line emits a masked token (first 8 chars + "..."), not the raw +// 64-char idempotency token (issue #656). +func TestPurchaseCommitment_IdempotencySkipLogMasked(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + t.Cleanup(func() { mockEC2.AssertExpectations(t) }) + client := &Client{client: mockEC2, region: "us-east-1"} + + token := common.DeriveIdempotencyToken("exec-idem-656", 0) + + // Simulate that an RI tagged with this token already exists (re-drive path). + mockEC2.On("DescribeReservedInstances", mock.Anything, mock.MatchedBy(func(in *ec2.DescribeReservedInstancesInput) bool { + for _, f := range in.Filters { + if aws.ToString(f.Name) == "tag:"+common.IdempotencyTagKey { + return len(f.Values) == 1 && f.Values[0] == token + } + } + return false + })).Return(&ec2.DescribeReservedInstancesOutput{ + ReservedInstances: []types.ReservedInstances{ + {ReservedInstancesId: aws.String("ri-existing-656")}, + }, + }, nil).Once() + + rec := common.Recommendation{ + ResourceType: "t3.micro", + Count: 1, + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}, + } + + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) + + assert.NoError(t, err) + assert.True(t, result.Success) + assert.Equal(t, "ri-existing-656", result.CommitmentID) + + // Assert the masked form matches what MaskToken produces (first 8 chars + "..."). + masked := common.MaskToken(token) + assert.Equal(t, token[:8]+"...", masked, "MaskToken shape: first 8 chars + ellipsis") + assert.NotEqual(t, token, masked, "masked token must not equal raw token") +} From 69dc2bef09d0087e63645810273fe2d1141d3aba Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 16:34:50 +0200 Subject: [PATCH 2/2] test(ec2): assert raw idempotency token absent from emitted re-drive log (refs #656) Strengthen TestPurchaseCommitment_IdempotencySkipLogMasked into a real regression test: capture the standard logger output around the re-drive skip path, then assert that the raw 64-char token is absent and that the masked form (first 8 chars + "...") is present. Previously the test only verified MaskToken's output shape in isolation, so it stayed green even if line 137 of client.go reverted to logging the raw token. The new assertion fails exactly on that regression. --- providers/aws/services/ec2/client_test.go | 24 +++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index b57dcd0cf..14c3c893f 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -1,8 +1,11 @@ package ec2 import ( + "bytes" "context" "fmt" + "log" + "os" "testing" "time" @@ -824,14 +827,25 @@ func TestBuildEC2OfferingQuery_ValidPlatform(t *testing.T) { // TestPurchaseCommitment_IdempotencySkipLogMasked asserts that the re-drive // skip-log line emits a masked token (first 8 chars + "..."), not the raw // 64-char idempotency token (issue #656). +// +// This is a real ยง4 regression test: it captures the bytes written to the +// standard logger and asserts both that the raw token is absent AND that +// the masked form is present. Reverting line 137 of client.go to log +// opts.IdempotencyToken raw causes the NotContains assertion to fail. func TestPurchaseCommitment_IdempotencySkipLogMasked(t *testing.T) { - t.Parallel() + // Not parallel: we swap the global log writer and must restore it before + // any other test that also captures the logger races with us. mockEC2 := &MockEC2Client{} t.Cleanup(func() { mockEC2.AssertExpectations(t) }) client := &Client{client: mockEC2, region: "us-east-1"} token := common.DeriveIdempotencyToken("exec-idem-656", 0) + // Capture the standard logger so we can assert what is actually emitted. + var logBuf bytes.Buffer + log.SetOutput(&logBuf) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + // Simulate that an RI tagged with this token already exists (re-drive path). mockEC2.On("DescribeReservedInstances", mock.Anything, mock.MatchedBy(func(in *ec2.DescribeReservedInstancesInput) bool { for _, f := range in.Filters { @@ -860,8 +874,14 @@ func TestPurchaseCommitment_IdempotencySkipLogMasked(t *testing.T) { assert.True(t, result.Success) assert.Equal(t, "ri-existing-656", result.CommitmentID) - // Assert the masked form matches what MaskToken produces (first 8 chars + "..."). + // Core regression assertions: the re-drive log line must contain the masked + // form and must NOT contain the full raw token. + logOutput := logBuf.String() masked := common.MaskToken(token) + assert.Contains(t, logOutput, masked, "re-drive log must emit the masked token") + assert.NotContains(t, logOutput, token, "re-drive log must NOT emit the raw idempotency token") + + // Sanity-check that MaskToken itself has the expected shape (first 8 chars + "..."). assert.Equal(t, token[:8]+"...", masked, "MaskToken shape: first 8 chars + ellipsis") assert.NotEqual(t, token, masked, "masked token must not equal raw token") }