diff --git a/core/deleter/deleter.go b/core/deleter/deleter.go index 824b9b82e..62f895fdc 100644 --- a/core/deleter/deleter.go +++ b/core/deleter/deleter.go @@ -41,14 +41,24 @@ type Blocker struct { // caller gets one checklist instead of discovering blockers one retry at // a time. type BlockedError struct { - OrgID string - Blockers []Blocker + OrgID string + ProjectID string + Blockers []Blocker } func (e *BlockedError) Error() string { - msgs := make([]string, 0, len(e.Blockers)) + target := "organization" + if e.ProjectID != "" { + target = "project" + } + var msgs []string for _, b := range e.Blockers { - msgs = append(msgs, b.Message) + if b.Message != "" { + msgs = append(msgs, b.Message) + } + } + if len(msgs) == 0 { + return target + " cannot be deleted yet" } - return "organization cannot be deleted yet: " + strings.Join(msgs, "; ") + return target + " cannot be deleted yet: " + strings.Join(msgs, "; ") } diff --git a/core/deleter/service.go b/core/deleter/service.go index 80f9c346d..dfef0a450 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -44,6 +44,7 @@ import ( "github.com/raystack/frontier/core/serviceuser" "github.com/raystack/frontier/core/user" "github.com/raystack/frontier/pkg/mailer" + "github.com/raystack/frontier/pkg/utils" "github.com/raystack/salt/rql" ) @@ -219,22 +220,43 @@ func NewCascadeDeleter(orgService OrganizationService, projService ProjectServic } func (d Service) DeleteProject(ctx context.Context, id string) error { + if !utils.IsValidUUID(id) { + return project.ErrInvalidUUID + } + projects, err := d.projService.List(ctx, project.Filter{ProjectIDs: []string{id}, IncludeDisabled: true}) + if err != nil { + return err + } + if len(projects) == 0 { + return nil + } + blockers, err := d.ProjectResourceBlockers(ctx, id) + if err != nil { + return err + } + if len(blockers) > 0 { + return &BlockedError{ProjectID: id, Blockers: blockers} + } + return d.deleteProject(ctx, projects[0]) +} + +func (d Service) deleteProject(ctx context.Context, prj project.Project) error { // delete all project-level policies (and their rolebinding relations) policies, err := d.policyService.List(ctx, policy.Filter{ - ProjectID: id, + ProjectID: prj.ID, }) if err != nil { return err } for _, p := range policies { - if err = d.policyService.Delete(ctx, p.ID); err != nil { + if err = d.policyService.Delete(ctx, p.ID); err != nil && !errors.Is(err, policy.ErrNotExist) { return fmt.Errorf("failed to delete project while deleting a policy[%s]: %w", p.ID, err) } } // TODO(fix): soft-delete the live resources instead of purging every row resources, err := d.resService.List(ctx, resource.Filter{ - ProjectID: id, + ProjectID: prj.ID, IncludeDeleted: true, }) if err != nil { @@ -246,7 +268,7 @@ func (d Service) DeleteProject(ctx context.Context, id string) error { } } - if err := d.projService.DeleteModel(ctx, id); err != nil && !errors.Is(err, project.ErrNotExist) { + if err := d.projService.DeleteModel(ctx, prj.ID); err != nil && !errors.Is(err, project.ErrNotExist) { return err } return nil @@ -319,7 +341,7 @@ func (d Service) DeleteOrganization(ctx context.Context, id string) error { return err } for _, p := range projects { - if err = d.DeleteProject(ctx, p.ID); err != nil { + if err = d.deleteProject(ctx, p); err != nil { return fmt.Errorf("failed to delete org while deleting a project[%s]: %w", p.Name, err) } } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 8400536cd..2887fef4e 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -124,76 +124,135 @@ func (m deleterMocks) buildWithNotice(notice billing.OrgDeleteNoticeConfig) *del } func TestDeleteProject(t *testing.T) { + projectID := uuid.NewString() + expectProject := func(m deleterMocks) { + m.projSvc.EXPECT().List(mock.Anything, project.Filter{ProjectIDs: []string{projectID}, IncludeDisabled: true}). + Return([]project.Project{{ID: projectID, Name: "p1"}}, nil) + } + t.Run("deletes policies, resources, then project model", func(t *testing.T) { m := newMocks(t) var steps []string + expectProject(m) - m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: projectID}). Return([]policy.Policy{{ID: "pol-1"}, {ID: "pol-2"}}, nil) m.polSvc.EXPECT().Delete(mock.Anything, "pol-1"). Run(func(_ context.Context, id string) { steps = append(steps, "policy:"+id) }).Return(nil) m.polSvc.EXPECT().Delete(mock.Anything, "pol-2"). Run(func(_ context.Context, id string) { steps = append(steps, "policy:"+id) }).Return(nil) - m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: projectID, IncludeDeleted: true}). Return([]resource.Resource{{ID: "res-1", NamespaceID: "ns-1", Name: "r1"}}, nil) m.resSvc.EXPECT().Purge(mock.Anything, "ns-1", "res-1"). Run(func(_ context.Context, _ string, id string) { steps = append(steps, "resource:"+id) }).Return(nil) - m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1"). + m.projSvc.EXPECT().DeleteModel(mock.Anything, projectID). Run(func(_ context.Context, id string) { steps = append(steps, "project:"+id) }).Return(nil) - err := m.build().DeleteProject(context.Background(), "proj-1") + err := m.build().DeleteProject(context.Background(), projectID) assert.NoError(t, err) - assert.Equal(t, []string{"policy:pol-1", "policy:pol-2", "resource:res-1", "project:proj-1"}, steps) + assert.Equal(t, []string{"policy:pol-1", "policy:pol-2", "resource:res-1", "project:" + projectID}, steps) }) t.Run("a project that is already gone counts as deleted", func(t *testing.T) { m := newMocks(t) + expectProject(m) - m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: projectID}). Return([]policy.Policy{}, nil) - m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: projectID, IncludeDeleted: true}). Return([]resource.Resource{}, nil) - m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(project.ErrNotExist) + m.projSvc.EXPECT().DeleteModel(mock.Anything, projectID).Return(project.ErrNotExist) - err := m.build().DeleteProject(context.Background(), "proj-1") + err := m.build().DeleteProject(context.Background(), projectID) assert.NoError(t, err) }) t.Run("returns error when policy list fails", func(t *testing.T) { m := newMocks(t) + expectProject(m) - m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: projectID}). Return(nil, errors.New("db error")) - err := m.build().DeleteProject(context.Background(), "proj-1") + err := m.build().DeleteProject(context.Background(), projectID) assert.ErrorContains(t, err, "db error") }) t.Run("returns error when policy delete fails", func(t *testing.T) { m := newMocks(t) + expectProject(m) - m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: projectID}). Return([]policy.Policy{{ID: "pol-fail"}}, nil) m.polSvc.EXPECT().Delete(mock.Anything, "pol-fail").Return(errors.New("delete error")) - err := m.build().DeleteProject(context.Background(), "proj-1") + err := m.build().DeleteProject(context.Background(), projectID) assert.ErrorContains(t, err, "pol-fail") }) + t.Run("a policy that is already gone does not stop the delete", func(t *testing.T) { + m := newMocks(t) + expectProject(m) + + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: projectID}). + Return([]policy.Policy{{ID: "pol-gone"}}, nil) + m.polSvc.EXPECT().Delete(mock.Anything, "pol-gone").Return(policy.ErrNotExist) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: projectID, IncludeDeleted: true}). + Return([]resource.Resource{}, nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, projectID).Return(nil) + + err := m.build().DeleteProject(context.Background(), projectID) + assert.NoError(t, err) + }) + t.Run("no policies — still deletes resources and project", func(t *testing.T) { m := newMocks(t) + expectProject(m) - m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: projectID}). Return([]policy.Policy{}, nil) - m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: projectID, IncludeDeleted: true}). Return([]resource.Resource{}, nil) - m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, projectID).Return(nil) - err := m.build().DeleteProject(context.Background(), "proj-1") + err := m.build().DeleteProject(context.Background(), projectID) assert.NoError(t, err) }) + + t.Run("refuses while resources block and removes nothing", func(t *testing.T) { + m := newMocks(t) + m.deletion = blockerConfig() + expectProject(m) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: projectID}). + Return([]resource.Resource{{ID: "m1", NamespaceID: "compute/machine"}}, nil) + + err := m.build().DeleteProject(context.Background(), projectID) + + var blocked *deleter.BlockedError + if assert.ErrorAs(t, err, &blocked) { + assert.Equal(t, projectID, blocked.ProjectID) + assert.Equal(t, []deleter.Blocker{resourcesExist("compute/machine", 1)}, blocked.Blockers) + } + assert.EqualError(t, err, "project cannot be deleted yet") + }) + + t.Run("succeeds and removes nothing when no live project has the id", func(t *testing.T) { + m := newMocks(t) + m.projSvc.EXPECT().List(mock.Anything, project.Filter{ProjectIDs: []string{projectID}, IncludeDisabled: true}). + Return([]project.Project{}, nil) + + err := m.build().DeleteProject(context.Background(), projectID) + assert.NoError(t, err) + }) + + t.Run("rejects an id that is not a uuid", func(t *testing.T) { + m := newMocks(t) + + err := m.build().DeleteProject(context.Background(), "p1") + assert.ErrorIs(t, err, project.ErrInvalidUUID) + }) } func expectEmptyOrgTail(m deleterMocks, steps *[]string) { @@ -369,6 +428,36 @@ func TestDeleteOrganization(t *testing.T) { }, steps) }) + t.Run("a project resource that blocks a project delete does not stop the org delete", func(t *testing.T) { + m := newBareMocks(t) + m.deletion = blockerConfig() + var steps []string + + 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.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). + Return([]project.Project{{ID: "proj-1", Name: "p1"}}, 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", IncludeDeleted: true}). + Return([]resource.Resource{{ID: "m1", NamespaceID: "compute/machine", Name: "machine-1"}}, nil) + m.resSvc.EXPECT().Purge(mock.Anything, "compute/machine", "m1").Return(nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1"). + Run(func(_ context.Context, id string) { steps = append(steps, "project:"+id) }).Return(nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}). + Return([]resource.Resource{}, nil) + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). + Return([]group.Group{}, nil) + + expectEmptyOrgTail(m, &steps) + + err := m.build().DeleteOrganization(context.Background(), "org-1") + assert.NoError(t, err) + assert.Equal(t, []string{"project:proj-1", "org"}, steps) + }) + t.Run("an org with only enabled children lists nothing extra to delete", func(t *testing.T) { m := newBareMocks(t) var steps []string diff --git a/internal/api/v1beta1connect/deleter.go b/internal/api/v1beta1connect/deleter.go index b7290da2c..c1853335d 100644 --- a/internal/api/v1beta1connect/deleter.go +++ b/internal/api/v1beta1connect/deleter.go @@ -9,12 +9,20 @@ import ( "connectrpc.com/connect" "github.com/raystack/frontier/core/deleter" "github.com/raystack/frontier/core/organization" + "github.com/raystack/frontier/core/project" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" "google.golang.org/genproto/googleapis/rpc/errdetails" ) func (h *ConnectHandler) DeleteProject(ctx context.Context, request *connect.Request[frontierv1beta1.DeleteProjectRequest]) (*connect.Response[frontierv1beta1.DeleteProjectResponse], error) { if err := h.deleterService.DeleteProject(ctx, request.Msg.GetId()); err != nil { + var blocked *deleter.BlockedError + if errors.As(err, &blocked) { + return nil, deleteBlockedError(ctx, blocked) + } + if errors.Is(err, project.ErrNotExist) || errors.Is(err, project.ErrInvalidUUID) { + return nil, connect.NewError(connect.CodeNotFound, ErrProjectNotFound) + } return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteProject.DeleteProject: project_id=%s: %w", request.Msg.GetId(), err)) } return connect.NewResponse(&frontierv1beta1.DeleteProjectResponse{}), nil @@ -73,10 +81,10 @@ func deleteBlockedError(ctx context.Context, blocked *deleter.BlockedError) *con }) } if detail, err := connect.NewErrorDetail(failure); err != nil { - slog.WarnContext(ctx, "failed to attach precondition failure details", "error", err, "org_id", blocked.OrgID) + slog.WarnContext(ctx, "failed to attach precondition failure details", "error", err, "org_id", blocked.OrgID, "project_id", blocked.ProjectID) } else { connectErr.AddDetail(detail) } - slog.WarnContext(ctx, "organization delete blocked", "org_id", blocked.OrgID, "blockers", len(blocked.Blockers)) + slog.WarnContext(ctx, "delete blocked", "org_id", blocked.OrgID, "project_id", blocked.ProjectID, "blockers", len(blocked.Blockers)) return connectErr } diff --git a/internal/api/v1beta1connect/deleter_test.go b/internal/api/v1beta1connect/deleter_test.go index ef3e60c61..bbc63293b 100644 --- a/internal/api/v1beta1connect/deleter_test.go +++ b/internal/api/v1beta1connect/deleter_test.go @@ -8,6 +8,7 @@ import ( "connectrpc.com/connect" "github.com/raystack/frontier/core/deleter" "github.com/raystack/frontier/core/organization" + "github.com/raystack/frontier/core/project" "github.com/raystack/frontier/internal/api/v1beta1connect/mocks" "github.com/raystack/frontier/pkg/errors" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" @@ -35,6 +36,28 @@ func TestHandler_DeleteProject(t *testing.T) { want: connect.NewResponse(&frontierv1beta1.DeleteProjectResponse{}), wantErr: nil, }, + { + name: "should return not found when the project is gone", + setup: func(as *mocks.CascadeDeleter) { + as.EXPECT().DeleteProject(mock.Anything, "some-id").Return(project.ErrNotExist) + }, + request: connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{ + Id: "some-id", + }), + want: nil, + wantErr: connect.NewError(connect.CodeNotFound, ErrProjectNotFound), + }, + { + name: "should return not found for an id that is not a uuid", + setup: func(as *mocks.CascadeDeleter) { + as.EXPECT().DeleteProject(mock.Anything, "some-name").Return(project.ErrInvalidUUID) + }, + request: connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{ + Id: "some-name", + }), + want: nil, + wantErr: connect.NewError(connect.CodeNotFound, ErrProjectNotFound), + }, { name: "should return error if deleter service encounters an error", setup: func(as *mocks.CascadeDeleter) { @@ -59,6 +82,38 @@ func TestHandler_DeleteProject(t *testing.T) { assert.Equal(t, tt.wantErr, err) }) } + + t.Run("should return failed precondition with one violation per namespace when resources block", func(t *testing.T) { + blocked := &deleter.BlockedError{ + ProjectID: "some-id", + Blockers: []deleter.Blocker{ + {Type: deleter.BlockerResourcesExist, Subject: "compute/order", SubjectType: deleter.SubjectResourceNamespace, Count: 2}, + }, + } + mockDeleteSrv := new(mocks.CascadeDeleter) + mockDeleteSrv.EXPECT().DeleteProject(mock.Anything, "some-id").Return(blocked) + mockDel := &ConnectHandler{deleterService: mockDeleteSrv} + + resp, err := mockDel.DeleteProject(context.Background(), connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{ + Id: "some-id", + })) + assert.Nil(t, resp) + + var connectErr *connect.Error + assert.ErrorAs(t, err, &connectErr) + assert.Equal(t, connect.CodeFailedPrecondition, connectErr.Code()) + assert.Equal(t, "project cannot be deleted yet", 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(), 1) + assert.Equal(t, deleter.BlockerResourcesExist, failure.GetViolations()[0].GetType()) + assert.Equal(t, "compute/order", failure.GetViolations()[0].GetSubject()) + assert.Empty(t, failure.GetViolations()[0].GetDescription()) + }) } func TestHandler_DeleteOrganization(t *testing.T) { diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index f8a930bb0..299e877cf 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -14,7 +14,9 @@ import ( "github.com/raystack/frontier/core/authenticate" testusers "github.com/raystack/frontier/core/authenticate/test_users" + "github.com/raystack/frontier/core/deleter" "github.com/raystack/frontier/pkg/webhook" + "google.golang.org/genproto/googleapis/rpc/errdetails" "github.com/raystack/frontier/core/organization" @@ -100,6 +102,12 @@ func (s *APIRegressionTestSuite) SetupSuite() { }, TestUsers: testusers.Config{Enabled: true, Domain: "raystack.org", OTP: testbench.TestOTP}, }, + Deletion: deleter.Config{ + Enabled: true, + Resources: map[string]deleter.Rules{ + computeOrderNamespace: {deleter.ParentProject: deleter.RuleBlock, deleter.ParentOrganization: deleter.RuleAllow}, + }, + }, }, } @@ -3945,6 +3953,74 @@ func (s *APIRegressionTestSuite) TestProjectDeleteKeepsTheRowAndRemovesAccess() }) } +func (s *APIRegressionTestSuite) TestProjectDeleteRefusedWhileResourcesBlock() { + ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie) + + createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{ + Title: "org project delete blockers", + Name: "org-project-delete-blockers", + }, + })) + s.Require().NoError(err) + orgID := createOrgResp.Msg.GetOrganization().GetId() + + createProjectWithOrder := func(name string) (string, string) { + projectResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{Name: name, OrgId: orgID}, + })) + s.Require().NoError(err) + projectID := projectResp.Msg.GetProject().GetId() + resourceResp, err := s.testBench.Client.CreateProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectResourceRequest{ + ProjectId: projectID, + Body: &frontierv1beta1.ResourceRequestBody{Name: name + "-order", Namespace: computeOrderNamespace}, + })) + s.Require().NoError(err) + return projectID, resourceResp.Msg.GetResource().GetId() + } + deleteProject := func(id string) error { + _, err := s.testBench.Client.DeleteProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{Id: id})) + return err + } + emptiedID, orderID := createProjectWithOrder("project-blockers-emptied") + keptID, _ := createProjectWithOrder("project-blockers-kept") + + s.Run("a project with a blocking resource is refused with the blocker", func() { + err := deleteProject(emptiedID) + var connectErr *connect.Error + s.Require().ErrorAs(err, &connectErr) + s.Assert().Equal(connect.CodeFailedPrecondition, connectErr.Code()) + s.Assert().Equal("project 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(computeOrderNamespace, failure.GetViolations()[0].GetSubject()) + + _, err = s.testBench.Client.GetProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetProjectRequest{Id: emptiedID})) + s.Assert().NoError(err) + }) + + s.Run("the project can be deleted once the resource is gone", func() { + _, err := s.testBench.Client.DeleteProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectResourceRequest{ + ProjectId: emptiedID, + Id: orderID, + })) + s.Require().NoError(err) + s.Assert().NoError(deleteProject(emptiedID)) + }) + + s.Run("the org can be deleted while its project still holds the resource", func() { + s.Require().Error(deleteProject(keptID)) + _, err := s.testBench.Client.DeleteOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRequest{Id: orgID})) + s.Assert().NoError(err) + }) +} + func TestEndToEndAPIRegressionTestSuite(t *testing.T) { suite.Run(t, new(APIRegressionTestSuite)) }