From 91827ed2310e5207bb726ad6a4979ac78fbcd195 Mon Sep 17 00:00:00 2001 From: Giuseppe Capizzi Date: Mon, 10 Jul 2023 12:07:30 +0000 Subject: [PATCH 1/4] Extract presenter.Job This makes the job handler more generic --- api/handlers/job.go | 81 +++++++++++++-------------------------- api/presenter/job.go | 44 +++++++++++++++++---- api/presenter/job_test.go | 41 ++++++++++++++++++-- 3 files changed, 101 insertions(+), 65 deletions(-) diff --git a/api/handlers/job.go b/api/handlers/job.go index d3e56259f..de1486577 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "net/url" - "regexp" "time" "code.cloudfoundry.org/korifi/api/authorization" @@ -53,8 +52,7 @@ func (h *Job) get(r *http.Request) (*routing.Response, error) { jobGUID := routing.URLParam(r, "guid") - jobType, resourceGUID, match := parseJobGUID(jobGUID) - + job, match := presenter.JobFromGUID(jobGUID) if !match { return nil, apierrors.LogAndReturn( log, @@ -68,67 +66,62 @@ func (h *Job) get(r *http.Request) (*routing.Response, error) { jobResponse presenter.JobResponse ) - switch jobType { + switch job.Type { case syncSpacePrefix: - jobResponse = presenter.ForManifestApplyJob(jobGUID, resourceGUID, h.serverURL) + jobResponse = presenter.ForManifestApplyJob(job, h.serverURL) case appDeletePrefix, routeDeletePrefix, domainDeletePrefix, roleDeletePrefix: - jobResponse = presenter.ForJob(jobGUID, []presenter.JobResponseError{}, presenter.StateComplete, jobType, h.serverURL) + jobResponse = presenter.ForJob(job, []presenter.JobResponseError{}, presenter.StateComplete, h.serverURL) case orgDeletePrefix, spaceDeletePrefix: - jobResponse, err = h.handleDeleteJob(r.Context(), jobType, resourceGUID) + jobResponse, err = h.handleDeleteJob(r.Context(), job) if err != nil { return nil, err } default: return nil, apierrors.LogAndReturn( log, - apierrors.NewNotFoundError(fmt.Errorf("invalid job type: %s", jobType), JobResourceType), - fmt.Sprintf("Invalid Job type: %s", jobType), + apierrors.NewNotFoundError(fmt.Errorf("invalid job type: %s", job.Type), JobResourceType), + fmt.Sprintf("Invalid Job type: %s", job.Type), ) } return routing.NewResponse(http.StatusOK).WithBody(jobResponse), nil } -func (h *Job) handleDeleteJob(ctx context.Context, jobType, resourceGUID string) (presenter.JobResponse, error) { +func (h *Job) handleDeleteJob(ctx context.Context, job presenter.Job) (presenter.JobResponse, error) { authInfo, _ := authorization.InfoFromContext(ctx) - jobGUID := jobType + presenter.JobGUIDDelimiter + resourceGUID log := logr.FromContextOrDiscard(ctx).WithName("handlers.job.get.handleDeleteJob") var ( - org repositories.OrgRecord - space repositories.SpaceRecord - err error - resourceType string - deletedAt *time.Time + org repositories.OrgRecord + space repositories.SpaceRecord + err error + deletedAt *time.Time ) for retries := 0; retries < 40; retries++ { - switch jobType { + switch job.Type { case orgDeletePrefix: - org, err = h.orgRepo.GetOrgUnfiltered(ctx, authInfo, resourceGUID) - resourceType = "Org" + org, err = h.orgRepo.GetOrgUnfiltered(ctx, authInfo, job.ResourceGUID) deletedAt = org.DeletedAt case spaceDeletePrefix: - space, err = h.spaceRepo.GetSpace(ctx, authInfo, resourceGUID) - resourceType = "Space" + space, err = h.spaceRepo.GetSpace(ctx, authInfo, job.ResourceGUID) deletedAt = space.DeletedAt } if err != nil { switch err.(type) { case apierrors.NotFoundError, apierrors.ForbiddenError: - return presenter.ForJob(jobGUID, + return presenter.ForJob(job, []presenter.JobResponseError{}, presenter.StateComplete, - jobType, h.serverURL, ), nil default: return presenter.JobResponse{}, apierrors.LogAndReturn( log, err, - "failed to fetch "+resourceType+" from Kubernetes", - resourceType+"GUID", resourceGUID, + "failed to fetch "+job.ResourceType+" from Kubernetes", + job.ResourceType+"GUID", job.ResourceGUID, ) } } @@ -137,45 +130,42 @@ func (h *Job) handleDeleteJob(ctx context.Context, jobType, resourceGUID string) break } - log.V(1).Info("Waiting for deletion timestamp", resourceType+"GUID", resourceGUID) + log.V(1).Info("Waiting for deletion timestamp", job.ResourceType+"GUID", job.ResourceGUID) time.Sleep(h.pollingInterval) } - return h.handleDeleteJobResponse(ctx, deletedAt, jobType, resourceGUID, resourceType) + return h.handleDeleteJobResponse(ctx, deletedAt, job) } -func (h *Job) handleDeleteJobResponse(ctx context.Context, deletedAt *time.Time, jobType, resourceGUID, resourceType string) (presenter.JobResponse, error) { - jobGUID := jobType + presenter.JobGUIDDelimiter + resourceGUID +func (h *Job) handleDeleteJobResponse(ctx context.Context, deletedAt *time.Time, job presenter.Job) (presenter.JobResponse, error) { log := logr.FromContextOrDiscard(ctx).WithName("handlers.job.get.handleDeleteJobResponse") if deletedAt == nil { return presenter.JobResponse{}, apierrors.LogAndReturn( log, - apierrors.NewNotFoundError(fmt.Errorf("job %q not found", jobGUID), JobResourceType), - resourceType+" not marked for deletion", - resourceType+"GUID", resourceGUID, + apierrors.NewNotFoundError(fmt.Errorf("job %q not found", job.GUID), JobResourceType), + job.ResourceType+" not marked for deletion", + job.ResourceType+"GUID", job.GUID, ) } if time.Since(*deletedAt).Seconds() < JobTimeoutDuration { return presenter.ForJob( - jobGUID, + job, []presenter.JobResponseError{}, presenter.StateProcessing, - jobType, h.serverURL, ), nil } return presenter.ForJob( - jobGUID, + job, []presenter.JobResponseError{{ Code: 10008, - Detail: fmt.Sprintf("%s deletion timed out. Check for remaining resources in the %q namespace", resourceType, resourceGUID), + Detail: fmt.Sprintf("%s deletion timed out. Check for remaining resources in the %q namespace", job.ResourceType, job.ResourceGUID), Title: "CF-UnprocessableEntity", }}, presenter.StateFailed, - jobType, h.serverURL, ), nil } @@ -189,20 +179,3 @@ func (h *Job) AuthenticatedRoutes() []routing.Route { {Method: "GET", Pattern: JobPath, Handler: h.get}, } } - -var ( - jobOperationPattern = `([a-z_\-]+\.[a-z_]+)` // (e.g. app.delete, space.apply_manifest, etc.) - resourceIdentifierPattern = `([A-Za-z0-9\-\.]+)` // (e.g. cf-space-a4cd478b-0b02-452f-8498-ce87ec5c6649, CUSTOM_ORG_ID, etc.) - jobRegexp = regexp.MustCompile(jobOperationPattern + presenter.JobGUIDDelimiter + resourceIdentifierPattern) -) - -func parseJobGUID(jobGUID string) (string, string, bool) { - // Parse the job identifier and capture the job operation and resource name for later use - matches := jobRegexp.FindStringSubmatch(jobGUID) - - if len(matches) != 3 { - return "", "", false - } else { - return matches[1], matches[2], true - } -} diff --git a/api/presenter/job.go b/api/presenter/job.go index 1c7759d23..52853a70d 100644 --- a/api/presenter/job.go +++ b/api/presenter/job.go @@ -3,6 +3,8 @@ package presenter import ( "fmt" "net/url" + "regexp" + "strings" ) const ( @@ -21,6 +23,34 @@ const ( RoleDeleteOperation = "role.delete" ) +var ( + jobOperationPattern = `(([a-z_\-]+)\.([a-z_]+))` // (e.g. app.delete, space.apply_manifest, etc.) + resourceIdentifierPattern = `([A-Za-z0-9\-\.]+)` // (e.g. cf-space-a4cd478b-0b02-452f-8498-ce87ec5c6649, CUSTOM_ORG_ID, etc.) + jobRegexp = regexp.MustCompile(jobOperationPattern + JobGUIDDelimiter + resourceIdentifierPattern) +) + +type Job struct { + GUID string + Type string + ResourceGUID string + ResourceType string +} + +func JobFromGUID(guid string) (Job, bool) { + matches := jobRegexp.FindStringSubmatch(guid) + + if len(matches) != 5 { + return Job{}, false + } else { + return Job{ + GUID: guid, + Type: matches[1], + ResourceType: strings.Title(matches[2]), + ResourceGUID: matches[4], + }, true + } +} + type JobResponseError struct { Detail string `json:"detail"` Title string `json:"title"` @@ -43,26 +73,26 @@ type JobLinks struct { Space *Link `json:"space,omitempty"` } -func ForManifestApplyJob(jobGUID string, spaceGUID string, baseURL url.URL) JobResponse { - response := ForJob(jobGUID, []JobResponseError{}, StateComplete, SpaceApplyManifestOperation, baseURL) +func ForManifestApplyJob(job Job, baseURL url.URL) JobResponse { + response := ForJob(job, []JobResponseError{}, StateComplete, baseURL) response.Links.Space = &Link{ - HRef: buildURL(baseURL).appendPath("/v3/spaces", spaceGUID).build(), + HRef: buildURL(baseURL).appendPath("/v3/spaces", job.ResourceGUID).build(), } return response } -func ForJob(jobGUID string, errors []JobResponseError, state string, operation string, baseURL url.URL) JobResponse { +func ForJob(job Job, errors []JobResponseError, state string, baseURL url.URL) JobResponse { return JobResponse{ - GUID: jobGUID, + GUID: job.GUID, Errors: errors, Warnings: nil, - Operation: operation, + Operation: job.Type, State: state, CreatedAt: "", UpdatedAt: "", Links: JobLinks{ Self: Link{ - HRef: buildURL(baseURL).appendPath("/v3/jobs", jobGUID).build(), + HRef: buildURL(baseURL).appendPath("/v3/jobs", job.GUID).build(), }, }, } diff --git a/api/presenter/job_test.go b/api/presenter/job_test.go index 910210d17..a2f472e14 100644 --- a/api/presenter/job_test.go +++ b/api/presenter/job_test.go @@ -21,9 +21,39 @@ var _ = Describe("", func() { Expect(err).NotTo(HaveOccurred()) }) + Describe("JobFromGUID", func() { + var ( + job presenter.Job + match bool + guid string + ) + + BeforeEach(func() { + guid = "resource.operation~guid" + }) + + JustBeforeEach(func() { + job, match = presenter.JobFromGUID(guid) + }) + + It("parses a job GUID into a Job struct", func() { + Expect(match).To(BeTrue()) + Expect(job).To(Equal(presenter.Job{ + GUID: "resource.operation~guid", + Type: "resource.operation", + ResourceGUID: "guid", + ResourceType: "Resource", + })) + }) + }) + Describe("ForManifestApplyJob", func() { JustBeforeEach(func() { - response := presenter.ForManifestApplyJob("the-job-guid", "the-space-guid", *baseURL) + response := presenter.ForManifestApplyJob(presenter.Job{ + GUID: "the-job-guid", + Type: presenter.SpaceApplyManifestOperation, + ResourceGUID: "the-space-guid", + }, *baseURL) var err error output, err = json.Marshal(response) Expect(err).NotTo(HaveOccurred()) @@ -50,13 +80,16 @@ var _ = Describe("", func() { }) }) - Describe("ForDeleteJob", func() { + Describe("ForJob", func() { JustBeforeEach(func() { - response := presenter.ForJob("the-job-guid", []presenter.JobResponseError{{ + response := presenter.ForJob(presenter.Job{ + GUID: "the-job-guid", + Type: "the.operation", + }, []presenter.JobResponseError{{ Detail: "error detail", Title: "CF-JobErrorTitle", Code: 12345, - }}, "COMPLETE", "the.operation", *baseURL) + }}, "COMPLETE", *baseURL) var err error output, err = json.Marshal(response) Expect(err).NotTo(HaveOccurred()) From 38c6e0bfa6a85631f95fe045e2aebc597ec555dd Mon Sep 17 00:00:00 2001 From: Giuseppe Capizzi Date: Mon, 10 Jul 2023 15:49:55 +0000 Subject: [PATCH 2/4] Make the jobs handler generic over the job type The handler now receives a map from job type to a `DeletionRepository`, which is all it needs to implement the deletion job logic. Supporting deletion jobs for new resources is now only a matter of implementing the `DeletionRepository` interface for the resource repository, and then add an entry to the map. --- api/handlers/fake/cforg_repository.go | 161 +++++------ api/handlers/fake/cfspace_repository.go | 84 ++++++ api/handlers/fake/deletion_repository.go | 123 ++++++++ api/handlers/job.go | 61 ++-- api/handlers/job_test.go | 333 +++++++--------------- api/handlers/org.go | 2 +- api/handlers/space.go | 2 + api/main.go | 3 +- api/presenter/job.go | 6 +- api/repositories/org_repository.go | 30 +- api/repositories/org_repository_test.go | 49 ++-- api/repositories/space_repository.go | 8 + api/repositories/space_repository_test.go | 48 ++++ 13 files changed, 528 insertions(+), 382 deletions(-) create mode 100644 api/handlers/fake/deletion_repository.go diff --git a/api/handlers/fake/cforg_repository.go b/api/handlers/fake/cforg_repository.go index 78ec5da35..bfe8013c0 100644 --- a/api/handlers/fake/cforg_repository.go +++ b/api/handlers/fake/cforg_repository.go @@ -4,6 +4,7 @@ package fake import ( "context" "sync" + "time" "code.cloudfoundry.org/korifi/api/authorization" "code.cloudfoundry.org/korifi/api/handlers" @@ -39,33 +40,33 @@ type CFOrgRepository struct { deleteOrgReturnsOnCall map[int]struct { result1 error } - GetOrgStub func(context.Context, authorization.Info, string) (repositories.OrgRecord, error) - getOrgMutex sync.RWMutex - getOrgArgsForCall []struct { + GetDeletedAtStub func(context.Context, authorization.Info, string) (*time.Time, error) + getDeletedAtMutex sync.RWMutex + getDeletedAtArgsForCall []struct { arg1 context.Context arg2 authorization.Info arg3 string } - getOrgReturns struct { - result1 repositories.OrgRecord + getDeletedAtReturns struct { + result1 *time.Time result2 error } - getOrgReturnsOnCall map[int]struct { - result1 repositories.OrgRecord + getDeletedAtReturnsOnCall map[int]struct { + result1 *time.Time result2 error } - GetOrgUnfilteredStub func(context.Context, authorization.Info, string) (repositories.OrgRecord, error) - getOrgUnfilteredMutex sync.RWMutex - getOrgUnfilteredArgsForCall []struct { + GetOrgStub func(context.Context, authorization.Info, string) (repositories.OrgRecord, error) + getOrgMutex sync.RWMutex + getOrgArgsForCall []struct { arg1 context.Context arg2 authorization.Info arg3 string } - getOrgUnfilteredReturns struct { + getOrgReturns struct { result1 repositories.OrgRecord result2 error } - getOrgUnfilteredReturnsOnCall map[int]struct { + getOrgReturnsOnCall map[int]struct { result1 repositories.OrgRecord result2 error } @@ -232,6 +233,72 @@ func (fake *CFOrgRepository) DeleteOrgReturnsOnCall(i int, result1 error) { }{result1} } +func (fake *CFOrgRepository) GetDeletedAt(arg1 context.Context, arg2 authorization.Info, arg3 string) (*time.Time, error) { + fake.getDeletedAtMutex.Lock() + ret, specificReturn := fake.getDeletedAtReturnsOnCall[len(fake.getDeletedAtArgsForCall)] + fake.getDeletedAtArgsForCall = append(fake.getDeletedAtArgsForCall, struct { + arg1 context.Context + arg2 authorization.Info + arg3 string + }{arg1, arg2, arg3}) + stub := fake.GetDeletedAtStub + fakeReturns := fake.getDeletedAtReturns + fake.recordInvocation("GetDeletedAt", []interface{}{arg1, arg2, arg3}) + fake.getDeletedAtMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *CFOrgRepository) GetDeletedAtCallCount() int { + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() + return len(fake.getDeletedAtArgsForCall) +} + +func (fake *CFOrgRepository) GetDeletedAtCalls(stub func(context.Context, authorization.Info, string) (*time.Time, error)) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = stub +} + +func (fake *CFOrgRepository) GetDeletedAtArgsForCall(i int) (context.Context, authorization.Info, string) { + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() + argsForCall := fake.getDeletedAtArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *CFOrgRepository) GetDeletedAtReturns(result1 *time.Time, result2 error) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = nil + fake.getDeletedAtReturns = struct { + result1 *time.Time + result2 error + }{result1, result2} +} + +func (fake *CFOrgRepository) GetDeletedAtReturnsOnCall(i int, result1 *time.Time, result2 error) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = nil + if fake.getDeletedAtReturnsOnCall == nil { + fake.getDeletedAtReturnsOnCall = make(map[int]struct { + result1 *time.Time + result2 error + }) + } + fake.getDeletedAtReturnsOnCall[i] = struct { + result1 *time.Time + result2 error + }{result1, result2} +} + func (fake *CFOrgRepository) GetOrg(arg1 context.Context, arg2 authorization.Info, arg3 string) (repositories.OrgRecord, error) { fake.getOrgMutex.Lock() ret, specificReturn := fake.getOrgReturnsOnCall[len(fake.getOrgArgsForCall)] @@ -298,72 +365,6 @@ func (fake *CFOrgRepository) GetOrgReturnsOnCall(i int, result1 repositories.Org }{result1, result2} } -func (fake *CFOrgRepository) GetOrgUnfiltered(arg1 context.Context, arg2 authorization.Info, arg3 string) (repositories.OrgRecord, error) { - fake.getOrgUnfilteredMutex.Lock() - ret, specificReturn := fake.getOrgUnfilteredReturnsOnCall[len(fake.getOrgUnfilteredArgsForCall)] - fake.getOrgUnfilteredArgsForCall = append(fake.getOrgUnfilteredArgsForCall, struct { - arg1 context.Context - arg2 authorization.Info - arg3 string - }{arg1, arg2, arg3}) - stub := fake.GetOrgUnfilteredStub - fakeReturns := fake.getOrgUnfilteredReturns - fake.recordInvocation("GetOrgUnfiltered", []interface{}{arg1, arg2, arg3}) - fake.getOrgUnfilteredMutex.Unlock() - if stub != nil { - return stub(arg1, arg2, arg3) - } - if specificReturn { - return ret.result1, ret.result2 - } - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *CFOrgRepository) GetOrgUnfilteredCallCount() int { - fake.getOrgUnfilteredMutex.RLock() - defer fake.getOrgUnfilteredMutex.RUnlock() - return len(fake.getOrgUnfilteredArgsForCall) -} - -func (fake *CFOrgRepository) GetOrgUnfilteredCalls(stub func(context.Context, authorization.Info, string) (repositories.OrgRecord, error)) { - fake.getOrgUnfilteredMutex.Lock() - defer fake.getOrgUnfilteredMutex.Unlock() - fake.GetOrgUnfilteredStub = stub -} - -func (fake *CFOrgRepository) GetOrgUnfilteredArgsForCall(i int) (context.Context, authorization.Info, string) { - fake.getOrgUnfilteredMutex.RLock() - defer fake.getOrgUnfilteredMutex.RUnlock() - argsForCall := fake.getOrgUnfilteredArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 -} - -func (fake *CFOrgRepository) GetOrgUnfilteredReturns(result1 repositories.OrgRecord, result2 error) { - fake.getOrgUnfilteredMutex.Lock() - defer fake.getOrgUnfilteredMutex.Unlock() - fake.GetOrgUnfilteredStub = nil - fake.getOrgUnfilteredReturns = struct { - result1 repositories.OrgRecord - result2 error - }{result1, result2} -} - -func (fake *CFOrgRepository) GetOrgUnfilteredReturnsOnCall(i int, result1 repositories.OrgRecord, result2 error) { - fake.getOrgUnfilteredMutex.Lock() - defer fake.getOrgUnfilteredMutex.Unlock() - fake.GetOrgUnfilteredStub = nil - if fake.getOrgUnfilteredReturnsOnCall == nil { - fake.getOrgUnfilteredReturnsOnCall = make(map[int]struct { - result1 repositories.OrgRecord - result2 error - }) - } - fake.getOrgUnfilteredReturnsOnCall[i] = struct { - result1 repositories.OrgRecord - result2 error - }{result1, result2} -} - func (fake *CFOrgRepository) ListOrgs(arg1 context.Context, arg2 authorization.Info, arg3 repositories.ListOrgsMessage) ([]repositories.OrgRecord, error) { fake.listOrgsMutex.Lock() ret, specificReturn := fake.listOrgsReturnsOnCall[len(fake.listOrgsArgsForCall)] @@ -503,10 +504,10 @@ func (fake *CFOrgRepository) Invocations() map[string][][]interface{} { defer fake.createOrgMutex.RUnlock() fake.deleteOrgMutex.RLock() defer fake.deleteOrgMutex.RUnlock() + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() fake.getOrgMutex.RLock() defer fake.getOrgMutex.RUnlock() - fake.getOrgUnfilteredMutex.RLock() - defer fake.getOrgUnfilteredMutex.RUnlock() fake.listOrgsMutex.RLock() defer fake.listOrgsMutex.RUnlock() fake.patchOrgMetadataMutex.RLock() diff --git a/api/handlers/fake/cfspace_repository.go b/api/handlers/fake/cfspace_repository.go index 8cfab6569..0db35c52c 100644 --- a/api/handlers/fake/cfspace_repository.go +++ b/api/handlers/fake/cfspace_repository.go @@ -4,6 +4,7 @@ package fake import ( "context" "sync" + "time" "code.cloudfoundry.org/korifi/api/authorization" "code.cloudfoundry.org/korifi/api/handlers" @@ -39,6 +40,21 @@ type CFSpaceRepository struct { deleteSpaceReturnsOnCall map[int]struct { result1 error } + GetDeletedAtStub func(context.Context, authorization.Info, string) (*time.Time, error) + getDeletedAtMutex sync.RWMutex + getDeletedAtArgsForCall []struct { + arg1 context.Context + arg2 authorization.Info + arg3 string + } + getDeletedAtReturns struct { + result1 *time.Time + result2 error + } + getDeletedAtReturnsOnCall map[int]struct { + result1 *time.Time + result2 error + } GetSpaceStub func(context.Context, authorization.Info, string) (repositories.SpaceRecord, error) getSpaceMutex sync.RWMutex getSpaceArgsForCall []struct { @@ -217,6 +233,72 @@ func (fake *CFSpaceRepository) DeleteSpaceReturnsOnCall(i int, result1 error) { }{result1} } +func (fake *CFSpaceRepository) GetDeletedAt(arg1 context.Context, arg2 authorization.Info, arg3 string) (*time.Time, error) { + fake.getDeletedAtMutex.Lock() + ret, specificReturn := fake.getDeletedAtReturnsOnCall[len(fake.getDeletedAtArgsForCall)] + fake.getDeletedAtArgsForCall = append(fake.getDeletedAtArgsForCall, struct { + arg1 context.Context + arg2 authorization.Info + arg3 string + }{arg1, arg2, arg3}) + stub := fake.GetDeletedAtStub + fakeReturns := fake.getDeletedAtReturns + fake.recordInvocation("GetDeletedAt", []interface{}{arg1, arg2, arg3}) + fake.getDeletedAtMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *CFSpaceRepository) GetDeletedAtCallCount() int { + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() + return len(fake.getDeletedAtArgsForCall) +} + +func (fake *CFSpaceRepository) GetDeletedAtCalls(stub func(context.Context, authorization.Info, string) (*time.Time, error)) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = stub +} + +func (fake *CFSpaceRepository) GetDeletedAtArgsForCall(i int) (context.Context, authorization.Info, string) { + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() + argsForCall := fake.getDeletedAtArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *CFSpaceRepository) GetDeletedAtReturns(result1 *time.Time, result2 error) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = nil + fake.getDeletedAtReturns = struct { + result1 *time.Time + result2 error + }{result1, result2} +} + +func (fake *CFSpaceRepository) GetDeletedAtReturnsOnCall(i int, result1 *time.Time, result2 error) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = nil + if fake.getDeletedAtReturnsOnCall == nil { + fake.getDeletedAtReturnsOnCall = make(map[int]struct { + result1 *time.Time + result2 error + }) + } + fake.getDeletedAtReturnsOnCall[i] = struct { + result1 *time.Time + result2 error + }{result1, result2} +} + func (fake *CFSpaceRepository) GetSpace(arg1 context.Context, arg2 authorization.Info, arg3 string) (repositories.SpaceRecord, error) { fake.getSpaceMutex.Lock() ret, specificReturn := fake.getSpaceReturnsOnCall[len(fake.getSpaceArgsForCall)] @@ -422,6 +504,8 @@ func (fake *CFSpaceRepository) Invocations() map[string][][]interface{} { defer fake.createSpaceMutex.RUnlock() fake.deleteSpaceMutex.RLock() defer fake.deleteSpaceMutex.RUnlock() + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() fake.getSpaceMutex.RLock() defer fake.getSpaceMutex.RUnlock() fake.listSpacesMutex.RLock() diff --git a/api/handlers/fake/deletion_repository.go b/api/handlers/fake/deletion_repository.go new file mode 100644 index 000000000..43592f9f6 --- /dev/null +++ b/api/handlers/fake/deletion_repository.go @@ -0,0 +1,123 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package fake + +import ( + "context" + "sync" + "time" + + "code.cloudfoundry.org/korifi/api/authorization" + "code.cloudfoundry.org/korifi/api/handlers" +) + +type DeletionRepository struct { + GetDeletedAtStub func(context.Context, authorization.Info, string) (*time.Time, error) + getDeletedAtMutex sync.RWMutex + getDeletedAtArgsForCall []struct { + arg1 context.Context + arg2 authorization.Info + arg3 string + } + getDeletedAtReturns struct { + result1 *time.Time + result2 error + } + getDeletedAtReturnsOnCall map[int]struct { + result1 *time.Time + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *DeletionRepository) GetDeletedAt(arg1 context.Context, arg2 authorization.Info, arg3 string) (*time.Time, error) { + fake.getDeletedAtMutex.Lock() + ret, specificReturn := fake.getDeletedAtReturnsOnCall[len(fake.getDeletedAtArgsForCall)] + fake.getDeletedAtArgsForCall = append(fake.getDeletedAtArgsForCall, struct { + arg1 context.Context + arg2 authorization.Info + arg3 string + }{arg1, arg2, arg3}) + stub := fake.GetDeletedAtStub + fakeReturns := fake.getDeletedAtReturns + fake.recordInvocation("GetDeletedAt", []interface{}{arg1, arg2, arg3}) + fake.getDeletedAtMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *DeletionRepository) GetDeletedAtCallCount() int { + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() + return len(fake.getDeletedAtArgsForCall) +} + +func (fake *DeletionRepository) GetDeletedAtCalls(stub func(context.Context, authorization.Info, string) (*time.Time, error)) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = stub +} + +func (fake *DeletionRepository) GetDeletedAtArgsForCall(i int) (context.Context, authorization.Info, string) { + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() + argsForCall := fake.getDeletedAtArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *DeletionRepository) GetDeletedAtReturns(result1 *time.Time, result2 error) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = nil + fake.getDeletedAtReturns = struct { + result1 *time.Time + result2 error + }{result1, result2} +} + +func (fake *DeletionRepository) GetDeletedAtReturnsOnCall(i int, result1 *time.Time, result2 error) { + fake.getDeletedAtMutex.Lock() + defer fake.getDeletedAtMutex.Unlock() + fake.GetDeletedAtStub = nil + if fake.getDeletedAtReturnsOnCall == nil { + fake.getDeletedAtReturnsOnCall = make(map[int]struct { + result1 *time.Time + result2 error + }) + } + fake.getDeletedAtReturnsOnCall[i] = struct { + result1 *time.Time + result2 error + }{result1, result2} +} + +func (fake *DeletionRepository) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.getDeletedAtMutex.RLock() + defer fake.getDeletedAtMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *DeletionRepository) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ handlers.DeletionRepository = new(DeletionRepository) diff --git a/api/handlers/job.go b/api/handlers/job.go index de1486577..e61b16b83 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -10,7 +10,6 @@ import ( "code.cloudfoundry.org/korifi/api/authorization" apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/presenter" - "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/api/routing" "github.com/go-logr/logr" @@ -31,18 +30,28 @@ const ( const JobResourceType = "Job" +//counterfeiter:generate -o fake -fake-name DeletionRepository . DeletionRepository +type DeletionRepository interface { + GetDeletedAt(context.Context, authorization.Info, string) (*time.Time, error) +} + +func DefaultDeletionRepositories(orgRepo DeletionRepository, spaceRepo DeletionRepository) map[string]DeletionRepository { + return map[string]DeletionRepository{ + orgDeletePrefix: orgRepo, + spaceDeletePrefix: spaceRepo, + } +} + type Job struct { serverURL url.URL - orgRepo CFOrgRepository - spaceRepo CFSpaceRepository + repositories map[string]DeletionRepository pollingInterval time.Duration } -func NewJob(serverURL url.URL, orgRepo CFOrgRepository, spaceRepo CFSpaceRepository, pollingInterval time.Duration) *Job { +func NewJob(serverURL url.URL, repositories map[string]DeletionRepository, pollingInterval time.Duration) *Job { return &Job{ serverURL: serverURL, - orgRepo: orgRepo, - spaceRepo: spaceRepo, + repositories: repositories, pollingInterval: pollingInterval, } } @@ -71,42 +80,36 @@ func (h *Job) get(r *http.Request) (*routing.Response, error) { jobResponse = presenter.ForManifestApplyJob(job, h.serverURL) case appDeletePrefix, routeDeletePrefix, domainDeletePrefix, roleDeletePrefix: jobResponse = presenter.ForJob(job, []presenter.JobResponseError{}, presenter.StateComplete, h.serverURL) - case orgDeletePrefix, spaceDeletePrefix: - jobResponse, err = h.handleDeleteJob(r.Context(), job) + default: + repository, ok := h.repositories[job.Type] + if !ok { + return nil, apierrors.LogAndReturn( + log, + apierrors.NewNotFoundError(fmt.Errorf("invalid job type: %s", job.Type), JobResourceType), + fmt.Sprintf("Invalid Job type: %s", job.Type), + ) + } + + jobResponse, err = h.handleDeleteJob(r.Context(), repository, job) if err != nil { return nil, err } - default: - return nil, apierrors.LogAndReturn( - log, - apierrors.NewNotFoundError(fmt.Errorf("invalid job type: %s", job.Type), JobResourceType), - fmt.Sprintf("Invalid Job type: %s", job.Type), - ) } return routing.NewResponse(http.StatusOK).WithBody(jobResponse), nil } -func (h *Job) handleDeleteJob(ctx context.Context, job presenter.Job) (presenter.JobResponse, error) { +func (h *Job) handleDeleteJob(ctx context.Context, repository DeletionRepository, job presenter.Job) (presenter.JobResponse, error) { authInfo, _ := authorization.InfoFromContext(ctx) log := logr.FromContextOrDiscard(ctx).WithName("handlers.job.get.handleDeleteJob") var ( - org repositories.OrgRecord - space repositories.SpaceRecord err error deletedAt *time.Time ) for retries := 0; retries < 40; retries++ { - switch job.Type { - case orgDeletePrefix: - org, err = h.orgRepo.GetOrgUnfiltered(ctx, authInfo, job.ResourceGUID) - deletedAt = org.DeletedAt - case spaceDeletePrefix: - space, err = h.spaceRepo.GetSpace(ctx, authInfo, job.ResourceGUID) - deletedAt = space.DeletedAt - } + deletedAt, err = repository.GetDeletedAt(ctx, authInfo, job.ResourceGUID) if err != nil { switch err.(type) { @@ -134,12 +137,6 @@ func (h *Job) handleDeleteJob(ctx context.Context, job presenter.Job) (presenter time.Sleep(h.pollingInterval) } - return h.handleDeleteJobResponse(ctx, deletedAt, job) -} - -func (h *Job) handleDeleteJobResponse(ctx context.Context, deletedAt *time.Time, job presenter.Job) (presenter.JobResponse, error) { - log := logr.FromContextOrDiscard(ctx).WithName("handlers.job.get.handleDeleteJobResponse") - if deletedAt == nil { return presenter.JobResponse{}, apierrors.LogAndReturn( log, @@ -162,7 +159,7 @@ func (h *Job) handleDeleteJobResponse(ctx context.Context, deletedAt *time.Time, job, []presenter.JobResponseError{{ Code: 10008, - Detail: fmt.Sprintf("%s deletion timed out. Check for remaining resources in the %q namespace", job.ResourceType, job.ResourceGUID), + Detail: fmt.Sprintf("%s deletion timed out, check the remaining %q resource", job.ResourceType, job.ResourceGUID), Title: "CF-UnprocessableEntity", }}, presenter.StateFailed, diff --git a/api/handlers/job_test.go b/api/handlers/job_test.go index 11bb14767..5292338ae 100644 --- a/api/handlers/job_test.go +++ b/api/handlers/job_test.go @@ -1,7 +1,6 @@ package handlers_test import ( - "fmt" "net/http" "net/http/httptest" "time" @@ -9,31 +8,56 @@ import ( apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/handlers" "code.cloudfoundry.org/korifi/api/handlers/fake" - "code.cloudfoundry.org/korifi/api/repositories" . "code.cloudfoundry.org/korifi/tests/matchers" "code.cloudfoundry.org/korifi/tools" - "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/types" ) var _ = Describe("Job", func() { - Describe("GET /v3/jobs endpoint", func() { + Describe("GET /v3/jobs", func() { + DescribeTable("response stubs", func(operation, resourceGUID string) { + jobGUID := operation + "~" + resourceGUID + req, err := http.NewRequestWithContext(ctx, "GET", "/v3/jobs/"+jobGUID, nil) + Expect(err).NotTo(HaveOccurred()) + + rr = httptest.NewRecorder() + routerBuilder.Build().ServeHTTP(rr, req) + + Expect(rr).To(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) + bodyMatchers := []types.GomegaMatcher{ + MatchJSONPath("$.guid", jobGUID), + MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), + MatchJSONPath("$.operation", operation), + MatchJSONPath("$.state", "COMPLETE"), + } + if operation == "space.apply_manifest" { + bodyMatchers = append(bodyMatchers, MatchJSONPath("$.links.space.href", defaultServerURL+"/v3/spaces/"+resourceGUID)) + } + + Expect(rr).To(HaveHTTPBody(SatisfyAll(bodyMatchers...))) + }, + Entry("app delete", "app.delete", "cf-app-guid"), + Entry("route delete", "route.delete", "cf-route-guid"), + Entry("domain delete", "domain.delete", "cf-domain-guid"), + Entry("role delete", "role.delete", "cf-role-guid"), + Entry("apply manifest", "space.apply_manifest", "cf-space-guid"), + ) + var ( - spaceGUID string - jobGUID string - req *http.Request - orgRepo *fake.CFOrgRepository - spaceRepo *fake.CFSpaceRepository + jobGUID string + req *http.Request + deletionRepo *fake.DeletionRepository ) BeforeEach(func() { - spaceGUID = uuid.NewString() - - orgRepo = new(fake.CFOrgRepository) - spaceRepo = new(fake.CFSpaceRepository) - apiHandler := handlers.NewJob(*serverURL, orgRepo, spaceRepo, 0) + jobGUID = "testing.delete~my-resource-guid" + deletionRepo = new(fake.DeletionRepository) + deletionRepo.GetDeletedAtReturns(tools.PtrTo(time.Now()), nil) + apiHandler := handlers.NewJob(*serverURL, map[string]handlers.DeletionRepository{"testing.delete": deletionRepo}, 0) routerBuilder.LoadRoutes(apiHandler) }) @@ -45,253 +69,102 @@ var _ = Describe("Job", func() { routerBuilder.Build().ServeHTTP(rr, req) }) - Describe("apply manifest jobs", func() { - BeforeEach(func() { - jobGUID = "space.apply_manifest~" + spaceGUID - }) - - It("returns the job", func() { - Expect(rr).To(HaveHTTPStatus(http.StatusOK)) - Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.space.href", defaultServerURL+"/v3/spaces/"+spaceGUID), - MatchJSONPath("$.operation", "space.apply_manifest"), - ))) - }) - }) - - DescribeTable("delete jobs", func(operation, resourceGUID string) { - guid := operation + "~" + resourceGUID - req, err := http.NewRequestWithContext(ctx, "GET", "/v3/jobs/"+guid, nil) - Expect(err).NotTo(HaveOccurred()) - - rr = httptest.NewRecorder() - routerBuilder.Build().ServeHTTP(rr, req) + It("returns a processing status", func() { + Expect(deletionRepo.GetDeletedAtCallCount()).To(Equal(1)) + _, actualAuthInfo, actualResourceGUID := deletionRepo.GetDeletedAtArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualResourceGUID).To(Equal("my-resource-guid")) + Expect(rr).To(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", guid), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+guid), - MatchJSONPath("$.operation", operation), + MatchJSONPath("$.guid", jobGUID), + MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), + MatchJSONPath("$.operation", "testing.delete"), + MatchJSONPath("$.state", "PROCESSING"), + MatchJSONPath("$.errors", BeEmpty()), ))) - }, - Entry("app delete", "app.delete", "cf-app-guid"), - Entry("route delete", "route.delete", "cf-route-guid"), - Entry("domain delete", "domain.delete", "cf-domain-guid"), - Entry("role delete", "role.delete", "cf-role-guid"), - ) + }) - When("the job guid provided does not have the expected delimiter", func() { + When("the resource does not exist", func() { BeforeEach(func() { - jobGUID = "job.operation;some-resource-guid" + deletionRepo.GetDeletedAtReturns(nil, apierrors.NewNotFoundError(nil, "foo")) }) - It("returns an error", func() { - expectNotFoundError("Job") + It("returns a complete status", func() { + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.state", "COMPLETE"), + MatchJSONPath("$.errors", BeEmpty()), + ))) }) }) - Describe("org delete", func() { - const ( - operation = "org.delete" - resourceGUID = "cf-org-guid" - ) - + When("the resource deletion times out", func() { BeforeEach(func() { - jobGUID = operation + "~" + resourceGUID - }) - - When("the org deletion is in progress", func() { - BeforeEach(func() { - orgRepo.GetOrgUnfilteredReturns(repositories.OrgRecord{ - GUID: "cf-org-guid", - DeletedAt: tools.PtrTo(time.Now()), - }, nil) - }) - - It("returns a processing status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "PROCESSING"), - MatchJSONPath("$.errors", BeEmpty()), - ))) - }) - }) - - When("the org does not exist", func() { - BeforeEach(func() { - orgRepo.GetOrgUnfilteredReturns(repositories.OrgRecord{}, apierrors.NewNotFoundError(nil, repositories.OrgResourceType)) - }) - - It("returns a complete status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "COMPLETE"), - MatchJSONPath("$.errors", BeEmpty()), - ))) - }) + deletionRepo.GetDeletedAtReturns(tools.PtrTo(time.Now().Add(-180*time.Second)), nil) }) - When("the org deletion times out", func() { - BeforeEach(func() { - orgRepo.GetOrgUnfilteredReturns(repositories.OrgRecord{ - GUID: "cf-org-guid", - DeletedAt: tools.PtrTo(time.Now().Add(-180 * time.Second)), - }, nil) - }) - - It("returns a failed status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "FAILED"), - MatchJSONPath("$.errors", ConsistOf(map[string]interface{}{ - "code": float64(10008), - "detail": fmt.Sprintf("Org deletion timed out. Check for remaining resources in the %q namespace", resourceGUID), - "title": "CF-UnprocessableEntity", - })), - ))) - }) + It("returns a failed status", func() { + Expect(deletionRepo.GetDeletedAtCallCount()).To(Equal(1)) + _, actualAuthInfo, actualResourceGUID := deletionRepo.GetDeletedAtArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualResourceGUID).To(Equal("my-resource-guid")) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.state", "FAILED"), + MatchJSONPath("$.errors", ConsistOf(map[string]interface{}{ + "code": float64(10008), + "detail": "Testing deletion timed out, check the remaining \"my-resource-guid\" resource", + "title": "CF-UnprocessableEntity", + })), + ))) }) + }) - When("the user does not have permission to see the org", func() { - BeforeEach(func() { - orgRepo.GetOrgUnfilteredReturns(repositories.OrgRecord{}, apierrors.NewForbiddenError(nil, repositories.OrgResourceType)) - }) - - It("returns a complete status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "COMPLETE"), - MatchJSONPath("$.errors", BeEmpty()), - ))) - }) + When("the user does not have permission to see the resource", func() { + BeforeEach(func() { + deletionRepo.GetDeletedAtReturns(nil, apierrors.NewForbiddenError(nil, "foo")) }) - When("the org has not been marked for deletion", func() { - BeforeEach(func() { - orgRepo.GetOrgUnfilteredReturns(repositories.OrgRecord{ - GUID: resourceGUID, - }, nil) - }) - - It("returns a not found error", func() { - Expect(rr).To(HaveHTTPStatus(http.StatusNotFound)) - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.errors[0].code", float64(10010)), - MatchJSONPath("$.errors[0].detail", "Job not found. Ensure it exists and you have access to it."), - MatchJSONPath("$.errors[0].title", "CF-ResourceNotFound"), - ))) - }) + It("returns a complete status", func() { + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.state", "COMPLETE"), + MatchJSONPath("$.errors", BeEmpty()), + ))) }) }) - Describe("space delete", func() { - const ( - operation = "space.delete" - resourceGUID = "cf-space-guid" - ) - + When("the resource has not been marked for deletion", func() { BeforeEach(func() { - jobGUID = operation + "~" + resourceGUID + deletionRepo.GetDeletedAtReturns(nil, nil) }) - When("the space deletion is in progress", func() { - BeforeEach(func() { - spaceRepo.GetSpaceReturns(repositories.SpaceRecord{ - GUID: "cf-space-guid", - DeletedAt: tools.PtrTo(time.Now()), - }, nil) - }) - - It("returns a processing status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "PROCESSING"), - MatchJSONPath("$.errors", BeEmpty()), - ))) - }) + It("returns a not found error", func() { + Expect(rr).To(HaveHTTPStatus(http.StatusNotFound)) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.errors[0].code", float64(10010)), + MatchJSONPath("$.errors[0].detail", "Job not found. Ensure it exists and you have access to it."), + MatchJSONPath("$.errors[0].title", "CF-ResourceNotFound"), + ))) }) + }) - When("the space does not exist", func() { - BeforeEach(func() { - spaceRepo.GetSpaceReturns(repositories.SpaceRecord{}, apierrors.NewNotFoundError(nil, repositories.SpaceResourceType)) - }) - - It("returns a complete status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "COMPLETE"), - MatchJSONPath("$.errors", BeEmpty()), - ))) - }) + When("the job guid is invalid", func() { + BeforeEach(func() { + jobGUID = "job.operation;some-resource-guid" }) - When("the space deletion times out", func() { - BeforeEach(func() { - spaceRepo.GetSpaceReturns(repositories.SpaceRecord{ - GUID: "cf-space-guid", - DeletedAt: tools.PtrTo(time.Now().Add(-180 * time.Second)), - }, nil) - }) - - It("returns a failed status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "FAILED"), - MatchJSONPath("$.errors", ConsistOf(map[string]interface{}{ - "code": float64(10008), - "detail": fmt.Sprintf("Space deletion timed out. Check for remaining resources in the %q namespace", resourceGUID), - "title": "CF-UnprocessableEntity", - })), - ))) - }) + It("returns an error", func() { + expectNotFoundError("Job") }) + }) - When("the user does not have permission to see the space", func() { - BeforeEach(func() { - spaceRepo.GetSpaceReturns(repositories.SpaceRecord{}, apierrors.NewForbiddenError(nil, repositories.SpaceResourceType)) - }) - - It("returns a complete status", func() { - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", jobGUID), - MatchJSONPath("$.links.self.href", defaultServerURL+"/v3/jobs/"+jobGUID), - MatchJSONPath("$.operation", operation), - MatchJSONPath("$.state", "COMPLETE"), - MatchJSONPath("$.errors", BeEmpty()), - ))) - }) + When("there is no deletion repository registered for the operation", func() { + BeforeEach(func() { + apiHandler := handlers.NewJob(*serverURL, map[string]handlers.DeletionRepository{}, 0) + routerBuilder.LoadRoutes(apiHandler) }) - When("the space has not been marked for deletion", func() { - BeforeEach(func() { - spaceRepo.GetSpaceReturns(repositories.SpaceRecord{ - GUID: resourceGUID, - }, nil) - }) - - It("returns a not found error", func() { - Expect(rr).To(HaveHTTPStatus(http.StatusNotFound)) - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.errors[0].code", float64(10010)), - MatchJSONPath("$.errors[0].detail", "Job not found. Ensure it exists and you have access to it."), - MatchJSONPath("$.errors[0].title", "CF-ResourceNotFound"), - ))) - }) + It("returns an error", func() { + expectNotFoundError("Job") }) }) }) diff --git a/api/handlers/org.go b/api/handlers/org.go index 1f9d67910..7784380e2 100644 --- a/api/handlers/org.go +++ b/api/handlers/org.go @@ -32,8 +32,8 @@ type CFOrgRepository interface { ListOrgs(context.Context, authorization.Info, repositories.ListOrgsMessage) ([]repositories.OrgRecord, error) DeleteOrg(context.Context, authorization.Info, repositories.DeleteOrgMessage) error GetOrg(context.Context, authorization.Info, string) (repositories.OrgRecord, error) - GetOrgUnfiltered(context.Context, authorization.Info, string) (repositories.OrgRecord, error) PatchOrgMetadata(context.Context, authorization.Info, repositories.PatchOrgMetadataMessage) (repositories.OrgRecord, error) + GetDeletedAt(context.Context, authorization.Info, string) (*time.Time, error) } type Org struct { diff --git a/api/handlers/space.go b/api/handlers/space.go index 232a5659c..80a53d6d4 100644 --- a/api/handlers/space.go +++ b/api/handlers/space.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/url" + "time" "code.cloudfoundry.org/korifi/api/authorization" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -28,6 +29,7 @@ type CFSpaceRepository interface { GetSpace(context.Context, authorization.Info, string) (repositories.SpaceRecord, error) DeleteSpace(context.Context, authorization.Info, repositories.DeleteSpaceMessage) error PatchSpaceMetadata(context.Context, authorization.Info, repositories.PatchSpaceMetadataMessage) (repositories.SpaceRecord, error) + GetDeletedAt(context.Context, authorization.Info, string) (*time.Time, error) } type Space struct { diff --git a/api/main.go b/api/main.go index be63728ac..a66e6bcfe 100644 --- a/api/main.go +++ b/api/main.go @@ -327,8 +327,7 @@ func main() { ), handlers.NewJob( *serverURL, - orgRepo, - spaceRepo, + handlers.DefaultDeletionRepositories(orgRepo, spaceRepo), 500*time.Millisecond, ), handlers.NewLogCache( diff --git a/api/presenter/job.go b/api/presenter/job.go index 52853a70d..354056e14 100644 --- a/api/presenter/job.go +++ b/api/presenter/job.go @@ -4,7 +4,9 @@ import ( "fmt" "net/url" "regexp" - "strings" + + "golang.org/x/text/cases" + "golang.org/x/text/language" ) const ( @@ -45,7 +47,7 @@ func JobFromGUID(guid string) (Job, bool) { return Job{ GUID: guid, Type: matches[1], - ResourceType: strings.Title(matches[2]), + ResourceType: cases.Title(language.AmericanEnglish).String(matches[2]), ResourceGUID: matches[4], }, true } diff --git a/api/repositories/org_repository.go b/api/repositories/org_repository.go index c51386b78..67877131c 100644 --- a/api/repositories/org_repository.go +++ b/api/repositories/org_repository.go @@ -205,21 +205,6 @@ func (r *OrgRepo) GetOrg(ctx context.Context, info authorization.Info, orgGUID s return orgRecords[0], nil } -func (r *OrgRepo) GetOrgUnfiltered(ctx context.Context, info authorization.Info, orgGUID string) (OrgRecord, error) { - userClient, err := r.userClientFactory.BuildClient(info) - if err != nil { - return OrgRecord{}, fmt.Errorf("get-org failed to build user client: %w", err) - } - - cfOrg := new(korifiv1alpha1.CFOrg) - err = userClient.Get(ctx, client.ObjectKey{Namespace: r.rootNamespace, Name: orgGUID}, cfOrg) - if err != nil { - return OrgRecord{}, apierrors.FromK8sError(err, OrgResourceType) - } - - return cfOrgToOrgRecord(*cfOrg), nil -} - func (r *OrgRepo) DeleteOrg(ctx context.Context, info authorization.Info, message DeleteOrgMessage) error { userClient, err := r.userClientFactory.BuildClient(info) if err != nil { @@ -257,6 +242,21 @@ func (r *OrgRepo) PatchOrgMetadata(ctx context.Context, authInfo authorization.I return cfOrgToOrgRecord(*cfOrg), nil } +func (r *OrgRepo) GetDeletedAt(ctx context.Context, authInfo authorization.Info, orgGUID string) (*time.Time, error) { + userClient, err := r.userClientFactory.BuildClient(authInfo) + if err != nil { + return nil, fmt.Errorf("get-deleted-at failed to build user client: %w", err) + } + + cfOrg := new(korifiv1alpha1.CFOrg) + err = userClient.Get(ctx, client.ObjectKey{Namespace: r.rootNamespace, Name: orgGUID}, cfOrg) + if err != nil { + return nil, apierrors.FromK8sError(err, OrgResourceType) + } + + return cfOrgToOrgRecord(*cfOrg).DeletedAt, nil +} + func cfOrgToOrgRecord(cfOrg korifiv1alpha1.CFOrg) OrgRecord { return OrgRecord{ GUID: cfOrg.Name, diff --git a/api/repositories/org_repository_test.go b/api/repositories/org_repository_test.go index e29d7f6ea..26f6a4d33 100644 --- a/api/repositories/org_repository_test.go +++ b/api/repositories/org_repository_test.go @@ -354,39 +354,48 @@ var _ = Describe("OrgRepository", func() { }) }) - Describe("GetOrgUnfiltered", func() { - var cfOrg *korifiv1alpha1.CFOrg + Describe("GetDeletedAt", func() { + var ( + cfOrg *korifiv1alpha1.CFOrg + deletionTime *time.Time + getErr error + ) BeforeEach(func() { cfOrg = createOrgWithCleanup(ctx, prefixedGUID("the-org")) - Expect(k8s.PatchResource(ctx, k8sClient, cfOrg, func() { - cfOrg.Labels = map[string]string{ - "test-label-key": "test-label-val", - } - cfOrg.Annotations = map[string]string{ - "test-annotation-key": "test-annotation-val", - } - })).To(Succeed()) }) - When("the org exists", func() { + JustBeforeEach(func() { + deletionTime, getErr = orgRepo.GetDeletedAt(ctx, authInfo, cfOrg.Name) + }) + + It("returns nil", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(BeNil()) + }) + + When("the org is being deleted", func() { BeforeEach(func() { - createRoleBinding(ctx, userName, orgUserRole.Name, cfOrg.Name) + Expect(k8s.PatchResource(ctx, k8sClient, cfOrg, func() { + cfOrg.Finalizers = append(cfOrg.Finalizers, "foo") + })).To(Succeed()) + + Expect(k8sClient.Delete(ctx, cfOrg)).To(Succeed()) }) - It("gets the org", func() { - orgRecord, err := orgRepo.GetOrgUnfiltered(ctx, authInfo, cfOrg.Name) - Expect(err).NotTo(HaveOccurred()) - Expect(orgRecord.Name).To(Equal(cfOrg.Spec.DisplayName)) - Expect(orgRecord.Labels).To(Equal(map[string]string{"test-label-key": "test-label-val"})) - Expect(orgRecord.Annotations).To(Equal(map[string]string{"test-annotation-key": "test-annotation-val"})) + It("returns the deletion time", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(PointTo(BeTemporally("~", time.Now(), time.Minute))) }) }) When("the org isn't found", func() { + BeforeEach(func() { + Expect(k8sClient.Delete(ctx, cfOrg)).To(Succeed()) + }) + It("errors", func() { - _, err := orgRepo.GetOrgUnfiltered(ctx, authInfo, "non-existent-org") - Expect(err).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) + Expect(getErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) }) }) }) diff --git a/api/repositories/space_repository.go b/api/repositories/space_repository.go index e76441dde..311b97062 100644 --- a/api/repositories/space_repository.go +++ b/api/repositories/space_repository.go @@ -285,3 +285,11 @@ func (r *SpaceRepo) PatchSpaceMetadata(ctx context.Context, authInfo authorizati return cfSpaceToSpaceRecord(*cfSpace), nil } + +func (r *SpaceRepo) GetDeletedAt(ctx context.Context, authInfo authorization.Info, spaceGUID string) (*time.Time, error) { + space, err := r.GetSpace(ctx, authInfo, spaceGUID) + if err != nil { + return nil, err + } + return space.DeletedAt, nil +} diff --git a/api/repositories/space_repository_test.go b/api/repositories/space_repository_test.go index ef0bdeedf..2f69dfcdc 100644 --- a/api/repositories/space_repository_test.go +++ b/api/repositories/space_repository_test.go @@ -719,4 +719,52 @@ var _ = Describe("SpaceRepository", func() { }) }) }) + + Describe("GetDeletedAt", func() { + var ( + cfSpace *korifiv1alpha1.CFSpace + deletionTime *time.Time + getErr error + ) + + BeforeEach(func() { + cfOrg := createOrgWithCleanup(ctx, "the-org") + createRoleBinding(ctx, userName, orgUserRole.Name, cfOrg.Name) + cfSpace = createSpaceWithCleanup(ctx, cfOrg.Name, "the-space") + }) + + JustBeforeEach(func() { + deletionTime, getErr = spaceRepo.GetDeletedAt(ctx, authInfo, cfSpace.Name) + }) + + It("returns nil", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(BeNil()) + }) + + When("the space is being deleted", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, cfSpace, func() { + cfSpace.Finalizers = append(cfSpace.Finalizers, "foo") + })).To(Succeed()) + + Expect(k8sClient.Delete(ctx, cfSpace)).To(Succeed()) + }) + + It("returns the deletion time", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(PointTo(BeTemporally("~", time.Now(), time.Minute))) + }) + }) + + When("the space isn't found", func() { + BeforeEach(func() { + Expect(k8sClient.Delete(ctx, cfSpace)).To(Succeed()) + }) + + It("errors", func() { + Expect(getErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) + }) + }) + }) }) From b1e66571aca0e82912e8df1fefe34db738cd637b Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Tue, 11 Jul 2023 10:43:11 +0000 Subject: [PATCH 3/4] Extract the tools/logger package This makes it easy to create a nested logger starting from the one in the context *and* re-inject it into the context so that it can be easily passed on. Co-authored-by: Giuseppe Capizzi --- api/handlers/job.go | 9 ++++----- tools/logger/context.go | 12 ++++++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 tools/logger/context.go diff --git a/api/handlers/job.go b/api/handlers/job.go index e61b16b83..22f23a238 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -11,8 +11,7 @@ import ( apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/presenter" "code.cloudfoundry.org/korifi/api/routing" - - "github.com/go-logr/logr" + "code.cloudfoundry.org/korifi/tools/logger" ) const ( @@ -57,7 +56,7 @@ func NewJob(serverURL url.URL, repositories map[string]DeletionRepository, polli } func (h *Job) get(r *http.Request) (*routing.Response, error) { - log := logr.FromContextOrDiscard(r.Context()).WithName("handlers.job.get") + ctx, log := logger.FromContext(r.Context(), "handlers.job.get") jobGUID := routing.URLParam(r, "guid") @@ -90,7 +89,7 @@ func (h *Job) get(r *http.Request) (*routing.Response, error) { ) } - jobResponse, err = h.handleDeleteJob(r.Context(), repository, job) + jobResponse, err = h.handleDeleteJob(ctx, repository, job) if err != nil { return nil, err } @@ -101,7 +100,7 @@ func (h *Job) get(r *http.Request) (*routing.Response, error) { func (h *Job) handleDeleteJob(ctx context.Context, repository DeletionRepository, job presenter.Job) (presenter.JobResponse, error) { authInfo, _ := authorization.InfoFromContext(ctx) - log := logr.FromContextOrDiscard(ctx).WithName("handlers.job.get.handleDeleteJob") + ctx, log := logger.FromContext(ctx, "handleDeleteJob") var ( err error diff --git a/tools/logger/context.go b/tools/logger/context.go new file mode 100644 index 000000000..56dee9e63 --- /dev/null +++ b/tools/logger/context.go @@ -0,0 +1,12 @@ +package logger + +import ( + "context" + + "github.com/go-logr/logr" +) + +func FromContext(ctx context.Context, loggerName string) (context.Context, logr.Logger) { + log := logr.FromContextOrDiscard(ctx).WithName(loggerName) + return logr.NewContext(ctx, log), log +} From a4eafe14e08e3b1973781fa2c9e9a361cff0a265 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Tue, 11 Jul 2023 10:49:11 +0000 Subject: [PATCH 4/4] Simplify the retry code in handlers.Job Co-authored-by: Giuseppe Capizzi --- api/handlers/job.go | 74 +++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 32 deletions(-) diff --git a/api/handlers/job.go b/api/handlers/job.go index 22f23a238..0cc799aad 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -99,41 +99,25 @@ func (h *Job) get(r *http.Request) (*routing.Response, error) { } func (h *Job) handleDeleteJob(ctx context.Context, repository DeletionRepository, job presenter.Job) (presenter.JobResponse, error) { - authInfo, _ := authorization.InfoFromContext(ctx) ctx, log := logger.FromContext(ctx, "handleDeleteJob") - var ( - err error - deletedAt *time.Time - ) - - for retries := 0; retries < 40; retries++ { - deletedAt, err = repository.GetDeletedAt(ctx, authInfo, job.ResourceGUID) - - if err != nil { - switch err.(type) { - case apierrors.NotFoundError, apierrors.ForbiddenError: - return presenter.ForJob(job, - []presenter.JobResponseError{}, - presenter.StateComplete, - h.serverURL, - ), nil - default: - return presenter.JobResponse{}, apierrors.LogAndReturn( - log, - err, - "failed to fetch "+job.ResourceType+" from Kubernetes", - job.ResourceType+"GUID", job.ResourceGUID, - ) - } - } - - if deletedAt != nil { - break + deletedAt, err := h.retryGetDeletedAt(ctx, repository, job) + if err != nil { + switch err.(type) { + case apierrors.NotFoundError, apierrors.ForbiddenError: + return presenter.ForJob(job, + []presenter.JobResponseError{}, + presenter.StateComplete, + h.serverURL, + ), nil + default: + return presenter.JobResponse{}, apierrors.LogAndReturn( + log, + err, + "failed to fetch "+job.ResourceType+" from Kubernetes", + job.ResourceType+"GUID", job.ResourceGUID, + ) } - - log.V(1).Info("Waiting for deletion timestamp", job.ResourceType+"GUID", job.ResourceGUID) - time.Sleep(h.pollingInterval) } if deletedAt == nil { @@ -166,6 +150,32 @@ func (h *Job) handleDeleteJob(ctx context.Context, repository DeletionRepository ), nil } +func (h *Job) retryGetDeletedAt(ctx context.Context, repository DeletionRepository, job presenter.Job) (*time.Time, error) { + ctx, log := logger.FromContext(ctx, "retryGetDeletedAt") + authInfo, _ := authorization.InfoFromContext(ctx) + + var ( + deletedAt *time.Time + err error + ) + + for retries := 0; retries < 40; retries++ { + deletedAt, err = repository.GetDeletedAt(ctx, authInfo, job.ResourceGUID) + if err != nil { + return nil, err + } + + if deletedAt != nil { + return deletedAt, nil + } + + log.V(1).Info("Waiting for deletion timestamp", job.ResourceType+"GUID", job.ResourceGUID) + time.Sleep(h.pollingInterval) + } + + return nil, nil +} + func (h *Job) UnauthenticatedRoutes() []routing.Route { return nil }