From 5cb2cb0ee7637659b32ac2eebd8f28f040b482ff Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:34:51 +0000 Subject: [PATCH] refactor(api): move RI exchange and Marketplace provider wiring into one composition file handler_ri_exchange.go built its AWS and Azure clients by calling the concrete cloud-commitments-go constructors (awsprovider.NewEC2ClientDirect, NewRecommendationsClientDirect, azurecompute.NewClient) and named the concrete provider types directly, so the generic HTTP layer imported four provider packages plus the Azure SDK and the AWS config loader. The same held for ri_exchange_audit.go, ri_exchange_refusals.go, ri_utilization_cache.go and the Marketplace handlers. Add internal/api/provider_wiring.go as the one file in the package that imports the concrete provider packages. It builds the clients behind the existing narrow interfaces (reshapeEC2Client, reshapeRecsClient, targetOfferingsEC2Client, azureExchangeClient, marketplaceEC2Client), loads the default AWS config, holds the Azure reservation term parser, and exposes the provider data types as type aliases, so values, JSON and the existing tests are unchanged. Three functions get named results because gocritic's unnamedResult misreads alias types. A guard test fails if handler_ri_exchange.go imports a concrete provider package, the Azure SDK, or the AWS config/service packages again, or if any other non-test file in internal/api imports a concrete provider package. Pure refactor: no behavior change. Refs #41 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_marketplace.go | 12 +- internal/api/handler_marketplace_attempt.go | 55 ++++--- internal/api/handler_marketplace_cancel.go | 9 +- internal/api/handler_ri_exchange.go | 144 ++++++++----------- internal/api/provider_wiring.go | 98 +++++++++++++ internal/api/provider_wiring_imports_test.go | 101 +++++++++++++ internal/api/ri_exchange_audit.go | 5 +- internal/api/ri_exchange_refusals.go | 10 +- internal/api/ri_utilization_cache.go | 11 +- 9 files changed, 308 insertions(+), 137 deletions(-) create mode 100644 internal/api/provider_wiring.go create mode 100644 internal/api/provider_wiring_imports_test.go diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index cded31ab4..527adff37 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -23,8 +23,6 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" - awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" - ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" @@ -38,9 +36,9 @@ import ( // marketplaceEC2Client is the narrow EC2 interface the marketplace handlers // need. Using a minimal interface keeps test stubs small. type marketplaceEC2Client interface { - CreateMarketplaceListing(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) - DescribeMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) - CancelMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) + CreateMarketplaceListing(ctx context.Context, req ec2MarketplaceListingRequest) (ec2MarketplaceListingResult, error) + DescribeMarketplaceListing(ctx context.Context, listingID string) (ec2MarketplaceListingResult, error) + CancelMarketplaceListing(ctx context.Context, listingID string) (ec2MarketplaceListingResult, error) // FetchOfferingClass calls AWS DescribeReservedInstances to determine // whether a given Reserved Instance is 'standard' or 'convertible'. Used // when the purchase_history row has no offering_class stored (pre-migration @@ -54,7 +52,7 @@ func (h *Handler) buildMarketplaceEC2Client(cfg aws.Config) marketplaceEC2Client if h.marketplaceEC2Factory != nil { return h.marketplaceEC2Factory(cfg) } - return awsprovider.NewEC2ClientDirect(cfg) + return newMarketplaceEC2Client(cfg) } // loadMarketplaceAWSConfig returns an AWS config for the purchase's region that @@ -304,7 +302,7 @@ const marketplaceCompensationTimeout = 10 * time.Second // keepUncanceledListing records a listing that could be neither persisted nor // canceled; releasing the claim instead would hide it from the cancel endpoint. -func (h *Handler) keepUncanceledListing(ctx context.Context, purchaseID string, listing ec2svc.MarketplaceListingResult, cancelErr error) error { +func (h *Handler) keepUncanceledListing(ctx context.Context, purchaseID string, listing ec2MarketplaceListingResult, cancelErr error) error { logging.Errorf("marketplace: rollback cancel for listing %s failed: %v", listing.ListingID, cancelErr) state := listing.State if state == "" { diff --git a/internal/api/handler_marketplace_attempt.go b/internal/api/handler_marketplace_attempt.go index b978aabf0..eaeba0de4 100644 --- a/internal/api/handler_marketplace_attempt.go +++ b/internal/api/handler_marketplace_attempt.go @@ -16,7 +16,6 @@ import ( "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" - ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-sdk-go-v2/aws" sdkec2 "github.com/aws/aws-sdk-go-v2/service/ec2" @@ -29,7 +28,7 @@ import ( // poller reads a listing's sale details through it too (issue #27), because // the wrapper's DescribeMarketplaceListing returns only the state. type marketplaceListingFinder interface { - FindMarketplaceListingByToken(ctx context.Context, reservedInstancesID, clientToken string) (ec2svc.MarketplaceListingResult, bool, error) + FindMarketplaceListingByToken(ctx context.Context, reservedInstancesID, clientToken string) (ec2MarketplaceListingResult, bool, error) DescribeMarketplaceListingStatus(ctx context.Context, listingID string) (marketplaceListingStatus, error) } @@ -39,20 +38,20 @@ type describeListingsAPI interface { type awsMarketplaceListingFinder struct{ api describeListingsAPI } -func (f awsMarketplaceListingFinder) FindMarketplaceListingByToken(ctx context.Context, reservedInstancesID, clientToken string) (ec2svc.MarketplaceListingResult, bool, error) { +func (f awsMarketplaceListingFinder) FindMarketplaceListingByToken(ctx context.Context, reservedInstancesID, clientToken string) (listing ec2MarketplaceListingResult, found bool, err error) { out, err := f.api.DescribeReservedInstancesListings(ctx, &sdkec2.DescribeReservedInstancesListingsInput{ ReservedInstancesId: aws.String(reservedInstancesID), }) if err != nil { - return ec2svc.MarketplaceListingResult{}, false, fmt.Errorf("DescribeReservedInstancesListings failed: %w", err) + return ec2MarketplaceListingResult{}, false, fmt.Errorf("DescribeReservedInstancesListings failed: %w", err) } for i := range out.ReservedInstancesListings { l := &out.ReservedInstancesListings[i] if aws.ToString(l.ClientToken) == clientToken { - return ec2svc.MarketplaceListingResult{ListingID: aws.ToString(l.ReservedInstancesListingId), State: string(l.Status)}, true, nil + return ec2MarketplaceListingResult{ListingID: aws.ToString(l.ReservedInstancesListingId), State: string(l.Status)}, true, nil } } - return ec2svc.MarketplaceListingResult{}, false, nil + return ec2MarketplaceListingResult{}, false, nil } func (h *Handler) buildMarketplaceFinder(cfg aws.Config) marketplaceListingFinder { @@ -78,10 +77,10 @@ func marketplaceInstanceCount(row *config.PurchaseHistoryRecord) (int32, error) return 1, nil } -func toAWSPriceSchedule(schedule []MarketplacePriceTier) []ec2svc.MarketplacePriceTier { - out := make([]ec2svc.MarketplacePriceTier, 0, len(schedule)) +func toAWSPriceSchedule(schedule []MarketplacePriceTier) []ec2MarketplacePriceTier { + out := make([]ec2MarketplacePriceTier, 0, len(schedule)) for _, t := range schedule { - out = append(out, ec2svc.MarketplacePriceTier{Term: t.TermMonths, Price: t.Price}) + out = append(out, ec2MarketplacePriceTier{Term: t.TermMonths, Price: t.Price}) } return out } @@ -93,14 +92,14 @@ func toAWSPriceSchedule(schedule []MarketplacePriceTier) []ec2svc.MarketplacePri // does not leave the row stuck in the transient pending state. Returns the // persisted listing and the schedule it was created with, which on a retry is // the stored one, not the one passed in. -func (h *Handler) reserveAndCreateListing(ctx context.Context, purchaseID string, row *config.PurchaseHistoryRecord, cfg aws.Config, ec2Client marketplaceEC2Client, schedule []MarketplacePriceTier) (ec2svc.MarketplaceListingResult, []MarketplacePriceTier, error) { +func (h *Handler) reserveAndCreateListing(ctx context.Context, purchaseID string, row *config.PurchaseHistoryRecord, cfg aws.Config, ec2Client marketplaceEC2Client, schedule []MarketplacePriceTier) (ec2MarketplaceListingResult, []MarketplacePriceTier, error) { instanceCount, err := marketplaceInstanceCount(row) if err != nil { - return ec2svc.MarketplaceListingResult{}, nil, err + return ec2MarketplaceListingResult{}, nil, err } claim, used, err := h.claimListingSlot(ctx, purchaseID, schedule) if err != nil { - return ec2svc.MarketplaceListingResult{}, nil, err + return ec2MarketplaceListingResult{}, nil, err } if claim.Resumed { result, found, recErr := h.reconcileAttempt(ctx, purchaseID, claim, h.buildMarketplaceFinder(cfg)) @@ -147,28 +146,28 @@ func (h *Handler) claimListingSlot(ctx context.Context, purchaseID string, sched // resolved: result holds the recorded listing, or err why it ended in a dead // one. Not found means AWS has nothing for the token, so the caller creates // the listing with that same token. -func (h *Handler) reconcileAttempt(ctx context.Context, purchaseID string, claim *config.MarketplaceListingClaim, finder marketplaceListingFinder) (ec2svc.MarketplaceListingResult, bool, error) { - listing, found, err := finder.FindMarketplaceListingByToken(ctx, purchaseID, claim.ClientToken) +func (h *Handler) reconcileAttempt(ctx context.Context, purchaseID string, claim *config.MarketplaceListingClaim, finder marketplaceListingFinder) (listing ec2MarketplaceListingResult, found bool, err error) { + listing, found, err = finder.FindMarketplaceListingByToken(ctx, purchaseID, claim.ClientToken) if err != nil { h.releaseMarketplaceClaim(ctx, purchaseID, claim, true) - return ec2svc.MarketplaceListingResult{}, false, mapAWSMarketplaceError("could not reconcile the earlier listing attempt on AWS", err) + return ec2MarketplaceListingResult{}, false, mapAWSMarketplaceError("could not reconcile the earlier listing attempt on AWS", err) } if !found { - return ec2svc.MarketplaceListingResult{}, false, nil + return ec2MarketplaceListingResult{}, false, nil } if listing.State == "" { listing.State = config.ListingStatePending } logging.Warnf("marketplace: purchase %s: earlier attempt already created listing %s (%s); recording it", purchaseID, listing.ListingID, listing.State) if isDeadListingState(listing.State) { - return ec2svc.MarketplaceListingResult{}, true, h.recordDeadListing(ctx, purchaseID, claim, listing) + return ec2MarketplaceListingResult{}, true, h.recordDeadListing(ctx, purchaseID, claim, listing) } if err = h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, listing.ListingID, listing.State); err != nil { if isListingEndedConflict(err) { - return ec2svc.MarketplaceListingResult{}, true, listingCanceledConcurrently(listing.ListingID) + return ec2MarketplaceListingResult{}, true, listingCanceledConcurrently(listing.ListingID) } h.releaseMarketplaceClaim(ctx, purchaseID, claim, true) - return ec2svc.MarketplaceListingResult{}, true, fmt.Errorf("listing %s exists on AWS but could not be recorded: %w", listing.ListingID, err) + return ec2MarketplaceListingResult{}, true, fmt.Errorf("listing %s exists on AWS but could not be recorded: %w", listing.ListingID, err) } return listing, true, nil } @@ -184,7 +183,7 @@ func listingCanceledConcurrently(listingID string) error { // "not listed" and let the next request create a second listing, so the // listing is read back, and recorded as pending (live, cancelable) if its // state is still unknown. -func createdListingState(ctx context.Context, ec2Client marketplaceEC2Client, created ec2svc.MarketplaceListingResult) string { +func createdListingState(ctx context.Context, ec2Client marketplaceEC2Client, created ec2MarketplaceListingResult) string { if created.State != "" { return created.State } @@ -196,8 +195,8 @@ func createdListingState(ctx context.Context, ec2Client marketplaceEC2Client, cr return config.ListingStatePending } -func (h *Handler) createAndPersistListing(ctx context.Context, purchaseID string, claim *config.MarketplaceListingClaim, ec2Client marketplaceEC2Client, instanceCount int32, awsSchedule []ec2svc.MarketplacePriceTier) (ec2svc.MarketplaceListingResult, error) { - result, err := ec2Client.CreateMarketplaceListing(ctx, ec2svc.MarketplaceListingRequest{ +func (h *Handler) createAndPersistListing(ctx context.Context, purchaseID string, claim *config.MarketplaceListingClaim, ec2Client marketplaceEC2Client, instanceCount int32, awsSchedule []ec2MarketplacePriceTier) (ec2MarketplaceListingResult, error) { + result, err := ec2Client.CreateMarketplaceListing(ctx, ec2MarketplaceListingRequest{ ReservedInstancesID: purchaseID, ClientToken: claim.ClientToken, PriceSchedule: awsSchedule, @@ -210,12 +209,12 @@ func (h *Handler) createAndPersistListing(ctx context.Context, purchaseID string // then is the token dropped; otherwise the retry resends it. h.releaseMarketplaceClaim(ctx, purchaseID, claim, !isClientFaultError(mapped)) logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed: %v", purchaseID, err) - return ec2svc.MarketplaceListingResult{}, mapped + return ec2MarketplaceListingResult{}, mapped } result.State = createdListingState(ctx, ec2Client, result) if isDeadListingState(result.State) { - return ec2svc.MarketplaceListingResult{}, h.recordDeadListing(ctx, purchaseID, claim, result) + return ec2MarketplaceListingResult{}, h.recordDeadListing(ctx, purchaseID, claim, result) } // Persist the listing ID and state. On DB failure, attempt a compensating @@ -226,17 +225,17 @@ func (h *Handler) createAndPersistListing(ctx context.Context, purchaseID string if isListingEndedConflict(dbErr) { // The cancel endpoint found this attempt's listing through its // token, canceled it on AWS and recorded that first. - return ec2svc.MarketplaceListingResult{}, listingCanceledConcurrently(result.ListingID) + return ec2MarketplaceListingResult{}, listingCanceledConcurrently(result.ListingID) } logging.Errorf("marketplace: listing created (%s / %s) but DB update failed: %v; attempting rollback", result.ListingID, result.State, dbErr) compCtx, cancelComp := context.WithTimeout(context.WithoutCancel(ctx), marketplaceCompensationTimeout) defer cancelComp() if _, rollbackErr := ec2Client.CancelMarketplaceListing(compCtx, result.ListingID); rollbackErr != nil { - return ec2svc.MarketplaceListingResult{}, h.keepUncanceledListing(compCtx, purchaseID, result, rollbackErr) + return ec2MarketplaceListingResult{}, h.keepUncanceledListing(compCtx, purchaseID, result, rollbackErr) } logging.Warnf("marketplace: listing %s rolled back (canceled) after DB failure", result.ListingID) h.recordRolledBackListing(compCtx, purchaseID, claim, result.ListingID) - return ec2svc.MarketplaceListingResult{}, fmt.Errorf("listing created but could not be persisted; listing has been rolled back: %w", dbErr) + return ec2MarketplaceListingResult{}, fmt.Errorf("listing created but could not be persisted; listing has been rolled back: %w", dbErr) } return result, nil @@ -254,7 +253,7 @@ func isDeadListingState(state string) bool { // recordDeadListing handles a listing of this attempt that is canceled or // closed on AWS: it records the listing, which also drops the attempt's token, // so the next attempt starts fresh instead of AWS replaying the dead listing. -func (h *Handler) recordDeadListing(ctx context.Context, purchaseID string, claim *config.MarketplaceListingClaim, listing ec2svc.MarketplaceListingResult) error { +func (h *Handler) recordDeadListing(ctx context.Context, purchaseID string, claim *config.MarketplaceListingClaim, listing ec2MarketplaceListingResult) error { if err := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, listing.ListingID, listing.State); err != nil { logging.Errorf("marketplace: failed to record dead listing %s for purchase %s: %v", listing.ListingID, purchaseID, err) h.releaseMarketplaceClaim(ctx, purchaseID, claim, false) diff --git a/internal/api/handler_marketplace_cancel.go b/internal/api/handler_marketplace_cancel.go index c000a3192..9b24fda3e 100644 --- a/internal/api/handler_marketplace_cancel.go +++ b/internal/api/handler_marketplace_cancel.go @@ -16,7 +16,6 @@ import ( "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" - ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-lambda-go/events" @@ -178,16 +177,16 @@ func (h *Handler) cancelListing(ctx context.Context, purchaseID string, seen *co // endedMarketplaceListing reports whether listingID has already ended on AWS, // read after its cancel failed. A role-assumption failure would fail the read // the same way, so it is not retried. -func endedMarketplaceListing(ctx context.Context, ec2Client marketplaceEC2Client, listingID string, cancelErr error) (ec2svc.MarketplaceListingResult, bool) { +func endedMarketplaceListing(ctx context.Context, ec2Client marketplaceEC2Client, listingID string, cancelErr error) (ec2MarketplaceListingResult, bool) { if isRoleAssumptionFailure(cancelErr) { - return ec2svc.MarketplaceListingResult{}, false + return ec2MarketplaceListingResult{}, false } listing, err := ec2Client.DescribeMarketplaceListing(ctx, listingID) if err != nil || !isDeadListingState(listing.State) { - return ec2svc.MarketplaceListingResult{}, false + return ec2MarketplaceListingResult{}, false } logging.Warnf("marketplace: listing %s is already %s on AWS (cancel said: %v); recording it", listingID, listing.State, cancelErr) - return ec2svc.MarketplaceListingResult{ListingID: listingID, State: listing.State}, true + return ec2MarketplaceListingResult{ListingID: listingID, State: listing.State}, true } // resolveListingAttempt records listingID in state on a pending row, only diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 8a26ea6d7..1dfbeac85 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -13,19 +13,13 @@ import ( "strings" "time" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" - awsconfig "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/smithy-go" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" - awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" - "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" - ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" - azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" @@ -35,24 +29,23 @@ import ( // reshapeEC2Client is the narrow slice of the EC2 client that // getReshapeRecommendations needs. Scoped to this handler so mocks -// stay small. The concrete *ec2svc.Client returned by -// awsprovider.NewEC2ClientDirect already implements these methods -// (Go structural typing), so the nil-factory fallback path casts it -// directly. +// stay small. The concrete EC2 client that newReshapeEC2Client +// (provider_wiring.go) builds for the nil-factory fallback path +// implements it. // // Cross-family alternatives no longer flow through here — they're // sourced from the cached AWS Cost Explorer purchase recommendations // in Postgres via purchaseRecLookupFromStore (see exchange_lookup.go), // so the EC2 client only needs to enumerate convertible RIs. type reshapeEC2Client interface { - ListConvertibleReservedInstances(ctx context.Context) ([]ec2svc.ConvertibleRI, error) + ListConvertibleReservedInstances(ctx context.Context) ([]ec2ConvertibleRI, error) } // reshapeRecsClient is the narrow slice of the recommendations // adapter that getReshapeRecommendations needs (the utilization // fetcher injected into the cache wrapper). Scoped identically. type reshapeRecsClient interface { - GetRIUtilization(ctx context.Context, lookbackDays int, region string) ([]recommendations.RIUtilization, error) + GetRIUtilization(ctx context.Context, lookbackDays int, region string) ([]riUtilization, error) } // buildReshapeEC2Client honors the injected factory when set, falling @@ -62,7 +55,7 @@ func (h *Handler) buildReshapeEC2Client(cfg aws.Config) reshapeEC2Client { if h.reshapeEC2Factory != nil { return h.reshapeEC2Factory(cfg) } - return awsprovider.NewEC2ClientDirect(cfg) + return newReshapeEC2Client(cfg) } // buildReshapeRecsClient mirrors buildReshapeEC2Client for the @@ -71,15 +64,15 @@ func (h *Handler) buildReshapeRecsClient(cfg aws.Config) reshapeRecsClient { if h.reshapeRecsFactory != nil { return h.reshapeRecsFactory(cfg) } - return awsprovider.NewRecommendationsClientDirect(cfg) + return newReshapeRecsClient(cfg) } // targetOfferingsEC2Client is the narrow EC2 interface that // listTargetOfferings needs. Scoped here so tests can inject a tiny -// stub without implementing the full ec2svc.Client surface. +// stub without implementing the full concrete EC2 client surface. type targetOfferingsEC2Client interface { - ListConvertibleReservedInstances(ctx context.Context) ([]ec2svc.ConvertibleRI, error) - ListTargetOfferings(ctx context.Context, params ec2svc.ListTargetOfferingsParams) ([]ec2svc.TargetOffering, error) + ListConvertibleReservedInstances(ctx context.Context) ([]ec2ConvertibleRI, error) + ListTargetOfferings(ctx context.Context, params ec2ListTargetOfferingsParams) ([]ec2TargetOffering, error) } // buildTargetOfferingsEC2Client honors the injected factory when set, @@ -88,13 +81,13 @@ func (h *Handler) buildTargetOfferingsEC2Client(cfg aws.Config) targetOfferingsE if h.targetOfferingsEC2Factory != nil { return h.targetOfferingsEC2Factory(cfg) } - return awsprovider.NewEC2ClientDirect(cfg) + return newTargetOfferingsEC2Client(cfg) } // TargetOfferingsResponse is the response for // GET /api/ri-exchange/target-offerings. type TargetOfferingsResponse struct { - Offerings []ec2svc.TargetOffering `json:"offerings"` + Offerings []ec2TargetOffering `json:"offerings"` } // offeringIDPattern matches a standard AWS offering UUID used for @@ -109,7 +102,7 @@ var offeringIDPattern = regexp.MustCompile( // // The handler looks up the source RI from DescribeReservedInstances, // extracts its ProductDescription / Tenancy / Scope / Duration, -// and passes those to ec2svc.ListTargetOfferings which +// and passes those to the EC2 client's ListTargetOfferings, which // calls DescribeReservedInstancesOfferings with the same typed-field // shape used by PR #690. Instance type is intentionally omitted from // the query so AWS returns all valid target instance types -- the full @@ -147,7 +140,7 @@ func (h *Handler) listTargetOfferings(ctx context.Context, req *events.LambdaFun return nil, fmt.Errorf("failed to list convertible RIs: %w", err) } - var sourceRI *ec2svc.ConvertibleRI + var sourceRI *ec2ConvertibleRI for i := range ris { if ris[i].ReservedInstanceID == sourceRIID { sourceRI = &ris[i] @@ -169,8 +162,8 @@ func (h *Handler) listTargetOfferings(ctx context.Context, req *events.LambdaFun // targetOfferingsForSource lists the exchange targets for sourceRI, keeping a // zonal RI's targets in its own AZ. A zone refusal (the source RI's scope and // AZ disagree) is a 422. -func targetOfferingsForSource(ctx context.Context, ec2Client targetOfferingsEC2Client, sourceRI *ec2svc.ConvertibleRI) ([]ec2svc.TargetOffering, error) { - offerings, err := ec2Client.ListTargetOfferings(ctx, ec2svc.ListTargetOfferingsParams{ +func targetOfferingsForSource(ctx context.Context, ec2Client targetOfferingsEC2Client, sourceRI *ec2ConvertibleRI) ([]ec2TargetOffering, error) { + offerings, err := ec2Client.ListTargetOfferings(ctx, ec2ListTargetOfferingsParams{ ProductDescription: sourceRI.ProductDescription, Tenancy: sourceRI.InstanceTenancy, Scope: sourceRI.Scope, @@ -211,10 +204,11 @@ func isOfferingZoneRefusal(err error) bool { // azureExchangeClient is the narrow interface that listExchangeableAzureRIs, // getAzureCompatibleOfferings, and executeAzureExchange need from the Azure -// compute client. Satisfied by *azurecompute.ComputeClient; a stub can be -// injected via Handler.azureExchangeFactory for tests. +// compute client. Satisfied by the compute client newAzureExchangeClient +// (provider_wiring.go) builds; a stub can be injected via +// Handler.azureExchangeFactory for tests. type azureExchangeClient interface { - ListExchangeableReservations(ctx context.Context) ([]azurecompute.ExchangeableReservation, error) + ListExchangeableReservations(ctx context.Context) ([]azureExchangeableReservation, error) // CalculateExchange prices a proposed exchange without committing it. // Both getAzureCompatibleOfferings (read-only quote) and @@ -222,8 +216,8 @@ type azureExchangeClient interface { // only executeAzureExchange ever calls ExecuteExchange, and only with // the SessionID this same call just returned -- see executeAzureExchange's // doc comment for why a client-supplied session is never trusted. - CalculateExchange(ctx context.Context, sources []azurecompute.ExchangeableReservation, targets []azurecompute.ExchangeTarget) (*azurecompute.ExchangePreview, []azurecompute.CompatibleOffering, error) - ExecuteExchange(ctx context.Context, sessionID string) (*azurecompute.ExchangeResult, error) + CalculateExchange(ctx context.Context, sources []azureExchangeableReservation, targets []azureExchangeTarget) (*azureExchangePreview, []azureCompatibleOffering, error) + ExecuteExchange(ctx context.Context, sessionID string) (*azureExchangeResult, error) } // buildAzureExchangeClient returns the injected factory result when one has @@ -285,9 +279,7 @@ func (h *Handler) azureExchangeClientForAccount(ctx context.Context, subscriptio return nil, fmt.Errorf("azure: resolve credentials for subscription %q: %w", subscriptionID, err) } - // Region is left empty -- ListExchangeableReservations uses the tenant- - // wide armreservations API which is not scoped to a region. - return azurecompute.NewClient(cred, subscriptionID, ""), nil + return newAzureExchangeClient(cred, subscriptionID), nil } // listExchangeableAzureRIs returns active Azure VM reservations that are @@ -359,7 +351,7 @@ func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.Lamb // No Azure account configured for this subscription (or no Azure accounts // registered at all). Return an empty list rather than a 500 so the page // renders a "no reservations" state instead of an opaque error banner. - return &ExchangeableAzureRIsResponse{Reservations: []azurecompute.ExchangeableReservation{}}, nil + return &ExchangeableAzureRIsResponse{Reservations: []azureExchangeableReservation{}}, nil } reservations, err := client.ListExchangeableReservations(ctx) @@ -391,9 +383,9 @@ func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.Lamb // (requireAzureSourceOwnership refuses cross-subscription sources), so // returning the wider tenant listing here would just be the enumeration leak // issue #1656 describes. -func filterAzureReservationsBySubscription(reservations []azurecompute.ExchangeableReservation, subscriptionID string) []azurecompute.ExchangeableReservation { +func filterAzureReservationsBySubscription(reservations []azureExchangeableReservation, subscriptionID string) []azureExchangeableReservation { scope := azureBillingScopeID(subscriptionID) - filtered := make([]azurecompute.ExchangeableReservation, 0, len(reservations)) + filtered := make([]azureExchangeableReservation, 0, len(reservations)) for i := range reservations { if strings.EqualFold(reservations[i].BillingScopeID, scope) { filtered = append(filtered, reservations[i]) @@ -419,7 +411,7 @@ func filterAzureReservationsBySubscription(reservations []azurecompute.Exchangea // the dropped reservation/billing-scope identifiers, are safe to log -- // logging the identifiers would just relocate the disclosure this filter // exists to prevent. -func (h *Handler) filterAzureReservationsByScope(ctx context.Context, session *Session, reservations []azurecompute.ExchangeableReservation) ([]azurecompute.ExchangeableReservation, error) { +func (h *Handler) filterAzureReservationsByScope(ctx context.Context, session *Session, reservations []azureExchangeableReservation) ([]azureExchangeableReservation, error) { allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) @@ -430,7 +422,7 @@ func (h *Handler) filterAzureReservationsByScope(ctx context.Context, session *S // normalize here too rather than letting an admin session alone see // {"reservations": null}. if reservations == nil { - reservations = []azurecompute.ExchangeableReservation{} + reservations = []azureExchangeableReservation{} } return reservations, nil } @@ -473,8 +465,8 @@ func azureScopeIndex(accounts []config.CloudAccount) map[string]config.CloudAcco // scope allows (AccountScope.Allows). Fails closed: a reservation with no // BillingScopeID, or one that resolves to no registered CloudAccount, is // dropped rather than kept. -func filterReservationsByScopeIndex(reservations []azurecompute.ExchangeableReservation, scopeToAccount map[string]config.CloudAccount, allowed auth.AccountScope) []azurecompute.ExchangeableReservation { - filtered := make([]azurecompute.ExchangeableReservation, 0, len(reservations)) +func filterReservationsByScopeIndex(reservations []azureExchangeableReservation, scopeToAccount map[string]config.CloudAccount, allowed auth.AccountScope) []azureExchangeableReservation { + filtered := make([]azureExchangeableReservation, 0, len(reservations)) for i := range reservations { r := reservations[i] if r.BillingScopeID == "" { @@ -537,8 +529,8 @@ type AzureCompatibleOfferingsRequestBody struct { // though execute never trusts a client-supplied session -- see // executeAzureExchange). type AzureCompatibleOfferingsResponse struct { - Offerings []azurecompute.CompatibleOffering `json:"offerings"` - Preview *azurecompute.ExchangePreview `json:"preview"` + Offerings []azureCompatibleOffering `json:"offerings"` + Preview *azureExchangePreview `json:"preview"` } // AzureExecuteExchangeRequestBody is the request body for the execute @@ -564,20 +556,6 @@ type AzureExecuteExchangeResponse struct { PurchasesTotal *float64 `json:"purchases_total"` } -// azureReservationTermFromString converts the HTTP-layer term string to the -// typed SDK enum, rejecting anything outside armreservations' -// PossibleReservationTermValues(). No fallback: an unrecognized term is a -// 400, never silently coerced to a default term (feedback_sdk_enum_string_literals). -func azureReservationTermFromString(s string) (armreservations.ReservationTerm, error) { - term := armreservations.ReservationTerm(s) - for _, t := range armreservations.PossibleReservationTermValues() { - if t == term { - return term, nil - } - } - return "", fmt.Errorf("unsupported term %q", s) -} - // validateAzureExchangeSources checks the shared sources[] shape for both // the offerings and execute request bodies. func validateAzureExchangeSources(sources []AzureExchangeSourceBody) error { @@ -692,12 +670,12 @@ func validateAzureExecuteBody(body AzureExecuteExchangeRequestBody) error { // toAzureExchangeSources keeps the server-listed reservation metadata and // applies the requested quantities after requireAzureSourceOwnership succeeds. -func toAzureExchangeSources(sources []AzureExchangeSourceBody, owned []azurecompute.ExchangeableReservation) []azurecompute.ExchangeableReservation { - byID := make(map[string]azurecompute.ExchangeableReservation, len(owned)) +func toAzureExchangeSources(sources []AzureExchangeSourceBody, owned []azureExchangeableReservation) []azureExchangeableReservation { + byID := make(map[string]azureExchangeableReservation, len(owned)) for i := range owned { byID[strings.ToLower(owned[i].ReservationID)] = owned[i] } - out := make([]azurecompute.ExchangeableReservation, len(sources)) + out := make([]azureExchangeableReservation, len(sources)) for i, s := range sources { out[i] = byID[strings.ToLower(s.ReservationID)] out[i].Quantity = s.Quantity @@ -711,15 +689,15 @@ func toAzureExchangeSources(sources []AzureExchangeSourceBody, owned []azurecomp // body (see azureBillingScopeID). validateAzureExchangeTargets must be // called first; a term error here indicates an internal invariant break // rather than a fresh client mistake. -func toAzureExchangeTargets(targets []AzureExchangeTargetBody, subscriptionID string) ([]azurecompute.ExchangeTarget, error) { - out := make([]azurecompute.ExchangeTarget, len(targets)) +func toAzureExchangeTargets(targets []AzureExchangeTargetBody, subscriptionID string) ([]azureExchangeTarget, error) { + out := make([]azureExchangeTarget, len(targets)) scope := azureBillingScopeID(subscriptionID) for i, t := range targets { term, err := azureReservationTermFromString(t.Term) if err != nil { return nil, fmt.Errorf("targets[%d]: %w", i, err) } - out[i] = azurecompute.ExchangeTarget{ + out[i] = azureExchangeTarget{ SKU: t.SKU, Location: t.Location, Term: term, @@ -819,7 +797,7 @@ const unknownRegionConstraint = "unknown-region" // means specifically "owned, but Azure reported no region". The missing-source // branch is kept as a fail-closed default for any future caller that reaches // this function without that guarantee. -func exchangeRegions(targets []AzureExchangeTargetBody, sources []AzureExchangeSourceBody, owned []azurecompute.ExchangeableReservation) []string { +func exchangeRegions(targets []AzureExchangeTargetBody, sources []AzureExchangeSourceBody, owned []azureExchangeableReservation) []string { regionByID := make(map[string]string, len(owned)) for i := range owned { regionByID[strings.ToLower(owned[i].ReservationID)] = owned[i].Region @@ -931,7 +909,7 @@ func (h *Handler) lookupAzureAccount(ctx context.Context, subscriptionID string) // "belongs to someone else", so this cannot be used to enumerate another // subscription's reservation IDs (same posture as // requireAzureSubscriptionScope). -func requireAzureSourceOwnership(owned []azurecompute.ExchangeableReservation, sources []AzureExchangeSourceBody, subscriptionID string) error { +func requireAzureSourceOwnership(owned []azureExchangeableReservation, sources []AzureExchangeSourceBody, subscriptionID string) error { scope := azureBillingScopeID(subscriptionID) byID := make(map[string]string, len(owned)) for i := range owned { @@ -952,7 +930,7 @@ func requireAzureSourceOwnership(owned []azurecompute.ExchangeableReservation, s // applies requireAzureSourceOwnership. Split from the pure check so the // authorization rule itself is testable without a client, and so both the // pricing and execute endpoints share one code path. -func checkAzureSourceOwnership(ctx context.Context, client azureExchangeClient, sources []AzureExchangeSourceBody, subscriptionID string) ([]azurecompute.ExchangeableReservation, error) { +func checkAzureSourceOwnership(ctx context.Context, client azureExchangeClient, sources []AzureExchangeSourceBody, subscriptionID string) ([]azureExchangeableReservation, error) { owned, err := listOwnedAzureReservations(ctx, client) if err != nil { return nil, err @@ -973,7 +951,7 @@ func checkAzureSourceOwnership(ctx context.Context, client azureExchangeClient, // region, and permitting the exchange would restore the very gaps those // checks exist to close. 502 rather than 500 because the upstream dependency, // not this service, is what failed. -func listOwnedAzureReservations(ctx context.Context, client azureExchangeClient) ([]azurecompute.ExchangeableReservation, error) { +func listOwnedAzureReservations(ctx context.Context, client azureExchangeClient) ([]azureExchangeableReservation, error) { owned, err := client.ListExchangeableReservations(ctx) if err != nil { logging.Errorf("azure exchange source ownership lookup failed: %v", err) @@ -1073,7 +1051,7 @@ const azureMaxPurchaseAmountCurrency = "USD" // It also sharpens exchangeRegions: every source reaching it is // known-owned, so unknownRegionConstraint means only "owned, but Azure // reported no region" rather than doubling as "not yours" or "not real". -func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session *Session, body AzureExecuteExchangeRequestBody, maxRat *big.Rat) (azureExchangeClient, []azurecompute.ExchangeableReservation, string, error) { +func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session *Session, body AzureExecuteExchangeRequestBody, maxRat *big.Rat) (client azureExchangeClient, sources []azureExchangeableReservation, accountID string, err error) { // The subscription's CloudAccount is resolved once and that one row // feeds the scope check, the client's credentials and the constraint // AccountIDs (issue #311), so the three cannot see different rows. @@ -1093,7 +1071,7 @@ func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session * return nil, nil, "", scopeErr } - client, err := h.azureExchangeClientForAccount(ctx, body.SubscriptionID, account) + client, err = h.azureExchangeClientForAccount(ctx, body.SubscriptionID, account) if err != nil { return nil, nil, "", fmt.Errorf("failed to build Azure exchange client: %w", err) } @@ -1110,7 +1088,7 @@ func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session * return nil, nil, "", ownErr } - accountID := azureExchangeAccountID(account) + accountID = azureExchangeAccountID(account) if err := h.checkAzureExecuteConstraints(ctx, session, body, accountID, maxRat, exchangeRegions(body.Targets, body.Sources, owned)); err != nil { return nil, nil, "", err } @@ -1201,7 +1179,7 @@ func (h *Handler) checkAzureExecuteConstraints(ctx context.Context, session *Ses // azureExchangeClient.CalculateExchange contract never returns (nil, nil, // nil), but a future implementation of the interface making that mistake // must not panic this handler. -func checkAzureExchangeMoneyGuardrails(preview *azurecompute.ExchangePreview, maxRat *big.Rat, currency string) error { +func checkAzureExchangeMoneyGuardrails(preview *azureExchangePreview, maxRat *big.Rat, currency string) error { if preview == nil { return fmt.Errorf("internal error: CalculateExchange returned a nil preview") } @@ -1372,7 +1350,7 @@ func mapAzureExchangeError(opMsg string, err error) error { // getBaseAWSConfig returns the cached base AWS config, loading it once via sync.Once. func (h *Handler) getBaseAWSConfig(ctx context.Context) (aws.Config, error) { h.awsCfgOnce.Do(func() { - h.awsCfg, h.awsCfgErr = awsconfig.LoadDefaultConfig(ctx) + h.awsCfg, h.awsCfgErr = loadDefaultAWSConfig(ctx) }) return h.awsCfg, h.awsCfgErr } @@ -1511,13 +1489,13 @@ func (h *Handler) listConvertibleRIs(ctx context.Context, req *events.LambdaFunc if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { return nil, sErr } else if !inScope { - return &ConvertibleRIsResponse{Instances: []ec2svc.ConvertibleRI{}}, nil + return &ConvertibleRIsResponse{Instances: []ec2ConvertibleRI{}}, nil } if inScope, sErr := h.checkListRIsAccountIDParam(ctx, req.QueryStringParameters); sErr != nil { return nil, sErr } else if !inScope { - return &ConvertibleRIsResponse{Instances: []ec2svc.ConvertibleRI{}}, nil + return &ConvertibleRIsResponse{Instances: []ec2ConvertibleRI{}}, nil } region := req.QueryStringParameters["region"] @@ -1526,7 +1504,7 @@ func (h *Handler) listConvertibleRIs(ctx context.Context, req *events.LambdaFunc return nil, fmt.Errorf("failed to load AWS config: %w", err) } - ec2Client := awsprovider.NewEC2ClientDirect(cfg) + ec2Client := newReshapeEC2Client(cfg) instances, err := ec2Client.ListConvertibleReservedInstances(ctx) if err != nil { return nil, fmt.Errorf("failed to list convertible RIs: %w", err) @@ -1546,7 +1524,7 @@ func (h *Handler) getRIUtilization(ctx context.Context, req *events.LambdaFuncti if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { return nil, sErr } else if !inScope { - return &RIUtilizationResponse{Utilization: []recommendations.RIUtilization{}}, nil + return &RIUtilizationResponse{Utilization: []riUtilization{}}, nil } lookbackDays, err := parseLookbackDaysParam(req.QueryStringParameters) @@ -1567,7 +1545,7 @@ func (h *Handler) getRIUtilization(ctx context.Context, req *events.LambdaFuncti } recsAdapter := h.buildReshapeRecsClient(cfg) - fetch := func(fetchCtx context.Context, days int) ([]recommendations.RIUtilization, error) { + fetch := func(fetchCtx context.Context, days int) ([]riUtilization, error) { return recsAdapter.GetRIUtilization(fetchCtx, days, region) } utilization, err := h.getRIUtilizationCache().getOrFetch(ctx, region, lookbackDays, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, fetch) @@ -1592,7 +1570,7 @@ func (h *Handler) getRIUtilization(ctx context.Context, req *events.LambdaFuncti // only rows the scope gate in the caller has authorized; unknown ids are // dropped, so an unregistered or out-of-scope member's RIs stay hidden. // Unrestricted sessions keep every row and make no extra AWS call. -func (h *Handler) restrictUtilizationToDeploymentRIs(ctx context.Context, session *Session, cfg aws.Config, utilization []recommendations.RIUtilization) ([]recommendations.RIUtilization, error) { +func (h *Handler) restrictUtilizationToDeploymentRIs(ctx context.Context, session *Session, cfg aws.Config, utilization []riUtilization) ([]riUtilization, error) { allowed, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) @@ -1608,7 +1586,7 @@ func (h *Handler) restrictUtilizationToDeploymentRIs(ctx context.Context, sessio for i := range own { ownIDs[own[i].ReservedInstanceID] = struct{}{} } - visible := make([]recommendations.RIUtilization, 0, len(utilization)) + visible := make([]riUtilization, 0, len(utilization)) for i := range utilization { if _, ok := ownIDs[utilization[i].ReservedInstanceID]; ok { visible = append(visible, utilization[i]) @@ -1648,7 +1626,7 @@ func parseThresholdParam(params map[string]string) (float64, error) { // the single implementation of the convertible RI monthly cost formula shared // with internal/server. Its error wraps ricost.ErrNonPositiveDuration or // ricost.ErrNoPricing, and callers must exclude the RI. -func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { +func monthlyCostFromConvertibleRI(ri ec2ConvertibleRI) (float64, error) { return ricost.MonthlyCostFromConvertibleRI(ri) } @@ -1664,7 +1642,7 @@ func excludedRIReason(err error) string { // convertToExchangeTypes converts provider-specific types to the exchange package types. // RIs whose monthly cost cannot be computed are returned as excluded rather // than dropped silently, so the response can surface them. -func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recommendations.RIUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo, []ExcludedRI) { +func convertToExchangeTypes(instances []ec2ConvertibleRI, utilData []riUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo, []ExcludedRI) { var excluded []ExcludedRI riInfos := make([]exchange.RIInfo, 0, len(instances)) for i := range instances { @@ -1770,7 +1748,7 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam } recsAdapter := h.buildReshapeRecsClient(cfg) - fetch := func(fetchCtx context.Context, days int) ([]recommendations.RIUtilization, error) { + fetch := func(fetchCtx context.Context, days int) ([]riUtilization, error) { return recsAdapter.GetRIUtilization(fetchCtx, days, p.region) } utilData, err := h.getRIUtilizationCache().getOrFetch(ctx, p.region, p.lookbackDays, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, fetch) @@ -1824,7 +1802,7 @@ func (h *Handler) attachReshapeStaleness(ctx context.Context, resp *ReshapeRecom // firstNonEmptyCurrency returns the first RI CurrencyCode set, or "" when none // is, so the response omits currency_code instead of claiming USD (#361). -func firstNonEmptyCurrency(instances []ec2svc.ConvertibleRI) string { +func firstNonEmptyCurrency(instances []ec2ConvertibleRI) string { for _rvc := range instances { inst := instances[_rvc] if inst.CurrencyCode != "" { @@ -2082,18 +2060,18 @@ func mapAWSExchangeError(opMsg string, err error) error { // ConvertibleRIsResponse holds the list of convertible RIs. type ConvertibleRIsResponse struct { - Instances []ec2svc.ConvertibleRI `json:"instances"` + Instances []ec2ConvertibleRI `json:"instances"` } // ExchangeableAzureRIsResponse holds the list of Azure VM reservations that // are eligible for the cross-SKU/cross-region exchange flow. type ExchangeableAzureRIsResponse struct { - Reservations []azurecompute.ExchangeableReservation `json:"reservations"` + Reservations []azureExchangeableReservation `json:"reservations"` } // RIUtilizationResponse holds per-RI utilization data. type RIUtilizationResponse struct { - Utilization []recommendations.RIUtilization `json:"utilization"` + Utilization []riUtilization `json:"utilization"` } // ReshapeRecommendationsResponse holds reshape recommendations. diff --git a/internal/api/provider_wiring.go b/internal/api/provider_wiring.go new file mode 100644 index 000000000..7b4c4eca8 --- /dev/null +++ b/internal/api/provider_wiring.go @@ -0,0 +1,98 @@ +package api + +// provider_wiring.go is the composition file for the concrete +// cloud-commitments-go provider packages the RI exchange and Marketplace +// handlers use (platform#41). It is the only non-test file in this package +// that imports them: the handlers reach AWS and Azure through the narrow +// client interfaces they declare (reshapeEC2Client, reshapeRecsClient, +// targetOfferingsEC2Client, azureExchangeClient, marketplaceEC2Client), built +// by the constructors below, and name the provider data types through the +// aliases below. provider_wiring_imports_test.go enforces this. +// +// The aliases are Go type aliases, not new types, so values, JSON encoding and +// the tests that build the provider types directly are unchanged. + +import ( + "context" + "fmt" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/aws/aws-sdk-go-v2/aws" + awsconfig "github.com/aws/aws-sdk-go-v2/config" + + awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" + ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" + azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" +) + +// Provider data types: AWS EC2 and Cost Explorer, Azure compute exchange and +// the Azure reservation term enum. +type ( + ec2ConvertibleRI = ec2svc.ConvertibleRI + ec2TargetOffering = ec2svc.TargetOffering + ec2ListTargetOfferingsParams = ec2svc.ListTargetOfferingsParams + ec2MarketplaceListingRequest = ec2svc.MarketplaceListingRequest + ec2MarketplaceListingResult = ec2svc.MarketplaceListingResult + ec2MarketplacePriceTier = ec2svc.MarketplacePriceTier + riUtilization = recommendations.RIUtilization + azureExchangeableReservation = azurecompute.ExchangeableReservation + azureExchangeTarget = azurecompute.ExchangeTarget + azureExchangePreview = azurecompute.ExchangePreview + azureExchangeResult = azurecompute.ExchangeResult + azureCompatibleOffering = azurecompute.CompatibleOffering + azureReservationTerm = armreservations.ReservationTerm +) + +// errAzureUnsupportedAppliedScope is azurecompute.ErrUnsupportedAppliedScope: +// CalculateExchange refuses a source whose applied scope cannot carry over. +var errAzureUnsupportedAppliedScope = azurecompute.ErrUnsupportedAppliedScope + +// loadDefaultAWSConfig loads the ambient AWS SDK config (env, shared files, +// instance or task role). +func loadDefaultAWSConfig(ctx context.Context) (aws.Config, error) { + return awsconfig.LoadDefaultConfig(ctx) +} + +// newReshapeEC2Client builds the EC2 client that lists convertible RIs. +func newReshapeEC2Client(cfg aws.Config) reshapeEC2Client { + return awsprovider.NewEC2ClientDirect(cfg) +} + +// newReshapeRecsClient builds the Cost Explorer recommendations adapter. +func newReshapeRecsClient(cfg aws.Config) reshapeRecsClient { + return awsprovider.NewRecommendationsClientDirect(cfg) +} + +// newTargetOfferingsEC2Client builds the EC2 client that lists convertible RIs +// and their exchange target offerings. +func newTargetOfferingsEC2Client(cfg aws.Config) targetOfferingsEC2Client { + return awsprovider.NewEC2ClientDirect(cfg) +} + +// newMarketplaceEC2Client builds the EC2 client for RI Marketplace listings. +func newMarketplaceEC2Client(cfg aws.Config) marketplaceEC2Client { + return awsprovider.NewEC2ClientDirect(cfg) +} + +// newAzureExchangeClient builds the Azure compute client for the exchange +// flow. Region is left empty: ListExchangeableReservations uses the +// tenant-wide armreservations API, which is not scoped to a region. +func newAzureExchangeClient(cred azcore.TokenCredential, subscriptionID string) azureExchangeClient { + return azurecompute.NewClient(cred, subscriptionID, "") +} + +// azureReservationTermFromString converts the HTTP-layer term string to the +// typed SDK enum, rejecting anything outside armreservations' +// PossibleReservationTermValues(). No fallback: an unrecognized term is a +// 400, never silently coerced to a default term (feedback_sdk_enum_string_literals). +func azureReservationTermFromString(s string) (azureReservationTerm, error) { + term := armreservations.ReservationTerm(s) + for _, t := range armreservations.PossibleReservationTermValues() { + if t == term { + return term, nil + } + } + return "", fmt.Errorf("unsupported term %q", s) +} diff --git a/internal/api/provider_wiring_imports_test.go b/internal/api/provider_wiring_imports_test.go new file mode 100644 index 000000000..b1242d284 --- /dev/null +++ b/internal/api/provider_wiring_imports_test.go @@ -0,0 +1,101 @@ +package api + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// providerWiringFile is the one non-test file in this package allowed to +// import the concrete cloud-commitments-go provider packages (platform#41). +const providerWiringFile = "provider_wiring.go" + +// concreteProviderPkgPrefix covers every concrete provider package +// (providers/aws, providers/aws/services/ec2, providers/azure/..., ...). +const concreteProviderPkgPrefix = "github.com/LeanerCloud/cloud-commitments-go/providers/" + +// riExchangeForbiddenImportPrefixes are the imports handler_ri_exchange.go +// must not take: concrete provider packages, the Azure SDK, and the AWS SDK's +// config loader and service clients. The generic aws.Config value type and +// smithy's error interface stay allowed. +var riExchangeForbiddenImportPrefixes = []string{ + concreteProviderPkgPrefix, + "github.com/Azure/azure-sdk-for-go/", + "github.com/aws/aws-sdk-go-v2/config", + "github.com/aws/aws-sdk-go-v2/service/", +} + +// fileImports returns the import paths of the Go file at path. +func fileImports(t *testing.T, path string) []string { + t.Helper() + f, err := parser.ParseFile(token.NewFileSet(), path, nil, parser.ImportsOnly) + require.NoError(t, err, path) + out := make([]string, 0, len(f.Imports)) + for _, imp := range f.Imports { + p, err := strconv.Unquote(imp.Path.Value) + require.NoError(t, err, path) + out = append(out, p) + } + return out +} + +func hasAnyPrefix(s string, prefixes []string) bool { + for _, p := range prefixes { + if strings.HasPrefix(s, p) { + return true + } + } + return false +} + +// The RI exchange HTTP handler reaches AWS and Azure only through the narrow +// client interfaces; the concrete constructors and types live in +// provider_wiring.go. +func TestRIExchangeHandlerHasNoConcreteProviderImports(t *testing.T) { + imports := fileImports(t, "handler_ri_exchange.go") + require.NotEmpty(t, imports, "the guard must actually read the handler's imports") + var bad []string + for _, p := range imports { + if hasAnyPrefix(p, riExchangeForbiddenImportPrefixes) { + bad = append(bad, p) + } + } + assert.Empty(t, bad, "move concrete provider construction and types into %s", providerWiringFile) +} + +// No non-test file in this package other than provider_wiring.go imports a +// concrete cloud-commitments-go provider package. +func TestOnlyProviderWiringImportsConcreteProviders(t *testing.T) { + entries, err := os.ReadDir(".") + require.NoError(t, err) + var bad []string + scanned := 0 + wiringImportsProviders := false + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + scanned++ + for _, p := range fileImports(t, filepath.Join(".", name)) { + if !strings.HasPrefix(p, concreteProviderPkgPrefix) { + continue + } + if name == providerWiringFile { + wiringImportsProviders = true + continue + } + bad = append(bad, name+": "+p) + } + } + require.Greater(t, scanned, 50, "the scan must actually reach the package sources") + assert.Empty(t, bad, "import concrete provider packages only from %s", providerWiringFile) + assert.True(t, wiringImportsProviders, "%s must exist and hold the concrete provider imports", providerWiringFile) +} diff --git a/internal/api/ri_exchange_audit.go b/internal/api/ri_exchange_audit.go index 7d12a9871..28d475b5a 100644 --- a/internal/api/ri_exchange_audit.go +++ b/internal/api/ri_exchange_audit.go @@ -10,7 +10,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" - azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/ctxutil" "github.com/google/uuid" @@ -200,7 +199,7 @@ func awsDirectExchangeOutcome(exchangeID string, quote *exchange.ExchangeQuoteSu // azureDirectExchangeAudit builds the audit row for a direct Azure execute // from the request and the fresh re-quote that cleared the guardrails. -func azureDirectExchangeAudit(session *Session, accountID string, body AzureExecuteExchangeRequestBody, maxRat *big.Rat, preview *azurecompute.ExchangePreview) *config.RIExchangeDirectExecution { +func azureDirectExchangeAudit(session *Session, accountID string, body AzureExecuteExchangeRequestBody, maxRat *big.Rat, preview *azureExchangePreview) *config.RIExchangeDirectExecution { sourceIDs := make([]string, 0, len(body.Sources)) for _, s := range body.Sources { sourceIDs = append(sourceIDs, s.ReservationID) @@ -223,7 +222,7 @@ func azureDirectExchangeAudit(session *Session, accountID string, body AzureExec } // azureDirectExchangeOutcome builds the outcome of a direct Azure execute. -func azureDirectExchangeOutcome(result *azurecompute.ExchangeResult, providerErr, mapped error) config.RIExchangeDirectOutcome { +func azureDirectExchangeOutcome(result *azureExchangeResult, providerErr, mapped error) config.RIExchangeDirectOutcome { if providerErr != nil { return config.RIExchangeDirectOutcome{ Status: directExchangeFailureStatus(mapped), diff --git a/internal/api/ri_exchange_refusals.go b/internal/api/ri_exchange_refusals.go index ef19f4fee..537802c17 100644 --- a/internal/api/ri_exchange_refusals.go +++ b/internal/api/ri_exchange_refusals.go @@ -4,7 +4,6 @@ import ( "errors" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" - azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" ) // exchangeRefusals are the exported sentinels cloud-commitments-go wraps when @@ -14,15 +13,16 @@ import ( // currency), so the USD max_payment_due_usd cap cannot be enforced // against it. Returned by ExecuteExchange before // AcceptReservedInstancesExchangeQuote. -// - azurecompute.ErrUnsupportedAppliedScope: a source reservation's applied -// scope cannot be carried onto the purchased reservation, or the sources -// mix scopes. Returned by CalculateExchange before any Azure call. +// - errAzureUnsupportedAppliedScope (azurecompute.ErrUnsupportedAppliedScope, +// aliased in provider_wiring.go): a source reservation's applied scope +// cannot be carried onto the purchased reservation, or the sources mix +// scopes. Returned by CalculateExchange before any Azure call. // // Both describe the request or the account it targets, not a server fault, // and both prove the exchange did not happen. var exchangeRefusals = []error{ exchange.ErrQuoteNotUSD, - azurecompute.ErrUnsupportedAppliedScope, + errAzureUnsupportedAppliedScope, } // exchangeRefusalError returns a 422 carrying err's message when err wraps diff --git a/internal/api/ri_utilization_cache.go b/internal/api/ri_utilization_cache.go index 616811890..7825a79b8 100644 --- a/internal/api/ri_utilization_cache.go +++ b/internal/api/ri_utilization_cache.go @@ -8,7 +8,6 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" - "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "golang.org/x/sync/singleflight" ) @@ -59,7 +58,7 @@ type riUtilizationCacheStore interface { // riUtilizationFetcher is the signature of the function that retrieves // fresh data from Cost Explorer. Parameterising lets tests inject a // fake. -type riUtilizationFetcher func(ctx context.Context, lookbackDays int) ([]recommendations.RIUtilization, error) +type riUtilizationFetcher func(ctx context.Context, lookbackDays int) ([]riUtilization, error) // riUtilizationCache wraps a Postgres-backed store with stale-while- // revalidate semantics on non-Lambda runtimes. Lambda containers can't @@ -97,7 +96,7 @@ func (c *riUtilizationCache) getOrFetch( lookbackDays int, softTTL, hardTTL time.Duration, fetch riUtilizationFetcher, -) ([]recommendations.RIUtilization, error) { +) ([]riUtilization, error) { key := fmt.Sprintf("%s#%d", region, lookbackDays) entry, err := c.store.GetRIUtilizationCache(ctx, region, lookbackDays) @@ -175,7 +174,7 @@ func (c *riUtilizationCache) kickBackgroundRefresh(key, region string, lookbackD // storePayload marshals and upserts the fresh data. Errors are logged // but not returned — callers have the data in hand either way. -func (c *riUtilizationCache) storePayload(ctx context.Context, region string, lookbackDays int, data []recommendations.RIUtilization) { +func (c *riUtilizationCache) storePayload(ctx context.Context, region string, lookbackDays int, data []riUtilization) { payload, err := json.Marshal(data) if err != nil { logging.Warnf("ri_utilization_cache: failed to marshal payload for region=%q lookback=%d: %v", region, lookbackDays, err) @@ -186,8 +185,8 @@ func (c *riUtilizationCache) storePayload(ctx context.Context, region string, lo } } -func unmarshalRIUtilization(payload []byte) ([]recommendations.RIUtilization, error) { - var data []recommendations.RIUtilization +func unmarshalRIUtilization(payload []byte) ([]riUtilization, error) { + var data []riUtilization if err := json.Unmarshal(payload, &data); err != nil { return nil, err }