Error when applying manifests referencing unexisting services

Co-authored-by: Danail Branekov <danailster@gmail.com>

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,
 		})

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())
+			})
+		})
 	})
 })

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,
 		},
 	}
 }

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"})