From 39198c1f998baa9a221fbf8f338297e7dd511287 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 04:33:38 +0200 Subject: [PATCH 1/2] test(cli): route MaxInstances duplicate checks to a stub transport The MaxInstances tests mock only recommendation fetching. The duplicate check (checkDuplicates -> GetExistingCommitments) still built a real AWS client from aws.Config{Region: "us-east-1"} and sent HTTPS requests to rds/elasticache endpoints in three regions, so pass/fail and runtime depended on the public network (one full-suite run hit the 10 minute timeout in DescribeReservedCacheNodes). Give the tests an aws.Config with static credentials and an injected HTTP client that serves empty DescribeReservedDBInstances and DescribeReservedCacheNodes fixtures and fails the test on any other request. Each test also asserts the stub received exactly one duplicate-check request per service/region pair. Evidence: before, an offline logging proxy saw 12 CONNECT attempts to the six rds/elasticache hosts in one test; after, the tests pass with the proxy pointed at a dead port, and removing one fixture fails the test with "unexpected AWS request". Closes #2148 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_max_instances_test.go | 104 +++++++++++++++++++++++- 1 file changed, 100 insertions(+), 4 deletions(-) diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 8e52d4bd3..14c67a9af 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -1,8 +1,15 @@ package main import ( + "bytes" "context" + "fmt" + "io" + "net/http" + "net/url" "path/filepath" + "strings" + "sync" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -12,6 +19,91 @@ import ( "github.com/stretchr/testify/require" ) +// maxInstancesAWSStub is the offline AWS endpoint behind the duplicate check +// (checkDuplicates -> GetExistingCommitments). Every request must match a +// fixture keyed "host Action"; anything else fails the test, so a request that +// would have left the machine is reported instead of silently timing out. +type maxInstancesAWSStub struct { + t *testing.T + mu sync.Mutex + received map[string]int + log *bytes.Buffer +} + +// maxInstancesAWSFixtures answer the Query-protocol DescribeReserved* calls +// with an empty existing-commitment list. +var maxInstancesAWSFixtures = map[string]string{ + "rds.us-east-1.amazonaws.com DescribeReservedDBInstances": maxInstancesEmptyRDS, + "rds.us-west-2.amazonaws.com DescribeReservedDBInstances": maxInstancesEmptyRDS, + "rds.eu-west-1.amazonaws.com DescribeReservedDBInstances": maxInstancesEmptyRDS, + "elasticache.us-east-1.amazonaws.com DescribeReservedCacheNodes": maxInstancesEmptyElastiCache, + "elasticache.us-west-2.amazonaws.com DescribeReservedCacheNodes": maxInstancesEmptyElastiCache, + "elasticache.eu-west-1.amazonaws.com DescribeReservedCacheNodes": maxInstancesEmptyElastiCache, +} + +const ( + maxInstancesEmptyRDS = `` + + `` + + `fixture` + maxInstancesEmptyElastiCache = `` + + `` + + `fixture` +) + +func (s *maxInstancesAWSStub) RoundTrip(r *http.Request) (*http.Response, error) { + b, _ := io.ReadAll(r.Body) + form, _ := url.ParseQuery(string(b)) + key := r.URL.Host + " " + form.Get("Action") + s.mu.Lock() + s.received[key]++ + s.mu.Unlock() + body, ok := maxInstancesAWSFixtures[key] + status := http.StatusOK + if !ok { + s.t.Errorf("unexpected AWS request %s %s%s", r.Method, key, r.URL.Path) + status, body = http.StatusInternalServerError, "unexpected request" + } + return &http.Response{ + StatusCode: status, + Header: http.Header{"Content-Type": []string{"text/xml"}}, + Body: io.NopCloser(strings.NewReader(body)), + Request: r, + }, nil +} + +// newMaxInstancesAWSConfig returns an aws.Config whose HTTP client is the +// stub and whose credentials are static, so signing needs no environment. +func newMaxInstancesAWSConfig(t *testing.T) (aws.Config, *maxInstancesAWSStub) { + t.Helper() + stub := &maxInstancesAWSStub{t: t, received: map[string]int{}, log: captureAppLog(t)} + return aws.Config{ + Region: "us-east-1", + HTTPClient: &http.Client{Transport: stub}, + Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { + return aws.Credentials{AccessKeyID: "AKIDEXAMPLE", SecretAccessKey: "secret"}, nil + }), + Retryer: func() aws.Retryer { return aws.NopRetryer{} }, + }, stub +} + +// requireDuplicateChecks asserts the duplicate check really ran against the +// stub: one call per (service, region) pair, no other request, and every +// response parsed. +func (s *maxInstancesAWSStub) requireDuplicateChecks(t *testing.T, pairs int) { + t.Helper() + s.mu.Lock() + defer s.mu.Unlock() + total := 0 + for k, n := range s.received { + assert.Contains(t, maxInstancesAWSFixtures, k) + total += n + } + // A fixture the SDK cannot parse only logs this warning in dry-run, so the + // request count alone would not notice a broken fixture. + assert.NotContains(t, s.log.String(), "Could not check for existing RIs") + require.Equal(t, pairs, total, fmt.Sprintf("duplicate-check requests received: %v", s.received)) +} + // maxInstancesFixtureServices and maxInstancesFixtureRegions describe the // multi-service, multi-region fan-out the run-wide cap has to survive. A // single-service or single-region fixture stays green with the per-region bug @@ -77,7 +169,7 @@ func TestMaxInstancesCapsWholeRunAcrossServicesAndRegions(t *testing.T) { const maxInstances = 10 ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newMaxInstancesAWSConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -94,6 +186,7 @@ func TestMaxInstancesCapsWholeRunAcrossServicesAndRegions(t *testing.T) { accountCache := NewAccountAliasCache(awsCfg) allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + awsStub.requireDuplicateChecks(t, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) pairs := len(maxInstancesFixtureServices) * len(maxInstancesFixtureRegions) naturalTotal := pairs * countPerFixtureRec @@ -149,7 +242,7 @@ func TestMaxInstancesKeepsHighestSavingsNotFirstFetched(t *testing.T) { ) ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newMaxInstancesAWSConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -174,6 +267,7 @@ func TestMaxInstancesKeepsHighestSavingsNotFirstFetched(t *testing.T) { accountCache := NewAccountAliasCache(awsCfg) allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + awsStub.requireDuplicateChecks(t, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) @@ -223,7 +317,7 @@ func TestMaxInstancesNeverPurchasesBelowMinCount(t *testing.T) { ) ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newMaxInstancesAWSConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -243,6 +337,7 @@ func TestMaxInstancesNeverPurchasesBelowMinCount(t *testing.T) { accountCache := NewAccountAliasCache(awsCfg) allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + awsStub.requireDuplicateChecks(t, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) @@ -312,7 +407,7 @@ func TestDropTruncatedBelowMinCount(t *testing.T) { // run that never asked for one. func TestMaxInstancesNotAppliedWhenUnset(t *testing.T) { ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newMaxInstancesAWSConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -329,6 +424,7 @@ func TestMaxInstancesNotAppliedWhenUnset(t *testing.T) { accountCache := NewAccountAliasCache(awsCfg) allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + awsStub.requireDuplicateChecks(t, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) From 65d9feede1ce8ec50bc6145daed95e93795b147b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 14:03:10 +0200 Subject: [PATCH 2/2] test(cli): use anonymous creds and assert one duplicate check per pair Address two CodeRabbit findings on the max-instances AWS stub: - use aws.AnonymousCredentials so requests are unsigned and no credential-looking literals remain - assert each fixture key received exactly one duplicate-check request, so a double hit plus a miss can no longer sum to the expected total Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_max_instances_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 14c67a9af..51abe204e 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -72,17 +72,16 @@ func (s *maxInstancesAWSStub) RoundTrip(r *http.Request) (*http.Response, error) } // newMaxInstancesAWSConfig returns an aws.Config whose HTTP client is the -// stub and whose credentials are static, so signing needs no environment. +// stub and whose credentials are anonymous, so requests are unsigned and need +// no environment. func newMaxInstancesAWSConfig(t *testing.T) (aws.Config, *maxInstancesAWSStub) { t.Helper() stub := &maxInstancesAWSStub{t: t, received: map[string]int{}, log: captureAppLog(t)} return aws.Config{ - Region: "us-east-1", - HTTPClient: &http.Client{Transport: stub}, - Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { - return aws.Credentials{AccessKeyID: "AKIDEXAMPLE", SecretAccessKey: "secret"}, nil - }), - Retryer: func() aws.Retryer { return aws.NopRetryer{} }, + Region: "us-east-1", + HTTPClient: &http.Client{Transport: stub}, + Credentials: aws.AnonymousCredentials{}, + Retryer: func() aws.Retryer { return aws.NopRetryer{} }, }, stub } @@ -96,6 +95,7 @@ func (s *maxInstancesAWSStub) requireDuplicateChecks(t *testing.T, pairs int) { total := 0 for k, n := range s.received { assert.Contains(t, maxInstancesAWSFixtures, k) + assert.Equalf(t, 1, n, "duplicate-check requests for %s", k) total += n } // A fixture the SDK cannot parse only logs this warning in dry-run, so the