diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index c3b3b600..3d5bc6cf 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -290,7 +290,26 @@ func convertSavingsPlan(sp *armbillingbenefits.SavingsPlanModel, subscriptionID } } - return &commitment, nil + return requireStartDate(&commitment) +} + +// requireStartDate returns c, or fails closed on a non-terminal plan with no start date. +func requireStartDate(c *common.Commitment) (*common.Commitment, error) { + if c.StartDate.IsZero() && !isTerminalSavingsPlanState(c.State) { + return nil, fmt.Errorf("owned savings plan %s is in state %q but reports no effectiveDateTime; the duplicate guard cannot date it", c.CommitmentID, c.State) + } + return c, nil +} + +// isTerminalSavingsPlanState reports the states recfilter rejects before it +// reads StartDate, so a zero start is harmless. Anything else, including an +// unknown or defaulted state, is treated as live. +func isTerminalSavingsPlanState(s common.CommitmentState) bool { + switch s { + case common.CommitmentStateFailed, common.CommitmentStateCanceled, common.CommitmentStateExpired: + return true + } + return false } var currencyCodePattern = regexp.MustCompile(`^[A-Za-z]{3}$`) diff --git a/providers/azure/services/savingsplans/client_completeness_sdk_test.go b/providers/azure/services/savingsplans/client_completeness_sdk_test.go index 6cd32095..95a8c1c0 100644 --- a/providers/azure/services/savingsplans/client_completeness_sdk_test.go +++ b/providers/azure/services/savingsplans/client_completeness_sdk_test.go @@ -122,3 +122,57 @@ func newFixtureSavingsPlanClient(t *testing.T, server *httptest.Server) *armbill require.NoError(t, err) return sdk } + +func TestGetExistingCommitments_EffectiveDateTimeCompleteness(t *testing.T) { + const planID = "/providers/Microsoft.BillingBenefits/savingsPlanOrders/o1/savingsPlans/p1" + cases := []struct { + name string + props string + wantErr bool + }{ + {"live state without start", `"provisioningState":"Succeeded"`, true}, + {"pending state without start", `"provisioningState":"PendingBilling"`, true}, + {"no state without start", ``, true}, + {"canceled-state without start", `"provisioningState":"Cancelled"`, false}, //nolint:misspell // Azure wire value + {"expired without start", `"provisioningState":"Expired"`, false}, + {"failed without start", `"provisioningState":"Failed"`, false}, + {"live state with start", `"provisioningState":"Succeeded","effectiveDateTime":"2026-01-02T03:04:05Z"`, false}, + } + for _, tc := range cases { + for _, dedupe := range []bool{false, true} { + if dedupe && !tc.wantErr { + continue + } + t.Run(fmt.Sprintf("%s/dedupe=%t", tc.name, dedupe), func(t *testing.T) { + props := `"billingScopeId":"/subscriptions/sub-a"` + if tc.props != "" { + props += "," + tc.props + } + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, err := fmt.Fprintf(w, `{"value":[{"id":%q,"sku":{"name":"Compute_Savings_Plan"},"properties":{%s}}]}`, planID, props) + assert.NoError(t, err) + })) + t.Cleanup(server.Close) + client := NewClient(nil, "sub-a", "eastus") + client.SetListAllPager(newFixtureSavingsPlanClient(t, server).NewListAllPager(nil)) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if dedupe { + recs := []common.Recommendation{{Provider: common.ProviderAzure, Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", Count: 1}} + _, _, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(ctx, recs, client) + require.ErrorContains(t, err, planID) + return + } + got, err := client.GetExistingCommitments(ctx) + if tc.wantErr { + require.ErrorContains(t, err, "incomplete savings plan inventory") + require.ErrorContains(t, err, planID) + return + } + require.NoError(t, err) + require.Len(t, got, 1) + }) + } + } +} diff --git a/providers/azure/services/savingsplans/client_pager_cap_test.go b/providers/azure/services/savingsplans/client_pager_cap_test.go index 4f6e1990..c5eeccba 100644 --- a/providers/azure/services/savingsplans/client_pager_cap_test.go +++ b/providers/azure/services/savingsplans/client_pager_cap_test.go @@ -3,6 +3,7 @@ package savingsplans import ( "context" "testing" + "time" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits" "github.com/stretchr/testify/assert" @@ -34,7 +35,7 @@ func ownedPlan(id string) *armbillingbenefits.SavingsPlanModel { return &armbillingbenefits.SavingsPlanModel{ ID: &id, SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, - Properties: &armbillingbenefits.SavingsPlanModelProperties{BillingScopeID: toPtr("/subscriptions/sub-abc")}, + Properties: &armbillingbenefits.SavingsPlanModelProperties{BillingScopeID: toPtr("/subscriptions/sub-abc"), EffectiveDateTime: toPtr(time.Now())}, } } diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index 5b46565d..3663d5ce 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -192,6 +192,7 @@ func TestGetExistingCommitments_SubscriptionOwnership(t *testing.T) { SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, Properties: &armbillingbenefits.SavingsPlanModelProperties{ BillingScopeID: toPtr(scope), + EffectiveDateTime: toPtr(time.Now()), AppliedScopeType: toPtr(applied), AppliedScopeProperties: &armbillingbenefits.AppliedScopeProperties{SubscriptionID: toPtr(subscription)}, }, diff --git a/providers/azure/services/savingsplans/scope_test.go b/providers/azure/services/savingsplans/scope_test.go index d0b0c016..6409ec67 100644 --- a/providers/azure/services/savingsplans/scope_test.go +++ b/providers/azure/services/savingsplans/scope_test.go @@ -25,7 +25,7 @@ const ( ) func accountPlan(scope string, typ *armbillingbenefits.AppliedScopeType, props *armbillingbenefits.AppliedScopeProperties) *armbillingbenefits.SavingsPlanModel { - return &armbillingbenefits.SavingsPlanModel{ID: toPtr("plan-id"), SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, Properties: &armbillingbenefits.SavingsPlanModelProperties{BillingScopeID: toPtr(scope), AppliedScopeType: typ, AppliedScopeProperties: props}} + return &armbillingbenefits.SavingsPlanModel{ID: toPtr("plan-id"), SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, Properties: &armbillingbenefits.SavingsPlanModelProperties{BillingScopeID: toPtr(scope), EffectiveDateTime: toPtr(time.Now()), AppliedScopeType: typ, AppliedScopeProperties: props}} } // scoped builds applied-scope properties; "" leaves the field nil.