From acfc537bd9cfed290781fff9ff93213637ee786c Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Wed, 12 Jul 2023 10:18:49 +0000 Subject: [PATCH] Report correct deletion status in route deletion job Issue: https://github.com/cloudfoundry/korifi/issues/2607 Co-authored-by: Kieron Browne --- api/handlers/job.go | 20 +++--- api/handlers/job_test.go | 6 +- api/main.go | 1 + api/repositories/route_repository.go | 53 +++++++++------- api/repositories/route_repository_test.go | 77 ++++++++++++++++++++++- tests/e2e/routes_test.go | 14 ++--- 6 files changed, 126 insertions(+), 45 deletions(-) diff --git a/api/handlers/job.go b/api/handlers/job.go index 2195e7d4a..02d1f5721 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -2,6 +2,7 @@ package handlers import ( "context" + "errors" "fmt" "net/http" "net/url" @@ -70,7 +71,7 @@ func (h *Job) get(r *http.Request) (*routing.Response, error) { switch job.Type { case syncSpaceJobType: jobResponse = presenter.ForManifestApplyJob(job, h.serverURL) - case RouteDeleteJobType, DomainDeleteJobType, RoleDeleteJobType: + case DomainDeleteJobType, RoleDeleteJobType: jobResponse = presenter.ForJob(job, []presenter.JobResponseError{}, presenter.StateComplete, h.serverURL) default: repository, ok := h.repositories[job.Type] @@ -96,21 +97,20 @@ func (h *Job) handleDeleteJob(ctx context.Context, repository DeletionRepository deletedAt, err := h.retryGetDeletedAt(ctx, repository, job) if err != nil { - switch err.(type) { - case apierrors.NotFoundError, apierrors.ForbiddenError: + if errors.As(err, &apierrors.NotFoundError{}) || errors.As(err, &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, - ) } + + return presenter.JobResponse{}, apierrors.LogAndReturn( + log, + err, + "failed to fetch "+job.ResourceType+" from Kubernetes", + job.ResourceType+"GUID", job.ResourceGUID, + ) } if deletedAt == nil { diff --git a/api/handlers/job_test.go b/api/handlers/job_test.go index d3adadea2..46df70003 100644 --- a/api/handlers/job_test.go +++ b/api/handlers/job_test.go @@ -1,6 +1,7 @@ package handlers_test import ( + "fmt" "net/http" "net/http/httptest" "time" @@ -40,7 +41,6 @@ var _ = Describe("Job", func() { Expect(rr).To(HaveHTTPBody(SatisfyAll(bodyMatchers...))) }, - 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"), @@ -86,7 +86,7 @@ var _ = Describe("Job", func() { When("the resource does not exist", func() { BeforeEach(func() { - deletionRepo.GetDeletedAtReturns(nil, apierrors.NewNotFoundError(nil, "foo")) + deletionRepo.GetDeletedAtReturns(nil, fmt.Errorf("wrapped error: %w", apierrors.NewNotFoundError(nil, "foo"))) }) It("returns a complete status", func() { @@ -120,7 +120,7 @@ var _ = Describe("Job", func() { When("the user does not have permission to see the resource", func() { BeforeEach(func() { - deletionRepo.GetDeletedAtReturns(nil, apierrors.NewForbiddenError(nil, "foo")) + deletionRepo.GetDeletedAtReturns(nil, fmt.Errorf("wrapped err: %w", apierrors.NewForbiddenError(nil, "foo"))) }) It("returns a complete status", func() { diff --git a/api/main.go b/api/main.go index 06686d019..348644212 100644 --- a/api/main.go +++ b/api/main.go @@ -331,6 +331,7 @@ func main() { handlers.OrgDeleteJobType: orgRepo, handlers.SpaceDeleteJobType: spaceRepo, handlers.AppDeleteJobType: appRepo, + handlers.RouteDeleteJobType: routeRepo, }, 500*time.Millisecond, ), diff --git a/api/repositories/route_repository.go b/api/repositories/route_repository.go index fe4333d60..cd7305480 100644 --- a/api/repositories/route_repository.go +++ b/api/repositories/route_repository.go @@ -57,6 +57,7 @@ type RouteRecord struct { Annotations map[string]string CreatedAt time.Time UpdatedAt *time.Time + DeletedAt *time.Time } type AddDestinationsToRouteMessage struct { @@ -146,13 +147,13 @@ func (m CreateRouteMessage) toCFRoute() korifiv1alpha1.CFRoute { } } -func (f *RouteRepo) GetRoute(ctx context.Context, authInfo authorization.Info, routeGUID string) (RouteRecord, error) { - ns, err := f.namespaceRetriever.NamespaceFor(ctx, routeGUID, RouteResourceType) +func (r *RouteRepo) GetRoute(ctx context.Context, authInfo authorization.Info, routeGUID string) (RouteRecord, error) { + ns, err := r.namespaceRetriever.NamespaceFor(ctx, routeGUID, RouteResourceType) if err != nil { return RouteRecord{}, fmt.Errorf("failed to get namespace for route: %w", err) } - userClient, err := f.userClientFactory.BuildClient(authInfo) + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return RouteRecord{}, fmt.Errorf("failed to build user client: %w", err) } @@ -166,13 +167,13 @@ func (f *RouteRepo) GetRoute(ctx context.Context, authInfo authorization.Info, r return cfRouteToRouteRecord(route), nil } -func (f *RouteRepo) ListRoutes(ctx context.Context, authInfo authorization.Info, message ListRoutesMessage) ([]RouteRecord, error) { - nsList, err := f.namespacePermissions.GetAuthorizedSpaceNamespaces(ctx, authInfo) +func (r *RouteRepo) ListRoutes(ctx context.Context, authInfo authorization.Info, message ListRoutesMessage) ([]RouteRecord, error) { + nsList, err := r.namespacePermissions.GetAuthorizedSpaceNamespaces(ctx, authInfo) if err != nil { return nil, fmt.Errorf("failed to list namespaces for spaces with user role bindings: %w", err) } - userClient, err := f.userClientFactory.BuildClient(authInfo) + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return []RouteRecord{}, fmt.Errorf("failed to build user client: %w", err) } @@ -215,8 +216,8 @@ func (f *RouteRepo) ListRoutes(ctx context.Context, authInfo authorization.Info, return returnRouteList(filteredRoutes), nil } -func (f *RouteRepo) ListRoutesForApp(ctx context.Context, authInfo authorization.Info, appGUID string, spaceGUID string) ([]RouteRecord, error) { - userClient, err := f.userClientFactory.BuildClient(authInfo) +func (r *RouteRepo) ListRoutesForApp(ctx context.Context, authInfo authorization.Info, appGUID string, spaceGUID string) ([]RouteRecord, error) { + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return []RouteRecord{}, fmt.Errorf("failed to build user client: %w", err) } @@ -275,6 +276,7 @@ func cfRouteToRouteRecord(cfRoute korifiv1alpha1.CFRoute) RouteRecord { Destinations: destinations, CreatedAt: cfRoute.CreationTimestamp.Time, UpdatedAt: getLastUpdatedTime(&cfRoute), + DeletedAt: golangTime(cfRoute.DeletionTimestamp), Labels: cfRoute.Labels, Annotations: cfRoute.Annotations, } @@ -290,9 +292,9 @@ func cfRouteDestinationToDestination(cfRouteDestination korifiv1alpha1.Destinati } } -func (f *RouteRepo) CreateRoute(ctx context.Context, authInfo authorization.Info, message CreateRouteMessage) (RouteRecord, error) { +func (r *RouteRepo) CreateRoute(ctx context.Context, authInfo authorization.Info, message CreateRouteMessage) (RouteRecord, error) { cfRoute := message.toCFRoute() - userClient, err := f.userClientFactory.BuildClient(authInfo) + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return RouteRecord{}, fmt.Errorf("failed to build user client: %w", err) } @@ -305,8 +307,8 @@ func (f *RouteRepo) CreateRoute(ctx context.Context, authInfo authorization.Info return cfRouteToRouteRecord(cfRoute), nil } -func (f *RouteRepo) DeleteRoute(ctx context.Context, authInfo authorization.Info, message DeleteRouteMessage) error { - userClient, err := f.userClientFactory.BuildClient(authInfo) +func (r *RouteRepo) DeleteRoute(ctx context.Context, authInfo authorization.Info, message DeleteRouteMessage) error { + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return fmt.Errorf("failed to build user client: %w", err) } @@ -320,8 +322,8 @@ func (f *RouteRepo) DeleteRoute(ctx context.Context, authInfo authorization.Info return apierrors.FromK8sError(err, RouteResourceType) } -func (f *RouteRepo) GetOrCreateRoute(ctx context.Context, authInfo authorization.Info, message CreateRouteMessage) (RouteRecord, error) { - existingRecord, exists, err := f.fetchRouteByFields(ctx, authInfo, message) +func (r *RouteRepo) GetOrCreateRoute(ctx context.Context, authInfo authorization.Info, message CreateRouteMessage) (RouteRecord, error) { + existingRecord, exists, err := r.fetchRouteByFields(ctx, authInfo, message) if err != nil { return RouteRecord{}, fmt.Errorf("GetOrCreateRoute: %w", err) } @@ -330,11 +332,11 @@ func (f *RouteRepo) GetOrCreateRoute(ctx context.Context, authInfo authorization return existingRecord, nil } - return f.CreateRoute(ctx, authInfo, message) + return r.CreateRoute(ctx, authInfo, message) } -func (f *RouteRepo) AddDestinationsToRoute(ctx context.Context, authInfo authorization.Info, message AddDestinationsToRouteMessage) (RouteRecord, error) { - userClient, err := f.userClientFactory.BuildClient(authInfo) +func (r *RouteRepo) AddDestinationsToRoute(ctx context.Context, authInfo authorization.Info, message AddDestinationsToRouteMessage) (RouteRecord, error) { + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return RouteRecord{}, fmt.Errorf("failed to build user client: %w", err) } @@ -355,8 +357,8 @@ func (f *RouteRepo) AddDestinationsToRoute(ctx context.Context, authInfo authori return cfRouteToRouteRecord(*cfRoute), err } -func (f *RouteRepo) RemoveDestinationFromRoute(ctx context.Context, authInfo authorization.Info, message RemoveDestinationFromRouteMessage) (RouteRecord, error) { - userClient, err := f.userClientFactory.BuildClient(authInfo) +func (r *RouteRepo) RemoveDestinationFromRoute(ctx context.Context, authInfo authorization.Info, message RemoveDestinationFromRouteMessage) (RouteRecord, error) { + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return RouteRecord{}, fmt.Errorf("failed to build user client: %w", err) } @@ -413,8 +415,8 @@ outer: return result } -func (f *RouteRepo) fetchRouteByFields(ctx context.Context, authInfo authorization.Info, message CreateRouteMessage) (RouteRecord, bool, error) { - matches, err := f.ListRoutes(ctx, authInfo, ListRoutesMessage{ +func (r *RouteRepo) fetchRouteByFields(ctx context.Context, authInfo authorization.Info, message CreateRouteMessage) (RouteRecord, bool, error) { + matches, err := r.ListRoutes(ctx, authInfo, ListRoutesMessage{ SpaceGUIDs: []string{message.SpaceGUID}, DomainGUIDs: []string{message.DomainGUID}, Hosts: []string{message.Host}, @@ -448,8 +450,8 @@ func destinationRecordsToCFDestinations(destinationRecords []DestinationRecord) return destinations } -func (f *RouteRepo) PatchRouteMetadata(ctx context.Context, authInfo authorization.Info, message PatchRouteMetadataMessage) (RouteRecord, error) { - userClient, err := f.userClientFactory.BuildClient(authInfo) +func (r *RouteRepo) PatchRouteMetadata(ctx context.Context, authInfo authorization.Info, message PatchRouteMetadataMessage) (RouteRecord, error) { + userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return RouteRecord{}, fmt.Errorf("failed to build user client: %w", err) } @@ -469,3 +471,8 @@ func (f *RouteRepo) PatchRouteMetadata(ctx context.Context, authInfo authorizati return cfRouteToRouteRecord(*route), nil } + +func (r *RouteRepo) GetDeletedAt(ctx context.Context, authInfo authorization.Info, routeGUID string) (*time.Time, error) { + route, err := r.GetRoute(ctx, authInfo, routeGUID) + return route.DeletedAt, err +} diff --git a/api/repositories/route_repository_test.go b/api/repositories/route_repository_test.go index fd6bdfe3f..5790b1ec9 100644 --- a/api/repositories/route_repository_test.go +++ b/api/repositories/route_repository_test.go @@ -184,9 +184,10 @@ var _ = Describe("RouteRepository", func() { Expect(destinationRecord.Protocol).To(Equal(cfRoute1.Spec.Destinations[0].Protocol)) }) - By("returning a record where the CreatedAt and UpdatedAt match the CR creation time", func() { + By("returning a record with timestamps", func() { Expect(route.CreatedAt).To(BeTemporally("~", time.Now(), timeCheckThreshold)) Expect(route.UpdatedAt).To(PointTo(BeTemporally("~", time.Now(), timeCheckThreshold))) + Expect(route.DeletedAt).To(BeNil()) }) Expect(route.Domain).To(Equal(DomainRecord{GUID: domainGUID})) @@ -1518,6 +1519,80 @@ var _ = Describe("RouteRepository", func() { }) }) }) + + Describe("GetDeletedAt", func() { + var ( + cfRoute *korifiv1alpha1.CFRoute + deletedAt *time.Time + getErr error + ) + + BeforeEach(func() { + createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name) + + cfRoute = &korifiv1alpha1.CFRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: route1GUID, + Namespace: space.Name, + }, + Spec: korifiv1alpha1.CFRouteSpec{ + Host: "my-subdomain-1", + Path: "", + Protocol: "http", + DomainRef: corev1.ObjectReference{ + Name: domainGUID, + Namespace: rootNamespace, + }, + Destinations: []korifiv1alpha1.Destination{ + { + GUID: "destination-guid", + Port: 8080, + AppRef: corev1.LocalObjectReference{ + Name: "some-app-guid", + }, + ProcessType: "web", + Protocol: "http1", + }, + }, + }, + } + Expect(k8sClient.Create(testCtx, cfRoute)).To(Succeed()) + }) + + JustBeforeEach(func() { + deletedAt, getErr = routeRepo.GetDeletedAt(testCtx, authInfo, route1GUID) + }) + + It("returns nil", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletedAt).To(BeNil()) + }) + + When("the route is being deleted", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, cfRoute, func() { + cfRoute.Finalizers = append(cfRoute.Finalizers, "foo") + })).To(Succeed()) + + Expect(k8sClient.Delete(ctx, cfRoute)).To(Succeed()) + }) + + It("returns the deletion time", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletedAt).To(PointTo(BeTemporally("~", time.Now(), time.Minute))) + }) + }) + + When("the route isn't found", func() { + BeforeEach(func() { + Expect(k8sClient.Delete(ctx, cfRoute)).To(Succeed()) + }) + + It("errors", func() { + Expect(getErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) + }) + }) + }) }) func createRoute(guid, namespace, host, path, domainGUID, appGUID string) *korifiv1alpha1.CFRoute { diff --git a/tests/e2e/routes_test.go b/tests/e2e/routes_test.go index d3b81a147..a96024514 100644 --- a/tests/e2e/routes_test.go +++ b/tests/e2e/routes_test.go @@ -315,15 +315,13 @@ var _ = Describe("Routes", func() { g.Expect(string(jobResp.Body())).To(ContainSubstring("COMPLETE")) }).Should(Succeed()) - Eventually(func(g Gomega) { - getRouteResp, err := client.R().Get("/v3/routes/" + routeGUID) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(getRouteResp).To(HaveRestyStatusCode(http.StatusNotFound)) - }).Should(Succeed()) - }) + getRouteResp, err := client.R().Get("/v3/routes/" + routeGUID) + Expect(err).NotTo(HaveOccurred()) + Expect(getRouteResp).To(HaveRestyStatusCode(http.StatusNotFound)) - It("frees up the deleted route's name for reuse", func() { - createRoute(host, path, spaceGUID, domainGUID) + By("freeing up the deleted route's name for reuse", func() { + createRoute(host, path, spaceGUID, domainGUID) + }) }) })