diff --git a/core/deleter/service.go b/core/deleter/service.go index a2ccf2965..db110a105 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -616,7 +616,7 @@ func (d Service) ensureDeletable(ctx context.Context, id string, customers []cus paidPlans := map[string]bool{} // the delete is about to happen, so judge invoices on fresh provider data - blockers, err := d.collectBlockers(ctx, customers, true, balances, paidPlans) + blockers, err := d.collectBlockers(ctx, id, customers, true, balances, paidPlans) if err != nil { return err } @@ -707,16 +707,17 @@ func (d Service) CheckOrganizationDelete(ctx context.Context, id string) ([]Bloc if err != nil { return nil, err } - return d.collectBlockers(ctx, customers, false, nil, map[string]bool{}) + return d.collectBlockers(ctx, id, customers, false, nil, map[string]bool{}) } // collectBlockers gathers the blockers across the org's billing accounts: // running subscriptions on a paid plan, invoices that still ask for money, -// and token debt. fromProvider judges the invoices on data read straight +// and token debt. It also adds the resources the deletion config says block +// an org delete. fromProvider judges the invoices on data read straight // from the billing provider instead of the local rows. balances carries // account balances the caller already read, nil means read them here; the // plan lookups land in the caller's paidPlans cache. -func (d Service) collectBlockers(ctx context.Context, customers []customer.Customer, fromProvider bool, balances map[string]int64, paidPlans map[string]bool) ([]Blocker, error) { +func (d Service) collectBlockers(ctx context.Context, orgID string, customers []customer.Customer, fromProvider bool, balances map[string]int64, paidPlans map[string]bool) ([]Blocker, error) { var blockers []Blocker for _, c := range customers { if !c.IsOffline() { @@ -756,7 +757,12 @@ func (d Service) collectBlockers(ctx context.Context, customers []customer.Custo }) } } - return blockers, nil + + resourceBlockers, err := d.OrganizationResourceBlockers(ctx, orgID) + if err != nil { + return nil, err + } + return append(blockers, resourceBlockers...), nil } // subscriptionBlockers returns a blocker for every running subscription on a diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index ad3f74aac..bbcec8b35 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -507,6 +507,8 @@ func TestDeleteOrganization(t *testing.T) { Return([]customer.Customer{}, nil) m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{{ID: "proj-1", Name: "p1"}}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}). + Return([]resource.Resource{{ID: "m1", NamespaceID: "compute/machine", Name: "machine-1"}}, nil) m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{}, nil) m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}). @@ -695,6 +697,57 @@ func TestDeleteOrganization(t *testing.T) { }, types) }) + t.Run("resources that block an org delete stop it before the subscription is canceled", func(t *testing.T) { + m := newMocks(t) + m.deletion = blockerConfig() + + c := customer.Customer{ID: "cust-1", ProviderID: "stripe-1"} + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1"). + Return(organization.Organization{ID: "org-1"}, nil) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). + Return([]customer.Customer{c}, nil) + m.subSvc.EXPECT().List(mock.Anything, subscription.Filter{CustomerID: "cust-1"}). + Return([]subscription.Subscription{{ID: "sub-1", State: "active", PlanID: "plan-free"}}, nil) + m.invocSvc.EXPECT().ListPayableOnProvider(mock.Anything, c). + Return([]invoice.Invoice{}, nil) + m.creditSvc.EXPECT().GetBalance(mock.Anything, "cust-1").Return(0, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}). + Return([]resource.Resource{{ID: "t1", NamespaceID: "storage/transfer"}}, nil) + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). + Return([]project.Project{{ID: "proj-1"}}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}). + Return([]resource.Resource{ + {ID: "m1", NamespaceID: "compute/machine"}, + {ID: "o1", NamespaceID: "compute/order"}, + }, nil) + + err := m.build().DeleteOrganization(context.Background(), "org-1") + + var blocked *deleter.BlockedError + assert.ErrorAs(t, err, &blocked) + assert.Equal(t, "org-1", blocked.OrgID) + assert.Equal(t, []deleter.Blocker{ + resourcesExist("compute/order", 1), + resourcesExist("storage/transfer", 1), + }, blocked.Blockers) + assert.EqualError(t, err, "organization cannot be deleted yet") + }) + + t.Run("a failed resource check stops the delete before anything changes", func(t *testing.T) { + m := newMocks(t) + m.deletion = blockerConfig() + + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1"). + Return(organization.Organization{ID: "org-1"}, nil) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). + Return([]customer.Customer{}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}). + Return(nil, errors.New("db error")) + + err := m.build().DeleteOrganization(context.Background(), "org-1") + assert.ErrorContains(t, err, "failed to list resources of organization[org-1]: db error") + }) + t.Run("paid plan subscription blocks until the caller downgrades", func(t *testing.T) { m := newMocks(t) @@ -1411,6 +1464,41 @@ func TestCheckOrganizationDelete(t *testing.T) { }, subjectTypes) }) + t.Run("reports the resource blockers with their counts after the billing ones", func(t *testing.T) { + m := newMocks(t) + m.deletion = blockerConfig() + + c := customer.Customer{ID: "cust-1", ProviderID: "stripe-1"} + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1"). + Return(organization.Organization{ID: "org-1"}, nil) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). + Return([]customer.Customer{c}, nil) + m.subSvc.EXPECT().List(mock.Anything, subscription.Filter{CustomerID: "cust-1"}). + Return([]subscription.Subscription{{ID: "sub-1", State: "active", PlanID: "plan-paid"}}, nil) + m.invocSvc.EXPECT().List(mock.Anything, invoice.Filter{CustomerID: "cust-1", NonZeroOnly: true}). + Return([]invoice.Invoice{}, nil) + m.creditSvc.EXPECT().GetBalance(mock.Anything, "cust-1").Return(0, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}). + Return([]resource.Resource{{ID: "t1", NamespaceID: "storage/transfer"}}, nil) + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). + Return([]project.Project{{ID: "proj-1"}}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}). + Return([]resource.Resource{ + {ID: "o1", NamespaceID: "compute/order"}, + {ID: "o2", NamespaceID: "compute/order"}, + }, nil) + + blockers, err := m.build().CheckOrganizationDelete(context.Background(), "org-1") + assert.NoError(t, err) + if assert.Len(t, blockers, 3) { + assert.Equal(t, deleter.BlockerActiveSubscription, blockers[0].Type) + assert.Equal(t, []deleter.Blocker{ + resourcesExist("compute/order", 2), + resourcesExist("storage/transfer", 1), + }, blockers[1:]) + } + }) + t.Run("returns empty when there are no blockers", func(t *testing.T) { m := newMocks(t) diff --git a/internal/api/v1beta1connect/deleter_test.go b/internal/api/v1beta1connect/deleter_test.go index f98e88346..86daa403f 100644 --- a/internal/api/v1beta1connect/deleter_test.go +++ b/internal/api/v1beta1connect/deleter_test.go @@ -177,6 +177,7 @@ func TestHandler_DeleteOrganization(t *testing.T) { Blockers: []deleter.Blocker{ {Type: deleter.BlockerUnpaidInvoice, Subject: "inv-1", Message: "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete"}, {Type: deleter.BlockerNegativeTokenBalance, Subject: "cust-1", Message: "billing account[cust-1] owes 50 tokens: contact support to settle the balance, then retry the delete"}, + {Type: deleter.BlockerResourcesExist, Subject: "compute/order", SubjectType: deleter.SubjectResourceNamespace, Count: 2}, }, } mockDelOrg := new(mocks.CascadeDeleter) @@ -191,19 +192,24 @@ func TestHandler_DeleteOrganization(t *testing.T) { var connectErr *connect.Error assert.ErrorAs(t, err, &connectErr) assert.Equal(t, connect.CodeFailedPrecondition, connectErr.Code()) - assert.Contains(t, connectErr.Message(), "pay it") - assert.Contains(t, connectErr.Message(), "contact support") + assert.Equal(t, "organization cannot be deleted yet: "+ + "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete; "+ + "billing account[cust-1] owes 50 tokens: contact support to settle the balance, then retry the delete", + connectErr.Message()) assert.Len(t, connectErr.Details(), 1) detail, detailErr := connectErr.Details()[0].Value() assert.NoError(t, detailErr) failure, ok := detail.(*errdetails.PreconditionFailure) assert.True(t, ok) - assert.Len(t, failure.GetViolations(), 2) + assert.Len(t, failure.GetViolations(), 3) assert.Equal(t, deleter.BlockerUnpaidInvoice, failure.GetViolations()[0].GetType()) assert.Equal(t, "inv-1", failure.GetViolations()[0].GetSubject()) assert.Equal(t, deleter.BlockerNegativeTokenBalance, failure.GetViolations()[1].GetType()) assert.Equal(t, "cust-1", failure.GetViolations()[1].GetSubject()) + assert.Equal(t, deleter.BlockerResourcesExist, failure.GetViolations()[2].GetType()) + assert.Equal(t, "compute/order", failure.GetViolations()[2].GetSubject()) + assert.Empty(t, failure.GetViolations()[2].GetDescription()) }) } @@ -213,6 +219,7 @@ func TestHandler_CheckOrganizationDelete(t *testing.T) { mockDel.EXPECT().CheckOrganizationDelete(mock.Anything, "some-id").Return([]deleter.Blocker{ {Type: deleter.BlockerActiveSubscription, Subject: "sub-1", SubjectType: deleter.SubjectSubscription, Message: "subscription[sub-1] is active on a paid plan: downgrade to the standard plan, then retry the delete"}, {Type: deleter.BlockerUnpaidInvoice, Subject: "inv-1", SubjectType: deleter.SubjectInvoice, Message: "invoice[inv-1] is unpaid: pay it via its hosted payment page, then retry the delete"}, + {Type: deleter.BlockerResourcesExist, Subject: "compute/order", SubjectType: deleter.SubjectResourceNamespace, Count: 2}, }, nil) mockDep := &ConnectHandler{deleterService: mockDel} @@ -221,12 +228,16 @@ func TestHandler_CheckOrganizationDelete(t *testing.T) { })) assert.NoError(t, err) assert.False(t, resp.Msg.GetCanDelete()) - assert.Len(t, resp.Msg.GetBlockers(), 2) + assert.Len(t, resp.Msg.GetBlockers(), 3) assert.Equal(t, deleter.BlockerActiveSubscription, resp.Msg.GetBlockers()[0].GetType()) assert.Equal(t, "sub-1", resp.Msg.GetBlockers()[0].GetSubject()) assert.Equal(t, deleter.SubjectSubscription, resp.Msg.GetBlockers()[0].GetSubjectType()) assert.Equal(t, deleter.BlockerUnpaidInvoice, resp.Msg.GetBlockers()[1].GetType()) assert.Equal(t, deleter.SubjectInvoice, resp.Msg.GetBlockers()[1].GetSubjectType()) + assert.Equal(t, deleter.BlockerResourcesExist, resp.Msg.GetBlockers()[2].GetType()) + assert.Equal(t, "compute/order", resp.Msg.GetBlockers()[2].GetSubject()) + assert.Equal(t, deleter.SubjectResourceNamespace, resp.Msg.GetBlockers()[2].GetSubjectType()) + assert.Equal(t, int32(2), resp.Msg.GetBlockers()[2].GetCount()) }) t.Run("should report can_delete true when nothing blocks", func(t *testing.T) { diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index a85a1ad53..f5a3bccee 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -107,6 +107,7 @@ func (s *APIRegressionTestSuite) SetupSuite() { Enabled: true, Resources: map[string]deleter.Rules{ computeOrderNamespace: {deleter.ParentProject: deleter.RuleBlock, deleter.ParentOrganization: deleter.RuleAllow}, + computeDiskNamespace: {deleter.ParentProject: deleter.RuleBlock, deleter.ParentOrganization: deleter.RuleBlockUnlessDeletable}, computeVolumeNamespace: {deleter.ParentProject: deleter.RuleAllow, deleter.ParentOrganization: deleter.RuleAllow}, }, }, @@ -4174,6 +4175,126 @@ func (s *APIRegressionTestSuite) TestProjectDeleteRefusedWhileResourcesBlock() { }) } +func (s *APIRegressionTestSuite) TestOrganizationDeleteRefusedWhileResourcesBlock() { + ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie) + + createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{ + Title: "org delete blockers", + Name: "org-delete-blockers", + }, + })) + s.Require().NoError(err) + orgID := createOrgResp.Msg.GetOrganization().GetId() + + projectResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{Name: "org-delete-blockers-project", OrgId: orgID}, + })) + s.Require().NoError(err) + projectID := projectResp.Msg.GetProject().GetId() + + createDisk := func(name string, metadata *structpb.Struct) string { + resp, err := s.testBench.Client.CreateProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectResourceRequest{ + ProjectId: projectID, + Body: &frontierv1beta1.ResourceRequestBody{Name: name, Namespace: computeDiskNamespace, Metadata: metadata}, + })) + s.Require().NoError(err) + return resp.Msg.GetResource().GetId() + } + blockingDiskID := createDisk("disk-in-use", nil) + createDisk("disk-deletable", &structpb.Struct{Fields: map[string]*structpb.Value{ + "deletable": structpb.NewBoolValue(true), + }}) + + orgDiskResp, err := s.testBench.Client.CreateOrganizationResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationResourceRequest{ + OrgId: orgID, + Body: &frontierv1beta1.ResourceRequestBody{Name: "org-disk-in-use", Namespace: computeDiskNamespace}, + })) + s.Require().NoError(err) + orgDiskID := orgDiskResp.Msg.GetResource().GetId() + + deleteOrg := func() error { + _, err := s.testBench.Client.DeleteOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRequest{Id: orgID})) + return err + } + checkOrg := func() *frontierv1beta1.CheckOrganizationDeleteResponse { + resp, err := s.testBench.Client.CheckOrganizationDelete(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CheckOrganizationDeleteRequest{Id: orgID})) + s.Require().NoError(err) + return resp.Msg + } + + s.Run("the check counts the project's and the org's own resources not marked deletable", func() { + resp := checkOrg() + s.Assert().False(resp.GetCanDelete()) + s.Require().Len(resp.GetBlockers(), 1) + s.Assert().Equal(deleter.BlockerResourcesExist, resp.GetBlockers()[0].GetType()) + s.Assert().Equal(computeDiskNamespace, resp.GetBlockers()[0].GetSubject()) + s.Assert().Equal(deleter.SubjectResourceNamespace, resp.GetBlockers()[0].GetSubjectType()) + s.Assert().Equal(int32(2), resp.GetBlockers()[0].GetCount()) + }) + + s.Run("the org delete is refused with the blocker and nothing is removed", func() { + err := deleteOrg() + var connectErr *connect.Error + s.Require().ErrorAs(err, &connectErr) + s.Assert().Equal(connect.CodeFailedPrecondition, connectErr.Code()) + s.Assert().Equal("organization cannot be deleted yet", connectErr.Message()) + + s.Require().Len(connectErr.Details(), 1) + detail, err := connectErr.Details()[0].Value() + s.Require().NoError(err) + failure, ok := detail.(*errdetails.PreconditionFailure) + s.Require().True(ok) + s.Require().Len(failure.GetViolations(), 1) + s.Assert().Equal(deleter.BlockerResourcesExist, failure.GetViolations()[0].GetType()) + s.Assert().Equal(computeDiskNamespace, failure.GetViolations()[0].GetSubject()) + + _, err = s.testBench.Client.GetOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetOrganizationRequest{Id: orgID})) + s.Assert().NoError(err) + _, err = s.testBench.Client.GetProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetProjectRequest{Id: projectID})) + s.Assert().NoError(err) + _, err = s.testBench.Client.GetProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetProjectResourceRequest{ + ProjectId: projectID, + Id: blockingDiskID, + })) + s.Assert().NoError(err) + + var liveOrgDisks int + s.Require().NoError(s.dbc.GetContext(context.Background(), &liveOrgDisks, + "SELECT count(*) FROM resources WHERE id = $1 AND deleted_at IS NULL", orgDiskID)) + s.Assert().Equal(1, liveOrgDisks) + }) + + s.Run("the org's own resource still blocks once the project's blocking resource is gone", func() { + _, err := s.testBench.Client.DeleteProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectResourceRequest{ + ProjectId: projectID, + Id: blockingDiskID, + })) + s.Require().NoError(err) + + resp := checkOrg() + s.Assert().False(resp.GetCanDelete()) + s.Require().Len(resp.GetBlockers(), 1) + s.Assert().Equal(computeDiskNamespace, resp.GetBlockers()[0].GetSubject()) + s.Assert().Equal(int32(1), resp.GetBlockers()[0].GetCount()) + s.Assert().Equal(connect.CodeFailedPrecondition, connect.CodeOf(deleteOrg())) + }) + + s.Run("the org can be deleted once no blocking resource is left, while the project rule still refuses", func() { + _, err := s.testBench.Client.DeleteOrganizationResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationResourceRequest{ + Id: orgDiskID, + OrgId: orgID, + })) + s.Require().NoError(err) + + _, err = s.testBench.Client.DeleteProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{Id: projectID})) + s.Assert().Equal(connect.CodeFailedPrecondition, connect.CodeOf(err)) + + s.Assert().True(checkOrg().GetCanDelete()) + s.Assert().NoError(deleteOrg()) + }) +} + func TestEndToEndAPIRegressionTestSuite(t *testing.T) { suite.Run(t, new(APIRegressionTestSuite)) }