From d8eaf43f20f33dab4a3a1a7a37ac6a0ded0d5c00 Mon Sep 17 00:00:00 2001 From: Konstantin Lapkov Date: Mon, 11 Nov 2024 23:07:32 +0200 Subject: [PATCH 1/4] Add Get and GetCredentials for CFServiceInstances --- .../fake/cfservice_instance_repository.go | 83 ++++++ api/handlers/service_instance.go | 52 +++- api/handlers/service_instance_test.go | 280 ++++++++++++++++++ api/payloads/service_instance.go | 55 ++++ api/payloads/service_instance_test.go | 40 +++ api/presenter/service_instance.go | 2 + .../service_instance_repository.go | 30 ++ .../service_instance_repository_test.go | 72 +++++ tools/credentials.go | 10 + tools/credentials_test.go | 45 ++- 10 files changed, 652 insertions(+), 17 deletions(-) diff --git a/api/handlers/fake/cfservice_instance_repository.go b/api/handlers/fake/cfservice_instance_repository.go index 6700557ca..00e941124 100644 --- a/api/handlers/fake/cfservice_instance_repository.go +++ b/api/handlers/fake/cfservice_instance_repository.go @@ -71,6 +71,21 @@ type CFServiceInstanceRepository struct { result1 repositories.ServiceInstanceRecord result2 error } + GetServiceInstanceCredentialsStub func(context.Context, authorization.Info, string) (map[string]any, error) + getServiceInstanceCredentialsMutex sync.RWMutex + getServiceInstanceCredentialsArgsForCall []struct { + arg1 context.Context + arg2 authorization.Info + arg3 string + } + getServiceInstanceCredentialsReturns struct { + result1 map[string]any + result2 error + } + getServiceInstanceCredentialsReturnsOnCall map[int]struct { + result1 map[string]any + result2 error + } ListServiceInstancesStub func(context.Context, authorization.Info, repositories.ListServiceInstanceMessage) ([]repositories.ServiceInstanceRecord, error) listServiceInstancesMutex sync.RWMutex listServiceInstancesArgsForCall []struct { @@ -369,6 +384,72 @@ func (fake *CFServiceInstanceRepository) GetServiceInstanceReturnsOnCall(i int, }{result1, result2} } +func (fake *CFServiceInstanceRepository) GetServiceInstanceCredentials(arg1 context.Context, arg2 authorization.Info, arg3 string) (map[string]any, error) { + fake.getServiceInstanceCredentialsMutex.Lock() + ret, specificReturn := fake.getServiceInstanceCredentialsReturnsOnCall[len(fake.getServiceInstanceCredentialsArgsForCall)] + fake.getServiceInstanceCredentialsArgsForCall = append(fake.getServiceInstanceCredentialsArgsForCall, struct { + arg1 context.Context + arg2 authorization.Info + arg3 string + }{arg1, arg2, arg3}) + stub := fake.GetServiceInstanceCredentialsStub + fakeReturns := fake.getServiceInstanceCredentialsReturns + fake.recordInvocation("GetServiceInstanceCredentials", []interface{}{arg1, arg2, arg3}) + fake.getServiceInstanceCredentialsMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *CFServiceInstanceRepository) GetServiceInstanceCredentialsCallCount() int { + fake.getServiceInstanceCredentialsMutex.RLock() + defer fake.getServiceInstanceCredentialsMutex.RUnlock() + return len(fake.getServiceInstanceCredentialsArgsForCall) +} + +func (fake *CFServiceInstanceRepository) GetServiceInstanceCredentialsCalls(stub func(context.Context, authorization.Info, string) (map[string]any, error)) { + fake.getServiceInstanceCredentialsMutex.Lock() + defer fake.getServiceInstanceCredentialsMutex.Unlock() + fake.GetServiceInstanceCredentialsStub = stub +} + +func (fake *CFServiceInstanceRepository) GetServiceInstanceCredentialsArgsForCall(i int) (context.Context, authorization.Info, string) { + fake.getServiceInstanceCredentialsMutex.RLock() + defer fake.getServiceInstanceCredentialsMutex.RUnlock() + argsForCall := fake.getServiceInstanceCredentialsArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *CFServiceInstanceRepository) GetServiceInstanceCredentialsReturns(result1 map[string]any, result2 error) { + fake.getServiceInstanceCredentialsMutex.Lock() + defer fake.getServiceInstanceCredentialsMutex.Unlock() + fake.GetServiceInstanceCredentialsStub = nil + fake.getServiceInstanceCredentialsReturns = struct { + result1 map[string]any + result2 error + }{result1, result2} +} + +func (fake *CFServiceInstanceRepository) GetServiceInstanceCredentialsReturnsOnCall(i int, result1 map[string]any, result2 error) { + fake.getServiceInstanceCredentialsMutex.Lock() + defer fake.getServiceInstanceCredentialsMutex.Unlock() + fake.GetServiceInstanceCredentialsStub = nil + if fake.getServiceInstanceCredentialsReturnsOnCall == nil { + fake.getServiceInstanceCredentialsReturnsOnCall = make(map[int]struct { + result1 map[string]any + result2 error + }) + } + fake.getServiceInstanceCredentialsReturnsOnCall[i] = struct { + result1 map[string]any + result2 error + }{result1, result2} +} + func (fake *CFServiceInstanceRepository) ListServiceInstances(arg1 context.Context, arg2 authorization.Info, arg3 repositories.ListServiceInstanceMessage) ([]repositories.ServiceInstanceRecord, error) { fake.listServiceInstancesMutex.Lock() ret, specificReturn := fake.listServiceInstancesReturnsOnCall[len(fake.listServiceInstancesArgsForCall)] @@ -512,6 +593,8 @@ func (fake *CFServiceInstanceRepository) Invocations() map[string][][]interface{ defer fake.deleteServiceInstanceMutex.RUnlock() fake.getServiceInstanceMutex.RLock() defer fake.getServiceInstanceMutex.RUnlock() + fake.getServiceInstanceCredentialsMutex.RLock() + defer fake.getServiceInstanceCredentialsMutex.RUnlock() fake.listServiceInstancesMutex.RLock() defer fake.listServiceInstancesMutex.RUnlock() fake.patchServiceInstanceMutex.RLock() diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index ecca5a85e..1f2331f5b 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -21,8 +21,9 @@ import ( ) const ( - ServiceInstancesPath = "/v3/service_instances" - ServiceInstancePath = "/v3/service_instances/{guid}" + ServiceInstancesPath = "/v3/service_instances" + ServiceInstancePath = "/v3/service_instances/{guid}" + ServiceInstanceCredentialsPath = ServiceInstancePath + "/credentials" ) //counterfeiter:generate -o fake -fake-name CFServiceInstanceRepository . CFServiceInstanceRepository @@ -32,6 +33,7 @@ type CFServiceInstanceRepository interface { PatchServiceInstance(context.Context, authorization.Info, repositories.PatchServiceInstanceMessage) (repositories.ServiceInstanceRecord, error) ListServiceInstances(context.Context, authorization.Info, repositories.ListServiceInstanceMessage) ([]repositories.ServiceInstanceRecord, error) GetServiceInstance(context.Context, authorization.Info, string) (repositories.ServiceInstanceRecord, error) + GetServiceInstanceCredentials(context.Context, authorization.Info, string) (map[string]any, error) DeleteServiceInstance(context.Context, authorization.Info, repositories.DeleteServiceInstanceMessage) (repositories.ServiceInstanceRecord, error) } @@ -62,6 +64,50 @@ func NewServiceInstance( } } +func (h *ServiceInstance) get(r *http.Request) (*routing.Response, error) { + authInfo, _ := authorization.InfoFromContext(r.Context()) + logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.service-instance.get") + + payload := new(payloads.ServiceInstanceGet) + err := h.requestValidator.DecodeAndValidateURLValues(r, payload) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "Unable to decode request query parameters") + } + + serviceInstanceGUID := routing.URLParam(r, "guid") + + serviceInstance, err := h.serviceInstanceRepo.GetServiceInstance(r.Context(), authInfo, serviceInstanceGUID) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance") + } + + includedResources, err := h.includeResolver.ResolveIncludes(r.Context(), authInfo, []repositories.ServiceInstanceRecord{serviceInstance}, payload.IncludeResourceRules) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "failed to build included resources") + } + + return routing.NewResponse(http.StatusOK).WithBody(presenter.ForServiceInstance(serviceInstance, h.serverURL, includedResources...)), nil +} + +func (h *ServiceInstance) getCredentials(r *http.Request) (*routing.Response, error) { + authInfo, _ := authorization.InfoFromContext(r.Context()) + logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.service-instance.get-credentials") + + serviceInstanceGUID := routing.URLParam(r, "guid") + + serviceInstance, err := h.serviceInstanceRepo.GetServiceInstance(r.Context(), authInfo, serviceInstanceGUID) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance") + } + + credentials, err := h.serviceInstanceRepo.GetServiceInstanceCredentials(r.Context(), authInfo, serviceInstance.SecretName) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance credentials") + } + + return routing.NewResponse(http.StatusOK).WithBody(credentials), nil +} + //nolint:dupl func (h *ServiceInstance) create(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) @@ -199,6 +245,8 @@ func (h *ServiceInstance) AuthenticatedRoutes() []routing.Route { {Method: "POST", Pattern: ServiceInstancesPath, Handler: h.create}, {Method: "PATCH", Pattern: ServiceInstancePath, Handler: h.patch}, {Method: "GET", Pattern: ServiceInstancesPath, Handler: h.list}, + {Method: "GET", Pattern: ServiceInstancePath, Handler: h.get}, + {Method: "GET", Pattern: ServiceInstanceCredentialsPath, Handler: h.getCredentials}, {Method: "DELETE", Pattern: ServiceInstancePath, Handler: h.delete}, } } diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index 3a4e92ccb..6b33fed26 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -73,6 +73,286 @@ var _ = Describe("ServiceInstance", func() { routerBuilder.Build().ServeHTTP(rr, req) }) + Describe("GET /v3/service_instances/:guid", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", + Type: korifiv1alpha1.UserProvidedType, + }, nil) + + reqPath += "/service-instance-guid" + }) + + It("gets the service instance", func() { + Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, actualServiceInstanceGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualServiceInstanceGUID).To(Equal("service-instance-guid")) + + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.guid", "service-instance-guid"), + MatchJSONPath("$.type", "user-provided"), + ))) + }) + + When("getting the service instance fails with not found", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), + ) + }) + + It("returns 404 Not Found", func() { + expectNotFoundError("Service Instance") + }) + }) + + When("getting the service instance fails with forbidden", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewForbiddenError(nil, repositories.ServiceInstanceResourceType), + ) + }) + + It("returns 404 Not Found", func() { + expectNotAuthorizedError() + }) + }) + + When("the query is invalid", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateURLValuesReturns(errors.New("boom")) + }) + + It("returns an error", func() { + expectUnknownError() + }) + }) + + Describe("fields", func() { + BeforeEach(func() { + serviceOfferingRepo.ListOfferingsReturns([]repositories.ServiceOfferingRecord{{ + ServiceOffering: services.ServiceOffering{ + Name: "service-offering-name", + }, + CFResource: model.CFResource{ + GUID: "service-offering-guid", + }, + ServiceBrokerGUID: "service-broker-guid", + }}, nil) + + servicePlanRepo.ListPlansReturns([]repositories.ServicePlanRecord{{ + ServicePlan: services.ServicePlan{ + Name: "service-plan-name", + }, + CFResource: model.CFResource{ + GUID: "service-plan-guid", + }, + ServiceOfferingGUID: "service-offering-guid", + }}, nil) + + serviceBrokerRepo.ListServiceBrokersReturns([]repositories.ServiceBrokerRecord{{ + ServiceBroker: services.ServiceBroker{ + Name: "service-broker-name", + }, + CFResource: model.CFResource{ + GUID: "service-broker-guid", + }, + }}, nil) + }) + + When("params to inlude fields[service_plan]", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ServiceInstanceGet{ + IncludeResourceRules: []params.IncludeResourceRule{{ + RelationshipPath: []string{"service_plan"}, + Fields: []string{"name", "guid"}, + }}, + }) + }) + + It("does not include resources", func() { + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPBody(Not(ContainSubstring("included")))) + }) + + When("the service instance is managed", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", Type: korifiv1alpha1.ManagedType, PlanGUID: "service-plan-guid", + }, nil) + }) + + It("includes offering fields in the response", func() { + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.included.service_plans[0].guid", "service-plan-guid"), + MatchJSONPath("$.included.service_plans[0].name", "service-plan-name"), + ))) + }) + }) + }) + + When("params to inlude fields[service_plan.service_offering]", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ServiceInstanceGet{ + IncludeResourceRules: []params.IncludeResourceRule{{ + RelationshipPath: []string{"service_plan", "service_offering"}, + Fields: []string{"name", "guid", "relationships.service_broker"}, + }}, + }) + }) + + It("does not include resources", func() { + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPBody(Not(ContainSubstring("included")))) + }) + + When("the service instance is managed", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", Type: korifiv1alpha1.ManagedType, PlanGUID: "service-plan-guid", + }, nil) + }) + + It("includes offering fields in the response", func() { + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.included.service_offerings[0].guid", "service-offering-guid"), + MatchJSONPath("$.included.service_offerings[0].name", "service-offering-name"), + MatchJSONPath("$.included.service_offerings[0].relationships.service_broker.data.guid", "service-broker-guid"), + ))) + }) + }) + }) + + When("params to inlude fields[service_plan.service_offering.service_broker]", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ServiceInstanceGet{ + IncludeResourceRules: []params.IncludeResourceRule{{ + RelationshipPath: []string{"service_plan", "service_offering", "service_broker"}, + Fields: []string{"name", "guid"}, + }}, + }) + }) + + It("does not include resources", func() { + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPBody(Not(ContainSubstring("included")))) + }) + + When("the service instance is managed", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", Type: korifiv1alpha1.ManagedType, PlanGUID: "service-plan-guid", + }, nil) + }) + + It("includes broker fields in the response", func() { + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.included.service_brokers[0].guid", "service-broker-guid"), + MatchJSONPath("$.included.service_brokers[0].name", "service-broker-name"), + ))) + }) + }) + }) + }) + }) + + Describe("GET /v3/service_instances/:guid/credentials", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", + Type: korifiv1alpha1.UserProvidedType, + SecretName: "secret-name", + }, nil) + + serviceInstanceRepo.GetServiceInstanceCredentialsReturns(map[string]any{ + "foo": "bar", + }, nil) + + reqPath += "/service-instance-guid/credentials" + }) + + It("gets the service instance credentials", func() { + Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) + Expect(serviceInstanceRepo.GetServiceInstanceCredentialsCallCount()).To(Equal(1)) + + _, actualAuthInfo, actualServiceInstanceGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualServiceInstanceGUID).To(Equal("service-instance-guid")) + + _, actualAuthInfo, actualSecretName := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualSecretName).To(Equal("secret-name")) + + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.foo", "bar"), + ))) + }) + + When("the service instance does not have credentials", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", + Type: korifiv1alpha1.UserProvidedType, + }, nil) + + serviceInstanceRepo.GetServiceInstanceCredentialsReturns(map[string]any{}, apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType)) + }) + + It("returns 404 Not Found", func() { + _, actualAuthInfo, actualSecretName := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualSecretName).To(Equal("")) + expectNotFoundError("Service Instance") + }) + }) + + When("getting the service instance fails with not found", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), + ) + }) + + It("returns 404 Not Found", func() { + expectNotFoundError("Service Instance") + }) + }) + + When("getting the service instance fails with forbidden", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewForbiddenError(nil, repositories.ServiceInstanceResourceType), + ) + }) + + It("returns 404 Not Found", func() { + expectNotAuthorizedError() + }) + }) + + When("the unmarshaling of the secret fails", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceCredentialsReturns(map[string]any{}, apierrors.NewUnprocessableEntityError(nil, "failed to decode")) + }) + + It("returns an error", func() { + expectUnprocessableEntityError("failed to decode") + }) + }) + }) + Describe("the POST /v3/service_instances endpoint", func() { BeforeEach(func() { reqMethod = http.MethodPost diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index 890ee1b37..4e0df8452 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -102,6 +102,61 @@ func (r ServiceInstanceRelationships) ValidateManagedRelationships() error { ) } +type ServiceInstanceGet struct { + IncludeResourceRules []params.IncludeResourceRule +} + +func (g ServiceInstanceGet) Validate() error { + return jellidation.ValidateStruct(&g, + jellidation.Field(&g.IncludeResourceRules, jellidation.Each(jellidation.By(func(value any) error { + rule, ok := value.(params.IncludeResourceRule) + if !ok { + return fmt.Errorf("%T is not supported, IncludeResourceRule is expected", value) + } + + relationshipsPath := strings.Join(rule.RelationshipPath, ".") + switch relationshipsPath { + case "service_plan": + return jellidation.Each(validation.OneOf( + "guid", + "name", + "relationships.service_offering", + )).Validate(rule.Fields) + case "service_plan.service_offering": + return jellidation.Each(validation.OneOf( + "guid", + "name", + "relationships.service_broker", + )).Validate(rule.Fields) + case "service_plan.service_offering.service_broker": + return jellidation.Each(validation.OneOf( + "guid", + "name", + )).Validate(rule.Fields) + } + return validation.OneOf( + "service_plan", + "service_plan.service_offering", + "service_plan.service_offering.service_broker", + ).Validate(relationshipsPath) + }))), + ) +} + +func (g *ServiceInstanceGet) SupportedKeys() []string { + return []string{ + "fields[service_plan.service_offering]", + "fields[service_plan.service_offering.service_broker]", + "fields[service_plan]", + } +} + +func (g *ServiceInstanceGet) DecodeFromURLValues(values url.Values) error { + g.IncludeResourceRules = append(g.IncludeResourceRules, params.ParseFields(values)...) + + return nil +} + type ServiceInstancePatch struct { Name *string `json:"name,omitempty"` Tags *[]string `json:"tags,omitempty"` diff --git a/api/payloads/service_instance_test.go b/api/payloads/service_instance_test.go index 8e1aba522..d5d1e66ee 100644 --- a/api/payloads/service_instance_test.go +++ b/api/payloads/service_instance_test.go @@ -97,6 +97,46 @@ var _ = Describe("ServiceInstanceList", func() { })) }) }) + + _ = Describe("ServiceInstanceGet", func() { + DescribeTable("valid query", + func(query string, expectedServiceInstanceGet payloads.ServiceInstanceGet) { + actualServiceInstanceGet, decodeErr := decodeQuery[payloads.ServiceInstanceGet](query) + + Expect(decodeErr).NotTo(HaveOccurred()) + Expect(*actualServiceInstanceGet).To(Equal(expectedServiceInstanceGet)) + }, + Entry("fields[service_plan.service_offering.service_broker]", + "fields[service_plan.service_offering.service_broker]=guid,name", + payloads.ServiceInstanceGet{IncludeResourceRules: []params.IncludeResourceRule{{ + RelationshipPath: []string{"service_plan", "service_offering", "service_broker"}, + Fields: []string{"guid", "name"}, + }}}), + Entry("fields[service_plan.service_offering]", + "fields[service_plan.service_offering]=guid,name,relationships.service_broker", + payloads.ServiceInstanceGet{IncludeResourceRules: []params.IncludeResourceRule{{ + RelationshipPath: []string{"service_plan", "service_offering"}, + Fields: []string{"guid", "name", "relationships.service_broker"}, + }}}), + + Entry("fields[service_plan]", + "fields[service_plan]=guid,name,relationships.service_offering", + payloads.ServiceInstanceGet{IncludeResourceRules: []params.IncludeResourceRule{{ + RelationshipPath: []string{"service_plan"}, + Fields: []string{"guid", "name", "relationships.service_offering"}, + }}}), + ) + + DescribeTable("invalid query", + func(query string, expectedErrMsg string) { + _, decodeErr := decodeQuery[payloads.ServiceInstanceGet](query) + Expect(decodeErr).To(MatchError(ContainSubstring(expectedErrMsg))) + }, + Entry("invalid service offering fields", "fields[service_plan.service_offering]=foo", "value must be one of"), + Entry("invalid service broker fields", "fields[service_plan.service_offering.service_broker]=foo", "value must be one of"), + Entry("invalid service plan fields", "fields[service_plan]=foo", "value must be one of"), + ) + }) }) var _ = Describe("ServiceInstanceCreate", func() { diff --git a/api/presenter/service_instance.go b/api/presenter/service_instance.go index a9d653e02..3e585a567 100644 --- a/api/presenter/service_instance.go +++ b/api/presenter/service_instance.go @@ -26,6 +26,7 @@ type ServiceInstanceResponse struct { Relationships map[string]model.ToOneRelationship `json:"relationships"` Metadata Metadata `json:"metadata"` Links ServiceInstanceLinks `json:"links"` + Included map[string][]any `json:"included,omitempty"` } type lastOperation struct { @@ -86,5 +87,6 @@ func ForServiceInstance(serviceInstanceRecord repositories.ServiceInstanceRecord HRef: buildURL(baseURL).appendPath(serviceRouteBindingsBase).setQuery("service_instance_guids=" + serviceInstanceRecord.GUID).build(), }, }, + Included: includedResources(includes...), } } diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 583bc5919..9336f1523 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -456,6 +456,36 @@ func (r *ServiceInstanceRepo) GetServiceInstance(ctx context.Context, authInfo a return cfServiceInstanceToRecord(*serviceInstance), nil } +func (r *ServiceInstanceRepo) GetServiceInstanceCredentials(ctx context.Context, authInfo authorization.Info, secretName string) (map[string]any, error) { + userClient, err := r.userClientFactory.BuildClient(authInfo) + if err != nil { + return map[string]any{}, fmt.Errorf("failed to build user client: %w", err) + } + + namespace, err := r.namespaceRetriever.NamespaceFor(ctx, secretName, ServiceInstanceResourceType) + if err != nil { + return map[string]any{}, fmt.Errorf("failed to get namespace for service instance: %w", err) + } + + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: namespace, + }, + } + + if err = userClient.Get(ctx, client.ObjectKeyFromObject(credentialsSecret), credentialsSecret); err != nil { + return map[string]any{}, fmt.Errorf("failed to get credentials secret for service instance: %w", apierrors.FromK8sError(err, ServiceInstanceResourceType)) + } + + credentials, err := tools.ToDecodedSecretDataCredentials(credentialsSecret.Data) + if err != nil { + return map[string]any{}, fmt.Errorf("failed to decode credentials secret for service instance: %w", err) + } + + return credentials, nil +} + func (r *ServiceInstanceRepo) DeleteServiceInstance(ctx context.Context, authInfo authorization.Info, message DeleteServiceInstanceMessage) (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 1097fb5f6..436aeb4c1 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -971,6 +971,78 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) + Describe("GetServiceInstanceCredentials", func() { + var ( + secretName string + secretData map[string][]byte + credential map[string]any + secret *corev1.Secret + // serviceInstance *korifiv1alpha1.CFServiceInstance + getErr error + decodeErr error + ) + + BeforeEach(func() { + secretName = prefixedGUID("secret") + + _ = createServiceInstanceCR(ctx, k8sClient, secretName, space.Name, "the-service-instance", secretName) + secretData, decodeErr = tools.ToCredentialsSecretData(map[string]any{"foo": "bar"}) + + secret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: space.Name, + }, + Data: secretData, + } + Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + }) + + JustBeforeEach(func() { + credential, getErr = serviceInstanceRepo.GetServiceInstanceCredentials(ctx, authInfo, secretName) + }) + + When("there are not permissions on the secret", func() { + It("returns a forbidden error", func() { + Expect(errors.As(getErr, &apierrors.ForbiddenError{})).To(BeTrue()) + }) + }) + + When("the user has permissions to get the secret", func() { + BeforeEach(func() { + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) + }) + + It("returns the correct secret", func() { + Expect(getErr).ToNot(HaveOccurred()) + Expect(decodeErr).ToNot(HaveOccurred()) + Expect(credential).To(HaveKeyWithValue("foo", "bar")) + }) + + When("the secret does not exist", func() { + BeforeEach(func() { + secretName = "does-not-exist" + }) + + It("returns a 404 error", func() { + Expect(errors.As(getErr, &apierrors.NotFoundError{})).To(BeTrue()) + }) + }) + }) + + When("the context has expired", func() { + BeforeEach(func() { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + cancel() + }) + + It("returns a error", func() { + Expect(getErr).To(HaveOccurred()) + }) + }) + }) + Describe("DeleteServiceInstance", func() { var ( serviceInstance *korifiv1alpha1.CFServiceInstance diff --git a/tools/credentials.go b/tools/credentials.go index 355cf6028..da6705cf3 100644 --- a/tools/credentials.go +++ b/tools/credentials.go @@ -18,3 +18,13 @@ func ToCredentialsSecretData(credentials any) (map[string][]byte, error) { CredentialsSecretKey: credentialBytes, }, nil } + +func ToDecodedSecretDataCredentials(data map[string][]byte) (map[string]any, error) { + var credentials map[string]any + err := json.Unmarshal(data[CredentialsSecretKey], &credentials) + if err != nil { + return nil, errors.New("failed to unmarshal credentials for service instance") + } + + return credentials, nil +} diff --git a/tools/credentials_test.go b/tools/credentials_test.go index 0debccef2..c604bdded 100644 --- a/tools/credentials_test.go +++ b/tools/credentials_test.go @@ -16,8 +16,10 @@ type credsType struct { var _ = Describe("Credentials", func() { var ( - credsObject any - secretData map[string][]byte + credsObject any + secretData map[string][]byte + decodedCredentials map[string]any + err error ) BeforeEach(func() { @@ -27,22 +29,35 @@ var _ = Describe("Credentials", func() { InBar: "in-bar", }, } - }) - JustBeforeEach(func() { - var err error secretData, err = tools.ToCredentialsSecretData(credsObject) - Expect(err).NotTo(HaveOccurred()) }) - It("creates credentials secret data", func() { - Expect(secretData).To(MatchAllKeys(Keys{ - tools.CredentialsSecretKey: MatchJSON(`{ - "Foo": "foo", - "Bar": { - "InBar": "in-bar" - } - }`), - })) + When("ToCredentialsSecretData", func() { + It("successfully creates credentials secret data", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(secretData).To(MatchAllKeys(Keys{ + tools.CredentialsSecretKey: MatchJSON(`{ + "Foo": "foo", + "Bar": { + "InBar": "in-bar" + } + }`), + })) + }) + }) + + When("ToDecodedSecretDataCredentials", func() { + BeforeEach(func() { + decodedCredentials, err = tools.ToDecodedSecretDataCredentials(secretData) + Expect(err).NotTo(HaveOccurred()) + }) + + It("successfully decodes credentials from secret data", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(decodedCredentials).To(HaveKeyWithValue("Foo", "foo")) + Expect(decodedCredentials).To(HaveKey("Bar")) + Expect(decodedCredentials["Bar"]).To(HaveKeyWithValue("InBar", "in-bar")) + }) }) }) From 0df3fb3585e143525802273e6f13a52c340614fd Mon Sep 17 00:00:00 2001 From: Konstantin Lapkov Date: Thu, 12 Dec 2024 10:23:22 +0200 Subject: [PATCH 2/4] Address comments --- api/handlers/service_instance.go | 10 ++-- api/handlers/service_instance_test.go | 39 +++++++++------ api/payloads/service_instance.go | 2 +- .../service_instance_repository.go | 13 +++-- .../service_instance_repository_test.go | 49 +++++++++---------- helm/korifi/controllers/role.yaml | 1 - tests/e2e/service_instances_test.go | 47 ++++++++++++++++-- tools/credentials.go | 2 +- tools/credentials_test.go | 30 ++++++++---- 9 files changed, 128 insertions(+), 65 deletions(-) diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index 1f2331f5b..fdc07413d 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -23,7 +23,7 @@ import ( const ( ServiceInstancesPath = "/v3/service_instances" ServiceInstancePath = "/v3/service_instances/{guid}" - ServiceInstanceCredentialsPath = ServiceInstancePath + "/credentials" + ServiceInstanceCredentialsPath = "/v3/service_instances/{guid}/credentials" ) //counterfeiter:generate -o fake -fake-name CFServiceInstanceRepository . CFServiceInstanceRepository @@ -97,10 +97,14 @@ func (h *ServiceInstance) getCredentials(r *http.Request) (*routing.Response, er serviceInstance, err := h.serviceInstanceRepo.GetServiceInstance(r.Context(), authInfo, serviceInstanceGUID) if err != nil { - return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance") + return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "failed to get service instance", "GUID", serviceInstanceGUID) + } + + if serviceInstance.Type != korifiv1alpha1.UserProvidedType { + return nil, apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType) } - credentials, err := h.serviceInstanceRepo.GetServiceInstanceCredentials(r.Context(), authInfo, serviceInstance.SecretName) + credentials, err := h.serviceInstanceRepo.GetServiceInstanceCredentials(r.Context(), authInfo, serviceInstanceGUID) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance credentials") } diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index 6b33fed26..c89ada61f 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -97,16 +97,13 @@ var _ = Describe("ServiceInstance", func() { ))) }) - When("getting the service instance fails with not found", func() { + When("getting the service instance fails with an error", func() { BeforeEach(func() { - serviceInstanceRepo.GetServiceInstanceReturns( - repositories.ServiceInstanceRecord{}, - apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), - ) + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("boom")) }) - It("returns 404 Not Found", func() { - expectNotFoundError("Service Instance") + It("returns an error", func() { + expectUnknownError() }) }) @@ -118,7 +115,7 @@ var _ = Describe("ServiceInstance", func() { ) }) - It("returns 404 Not Found", func() { + It("returns an not authorized error", func() { expectNotAuthorizedError() }) }) @@ -281,15 +278,14 @@ var _ = Describe("ServiceInstance", func() { It("gets the service instance credentials", func() { Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) - Expect(serviceInstanceRepo.GetServiceInstanceCredentialsCallCount()).To(Equal(1)) - _, actualAuthInfo, actualServiceInstanceGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) Expect(actualServiceInstanceGUID).To(Equal("service-instance-guid")) - _, actualAuthInfo, actualSecretName := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0) + Expect(serviceInstanceRepo.GetServiceInstanceCredentialsCallCount()).To(Equal(1)) + _, actualAuthInfo, actualInstanceGUID := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(actualSecretName).To(Equal("secret-name")) + Expect(actualInstanceGUID).To(Equal("service-instance-guid")) Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) @@ -298,6 +294,19 @@ var _ = Describe("ServiceInstance", func() { ))) }) + When("the service instance is not user-provided", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", + Type: korifiv1alpha1.ManagedType, + }, nil) + }) + + It("returns a 404 Not Found error", func() { + expectNotFoundError("Service Instance") + }) + }) + When("the service instance does not have credentials", func() { BeforeEach(func() { serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ @@ -309,9 +318,9 @@ var _ = Describe("ServiceInstance", func() { }) It("returns 404 Not Found", func() { - _, actualAuthInfo, actualSecretName := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0) + _, actualAuthInfo, actualInstanceGUID := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(actualSecretName).To(Equal("")) + Expect(actualInstanceGUID).To(Equal("service-instance-guid")) expectNotFoundError("Service Instance") }) }) @@ -338,7 +347,7 @@ var _ = Describe("ServiceInstance", func() { }) It("returns 404 Not Found", func() { - expectNotAuthorizedError() + expectNotFoundError("Service Instance") }) }) diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index 4e0df8452..f842fe695 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -152,7 +152,7 @@ func (g *ServiceInstanceGet) SupportedKeys() []string { } func (g *ServiceInstanceGet) DecodeFromURLValues(values url.Values) error { - g.IncludeResourceRules = append(g.IncludeResourceRules, params.ParseFields(values)...) + g.IncludeResourceRules = params.ParseFields(values) return nil } diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index bd15aa0c9..71a1a4b94 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -457,20 +457,25 @@ func (r *ServiceInstanceRepo) GetServiceInstance(ctx context.Context, authInfo a return cfServiceInstanceToRecord(*serviceInstance), nil } -func (r *ServiceInstanceRepo) GetServiceInstanceCredentials(ctx context.Context, authInfo authorization.Info, secretName string) (map[string]any, error) { +func (r *ServiceInstanceRepo) GetServiceInstanceCredentials(ctx context.Context, authInfo authorization.Info, instanceGUID string) (map[string]any, error) { userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return map[string]any{}, fmt.Errorf("failed to build user client: %w", err) } - namespace, err := r.namespaceRetriever.NamespaceFor(ctx, secretName, ServiceInstanceResourceType) + namespace, err := r.namespaceRetriever.NamespaceFor(ctx, instanceGUID, ServiceInstanceResourceType) if err != nil { return map[string]any{}, fmt.Errorf("failed to get namespace for service instance: %w", err) } + serviceInstance := &korifiv1alpha1.CFServiceInstance{} + if err = userClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: instanceGUID}, serviceInstance); err != nil { + return map[string]any{}, fmt.Errorf("failed to get service instance: %w", apierrors.FromK8sError(err, ServiceInstanceResourceType)) + } + credentialsSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secretName, + Name: serviceInstance.Spec.SecretName, Namespace: namespace, }, } @@ -479,7 +484,7 @@ func (r *ServiceInstanceRepo) GetServiceInstanceCredentials(ctx context.Context, return map[string]any{}, fmt.Errorf("failed to get credentials secret for service instance: %w", apierrors.FromK8sError(err, ServiceInstanceResourceType)) } - credentials, err := tools.ToDecodedSecretDataCredentials(credentialsSecret.Data) + credentials, err := tools.FromCredentialsSecretData(credentialsSecret.Data) if err != nil { return map[string]any{}, fmt.Errorf("failed to decode credentials secret for service instance: %w", err) } diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index d515a86d5..ea6545cb3 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -1018,20 +1018,32 @@ var _ = Describe("ServiceInstanceRepository", func() { Describe("GetServiceInstanceCredentials", func() { var ( - secretName string - secretData map[string][]byte - credential map[string]any - secret *corev1.Secret - // serviceInstance *korifiv1alpha1.CFServiceInstance - getErr error - decodeErr error + instanceGUID string + secretName string + credential map[string]any + secret *corev1.Secret + getErr error ) BeforeEach(func() { + instanceGUID = prefixedGUID("service-instance") secretName = prefixedGUID("secret") - _ = createServiceInstanceCR(ctx, k8sClient, secretName, space.Name, "the-service-instance", secretName) - secretData, decodeErr = tools.ToCredentialsSecretData(map[string]any{"foo": "bar"}) + serviceInstance := &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: instanceGUID, + Namespace: space.Name, + }, + Spec: korifiv1alpha1.CFServiceInstanceSpec{ + SecretName: secretName, + Type: "user-provided", + Tags: []string{"database", "mysql"}, + }, + } + Expect(k8sClient.Create(ctx, serviceInstance)).To(Succeed()) + + secretData, decodeErr := tools.ToCredentialsSecretData(map[string]any{"foo": "bar"}) + Expect(decodeErr).ToNot(HaveOccurred()) secret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -1044,7 +1056,7 @@ var _ = Describe("ServiceInstanceRepository", func() { }) JustBeforeEach(func() { - credential, getErr = serviceInstanceRepo.GetServiceInstanceCredentials(ctx, authInfo, secretName) + credential, getErr = serviceInstanceRepo.GetServiceInstanceCredentials(ctx, authInfo, instanceGUID) }) When("there are not permissions on the secret", func() { @@ -1060,13 +1072,12 @@ var _ = Describe("ServiceInstanceRepository", func() { It("returns the correct secret", func() { Expect(getErr).ToNot(HaveOccurred()) - Expect(decodeErr).ToNot(HaveOccurred()) Expect(credential).To(HaveKeyWithValue("foo", "bar")) }) - When("the secret does not exist", func() { + When("the service instance does not exist", func() { BeforeEach(func() { - secretName = "does-not-exist" + instanceGUID = "does-not-exist" }) It("returns a 404 error", func() { @@ -1074,18 +1085,6 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) }) - - When("the context has expired", func() { - BeforeEach(func() { - var cancel context.CancelFunc - ctx, cancel = context.WithCancel(ctx) - cancel() - }) - - It("returns a error", func() { - Expect(getErr).To(HaveOccurred()) - }) - }) }) Describe("DeleteServiceInstance", func() { diff --git a/helm/korifi/controllers/role.yaml b/helm/korifi/controllers/role.yaml index 45d93023b..ad805b568 100644 --- a/helm/korifi/controllers/role.yaml +++ b/helm/korifi/controllers/role.yaml @@ -161,7 +161,6 @@ rules: - cfservicebindings/status - cfservicebrokers/status - cfserviceinstances/status - - cfserviceinstances/status - cfspaces/status - cftasks/status verbs: diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index bb2a6ad43..1889a7992 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -14,16 +14,18 @@ import ( var _ = Describe("Service Instances", func() { var ( - spaceGUID string - upsiGUID string - upsiName string - httpResp *resty.Response - httpError error + spaceGUID string + upsiGUID string + upsiWithCredsGUID string + upsiName string + httpResp *resty.Response + httpError error ) BeforeEach(func() { spaceGUID = createSpace(generateGUID("space1"), commonTestOrgGUID) upsiName = generateGUID("service-instance") + upsiWithCredsGUID = generateGUID("service-instance-creds") upsiGUID = createServiceInstance(spaceGUID, upsiName, nil) }) @@ -31,6 +33,41 @@ var _ = Describe("Service Instances", func() { deleteSpace(spaceGUID) }) + Describe("Get", func() { + var result serviceInstanceResource + + BeforeEach(func() { + httpResp, httpError = adminClient.R().SetResult(&result).Get("/v3/service_instances/" + upsiGUID) + }) + + It("gets the service instance", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusOK)) + + Expect(result.GUID).To(Equal(upsiGUID)) + Expect(result.Name).To(Equal(upsiName)) + }) + }) + + Describe("GetCredentials", func() { + var result map[string]any + + BeforeEach(func() { + upsiWithCredsGUID = createServiceInstance(spaceGUID, generateGUID("service-instance2"), map[string]string{"a": "b"}) + }) + + JustBeforeEach(func() { + httpResp, httpError = adminClient.R().SetResult(&result).Get("/v3/service_instances/" + upsiWithCredsGUID + "/credentials") + }) + + It("returns the service instance credentials", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusOK)) + + Expect(result).To(Equal(map[string]any{"a": "b"})) + }) + }) + Describe("Create", func() { var ( instanceName string diff --git a/tools/credentials.go b/tools/credentials.go index da6705cf3..87ba6bda7 100644 --- a/tools/credentials.go +++ b/tools/credentials.go @@ -19,7 +19,7 @@ func ToCredentialsSecretData(credentials any) (map[string][]byte, error) { }, nil } -func ToDecodedSecretDataCredentials(data map[string][]byte) (map[string]any, error) { +func FromCredentialsSecretData(data map[string][]byte) (map[string]any, error) { var credentials map[string]any err := json.Unmarshal(data[CredentialsSecretKey], &credentials) if err != nil { diff --git a/tools/credentials_test.go b/tools/credentials_test.go index c604bdded..8739b7c7d 100644 --- a/tools/credentials_test.go +++ b/tools/credentials_test.go @@ -16,10 +16,8 @@ type credsType struct { var _ = Describe("Credentials", func() { var ( - credsObject any - secretData map[string][]byte - decodedCredentials map[string]any - err error + credsObject any + err error ) BeforeEach(func() { @@ -29,11 +27,15 @@ var _ = Describe("Credentials", func() { InBar: "in-bar", }, } - - secretData, err = tools.ToCredentialsSecretData(credsObject) }) - When("ToCredentialsSecretData", func() { + Describe("ToCredentialsSecretData", func() { + var secretData map[string][]byte + + BeforeEach(func() { + secretData, err = tools.ToCredentialsSecretData(credsObject) + }) + It("successfully creates credentials secret data", func() { Expect(err).NotTo(HaveOccurred()) Expect(secretData).To(MatchAllKeys(Keys{ @@ -47,14 +49,22 @@ var _ = Describe("Credentials", func() { }) }) - When("ToDecodedSecretDataCredentials", func() { + Describe("FromCredentialsSecretData", func() { + var ( + decodedCredentials map[string]any + secretData map[string][]byte + ) BeforeEach(func() { - decodedCredentials, err = tools.ToDecodedSecretDataCredentials(secretData) + secretData, err = tools.ToCredentialsSecretData(credsObject) Expect(err).NotTo(HaveOccurred()) }) - It("successfully decodes credentials from secret data", func() { + JustBeforeEach(func() { + decodedCredentials, err = tools.FromCredentialsSecretData(secretData) Expect(err).NotTo(HaveOccurred()) + }) + + It("successfully decodes credentials from secret data", func() { Expect(decodedCredentials).To(HaveKeyWithValue("Foo", "foo")) Expect(decodedCredentials).To(HaveKey("Bar")) Expect(decodedCredentials["Bar"]).To(HaveKeyWithValue("InBar", "in-bar")) From 76ed3177518f9e1a53fa92671138609c25b1aa20 Mon Sep 17 00:00:00 2001 From: Konstantin Lapkov Date: Thu, 12 Dec 2024 11:02:29 +0200 Subject: [PATCH 3/4] Remove SecretName from CFServiceInstance record --- api/handlers/service_instance_test.go | 5 ++--- api/presenter/service_instance_test.go | 15 +++++++-------- .../service_instance_repository.go | 2 -- .../service_instance_repository_test.go | 18 +++++++++--------- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index c89ada61f..2fd7e3aad 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -264,9 +264,8 @@ var _ = Describe("ServiceInstance", func() { Describe("GET /v3/service_instances/:guid/credentials", func() { BeforeEach(func() { serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ - GUID: "service-instance-guid", - Type: korifiv1alpha1.UserProvidedType, - SecretName: "secret-name", + GUID: "service-instance-guid", + Type: korifiv1alpha1.UserProvidedType, }, nil) serviceInstanceRepo.GetServiceInstanceCredentialsReturns(map[string]any{ diff --git a/api/presenter/service_instance_test.go b/api/presenter/service_instance_test.go index 8898e5606..c5f0fb3f1 100644 --- a/api/presenter/service_instance_test.go +++ b/api/presenter/service_instance_test.go @@ -26,14 +26,13 @@ var _ = Describe("Service Instance", func() { baseURL, err = url.Parse("https://api.example.org") Expect(err).NotTo(HaveOccurred()) record = repositories.ServiceInstanceRecord{ - Name: "service-instance-name", - GUID: "service-instance-guid", - SpaceGUID: "space-guid", - SecretName: "secret-name", - Tags: []string{"foo", "bar"}, - Type: "user-provided", - CreatedAt: time.UnixMilli(1000), - UpdatedAt: tools.PtrTo(time.UnixMilli(2000)), + Name: "service-instance-name", + GUID: "service-instance-guid", + SpaceGUID: "space-guid", + Tags: []string{"foo", "bar"}, + Type: "user-provided", + CreatedAt: time.UnixMilli(1000), + UpdatedAt: tools.PtrTo(time.UnixMilli(2000)), Labels: map[string]string{ "foo": "bar", }, diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 71a1a4b94..77dc1c148 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -174,7 +174,6 @@ type ServiceInstanceRecord struct { GUID string SpaceGUID string PlanGUID string - SecretName string Tags []string Type string Labels map[string]string @@ -586,7 +585,6 @@ func cfServiceInstanceToRecord(cfServiceInstance korifiv1alpha1.CFServiceInstanc GUID: cfServiceInstance.Name, SpaceGUID: cfServiceInstance.Namespace, PlanGUID: cfServiceInstance.Spec.PlanGUID, - SecretName: cfServiceInstance.Spec.SecretName, Tags: cfServiceInstance.Spec.Tags, Type: string(cfServiceInstance.Spec.Type), Labels: cfServiceInstance.Labels, diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index ea6545cb3..bd0612a7f 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -105,7 +105,6 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(record.Name).To(Equal(serviceInstanceName)) Expect(record.Type).To(Equal("user-provided")) Expect(record.Tags).To(ConsistOf([]string{"foo", "bar"})) - Expect(record.SecretName).NotTo(BeEmpty()) Expect(record.Relationships()).To(Equal(map[string]string{ "space": space.Name, })) @@ -134,10 +133,18 @@ var _ = Describe("ServiceInstanceRepository", func() { It("creates the credentials secret", func() { Expect(createErr).NotTo(HaveOccurred()) + cfServiceInstance := &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: record.SpaceGUID, + Name: record.GUID, + }, + } + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + credentialsSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Namespace: record.SpaceGUID, - Name: record.SecretName, + Name: cfServiceInstance.Spec.SecretName, }, } Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(credentialsSecret), credentialsSecret)).To(Succeed()) @@ -209,7 +216,6 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(record.Name).To(Equal(serviceInstanceName)) Expect(record.Type).To(Equal("managed")) Expect(record.Tags).To(ConsistOf([]string{"foo", "bar"})) - Expect(record.SecretName).To(BeEmpty()) Expect(record.Relationships()).To(Equal(map[string]string{ "service_plan": servicePlan.Name, "space": space.Name, @@ -659,11 +665,6 @@ var _ = Describe("ServiceInstanceRepository", func() { })).To(Succeed()) }) - It("updates the secret in the record", func() { - Expect(err).NotTo(HaveOccurred()) - Expect(serviceInstanceRecord.SecretName).To(Equal("foo")) - }) - It("updates the secret in the spec", func() { Expect(err).NotTo(HaveOccurred()) Eventually(func(g Gomega) { @@ -952,7 +953,6 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(record.Name).To(Equal(serviceInstance.Spec.DisplayName)) Expect(record.GUID).To(Equal(serviceInstance.Name)) Expect(record.SpaceGUID).To(Equal(serviceInstance.Namespace)) - Expect(record.SecretName).To(Equal(serviceInstance.Spec.SecretName)) Expect(record.Tags).To(Equal(serviceInstance.Spec.Tags)) Expect(record.Type).To(Equal(string(serviceInstance.Spec.Type))) Expect(record.Labels).To(Equal(map[string]string{"a-label": "a-label-value"})) From 4eaa4b7829e5fae17b06ee2dec670cad116b73ab Mon Sep 17 00:00:00 2001 From: Konstantin Lapkov Date: Fri, 13 Dec 2024 00:03:04 +0200 Subject: [PATCH 4/4] Some more changes --- api/handlers/service_instance.go | 2 +- api/handlers/service_instance_test.go | 27 +++++-------------- .../service_instance_repository.go | 2 +- .../service_instance_repository_test.go | 11 ++++++++ 4 files changed, 20 insertions(+), 22 deletions(-) diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index fdc07413d..a8d207337 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -78,7 +78,7 @@ func (h *ServiceInstance) get(r *http.Request) (*routing.Response, error) { serviceInstance, err := h.serviceInstanceRepo.GetServiceInstance(r.Context(), authInfo, serviceInstanceGUID) if err != nil { - return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance") + return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "failed to get service instance", "GUID", serviceInstanceGUID) } includedResources, err := h.includeResolver.ResolveIncludes(r.Context(), authInfo, []repositories.ServiceInstanceRecord{serviceInstance}, payload.IncludeResourceRules) diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index 2fd7e3aad..c025ea093 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -115,8 +115,8 @@ var _ = Describe("ServiceInstance", func() { ) }) - It("returns an not authorized error", func() { - expectNotAuthorizedError() + It("returns an 404 Not Found error", func() { + expectNotFoundError("Service Instance") }) }) @@ -316,24 +316,21 @@ var _ = Describe("ServiceInstance", func() { serviceInstanceRepo.GetServiceInstanceCredentialsReturns(map[string]any{}, apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType)) }) - It("returns 404 Not Found", func() { - _, actualAuthInfo, actualInstanceGUID := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0) - Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(actualInstanceGUID).To(Equal("service-instance-guid")) + It("returns an 404 Not Found error", func() { expectNotFoundError("Service Instance") }) }) - When("getting the service instance fails with not found", func() { + When("getting the service instance fails with an error", func() { BeforeEach(func() { serviceInstanceRepo.GetServiceInstanceReturns( repositories.ServiceInstanceRecord{}, - apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), + errors.New("boom"), ) }) - It("returns 404 Not Found", func() { - expectNotFoundError("Service Instance") + It("returns an error", func() { + expectUnknownError() }) }) @@ -349,16 +346,6 @@ var _ = Describe("ServiceInstance", func() { expectNotFoundError("Service Instance") }) }) - - When("the unmarshaling of the secret fails", func() { - BeforeEach(func() { - serviceInstanceRepo.GetServiceInstanceCredentialsReturns(map[string]any{}, apierrors.NewUnprocessableEntityError(nil, "failed to decode")) - }) - - It("returns an error", func() { - expectUnprocessableEntityError("failed to decode") - }) - }) }) Describe("the POST /v3/service_instances endpoint", func() { diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 77dc1c148..73b2099ca 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -485,7 +485,7 @@ func (r *ServiceInstanceRepo) GetServiceInstanceCredentials(ctx context.Context, credentials, err := tools.FromCredentialsSecretData(credentialsSecret.Data) if err != nil { - return map[string]any{}, fmt.Errorf("failed to decode credentials secret for service instance: %w", err) + return map[string]any{}, apierrors.NewUnprocessableEntityError(err, fmt.Sprintf("failed to decode credentials secret for service instance: %s", instanceGUID)) } return credentials, nil diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index bd0612a7f..1c4efc9f4 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -1084,6 +1084,17 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(errors.As(getErr, &apierrors.NotFoundError{})).To(BeTrue()) }) }) + + When("the secret data is invalid json", func() { + BeforeEach(func() { + secret.Data = map[string][]byte{tools.CredentialsSecretKey: []byte("not-json")} + Expect(k8sClient.Update(ctx, secret)).To(Succeed()) + }) + + It("returns a unprocessiable entity error", func() { + Expect(errors.As(getErr, &apierrors.UnprocessableEntityError{})).To(BeTrue()) + }) + }) }) })