From 23f012c9fb03dfbc73a0dfb648d8083d8104a793 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Fri, 18 Aug 2023 10:14:20 +0000 Subject: [PATCH] Error when applying manifests referencing unexisting services Co-authored-by: Danail Branekov --- api/actions/manifest/applier.go | 22 ++++++++++++-- api/actions/manifest/applier_test.go | 32 ++++++++++++++------ api/errors/errors.go | 45 ++++++++++++++++++++-------- api/errors/errors_test.go | 37 +++++++++++++++++++++++ 4 files changed, 112 insertions(+), 24 deletions(-) diff --git a/api/actions/manifest/applier.go b/api/actions/manifest/applier.go index 554251265..4f25ab858 100644 --- a/api/actions/manifest/applier.go +++ b/api/actions/manifest/applier.go @@ -7,6 +7,7 @@ import ( "code.cloudfoundry.org/korifi/api/actions/shared" "code.cloudfoundry.org/korifi/api/authorization" + apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/payloads" "code.cloudfoundry.org/korifi/api/repositories" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" @@ -219,15 +220,30 @@ func (a *Applier) applyServices(ctx context.Context, authInfo authorization.Info return err } + serviceNameToServiceInstance := map[string]repositories.ServiceInstanceRecord{} + for _, serviceInstance := range serviceInstances { + serviceNameToServiceInstance[serviceInstance.Name] = serviceInstance + } + serviceNameToServiceBinding := map[string]*string{} for _, manifestService := range appInfo.Services { serviceNameToServiceBinding[manifestService.Name] = manifestService.BindingName } - for _, si := range serviceInstances { + for serviceName := range desiredServiceNames { + serviceInstance, ok := serviceNameToServiceInstance[serviceName] + if !ok { + return apierrors.NewNotFoundError( + nil, + repositories.ServiceInstanceResourceType, + "application", appInfo.Name, + "service", serviceName, + ) + } + _, err := a.serviceBindingRepo.CreateServiceBinding(ctx, authInfo, repositories.CreateServiceBindingMessage{ - Name: serviceNameToServiceBinding[si.Name], - ServiceInstanceGUID: si.GUID, + Name: serviceNameToServiceBinding[serviceName], + ServiceInstanceGUID: serviceInstance.GUID, AppGUID: appState.App.GUID, SpaceGUID: appState.App.SpaceGUID, }) diff --git a/api/actions/manifest/applier_test.go b/api/actions/manifest/applier_test.go index a4c689258..29ca7dfde 100644 --- a/api/actions/manifest/applier_test.go +++ b/api/actions/manifest/applier_test.go @@ -7,6 +7,7 @@ import ( "code.cloudfoundry.org/korifi/api/actions/manifest" "code.cloudfoundry.org/korifi/api/actions/shared/fake" "code.cloudfoundry.org/korifi/api/authorization" + apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/payloads" "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/tools" @@ -455,21 +456,18 @@ var _ = Describe("Applier", func() { Describe("applying services", func() { BeforeEach(func() { serviceInstanceRepo.ListServiceInstancesReturns([]repositories.ServiceInstanceRecord{ - { - Name: "service-name", - GUID: "service-guid", - }, + {Name: "service-name", GUID: "service-guid"}, }, nil) appState.App.GUID = "app-guid" appState.App.SpaceGUID = "space-guid" appState.ServiceBindings = map[string]repositories.ServiceBindingRecord{ - "another-service-name": {}, + "already-bound-service-name": {}, } appInfo.Services = []payloads.ManifestApplicationService{ {Name: "service-name"}, - {Name: "another-service-name"}, + {Name: "already-bound-service-name"}, } }) @@ -495,7 +493,7 @@ var _ = Describe("Applier", func() { BeforeEach(func() { appInfo.Services = []payloads.ManifestApplicationService{ {Name: "service-name", BindingName: tools.PtrTo("service-binding")}, - {Name: "another-service-name"}, + {Name: "already-bound-service-name"}, } }) @@ -536,8 +534,8 @@ var _ = Describe("Applier", func() { When("the app is already bound to all the services in the manifest", func() { BeforeEach(func() { appState.ServiceBindings = map[string]repositories.ServiceBindingRecord{ - "service-name": {}, - "another-service-name": {}, + "service-name": {}, + "already-bound-service-name": {}, } }) @@ -546,5 +544,21 @@ var _ = Describe("Applier", func() { Expect(serviceBindingRepo.CreateServiceBindingCallCount()).To(BeZero()) }) }) + + When("the manifest references service instances that do not exist", func() { + BeforeEach(func() { + appInfo.Services = []payloads.ManifestApplicationService{ + {Name: "unexisting-service"}, + } + }) + + It("returns a not found error error", func() { + Expect(applierErr).To(BeAssignableToTypeOf(apierrors.NotFoundError{})) + notFoundErr := applierErr.(apierrors.NotFoundError) + Expect(notFoundErr.Detail()).To(ContainSubstring("unexisting-service")) + + Expect(serviceBindingRepo.CreateServiceBindingCallCount()).To(BeZero()) + }) + }) }) }) diff --git a/api/errors/errors.go b/api/errors/errors.go index 648ad5053..1ae735000 100644 --- a/api/errors/errors.go +++ b/api/errors/errors.go @@ -39,11 +39,12 @@ func LogAndReturn(logger logr.Logger, err error, msg string, keysAndValues ...in } type apiError struct { - cause error - detail string - title string - code int - httpStatus int + cause error + detail string + title string + code int + httpStatus int + additionalDetails map[string]string } func (e apiError) Error() string { @@ -59,7 +60,11 @@ func (e apiError) Unwrap() error { } func (e apiError) Detail() string { - return e.detail + detail := e.detail + for k, v := range e.additionalDetails { + detail += fmt.Sprintf(" %s=%q", k, v) + } + return detail } func (e apiError) Title() string { @@ -74,6 +79,21 @@ func (e apiError) HttpStatus() int { return e.httpStatus } +func toKeyValues(s ...string) map[string]string { + result := map[string]string{} + + for i := 0; i < len(s); i += 2 { + key := s[i] + val := "" + if i+1 < len(s) { + val = s[i+1] + } + result[key] = val + } + + return result +} + type UnprocessableEntityError struct { apiError } @@ -134,14 +154,15 @@ type NotFoundError struct { apiError } -func NewNotFoundError(cause error, resourceType string) NotFoundError { +func NewNotFoundError(cause error, resourceType string, additionalDetails ...string) NotFoundError { return NotFoundError{ apiError{ - cause: cause, - title: "CF-ResourceNotFound", - detail: fmt.Sprintf("%s not found. Ensure it exists and you have access to it.", resourceType), - code: 10010, - httpStatus: http.StatusNotFound, + cause: cause, + title: "CF-ResourceNotFound", + detail: fmt.Sprintf("%s not found. Ensure it exists and you have access to it.", resourceType), + additionalDetails: toKeyValues(additionalDetails...), + code: 10010, + httpStatus: http.StatusNotFound, }, } } diff --git a/api/errors/errors_test.go b/api/errors/errors_test.go index 8e7507a27..357e9e891 100644 --- a/api/errors/errors_test.go +++ b/api/errors/errors_test.go @@ -306,6 +306,43 @@ var _ = Describe("LogAndReturn", func() { }) }) +var _ = Describe("Additional details", func() { + var ( + err apierrors.ApiError + details []string + ) + + BeforeEach(func() { + details = []string{"foo", "FOO", "bar", "BAR"} + }) + + JustBeforeEach(func() { + err = apierrors.NewNotFoundError(nil, "my-resource", details...) + }) + + It("displays the additional details in the detail message", func() { + Expect(err.Detail()).To(SatisfyAll( + HavePrefix("my-resource not found. Ensure it exists and you have access to it."), + ContainSubstring(`foo="FOO"`), + ContainSubstring(`bar="BAR"`), + )) + }) + + When("the details count is odd", func() { + BeforeEach(func() { + details = []string{"foo", "FOO", "bar"} + }) + + It("displays an empty string for the last key", func() { + Expect(err.Detail()).To(SatisfyAll( + HavePrefix("my-resource not found. Ensure it exists and you have access to it."), + ContainSubstring(`foo="FOO"`), + ContainSubstring(`bar=""`), + )) + }) + }) +}) + var _ = Describe("invalid lists", func() { It("formats the invalid keys correctly", func() { err := apierrors.NewUnknownKeyError(errors.New("oops"), []string{"one", "two"})