From d04d1c67344750d06a209c71d906a8e7e80a0d49 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 04:53:45 +0200 Subject: [PATCH 1/4] fix(recfilter): format invalid-input drop target like sibling messages Refs #227 Co-Authored-By: Claude Opus 5.5 --- pkg/recfilter/sizing.go | 2 +- pkg/recfilter/sizing_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/recfilter/sizing.go b/pkg/recfilter/sizing.go index dcbb18bb..ba47f80f 100644 --- a/pkg/recfilter/sizing.go +++ b/pkg/recfilter/sizing.go @@ -373,7 +373,7 @@ func targetCoverageInputsValid(rec common.Recommendation, targetPct float64) boo } func dropInvalidTargetCoverageInput(rec common.Recommendation, targetPct float64, logf Logf) string { - logf.printf("WARNING: --target-coverage=%v has invalid or out-of-range inputs for %s/%s/%s (avg=%v, existing=%v%%); dropped recommendation\n", + logf.printf("WARNING: --target-coverage=%.1f%% has invalid or out-of-range inputs for %s/%s/%s (avg=%v, existing=%v%%); dropped recommendation\n", targetPct, rec.Service, rec.Region, rec.ResourceType, rec.AverageInstancesUsedPerHour, rec.ExistingCoveragePct) return common.DropTargetInputInvalid } diff --git a/pkg/recfilter/sizing_test.go b/pkg/recfilter/sizing_test.go index b057d199..26ec55ce 100644 --- a/pkg/recfilter/sizing_test.go +++ b/pkg/recfilter/sizing_test.go @@ -2,6 +2,7 @@ package recfilter import ( "fmt" + "math" "math/big" "strings" "testing" @@ -451,3 +452,15 @@ func TestApplyTargetCoverage_UnknownCoverageNoAvg_AzureGCPPassThrough(t *testing assert.Equal(t, rec, out[0]) assert.True(t, d.IsEmpty()) } + +// The invalid-input drop message formats the target like the sibling drop +// messages (%.1f%%), not with %v (#227). +func TestApplyTargetCoverage_InvalidInputLogFormatsTarget(t *testing.T) { + t.Parallel() + rec := mkRI(10, math.NaN(), 0) + var logs []string + out := ApplyTargetCoverage([]common.Recommendation{rec}, 75, captureLogf(&logs), nil) + assert.Empty(t, out) + require.Len(t, logs, 1) + assert.Contains(t, logs[0], "--target-coverage=75.0% has invalid or out-of-range inputs") +} From 75fbfef1857fd0cd77680990d2da0100717152d2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 04:54:30 +0200 Subject: [PATCH 2/4] fix(aws): log the Organizations first-page fallback and test pagination Log at info when ListAccounts is denied or the account is not in an organization on the first page, so the single-account fallback is visible. Add page-two success, three-plus page and page-two NotInUse tests. Refs #227 Co-Authored-By: Claude Opus 5.5 --- providers/aws/provider.go | 1 + providers/aws/provider_test.go | 59 ++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/providers/aws/provider.go b/providers/aws/provider.go index 8c801a8f..0654d1f0 100644 --- a/providers/aws/provider.go +++ b/providers/aws/provider.go @@ -298,6 +298,7 @@ func (p *Provider) appendOrgAccounts(ctx context.Context, accounts []common.Acco output, err := paginator.NextPage(ctx) if err != nil { if page == 0 && isOrgListAccountsSilentError(err) { + logging.Infof("AWS Organizations ListAccounts unavailable, using the caller account only: %v", err) return accounts, nil } logging.Warnf("AWS Organizations ListAccounts pagination failed mid-run: %v", err) diff --git a/providers/aws/provider_test.go b/providers/aws/provider_test.go index 718f1584..a6480ec9 100644 --- a/providers/aws/provider_test.go +++ b/providers/aws/provider_test.go @@ -594,6 +594,65 @@ func TestAWSProvider_GetAccounts_WithMock(t *testing.T) { assert.Contains(t, err.Error(), "network blew up") }) + newOrgProvider := func(pg *mockOrganizationsPaginator) *Provider { + p := &Provider{cfg: aws.Config{Region: "us-east-1"}} + p.SetSTSClient(&mockSTSClient{ + getCallerIdentityFunc: func(ctx context.Context, params *sts.GetCallerIdentityInput, optFns ...func(*sts.Options)) (*sts.GetCallerIdentityOutput, error) { + return &sts.GetCallerIdentityOutput{Account: aws.String("111111111111")}, nil + }, + }) + p.SetOrganizationsPaginator(pg) + return p + } + orgPage := func(ids ...string) *organizations.ListAccountsOutput { + out := &organizations.ListAccountsOutput{} + for _, id := range ids { + out.Accounts = append(out.Accounts, orgtypes.Account{Id: aws.String(id), Name: aws.String("acct-" + id)}) + } + return out + } + + t.Run("accumulates accounts across two successful pages", func(t *testing.T) { + p := newOrgProvider(&mockOrganizationsPaginator{ + pages: []*organizations.ListAccountsOutput{orgPage("222222222222"), orgPage("333333333333")}, + errOnPage: -1, + }) + + accounts, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, accounts, 3) + assert.Equal(t, "333333333333", accounts[2].ID) + }) + + t.Run("accumulates accounts across three or more pages and skips the caller account", func(t *testing.T) { + p := newOrgProvider(&mockOrganizationsPaginator{ + pages: []*organizations.ListAccountsOutput{ + orgPage("222222222222"), orgPage("111111111111", "333333333333"), orgPage("444444444444"), orgPage("555555555555"), + }, + errOnPage: -1, + }) + + accounts, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + ids := make([]string, 0, len(accounts)) + for _, a := range accounts { + ids = append(ids, a.ID) + } + assert.Equal(t, []string{"111111111111", "222222222222", "333333333333", "444444444444", "555555555555"}, ids) + }) + + t.Run("propagates NotInUse on page two instead of falling back", func(t *testing.T) { + p := newOrgProvider(&mockOrganizationsPaginator{ + pages: []*organizations.ListAccountsOutput{orgPage("222222222222"), {}}, + nextErr: &smithy.GenericAPIError{Code: "AWSOrganizationsNotInUseException", Message: "not in org"}, + errOnPage: 1, + }) + + _, err := p.GetAccounts(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "AWSOrganizationsNotInUseException") + }) + t.Run("returns error when STS fails", func(t *testing.T) { p := &Provider{ cfg: aws.Config{Region: "us-east-1"}, From b3276a29e48c7c42099047bc33fc0294cf63e72b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 04:54:53 +0200 Subject: [PATCH 3/4] test(httpclient): pin that neighbors of single-address metadata entries stay allowed Refs #227 Co-Authored-By: Claude Opus 5.5 --- pkg/httpclient/httpclient_test.go | 32 +++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/pkg/httpclient/httpclient_test.go b/pkg/httpclient/httpclient_test.go index 1620458f..93b14fc8 100644 --- a/pkg/httpclient/httpclient_test.go +++ b/pkg/httpclient/httpclient_test.go @@ -59,3 +59,35 @@ func TestNew_AllowsRegularEndpoints(t *testing.T) { t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusOK) } } + +// The single-address metadata entries (/128 and /32) must not widen: the +// neighbors of each stay allowed. IPv4 link-local is deliberately a whole /16, +// so 169.254.169.253 is blocked, not allowed (#227). +func TestBlockMetadata_NeighborsOfSingleAddressEntries(t *testing.T) { + blocked := []string{ + "169.254.169.253:80", // inside the link-local /16 + "169.254.169.255:80", + } + for _, addr := range blocked { + t.Run("blocked "+addr, func(t *testing.T) { + if err := blockMetadata("tcp", addr, nil); err == nil { + t.Fatalf("%s must be blocked", addr) + } + }) + } + + allowed := []string{ + "[fd00:ec2::253]:80", "[fd00:ec2::255]:80", // AWS IPv6 IMDS /128 + "[fd00:ec2::22]:80", "[fd00:ec2::24]:80", // AWS Pod Identity /128 + "[fd20:ce::253]:80", "[fd20:ce::255]:80", "[fd20:ce:0:1::254]:80", // GCP IPv6 /128 + "168.63.129.15:80", "168.63.129.17:80", // Azure WireServer /32 + "100.100.100.199:80", "100.100.100.201:80", // Alibaba IMDS /32 + } + for _, addr := range allowed { + t.Run("allowed "+addr, func(t *testing.T) { + if err := blockMetadata("tcp", addr, nil); err != nil { + t.Fatalf("%s must stay allowed: %v", addr, err) + } + }) + } +} From 9344fe78eae237f3c04b92dcf23632f672862af1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 04:57:48 +0200 Subject: [PATCH 4/4] test(aws): assert the single-account fallback is logged Refs #227 Co-Authored-By: Claude Opus 5.5 --- providers/aws/provider_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/providers/aws/provider_test.go b/providers/aws/provider_test.go index a6480ec9..5b6381c8 100644 --- a/providers/aws/provider_test.go +++ b/providers/aws/provider_test.go @@ -1,6 +1,7 @@ package aws import ( + "bytes" "context" "errors" "testing" @@ -17,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" ) @@ -495,10 +497,15 @@ func TestAWSProvider_GetAccounts_WithMock(t *testing.T) { errOnPage: 0, }) + var logs bytes.Buffer + prev := logging.SetOutput(&logs) + t.Cleanup(func() { logging.SetOutput(prev) }) + accounts, err := p.GetAccounts(context.Background()) require.NoError(t, err) require.Len(t, accounts, 1) assert.Equal(t, "123456789012", accounts[0].ID) + assert.Contains(t, logs.String(), "AWS Organizations ListAccounts unavailable") }) t.Run("returns current account when Organizations access is denied", func(t *testing.T) { @@ -516,10 +523,15 @@ func TestAWSProvider_GetAccounts_WithMock(t *testing.T) { errOnPage: 0, }) + var logs bytes.Buffer + prev := logging.SetOutput(&logs) + t.Cleanup(func() { logging.SetOutput(prev) }) + accounts, err := p.GetAccounts(context.Background()) require.NoError(t, err) require.Len(t, accounts, 1) assert.Equal(t, "123456789012", accounts[0].ID) + assert.Contains(t, logs.String(), "AWS Organizations ListAccounts unavailable") }) t.Run("propagates AccessDenied after a page succeeded instead of returning a partial org list", func(t *testing.T) {