From 171b2939a2175300c7711bc5d0619480fc1aa714 Mon Sep 17 00:00:00 2001 From: Yusmen Zabanov Date: Thu, 31 Oct 2024 16:43:48 +0000 Subject: [PATCH] Error when creating service instances for disabled plan fixes #3574 Co-authored-by: Danail Branekov --- api/main.go | 1 + .../service_instance_repository.go | 40 +++++++++ .../service_instance_repository_test.go | 82 ++++++++++++++++++- tests/e2e/e2e_suite_test.go | 20 +++++ tests/e2e/service_plans_test.go | 10 +-- tests/smoke/bind_service_test.go | 1 + tests/smoke/services_test.go | 1 + 7 files changed, 146 insertions(+), 9 deletions(-) diff --git a/api/main.go b/api/main.go index 535626b18..e1d15e272 100644 --- a/api/main.go +++ b/api/main.go @@ -201,6 +201,7 @@ func main() { nsPermissions, conditions.NewConditionAwaiter[*korifiv1alpha1.CFServiceInstance, korifiv1alpha1.CFServiceInstance, korifiv1alpha1.CFServiceInstanceList](conditionTimeout), repositories.NewServiceInstanceSorter(), + cfg.RootNamespace, ) serviceBindingRepo := repositories.NewServiceBindingRepo( namespaceRetriever, diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 137df1141..c30f0becc 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -46,6 +46,7 @@ type ServiceInstanceRepo struct { namespacePermissions *authorization.NamespacePermissions awaiter Awaiter[*korifiv1alpha1.CFServiceInstance] sorter ServiceInstanceSorter + rootNamespace string } //counterfeiter:generate -o fake -fake-name ServiceInstanceSorter . ServiceInstanceSorter @@ -93,6 +94,7 @@ func NewServiceInstanceRepo( namespacePermissions *authorization.NamespacePermissions, awaiter Awaiter[*korifiv1alpha1.CFServiceInstance], sorter ServiceInstanceSorter, + rootNamespace string, ) *ServiceInstanceRepo { return &ServiceInstanceRepo{ namespaceRetriever: namespaceRetriever, @@ -100,6 +102,7 @@ func NewServiceInstanceRepo( namespacePermissions: namespacePermissions, awaiter: awaiter, sorter: sorter, + rootNamespace: rootNamespace, } } @@ -230,6 +233,15 @@ func (r *ServiceInstanceRepo) CreateManagedServiceInstance(ctx context.Context, return ServiceInstanceRecord{}, fmt.Errorf("failed to build user client: %w", err) } + planVisible, err := r.servicePlanVisible(ctx, userClient, message.PlanGUID, message.SpaceGUID) + if err != nil { + return ServiceInstanceRecord{}, apierrors.NewUnprocessableEntityError(err, "Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.") + } + + if !planVisible { + return ServiceInstanceRecord{}, apierrors.NewUnprocessableEntityError(nil, "Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.") + } + parameterBytes, err := json.Marshal(message.Parameters) if err != nil { return ServiceInstanceRecord{}, fmt.Errorf("failed to marshal parameters: %w", err) @@ -260,6 +272,34 @@ func (r *ServiceInstanceRepo) CreateManagedServiceInstance(ctx context.Context, return cfServiceInstanceToRecord(*cfServiceInstance), nil } +func (r *ServiceInstanceRepo) servicePlanVisible(ctx context.Context, userClient client.Client, planGUID string, spaceGUID string) (bool, error) { + servicePlan := &korifiv1alpha1.CFServicePlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: planGUID, + Namespace: r.rootNamespace, + }, + } + err := userClient.Get(ctx, client.ObjectKeyFromObject(servicePlan), servicePlan) + if err != nil { + return false, err + } + + if servicePlan.Spec.Visibility.Type == korifiv1alpha1.PublicServicePlanVisibilityType { + return true, nil + } + + if servicePlan.Spec.Visibility.Type == korifiv1alpha1.AdminServicePlanVisibilityType { + return false, nil + } + + orgGUID, err := r.namespaceRetriever.NamespaceFor(ctx, spaceGUID, SpaceResourceType) + if err != nil { + return false, err + } + + return slices.Contains(servicePlan.Spec.Visibility.Organizations, orgGUID), nil +} + func (r *ServiceInstanceRepo) PatchServiceInstance(ctx context.Context, authInfo authorization.Info, message PatchServiceInstanceMessage) (ServiceInstanceRecord, error) { userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 82ef2850d..372a98d04 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -58,7 +58,7 @@ var _ = Describe("ServiceInstanceRepository", func() { return records } - serviceInstanceRepo = repositories.NewServiceInstanceRepo(namespaceRetriever, userClientFactory, nsPerms, conditionAwaiter, sorter) + serviceInstanceRepo = repositories.NewServiceInstanceRepo(namespaceRetriever, userClientFactory, nsPerms, conditionAwaiter, sorter, rootNamespace) org = createOrgWithCleanup(ctx, uuid.NewString()) space = createSpaceWithCleanup(ctx, org.Name, uuid.NewString()) @@ -154,16 +154,30 @@ var _ = Describe("ServiceInstanceRepository", func() { Describe("CreateManagedServiceInstance", func() { var ( + servicePlan *korifiv1alpha1.CFServicePlan serviceInstanceCreateMessage repositories.CreateManagedSIMessage record repositories.ServiceInstanceRecord createErr error ) BeforeEach(func() { + servicePlan = &korifiv1alpha1.CFServicePlan{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: rootNamespace, + Name: uuid.NewString(), + }, + Spec: korifiv1alpha1.CFServicePlanSpec{ + Visibility: korifiv1alpha1.ServicePlanVisibility{ + Type: korifiv1alpha1.PublicServicePlanVisibilityType, + }, + }, + } + Expect(k8sClient.Create(ctx, servicePlan)).To(Succeed()) + serviceInstanceCreateMessage = repositories.CreateManagedSIMessage{ Name: serviceInstanceName, SpaceGUID: space.Name, - PlanGUID: "plan-guid", + PlanGUID: servicePlan.Name, Tags: []string{"foo", "bar"}, Parameters: map[string]any{ "p1": map[string]any{ @@ -196,7 +210,7 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(record.Tags).To(ConsistOf([]string{"foo", "bar"})) Expect(record.SecretName).To(BeEmpty()) Expect(record.Relationships()).To(Equal(map[string]string{ - "service_plan": "plan-guid", + "service_plan": servicePlan.Name, "space": space.Name, })) Expect(record.CreatedAt).To(BeTemporally("~", time.Now(), timeCheckThreshold)) @@ -218,7 +232,7 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(cfServiceInstance.Spec.SecretName).To(BeEmpty()) Expect(cfServiceInstance.Spec.Type).To(BeEquivalentTo(korifiv1alpha1.ManagedType)) Expect(cfServiceInstance.Spec.Tags).To(ConsistOf("foo", "bar")) - Expect(cfServiceInstance.Spec.PlanGUID).To(Equal("plan-guid")) + Expect(cfServiceInstance.Spec.PlanGUID).To(Equal(servicePlan.Name)) Expect(cfServiceInstance.Spec.Parameters).NotTo(BeNil()) actualParams := map[string]any{} @@ -229,6 +243,66 @@ var _ = Describe("ServiceInstanceRepository", func() { }, })) }) + + When("the service plan visibility type is admin", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, servicePlan, func() { + servicePlan.Spec.Visibility.Type = korifiv1alpha1.AdminServicePlanVisibilityType + })).To(Succeed()) + }) + + It("returns unprocessable entity error", func() { + Expect(createErr).To(BeAssignableToTypeOf(apierrors.UnprocessableEntityError{})) + }) + }) + + When("the service plan visibility type is organization", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, servicePlan, func() { + servicePlan.Spec.Visibility.Type = korifiv1alpha1.OrganizationServicePlanVisibilityType + })).To(Succeed()) + }) + + It("returns unprocessable entity error", func() { + Expect(createErr).To(BeAssignableToTypeOf(apierrors.UnprocessableEntityError{})) + }) + + When("the plan is enabled for the current organization", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, servicePlan, func() { + servicePlan.Spec.Visibility.Organizations = append(servicePlan.Spec.Visibility.Organizations, org.Name) + })).To(Succeed()) + }) + + It("succeeds", func() { + Expect(createErr).NotTo(HaveOccurred()) + }) + + When("the space does not exist", func() { + BeforeEach(func() { + serviceInstanceCreateMessage.SpaceGUID = "does-not-exist" + }) + + It("returns unprocessable entity error", func() { + var unprocessableEntityErr apierrors.UnprocessableEntityError + Expect(errors.As(createErr, &unprocessableEntityErr)).To(BeTrue()) + Expect(unprocessableEntityErr).To(MatchError(ContainSubstring("does-not-exist"))) + }) + }) + }) + }) + + When("the service plan does not exist", func() { + BeforeEach(func() { + serviceInstanceCreateMessage.PlanGUID = "does-not-exist" + }) + + It("returns unprocessable entity error", func() { + var unprocessableEntityErr apierrors.UnprocessableEntityError + Expect(errors.As(createErr, &unprocessableEntityErr)).To(BeTrue()) + Expect(unprocessableEntityErr).To(MatchError(ContainSubstring("does-not-exist"))) + }) + }) }) }) diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index b54f66e75..4b6b3b07e 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -21,6 +21,7 @@ import ( "code.cloudfoundry.org/go-loggregator/v8/rpc/loggregator_v2" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "github.com/BooleanCat/go-functional/v2/it/itx" "github.com/go-resty/resty/v2" "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" @@ -1183,6 +1184,25 @@ func createBroker(brokerURL string) string { g.Expect(jobRespBody).To(ContainSubstring("COMPLETE")) }).Should(Succeed()) + plans := resourceList[resource]{} + listResp, err := adminClient.R().SetResult(&plans).Get("/v3/service_plans") + Expect(err).NotTo(HaveOccurred()) + Expect(listResp).To(HaveRestyStatusCode(http.StatusOK)) + + itx.FromSlice(plans.Resources).Filter(func(r resource) bool { + return r.Metadata.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel] == brokerGUID + }).ForEach(func(plan resource) { + resp, err := adminClient.R(). + SetBody(planVisibilityResource{ + Type: "public", + }). + Post(fmt.Sprintf("/v3/service_plans/%s/visibility", plan.GUID)) + Expect(err).NotTo(HaveOccurred()) + Expect(resp).To(SatisfyAll( + HaveRestyStatusCode(http.StatusOK), + )) + }) + return brokerGUID } diff --git a/tests/e2e/service_plans_test.go b/tests/e2e/service_plans_test.go index 981c5af79..96ae2f93d 100644 --- a/tests/e2e/service_plans_test.go +++ b/tests/e2e/service_plans_test.go @@ -75,7 +75,7 @@ var _ = Describe("Service Plans", func() { It("returns the service plan visibility", func() { Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) Expect(result).To(Equal(planVisibilityResource{ - Type: korifiv1alpha1.AdminServicePlanVisibilityType, + Type: "public", })) }) }) @@ -86,7 +86,7 @@ var _ = Describe("Service Plans", func() { resp, err = adminClient.R(). SetResult(&result). SetBody(planVisibilityResource{ - Type: "public", + Type: "admin", }). Post(fmt.Sprintf("/v3/service_plans/%s/visibility", planGUID)) Expect(err).NotTo(HaveOccurred()) @@ -96,7 +96,7 @@ var _ = Describe("Service Plans", func() { Expect(resp).To(SatisfyAll( HaveRestyStatusCode(http.StatusOK), HaveRestyBody(MatchJSON(`{ - "type": "public" + "type": "admin" }`)), )) }) @@ -108,7 +108,7 @@ var _ = Describe("Service Plans", func() { resp, err = adminClient.R(). SetResult(&result). SetBody(planVisibilityResource{ - Type: "public", + Type: "admin", }). Patch(fmt.Sprintf("/v3/service_plans/%s/visibility", planGUID)) Expect(err).NotTo(HaveOccurred()) @@ -118,7 +118,7 @@ var _ = Describe("Service Plans", func() { Expect(resp).To(SatisfyAll( HaveRestyStatusCode(http.StatusOK), HaveRestyBody(MatchJSON(`{ - "type": "public" + "type": "admin" }`)), )) }) diff --git a/tests/smoke/bind_service_test.go b/tests/smoke/bind_service_test.go index 894f02f29..956842f62 100644 --- a/tests/smoke/bind_service_test.go +++ b/tests/smoke/bind_service_test.go @@ -46,6 +46,7 @@ var _ = Describe("cf bind-service", func() { helpers.GetInClusterURL(getAppGUID(sharedData.BrokerAppName)), )).To(Exit(0)) + Expect(helpers.Cf("enable-service-access", "sample-service", "-b", brokerName)).To(Exit(0)) session := helpers.Cf("create-service", "sample-service", "sample", serviceName, "-b", brokerName) Expect(session).To(Exit(0)) }) diff --git a/tests/smoke/services_test.go b/tests/smoke/services_test.go index cb940d8d1..2f51f9656 100644 --- a/tests/smoke/services_test.go +++ b/tests/smoke/services_test.go @@ -22,6 +22,7 @@ var _ = Describe("Services", func() { "broker-password", helpers.GetInClusterURL(getAppGUID(sharedData.BrokerAppName)), )).To(Exit(0)) + Expect(helpers.Cf("enable-service-access", "sample-service", "-b", brokerName)).To(Exit(0)) }) AfterEach(func() {