fix: recfilter, aws and httpclient nits from the #220, #221 and #234 reviews

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)
+			}
+		})
+	}
+}

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
 }

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")
+}

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)

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) {
@@ -594,6 +606,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"},