fix(ri-exchange): run unattended exchanges as manual while 4-eyes is on

internal/api/handler_ri_exchange_azure_test.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math"
 	"math/big"
@@ -2252,3 +2253,29 @@ func TestExecuteAzureExchange_FourEyesRefusesDirectExecute(t *testing.T) {
 	opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything)
 	opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything)
 }
+
+// TestExecuteAzureExchange_FourEyesPolicyLookupErrorFailsClosed (issue #679):
+// an unreadable global config must not be treated as "4-eyes off". The
+// handler returns a server error before any quote or exchange call.
+func TestExecuteAzureExchange_FourEyesPolicyLookupErrorFailsClosed(t *testing.T) {
+	ctx := context.Background()
+	opsClient := new(mockAzureExchangeOpsClient)
+	ownsAzureSource(opsClient)
+	t.Cleanup(func() { opsClient.AssertExpectations(t) })
+
+	h := newAzureExecuteSourceGateHandler(t, opsClient)
+	store, ok := h.config.(*MockConfigStore)
+	require.True(t, ok)
+	store.On("GetGlobalConfig", mock.Anything).Return(nil, errors.New("db down"))
+
+	_, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer tok"},
+		Body:    validAzureExecuteBody,
+	})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "4-eyes policy check")
+	_, isClient := IsClientError(err)
+	assert.False(t, isClient, "a lookup failure is a server error, not a client refusal")
+	opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything)
+	opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything)
+}

internal/api/handler_ri_exchange_test.go

@@ -2592,20 +2592,34 @@ func newFourEyesExecuteExchangeHandler(t *testing.T, store *MockConfigStore) *Ha
 	}
 }
 
+// spyOnAWSExchange installs an executeExchangeFn that counts calls and
+// reports success, so a 4-eyes test asserts that no exchange was submitted.
+// The real executor would fail on missing AWS credentials, so an error alone
+// would not prove the gate held (issue #679).
+func spyOnAWSExchange(h *Handler) *int {
+	calls := 0
+	h.executeExchangeFn = func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) {
+		calls++
+		return "exch-must-not-run", &exchange.ExchangeQuoteSummary{PaymentDueUSDStr: "0"}, nil
+	}
+	return &calls
+}
+
 // TestExecuteExchange_FourEyesRefusesDirectExecute pins issue #517: with
 // RequireDifferentApprover on, the direct AWS execute endpoint must refuse
-// before any exchange is attempted. The handler has no exchange client or
-// idempotency store wired, so any code path past the gate would panic.
+// before any exchange is attempted. A spy executor records any exchange call.
 func TestExecuteExchange_FourEyesRefusesDirectExecute(t *testing.T) {
 	ctx := context.Background()
 	store := &MockConfigStore{}
 	store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{RequireDifferentApprover: true}, nil)
 	h := newFourEyesExecuteExchangeHandler(t, store)
+	exchangeCalls := spyOnAWSExchange(h)
 
 	_, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{
 		Headers: map[string]string{"authorization": "Bearer exchange-token"},
 		Body:    fourEyesExchangeBody,
 	})
+	assert.Equal(t, 0, *exchangeCalls, "no exchange may be submitted while 4-eyes is on")
 	require.Error(t, err)
 	ce, ok := IsClientError(err)
 	require.True(t, ok, "expected a ClientError, got: %v", err)
@@ -2620,11 +2634,13 @@ func TestExecuteExchange_FourEyesPolicyLookupErrorFailsClosed(t *testing.T) {
 	store := &MockConfigStore{}
 	store.On("GetGlobalConfig", mock.Anything).Return(nil, errors.New("db down"))
 	h := newFourEyesExecuteExchangeHandler(t, store)
+	exchangeCalls := spyOnAWSExchange(h)
 
 	_, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{
 		Headers: map[string]string{"authorization": "Bearer exchange-token"},
 		Body:    fourEyesExchangeBody,
 	})
+	assert.Equal(t, 0, *exchangeCalls, "an unreadable 4-eyes policy must not let the exchange run")
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "4-eyes policy check")
 	_, isClient := IsClientError(err)

internal/api/openapi.yaml

@@ -1354,7 +1354,11 @@ paths:
       operationId: updateRIExchangeConfig
       tags: [RIExchange]
       summary: Update RI exchange configuration
-      description: Requires `update:config` permission.
+      description: >-
+        Requires `update:config` permission. While 4-eyes approval mode
+        (`require_different_approver`) is on, the scheduler runs `auto` mode
+        as `manual`: it only raises pending exchanges, which need approval.
+        The stored mode is not changed.
       parameters:
         - $ref: '#/components/parameters/CSRFToken'
       requestBody:

internal/config/types.go

@@ -114,6 +114,9 @@ type GlobalConfig struct {
 	// a standard SOX / SOC2 segregation-of-duties control. Default: false.
 	// Admins who created an execution and need to approve it must disable this
 	// mode first (the admin wildcard is NOT exempt from the restriction).
+	// While it is on, unattended RI exchange runs (RIExchangeMode "auto" and
+	// ladder reshapes) run as "manual" and only raise pending records
+	// (issue #679).
 	RequireDifferentApprover bool `json:"require_different_approver" dynamodbav:"require_different_approver"`
 }
 

internal/server/handler_ri_exchange.go

@@ -114,7 +114,7 @@ func (app *Application) executeRIExchangeReshape(ctx context.Context, cfg *confi
 		RIs:            riInfos,
 		Utilization:    utilInfos,
 		Config: exchange.RIExchangeConfig{
-			Mode:                     cfg.RIExchangeMode,
+			Mode:                     fourEyesExchangeMode(cfg, cfg.RIExchangeMode),
 			UtilizationThreshold:     cfg.RIExchangeUtilizationThreshold,
 			MaxPaymentPerExchangeUSD: cfg.RIExchangeMaxPerExchangeUSD,
 			MaxPaymentDailyUSD:       cfg.RIExchangeMaxDailyUSD,

internal/server/ladder_write.go

@@ -11,6 +11,7 @@ import (
 	pkgladder "github.com/LeanerCloud/cloud-commitments-go/pkg/ladder"
 	awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws"
 	awsladder "github.com/LeanerCloud/cloud-commitments-go/providers/aws/ladder"
+	"github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations"
 )
 
 // exchangeRunnerAdapter bridges internal/server wiring (exchange store, EC2 exchange
@@ -31,8 +32,7 @@ type exchangeRunnerAdapter struct {
 }
 
 // RunAutoExchange implements providers/aws/ladder.exchangeRunner. It constructs
-// fresh AWS clients, lists convertible RIs and utilization, converts them for the
-// exchange package, then delegates to exchange.RunAutoExchange.
+// fresh AWS clients and hands them to run.
 //
 // ladderRunID is forwarded to RunAutoExchangeParams.LadderRunID so exchange scopes
 // its pending-cancellation to the ladder origin (issue #1348 / gap G10). DryRun
@@ -46,12 +46,36 @@ func (a *exchangeRunnerAdapter) RunAutoExchange(ctx context.Context, cfg exchang
 
 	ec2Client := awsprovider.NewEC2ClientDirect(awsCfg)
 	recsClient := awsprovider.NewRecommendationsClientDirect(awsCfg)
+	clients := riExchangeClients{
+		listConvertibleRIs: ec2Client.ListConvertibleReservedInstances,
+		getRIUtilization: func(ctx context.Context, lookbackDays int) ([]recommendations.RIUtilization, error) {
+			return recsClient.GetRIUtilization(ctx, lookbackDays, a.region)
+		},
+		exchangeClient: exchange.NewExchangeClient(awsCfg),
+		lookupOffering: newConvertibleOfferingLookup(ec2Client),
+		accountID:      a.accountID,
+		region:         a.region,
+	}
+	return a.run(ctx, cfg, clients, ladderRunID, dryRun)
+}
+
+// run applies the 4-eyes policy to cfg.Mode, lists convertible RIs and
+// utilization, converts them for the exchange package, then delegates to
+// exchange.RunAutoExchange. The ladder always asks for auto mode; under 4-eyes
+// it runs as manual, so exchanges wait for approval (issue #679). A global
+// config that cannot be read fails closed: the run stops before any AWS call.
+func (a *exchangeRunnerAdapter) run(ctx context.Context, cfg exchange.RIExchangeConfig, clients riExchangeClients, ladderRunID *string, dryRun bool) (*exchange.AutoExchangeResult, error) {
+	globalCfg, err := a.app.Config.GetGlobalConfig(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("exchangeRunnerAdapter: 4-eyes policy check: load global config: %w", err)
+	}
+	cfg.Mode = fourEyesExchangeMode(globalCfg, cfg.Mode)
 
-	instances, err := ec2Client.ListConvertibleReservedInstances(ctx)
+	instances, err := clients.listConvertibleRIs(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("exchangeRunnerAdapter: list convertible RIs: %w", err)
 	}
-	utilData, err := recsClient.GetRIUtilization(ctx, cfg.LookbackDays, a.region)
+	utilData, err := clients.getRIUtilization(ctx, cfg.LookbackDays)
 	if err != nil {
 		return nil, fmt.Errorf("exchangeRunnerAdapter: get RI utilization: %w", err)
 	}
@@ -61,13 +85,13 @@ func (a *exchangeRunnerAdapter) RunAutoExchange(ctx context.Context, cfg exchang
 
 	return exchange.RunAutoExchange(ctx, exchange.RunAutoExchangeParams{
 		Store:          store,
-		ExchangeClient: exchange.NewExchangeClient(awsCfg),
-		LookupOffering: newConvertibleOfferingLookup(ec2Client),
+		ExchangeClient: clients.exchangeClient,
+		LookupOffering: clients.lookupOffering,
 		RIs:            riInfos,
 		Utilization:    utilInfos,
 		Config:         cfg,
-		AccountID:      a.accountID,
-		Region:         a.region,
+		AccountID:      clients.accountID,
+		Region:         clients.region,
 		DashboardURL:   a.app.appConfig.DashboardURL,
 		RIMetadata:     riMetadata,
 		LadderRunID:    ladderRunID,

internal/server/ri_exchange_four_eyes.go

@@ -0,0 +1,33 @@
+package server
+
+import (
+	"log"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/exchange"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+)
+
+// fourEyesExchangeMode returns the mode an unattended RI exchange run (the
+// scheduled reshape task or a ladder reshape) may use under the current
+// 4-eyes policy (issue #679).
+//
+// In any mode but "manual", pkg/exchange executes exchanges on the spot with
+// no human approval. That bypasses RequireDifferentApprover, which the direct
+// execute endpoints already refuse (#517). So with 4-eyes on, every mode but
+// "manual" is downgraded to "manual": the run only raises pending records,
+// and those go through the approve endpoint and its 4-eyes check. "auto", ""
+// and typos all get the downgrade, because pkg/exchange treats every
+// non-"manual" value as auto.
+//
+// A nil global config fails closed: it is treated as 4-eyes on, never as off.
+func fourEyesExchangeMode(cfg *config.GlobalConfig, mode string) string {
+	manual := string(exchange.ExchangeModeManual)
+	if mode == manual {
+		return mode
+	}
+	if cfg != nil && !cfg.RequireDifferentApprover {
+		return mode
+	}
+	log.Printf("RI exchange: 4-eyes approval mode is on; running mode %q as %q so every exchange waits for approval", mode, manual)
+	return manual
+}