fix(purchase,commitmentopts): close out the open #243 review findings

internal/api/handler_config.go

@@ -4,12 +4,14 @@ package api
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
 	"strings"
 
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/logging"
 	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/commitmentopts"
 	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
 	"github.com/aws/aws-lambda-go/events"
 )
@@ -158,18 +160,25 @@ func (h *Handler) requireGlobalConfigScope(ctx context.Context, session *Session
 // combination we've dynamically confirmed the cloud doesn't sell. Returns
 // nil when: no probe service is wired, the service hasn't persisted data
 // yet (absent data → fall through to the frontend's hardcoded rules),
-// the save isn't AWS, or the combo is valid. Errors from Validate are
-// logged and swallowed (permissive) so a transient DB blip never blocks
-// a settings save.
+// the save isn't AWS, or the combo is valid.
+//
+// Any other Validate error (a store read failing, a timeout) fails the save
+// with a 503 instead of letting it through: the check exists for API
+// callers that bypass the frontend's rules, so waving the save through on an
+// error would persist an unsupported combo that fails only at purchase time
+// (issue #243, audit finding A02-022). The caller can retry.
 func (h *Handler) checkCommitmentOptionCombo(ctx context.Context, cfg config.ServiceConfig) error {
 	if h.commitmentOpts == nil || cfg.Provider != "aws" || cfg.Term <= 0 || cfg.Payment == "" {
 		return nil
 	}
 	ok, err := h.commitmentOpts.Validate(ctx, cfg.Provider, cfg.Service, cfg.Term, cfg.Payment)
-	if err != nil {
-		logging.Warnf("commitment-option validation error (allowing save): %v", err)
+	if errors.Is(err, commitmentopts.ErrNoData) {
 		return nil
 	}
+	if err != nil {
+		logging.Errorf("commitment-option validation error (refusing save): %v", err)
+		return NewClientError(503, "could not validate the commitment term and payment option; try again")
+	}
 	if !ok {
 		return NewClientError(400, fmt.Sprintf(
 			"%s does not support %dyr %s commitments",

internal/api/handler_config_test.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 	"testing"
@@ -827,6 +828,43 @@ func TestHandler_updateServiceConfig_CommitmentOptsAccept(t *testing.T) {
 	assert.Equal(t, "updated", result.Status)
 }
 
+// A Validate error other than ErrNoData must fail the save closed with a 503
+// rather than persist a combo nobody checked (issue #243, A02-022).
+func TestHandler_updateServiceConfig_CommitmentOptsErrorRefusesSave(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{
+		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
+		Email:  "admin@example.com",
+	}, nil)
+	mockAuth.grantAdmin()
+	mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(nil, nil)
+
+	handler := &Handler{
+		config: mockStore,
+		auth:   mockAuth,
+		commitmentOpts: &stubCommitmentOpts{
+			validateFn: func(context.Context, string, string, int, string) (bool, error) {
+				return false, errors.New("read commitmentopts store: connection refused")
+			},
+		},
+	}
+
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+		Body:    `{"enabled": true, "term": 3, "payment": "no-upfront", "coverage": 80}`,
+	}
+	result, err := handler.updateServiceConfig(ctx, req, "aws/rds")
+
+	require.Error(t, err)
+	assert.Nil(t, result)
+	ce, ok := IsClientError(err)
+	require.True(t, ok)
+	assert.Equal(t, 503, ce.code)
+	mockStore.AssertNotCalled(t, "SaveServiceConfig", mock.Anything, mock.Anything)
+}
+
 func TestHandler_updateServiceConfig_NoSlash(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)

internal/commitmentopts/probe.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/logging"
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/ec2"
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
@@ -81,20 +82,46 @@ func walkPaginated(
 	service string,
 	fetchPage func(ctx context.Context, token *string) ([]rawOffer, *string, error),
 ) ([]rawOffer, error) {
-	var raw []rawOffer
+	raw, _, err := walkPaginatedCapped(ctx, service, fetchPage)
+	return raw, err
+}
+
+// walkPaginatedCapped is walkPaginated that also reports whether the page cap
+// stopped the walk while AWS still had pages to return (capped=true), as
+// opposed to pagination running out.
+func walkPaginatedCapped(
+	ctx context.Context,
+	service string,
+	fetchPage func(ctx context.Context, token *string) ([]rawOffer, *string, error),
+) (raw []rawOffer, capped bool, err error) {
 	var token *string
 	for page := 0; page < maxPages; page++ {
-		offers, next, err := fetchPage(ctx, token)
-		if err != nil {
-			return nil, fmt.Errorf("%s: %w", service, err)
+		offers, next, fetchErr := fetchPage(ctx, token)
+		if fetchErr != nil {
+			return nil, false, fmt.Errorf("%s: %w", service, fetchErr)
 		}
 		raw = append(raw, offers...)
 		if next == nil || aws.ToString(next) == "" {
-			break
+			return raw, false, nil
 		}
 		token = next
 	}
-	return raw, nil
+	return raw, true, nil
+}
+
+// collectUnlessCapped is collect for the probers that filter the probe target
+// client-side (OpenSearch, Redshift): their APIs take no instance-type filter,
+// so the walk pages through every type and the page cap can cut off part of
+// the target's (term, payment) matrix. A capped walk yields no combos for the
+// service, which Service.Validate treats like an unprobed service
+// (permissive, with a warning), instead of a partial list that would reject
+// combos AWS does sell (issue #243).
+func collectUnlessCapped(service string, raw []rawOffer, capped bool) []Combo {
+	if capped {
+		logging.Warnf("commitmentopts: %s probe hit the %d-page cap before pagination ended; recording no combos for it so validation stays permissive", service, maxPages)
+		return nil
+	}
+	return collect(service, raw)
 }
 
 // collect dedupes a probe's raw (durationSeconds, rawPayment) pairs, runs
@@ -270,7 +297,7 @@ func (p *OpenSearchProber) Service() string { return "opensearch" }
 // Probe returns the combos for t3.small.search.
 func (p *OpenSearchProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) {
 	client := p.client(cfg)
-	raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) {
+	raw, capped, err := walkPaginatedCapped(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) {
 		out, err := client.DescribeReservedInstanceOfferings(ctx, &opensearch.DescribeReservedInstanceOfferingsInput{
 			MaxResults: pageSize,
 			NextToken:  token,
@@ -293,7 +320,7 @@ func (p *OpenSearchProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo,
 	if err != nil {
 		return nil, err
 	}
-	return collect(p.Service(), raw), nil
+	return collectUnlessCapped(p.Service(), raw, capped), nil
 }
 
 func (p *OpenSearchProber) client(cfg aws.Config) OpenSearchDescribeOfferings {
@@ -325,7 +352,7 @@ func (p *RedshiftProber) Service() string { return "redshift" }
 // Probe returns the combos for dc2.large.
 func (p *RedshiftProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) {
 	client := p.client(cfg)
-	raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) {
+	raw, capped, err := walkPaginatedCapped(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) {
 		out, err := client.DescribeReservedNodeOfferings(ctx, &redshift.DescribeReservedNodeOfferingsInput{
 			MaxRecords: aws.Int32(pageSize),
 			Marker:     token,
@@ -348,7 +375,7 @@ func (p *RedshiftProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, er
 	if err != nil {
 		return nil, err
 	}
-	return collect(p.Service(), raw), nil
+	return collectUnlessCapped(p.Service(), raw, capped), nil
 }
 
 func (p *RedshiftProber) client(cfg aws.Config) RedshiftDescribeOfferings {

internal/commitmentopts/probe_azure.go

@@ -163,9 +163,10 @@ func azureProbeCurrency(code string) (string, error) {
 // probeCombo calls ValidatePurchase for a single (term, billingPlan) pair and
 // returns true if the API considers it a valid offering.
 //
-// A response where any benefit has Valid=false is treated as "not offered"
-// (returns false, nil). Any other API error is returned as-is so the caller
-// can decide whether it is a transient failure or a configuration problem.
+// A response where any benefit has Valid=false, or where no benefit carries
+// Valid=true, is treated as "not offered" (returns false, nil). Any other API
+// error is returned as-is so the caller can decide whether it is a transient
+// failure or a configuration problem.
 func (p *AzureSPProber) probeCombo(
 	ctx context.Context,
 	client AzureSPValidateAPI,
@@ -210,15 +211,22 @@ func (p *AzureSPProber) probeCombo(
 		return false, err
 	}
 
-	// The API returns a per-benefit valid flag. If all reported benefits are
-	// valid (or the slice is empty, which can't happen for a single-item
-	// request in practice), the combo is available.
+	// The API returns a per-benefit valid flag. The combo counts as sold only
+	// on positive confirmation: at least one benefit with Valid=true and none
+	// with Valid=false. An empty Benefits slice or a nil flag says nothing, so
+	// it must not record a combo (such as a P5Y plan) as purchasable
+	// (issue #243, audit finding A05-012).
+	confirmed := false
 	for _, b := range resp.Benefits {
-		if b != nil && b.Valid != nil && !*b.Valid {
+		if b == nil || b.Valid == nil {
+			continue
+		}
+		if !*b.Valid {
 			return false, nil
 		}
+		confirmed = true
 	}
-	return true, nil
+	return confirmed, nil
 }
 
 func (p *AzureSPProber) client(cred azcore.TokenCredential) (AzureSPValidateAPI, error) {

internal/commitmentopts/probe_azure_test.go

@@ -136,19 +136,20 @@ func TestAzureSPProber_NoneValid(t *testing.T) {
 }
 
 func TestAzureSPProber_EmptyBenefitsSlice(t *testing.T) {
-	// An API response with an empty benefits slice counts as "valid"
-	// (no invalid flag returned) so the combo is included.
+	// An empty benefits slice confirms nothing, so no combo is recorded as
+	// sold: the probe requires positive confirmation (audit finding A05-012).
 	p := newFakeProber(func(_ armbillingbenefits.SavingsPlanPurchaseValidateRequest) (armbillingbenefits.RPClientValidatePurchaseResponse, error) {
 		return armbillingbenefits.RPClientValidatePurchaseResponse{}, nil
 	})
 
 	got, err := p.ProbeAzure(context.Background(), nil, "USD")
 	require.NoError(t, err)
-	assert.Len(t, got, 6, "empty benefits slice treated as valid")
+	assert.Empty(t, got, "an empty benefits slice must not record a combo as sold")
 }
 
 func TestAzureSPProber_NilValidFlag(t *testing.T) {
-	// A benefit with a nil Valid pointer is not invalid — treated as valid.
+	// A benefit with a nil Valid pointer confirms nothing, so the combo is
+	// not recorded as sold (audit finding A05-012).
 	p := newFakeProber(func(_ armbillingbenefits.SavingsPlanPurchaseValidateRequest) (armbillingbenefits.RPClientValidatePurchaseResponse, error) {
 		return armbillingbenefits.RPClientValidatePurchaseResponse{
 			SavingsPlanValidateResponse: armbillingbenefits.SavingsPlanValidateResponse{
@@ -161,7 +162,31 @@ func TestAzureSPProber_NilValidFlag(t *testing.T) {
 
 	got, err := p.ProbeAzure(context.Background(), nil, "USD")
 	require.NoError(t, err)
-	assert.Len(t, got, 6, "nil Valid flag treated as valid")
+	assert.Empty(t, got, "a nil Valid flag must not record a combo as sold")
+}
+
+func TestAzureSPProber_ConfirmationRule(t *testing.T) {
+	yes, no := true, false
+	cases := []struct {
+		name     string
+		benefits []*armbillingbenefits.SavingsPlanValidResponseProperty
+		want     int
+	}{
+		{"nil flag beside a confirmed benefit", []*armbillingbenefits.SavingsPlanValidResponseProperty{nil, {Valid: nil}, {Valid: &yes}}, 6},
+		{"any invalid benefit vetoes", []*armbillingbenefits.SavingsPlanValidResponseProperty{{Valid: &yes}, {Valid: &no}}, 0},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			p := newFakeProber(func(armbillingbenefits.SavingsPlanPurchaseValidateRequest) (armbillingbenefits.RPClientValidatePurchaseResponse, error) {
+				return armbillingbenefits.RPClientValidatePurchaseResponse{
+					SavingsPlanValidateResponse: armbillingbenefits.SavingsPlanValidateResponse{Benefits: tc.benefits},
+				}, nil
+			})
+			got, err := p.ProbeAzure(context.Background(), nil, "USD")
+			require.NoError(t, err)
+			assert.Len(t, got, tc.want)
+		})
+	}
 }
 
 // ---------------------------------------------------------------------------

internal/commitmentopts/probe_test.go

@@ -664,3 +664,59 @@ func TestDefaultProbers_IncludesSavingsPlans(t *testing.T) {
 	}
 	assert.True(t, found, "DefaultProbers must include SavingsPlansProber")
 }
+
+// ---------------------------------------------------------------------------
+// Page cap on the client-side-filtered probers (issue #243)
+// ---------------------------------------------------------------------------
+
+func TestWalkPaginatedCapped_ReportsWhyTheWalkStopped(t *testing.T) {
+	_, capped, err := walkPaginatedCapped(context.Background(), "test", func(context.Context, *string) ([]rawOffer, *string, error) {
+		return nil, aws.String("more"), nil
+	})
+	require.NoError(t, err)
+	assert.True(t, capped, "pages left when the cap fired")
+
+	_, capped, err = walkPaginatedCapped(context.Background(), "test", func(context.Context, *string) ([]rawOffer, *string, error) {
+		return nil, nil, nil
+	})
+	require.NoError(t, err)
+	assert.False(t, capped, "pagination ran out")
+}
+
+// OpenSearch and Redshift page through every instance type and filter for the
+// probe target client-side, so a walk the cap cut short holds only part of the
+// target's matrix. It must yield no combos (Validate then stays permissive for
+// the service) rather than a partial list that rejects combos AWS does sell.
+func TestFilteredProbers_PageCapYieldsNoCombos(t *testing.T) {
+	osFake := &fakeOpenSearch{fn: func(*opensearch.DescribeReservedInstanceOfferingsInput) (*opensearch.DescribeReservedInstanceOfferingsOutput, error) {
+		return &opensearch.DescribeReservedInstanceOfferingsOutput{
+			ReservedInstanceOfferings: []opensearchtypes.ReservedInstanceOffering{{
+				InstanceType:  opensearchtypes.OpenSearchPartitionInstanceType(probeTargetOpenSearch),
+				Duration:      31536000,
+				PaymentOption: opensearchtypes.ReservedInstancePaymentOptionNoUpfront,
+			}},
+			NextToken: aws.String("more"),
+		}, nil
+	}}
+	rsFake := &fakeRedshift{fn: func(*redshift.DescribeReservedNodeOfferingsInput) (*redshift.DescribeReservedNodeOfferingsOutput, error) {
+		return &redshift.DescribeReservedNodeOfferingsOutput{
+			ReservedNodeOfferings: []redshifttypes.ReservedNodeOffering{{
+				NodeType:     aws.String(probeTargetRedshift),
+				Duration:     aws.Int32(31536000),
+				OfferingType: aws.String("No Upfront"),
+			}},
+			Marker: aws.String("more"),
+		}, nil
+	}}
+	probers := []Prober{
+		&OpenSearchProber{NewClient: func(aws.Config) OpenSearchDescribeOfferings { return osFake }},
+		&RedshiftProber{NewClient: func(aws.Config) RedshiftDescribeOfferings { return rsFake }},
+	}
+	for _, p := range probers {
+		t.Run(p.Service(), func(t *testing.T) {
+			got, err := p.Probe(context.Background(), aws.Config{})
+			require.NoError(t, err)
+			assert.Empty(t, got, "a capped walk must not record a partial matrix")
+		})
+	}
+}

internal/commitmentopts/service.go

@@ -125,14 +125,22 @@ func (s *Service) probeAndPersist(ctx context.Context) (Options, error) {
 
 // findAWSAccount returns the first enabled AWS account, or nil if none
 // exist. Nil + nil error is the "no AWS connected" signal.
+//
+// The filter asks for enabled accounts only, as Scheduler.enabledAccounts
+// does: a disabled (for example decommissioned) account listed first would
+// otherwise be probed with revoked credentials, and the failed probe would
+// leave validation permissive while a healthy account sat later in the list
+// (issue #243). The loop re-checks both fields so an AccountLister that
+// ignores the filter cannot reintroduce that.
 func (s *Service) findAWSAccount(ctx context.Context) (*config.CloudAccount, error) {
 	provider := "aws"
-	accounts, err := s.accounts.ListCloudAccounts(ctx, config.CloudAccountFilter{Provider: &provider})
+	enabled := true
+	accounts, err := s.accounts.ListCloudAccounts(ctx, config.CloudAccountFilter{Provider: &provider, Enabled: &enabled})
 	if err != nil {
 		return nil, fmt.Errorf("list cloud accounts: %w", err)
 	}
 	for i := range accounts {
-		if accounts[i].Provider == "aws" {
+		if accounts[i].Provider == provider && accounts[i].Enabled {
 			return &accounts[i], nil
 		}
 	}