fix(aws/savingsplans): purchase EC2Instance SP for the recommended family/region, fail loud on ambiguity (adversarial-review C1)

pkg/common/types.go

@@ -552,6 +552,24 @@ type SavingsPlanDetails struct {
 	PlanType         string  `json:"plan_type"` // Compute, EC2Instance, SageMaker
 	HourlyCommitment float64 `json:"hourly_commitment"`
 	Coverage         string  `json:"coverage,omitempty"`
+	// InstanceFamily is the EC2 instance family recommended by Cost Explorer
+	// (e.g. "m5"). Only populated for EC2Instance Savings Plans; empty for
+	// Compute, SageMaker and Database plans which are family-agnostic. Used at
+	// purchase time to add an instanceFamily filter to DescribeSavingsPlansOfferings
+	// so the offering resolves to the correct family rather than the
+	// lexicographically-smallest across all families in the region.
+	InstanceFamily string `json:"instance_family,omitempty"`
+	// Region is the AWS region recommended by Cost Explorer for this EC2Instance
+	// Savings Plan (e.g. "us-east-1"). Only populated for EC2Instance plans;
+	// Compute, SageMaker and Database plans are global and carry no region.
+	// Used at purchase time as the region filter for DescribeSavingsPlansOfferings
+	// instead of the client's configured region, which may differ.
+	Region string `json:"region,omitempty"`
+	// OfferingID is the exact Savings Plans offering ID returned by Cost Explorer
+	// in SavingsPlansDetails.OfferingId. When non-empty the purchase path uses it
+	// directly and skips DescribeSavingsPlansOfferings entirely, which is the
+	// safest possible resolution. Only populated when CE provides it.
+	OfferingID string `json:"offering_id,omitempty"`
 }
 
 func (d SavingsPlanDetails) GetServiceType() ServiceType {

providers/aws/recommendations/parser_sp.go

@@ -194,6 +194,30 @@ func parseOptionalFloat(field string, s *string) float64 {
 // hoursPerMonth is the standard AWS billing constant for monthly cost calculations.
 const hoursPerMonth = 730.0
 
+// ec2SPFields holds the Cost Explorer SavingsPlansDetails fields that are
+// specific to EC2Instance Savings Plans. Extracted from parseSavingsPlanDetail
+// to keep that function's cyclomatic complexity under the gocyclo-10 threshold.
+type ec2SPFields struct {
+	instanceFamily string
+	region         string
+	offeringID     string
+}
+
+// extractEC2SPFields reads InstanceFamily, Region, and OfferingId from the
+// CE SavingsPlansDetails nested struct for EC2Instance plan recommendations.
+// Returns a zero-value struct (all empty strings) for non-EC2Instance plan
+// types and when SavingsPlansDetails is nil, so callers do not need a nil guard.
+func extractEC2SPFields(planType types.SupportedSavingsPlansType, detail *types.SavingsPlansPurchaseRecommendationDetail) ec2SPFields {
+	if planType != types.SupportedSavingsPlansTypeEc2InstanceSp || detail.SavingsPlansDetails == nil {
+		return ec2SPFields{}
+	}
+	return ec2SPFields{
+		instanceFamily: aws.ToString(detail.SavingsPlansDetails.InstanceFamily),
+		region:         aws.ToString(detail.SavingsPlansDetails.Region),
+		offeringID:     aws.ToString(detail.SavingsPlansDetails.OfferingId),
+	}
+}
+
 func (c *Client) parseSavingsPlanDetail(
 	detail *types.SavingsPlansPurchaseRecommendationDetail,
 	params common.RecommendationParams,
@@ -244,6 +268,10 @@ func (c *Client) parseSavingsPlanDetail(
 		accountID = aws.ToString(detail.AccountId)
 	}
 
+	// Extract EC2Instance-specific routing data from CE's SavingsPlansDetails.
+	// See extractEC2SPFields for the extraction logic and field semantics.
+	ec2Fields := extractEC2SPFields(planType, detail)
+
 	// RecurringMonthlyCost is the portion of the SP commitment that appears
 	// on monthly bills (i.e. excludes upfront payments).
 	//
@@ -286,6 +314,9 @@ func (c *Client) parseSavingsPlanDetail(
 			PlanType:         planTypeStr,
 			HourlyCommitment: hourlyCommitment,
 			Coverage:         fmt.Sprintf("%.1f%%", savingsPercent),
+			InstanceFamily:   ec2Fields.instanceFamily,
+			Region:           ec2Fields.region,
+			OfferingID:       ec2Fields.offeringID,
 		},
 	}
 }

providers/aws/recommendations/parser_sp_test.go

@@ -203,3 +203,108 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) {
 		})
 	}
 }
+
+// TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured is the C1 parser
+// regression test. It asserts that parseSavingsPlanDetail persists InstanceFamily,
+// Region, and OfferingID from CE's SavingsPlansDetails onto the recommendation so
+// the purchase path can use them. Pre-fix: the field was ignored; assertions on
+// InstanceFamily/Region/OfferingID would fail. Post-fix: the fields are captured.
+func TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured(t *testing.T) {
+	client := &Client{}
+	params := common.RecommendationParams{
+		Service:       common.ServiceSavingsPlansEC2Instance,
+		PaymentOption: "no-upfront",
+		Term:          "1yr",
+	}
+
+	tests := []struct {
+		name           string
+		detail         *types.SavingsPlansPurchaseRecommendationDetail
+		planType       types.SupportedSavingsPlansType
+		wantFamily     string
+		wantRegion     string
+		wantOfferingID string
+	}{
+		{
+			name: "EC2Instance SP with all CE fields populated",
+			detail: &types.SavingsPlansPurchaseRecommendationDetail{
+				HourlyCommitmentToPurchase:    aws.String("0.50"),
+				EstimatedMonthlySavingsAmount: aws.String("30.00"),
+				EstimatedSavingsPercentage:    aws.String("20.0"),
+				SavingsPlansDetails: &types.SavingsPlansDetails{
+					InstanceFamily: aws.String("m5"),
+					Region:         aws.String("us-east-1"),
+					OfferingId:     aws.String("ce-offering-abc123"),
+				},
+			},
+			planType:       types.SupportedSavingsPlansTypeEc2InstanceSp,
+			wantFamily:     "m5",
+			wantRegion:     "us-east-1",
+			wantOfferingID: "ce-offering-abc123",
+		},
+		{
+			name: "EC2Instance SP with partial CE fields (OfferingId absent)",
+			detail: &types.SavingsPlansPurchaseRecommendationDetail{
+				HourlyCommitmentToPurchase:    aws.String("1.00"),
+				EstimatedMonthlySavingsAmount: aws.String("50.00"),
+				EstimatedSavingsPercentage:    aws.String("15.0"),
+				SavingsPlansDetails: &types.SavingsPlansDetails{
+					InstanceFamily: aws.String("c5"),
+					Region:         aws.String("eu-west-1"),
+				},
+			},
+			planType:       types.SupportedSavingsPlansTypeEc2InstanceSp,
+			wantFamily:     "c5",
+			wantRegion:     "eu-west-1",
+			wantOfferingID: "",
+		},
+		{
+			name: "EC2Instance SP with nil SavingsPlansDetails (defensive case)",
+			detail: &types.SavingsPlansPurchaseRecommendationDetail{
+				HourlyCommitmentToPurchase:    aws.String("0.75"),
+				EstimatedMonthlySavingsAmount: aws.String("20.00"),
+				EstimatedSavingsPercentage:    aws.String("10.0"),
+				SavingsPlansDetails:           nil,
+			},
+			planType:       types.SupportedSavingsPlansTypeEc2InstanceSp,
+			wantFamily:     "",
+			wantRegion:     "",
+			wantOfferingID: "",
+		},
+		{
+			name: "Compute SP: SavingsPlansDetails ignored — no family/region populated",
+			detail: &types.SavingsPlansPurchaseRecommendationDetail{
+				HourlyCommitmentToPurchase:    aws.String("2.00"),
+				EstimatedMonthlySavingsAmount: aws.String("100.00"),
+				EstimatedSavingsPercentage:    aws.String("25.0"),
+				SavingsPlansDetails: &types.SavingsPlansDetails{
+					// CE may return these for Compute SPs but we must not use them
+					// to avoid imposing a family/region constraint on a global plan.
+					InstanceFamily: aws.String("m5"),
+					Region:         aws.String("us-east-1"),
+				},
+			},
+			planType:       types.SupportedSavingsPlansTypeComputeSp,
+			wantFamily:     "", // Compute is family-agnostic; must stay empty
+			wantRegion:     "", // Compute is global; must stay empty
+			wantOfferingID: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rec := client.parseSavingsPlanDetail(tt.detail, params, tt.planType)
+			require.NotNil(t, rec)
+
+			spDetails, ok := rec.Details.(*common.SavingsPlanDetails)
+			require.True(t, ok, "Details must be *common.SavingsPlanDetails")
+
+			assert.Equal(t, tt.wantFamily, spDetails.InstanceFamily,
+				"InstanceFamily must be captured from CE SavingsPlansDetails for EC2Instance SPs")
+			assert.Equal(t, tt.wantRegion, spDetails.Region,
+				"Region must be captured from CE SavingsPlansDetails for EC2Instance SPs")
+			assert.Equal(t, tt.wantOfferingID, spDetails.OfferingID,
+				"OfferingID must be captured from CE SavingsPlansDetails when present")
+		})
+	}
+}