From 0496a9ab381505f26b4e0fe5e2ddf01dc34c2a24 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Thu, 11 Jul 2024 14:18:24 +0000 Subject: [PATCH] Support listing broker by names fixes #3267 Co-authored-by: Danail Branekov --- .../fake/cfservice_broker_repository.go | 18 +++++----- api/handlers/service_broker.go | 9 +++-- api/handlers/service_broker_test.go | 31 +++++++++++++++- api/payloads/service_broker.go | 22 ++++++++++++ api/payloads/service_broker_test.go | 31 ++++++++++++++++ api/repositories/service_broker_repository.go | 29 ++++++++++----- .../service_broker_repository_test.go | 35 +++++++++++++++++-- go.mod | 1 + go.sum | 2 ++ 9 files changed, 156 insertions(+), 22 deletions(-) diff --git a/api/handlers/fake/cfservice_broker_repository.go b/api/handlers/fake/cfservice_broker_repository.go index d588f9374..8e482cc7a 100644 --- a/api/handlers/fake/cfservice_broker_repository.go +++ b/api/handlers/fake/cfservice_broker_repository.go @@ -26,11 +26,12 @@ type CFServiceBrokerRepository struct { result1 repositories.ServiceBrokerResource result2 error } - ListServiceBrokersStub func(context.Context, authorization.Info) ([]repositories.ServiceBrokerResource, error) + ListServiceBrokersStub func(context.Context, authorization.Info, repositories.ListServiceBrokerMessage) ([]repositories.ServiceBrokerResource, error) listServiceBrokersMutex sync.RWMutex listServiceBrokersArgsForCall []struct { arg1 context.Context arg2 authorization.Info + arg3 repositories.ListServiceBrokerMessage } listServiceBrokersReturns struct { result1 []repositories.ServiceBrokerResource @@ -110,19 +111,20 @@ func (fake *CFServiceBrokerRepository) CreateServiceBrokerReturnsOnCall(i int, r }{result1, result2} } -func (fake *CFServiceBrokerRepository) ListServiceBrokers(arg1 context.Context, arg2 authorization.Info) ([]repositories.ServiceBrokerResource, error) { +func (fake *CFServiceBrokerRepository) ListServiceBrokers(arg1 context.Context, arg2 authorization.Info, arg3 repositories.ListServiceBrokerMessage) ([]repositories.ServiceBrokerResource, error) { fake.listServiceBrokersMutex.Lock() ret, specificReturn := fake.listServiceBrokersReturnsOnCall[len(fake.listServiceBrokersArgsForCall)] fake.listServiceBrokersArgsForCall = append(fake.listServiceBrokersArgsForCall, struct { arg1 context.Context arg2 authorization.Info - }{arg1, arg2}) + arg3 repositories.ListServiceBrokerMessage + }{arg1, arg2, arg3}) stub := fake.ListServiceBrokersStub fakeReturns := fake.listServiceBrokersReturns - fake.recordInvocation("ListServiceBrokers", []interface{}{arg1, arg2}) + fake.recordInvocation("ListServiceBrokers", []interface{}{arg1, arg2, arg3}) fake.listServiceBrokersMutex.Unlock() if stub != nil { - return stub(arg1, arg2) + return stub(arg1, arg2, arg3) } if specificReturn { return ret.result1, ret.result2 @@ -136,17 +138,17 @@ func (fake *CFServiceBrokerRepository) ListServiceBrokersCallCount() int { return len(fake.listServiceBrokersArgsForCall) } -func (fake *CFServiceBrokerRepository) ListServiceBrokersCalls(stub func(context.Context, authorization.Info) ([]repositories.ServiceBrokerResource, error)) { +func (fake *CFServiceBrokerRepository) ListServiceBrokersCalls(stub func(context.Context, authorization.Info, repositories.ListServiceBrokerMessage) ([]repositories.ServiceBrokerResource, error)) { fake.listServiceBrokersMutex.Lock() defer fake.listServiceBrokersMutex.Unlock() fake.ListServiceBrokersStub = stub } -func (fake *CFServiceBrokerRepository) ListServiceBrokersArgsForCall(i int) (context.Context, authorization.Info) { +func (fake *CFServiceBrokerRepository) ListServiceBrokersArgsForCall(i int) (context.Context, authorization.Info, repositories.ListServiceBrokerMessage) { fake.listServiceBrokersMutex.RLock() defer fake.listServiceBrokersMutex.RUnlock() argsForCall := fake.listServiceBrokersArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 } func (fake *CFServiceBrokerRepository) ListServiceBrokersReturns(result1 []repositories.ServiceBrokerResource, result2 error) { diff --git a/api/handlers/service_broker.go b/api/handlers/service_broker.go index e3a2a04d2..7c5370e2e 100644 --- a/api/handlers/service_broker.go +++ b/api/handlers/service_broker.go @@ -21,7 +21,7 @@ const ( //counterfeiter:generate -o fake -fake-name CFServiceBrokerRepository . CFServiceBrokerRepository type CFServiceBrokerRepository interface { CreateServiceBroker(context.Context, authorization.Info, repositories.CreateServiceBrokerMessage) (repositories.ServiceBrokerResource, error) - ListServiceBrokers(context.Context, authorization.Info) ([]repositories.ServiceBrokerResource, error) + ListServiceBrokers(context.Context, authorization.Info, repositories.ListServiceBrokerMessage) ([]repositories.ServiceBrokerResource, error) } type ServiceBroker struct { @@ -64,7 +64,12 @@ func (h *ServiceBroker) list(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.service-broker.list") - brokers, err := h.serviceBrokerRepo.ListServiceBrokers(r.Context(), authInfo) + var serviceBrokerListFilter payloads.ServiceBrokerList + if err := h.requestValidator.DecodeAndValidateURLValues(r, &serviceBrokerListFilter); err != nil { + return nil, apierrors.LogAndReturn(logger, err, "failed to decode request values") + } + + brokers, err := h.serviceBrokerRepo.ListServiceBrokers(r.Context(), authInfo, serviceBrokerListFilter.ToMessage()) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to list service brokers") } diff --git a/api/handlers/service_broker_test.go b/api/handlers/service_broker_test.go index 4b9f82c40..8ad95ce8c 100644 --- a/api/handlers/service_broker_test.go +++ b/api/handlers/service_broker_test.go @@ -12,6 +12,7 @@ import ( "code.cloudfoundry.org/korifi/model" "code.cloudfoundry.org/korifi/model/services" . "code.cloudfoundry.org/korifi/tests/matchers" + . "github.com/onsi/gomega/gstruct" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -115,8 +116,11 @@ var _ = Describe("ServiceBroker", func() { It("lists the service brokers", func() { Expect(serviceBrokerRepo.ListServiceBrokersCallCount()).To(Equal(1)) - _, actualAuthInfo := serviceBrokerRepo.ListServiceBrokersArgsForCall(0) + _, actualAuthInfo, actualListMsg := serviceBrokerRepo.ListServiceBrokersArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualListMsg).To(MatchAllFields(Fields{ + "Names": BeEmpty(), + })) Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) @@ -128,6 +132,31 @@ var _ = Describe("ServiceBroker", func() { ))) }) + When("filtering query params are provided", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ServiceBrokerList{ + Names: "b1,b2", + }) + }) + + It("passes them to the repository", func() { + Expect(serviceBrokerRepo.ListServiceBrokersCallCount()).To(Equal(1)) + _, _, message := serviceBrokerRepo.ListServiceBrokersArgsForCall(0) + + Expect(message.Names).To(ConsistOf("b1", "b2")) + }) + }) + + When("decoding query parameters fails", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateURLValuesReturns(errors.New("decode-err")) + }) + + It("returns an error", func() { + expectUnknownError() + }) + }) + When("listing service brokers fails", func() { BeforeEach(func() { serviceBrokerRepo.ListServiceBrokersReturns(nil, errors.New("list-brokers-error")) diff --git a/api/payloads/service_broker.go b/api/payloads/service_broker.go index 9a87d8339..f54046ec0 100644 --- a/api/payloads/service_broker.go +++ b/api/payloads/service_broker.go @@ -1,6 +1,9 @@ package payloads import ( + "net/url" + + "code.cloudfoundry.org/korifi/api/payloads/parse" "code.cloudfoundry.org/korifi/api/payloads/validation" "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/model" @@ -40,3 +43,22 @@ func (c ServiceBrokerCreate) ToCreateServiceBrokerMessage() repositories.CreateS Credentials: c.Authentication.Credentials, } } + +type ServiceBrokerList struct { + Names string +} + +func (b *ServiceBrokerList) DecodeFromURLValues(values url.Values) error { + b.Names = values.Get("names") + return nil +} + +func (b *ServiceBrokerList) SupportedKeys() []string { + return []string{"names"} +} + +func (b *ServiceBrokerList) ToMessage() repositories.ListServiceBrokerMessage { + return repositories.ListServiceBrokerMessage{ + Names: parse.ArrayParam(b.Names), + } +} diff --git a/api/payloads/service_broker_test.go b/api/payloads/service_broker_test.go index b45a345a7..2a5c7a92d 100644 --- a/api/payloads/service_broker_test.go +++ b/api/payloads/service_broker_test.go @@ -1,6 +1,8 @@ package payloads_test import ( + "net/http" + "code.cloudfoundry.org/korifi/api/payloads" "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/model" @@ -111,3 +113,32 @@ var _ = Describe("ServiceBrokerCreate", func() { }) }) }) + +var _ = Describe("ServiceBrokerList", func() { + var serviceBrokerList payloads.ServiceBrokerList + + BeforeEach(func() { + serviceBrokerList = payloads.ServiceBrokerList{ + Names: "b1, b2", + } + }) + + Describe("decodes from url values", func() { + It("succeeds", func() { + req, err := http.NewRequest("GET", "http://foo.com/bar?names=foo,bar", nil) + Expect(err).NotTo(HaveOccurred()) + err = validator.DecodeAndValidateURLValues(req, &serviceBrokerList) + + Expect(err).NotTo(HaveOccurred()) + Expect(serviceBrokerList.Names).To(Equal("foo,bar")) + }) + }) + + Describe("ToMessage", func() { + It("converts to repo message correctly", func() { + Expect(serviceBrokerList.ToMessage()).To(Equal(repositories.ListServiceBrokerMessage{ + Names: []string{"b1", "b2"}, + })) + }) + }) +}) diff --git a/api/repositories/service_broker_repository.go b/api/repositories/service_broker_repository.go index 1f4890dc7..f5be022e8 100644 --- a/api/repositories/service_broker_repository.go +++ b/api/repositories/service_broker_repository.go @@ -3,6 +3,7 @@ package repositories import ( "context" "fmt" + "slices" "code.cloudfoundry.org/korifi/api/authorization" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -10,6 +11,7 @@ import ( "code.cloudfoundry.org/korifi/controllers/controllers/services/credentials" "code.cloudfoundry.org/korifi/model" "code.cloudfoundry.org/korifi/model/services" + "github.com/BooleanCat/go-functional/iter" "github.com/google/uuid" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -27,6 +29,18 @@ type CreateServiceBrokerMessage struct { Credentials services.BrokerCredentials } +type ListServiceBrokerMessage struct { + Names []string +} + +func (l ListServiceBrokerMessage) matches(b korifiv1alpha1.CFServiceBroker) bool { + if len(l.Names) == 0 { + return true + } + + return slices.Contains(l.Names, b.Spec.Name) +} + type ServiceBrokerRepo struct { userClientFactory authorization.UserK8sClientFactory rootNamespace string @@ -93,10 +107,10 @@ func (r *ServiceBrokerRepo) CreateServiceBroker(ctx context.Context, authInfo au return ServiceBrokerResource{}, apierrors.FromK8sError(err, ServiceBrokerResourceType) } - return toServiceBrokerResource(cfServiceBroker), nil + return toServiceBrokerResource(*cfServiceBroker), nil } -func toServiceBrokerResource(cfServiceBroker *korifiv1alpha1.CFServiceBroker) ServiceBrokerResource { +func toServiceBrokerResource(cfServiceBroker korifiv1alpha1.CFServiceBroker) ServiceBrokerResource { return ServiceBrokerResource{ ServiceBroker: cfServiceBroker.Spec.ServiceBroker, CFResource: model.CFResource{ @@ -136,7 +150,7 @@ func (r *ServiceBrokerRepo) GetState(ctx context.Context, authInfo authorization return model.CFResourceState{}, nil } -func (r *ServiceBrokerRepo) ListServiceBrokers(ctx context.Context, authInfo authorization.Info) ([]ServiceBrokerResource, error) { +func (r *ServiceBrokerRepo) ListServiceBrokers(ctx context.Context, authInfo authorization.Info, message ListServiceBrokerMessage) ([]ServiceBrokerResource, error) { userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return nil, fmt.Errorf("failed to build user client: %w", err) @@ -148,13 +162,10 @@ func (r *ServiceBrokerRepo) ListServiceBrokers(ctx context.Context, authInfo aut // All authenticated users are allowed to list brokers. Therefore, the // usual pattern of checking for forbidden error and return an empty // list does not make sense here - return nil, apierrors.FromK8sError(err, ServiceBrokerResourceType) + return nil, fmt.Errorf("failed to list brokers: %w", apierrors.FromK8sError(err, ServiceBrokerResourceType)) } - result := []ServiceBrokerResource{} - for _, broker := range brokersList.Items { - result = append(result, toServiceBrokerResource(&broker)) - } + brokers := iter.Filter(iter.Lift(brokersList.Items), message.matches) - return result, nil + return iter.Collect(iter.Map(brokers, toServiceBrokerResource)), nil } diff --git a/api/repositories/service_broker_repository_test.go b/api/repositories/service_broker_repository_test.go index ce36a3ed5..15b9f7326 100644 --- a/api/repositories/service_broker_repository_test.go +++ b/api/repositories/service_broker_repository_test.go @@ -228,9 +228,14 @@ var _ = Describe("ServiceBrokerRepo", func() { }) Describe("ListServiceBrokers", func() { - var brokers []repositories.ServiceBrokerResource + var ( + brokers []repositories.ServiceBrokerResource + message repositories.ListServiceBrokerMessage + ) BeforeEach(func() { + message = repositories.ListServiceBrokerMessage{} + Expect(k8sClient.Create(ctx, &korifiv1alpha1.CFServiceBroker{ ObjectMeta: metav1.ObjectMeta{ Namespace: rootNamespace, @@ -266,7 +271,7 @@ var _ = Describe("ServiceBrokerRepo", func() { JustBeforeEach(func() { var err error - brokers, err = repo.ListServiceBrokers(ctx, authInfo) + brokers, err = repo.ListServiceBrokers(ctx, authInfo, message) Expect(err).NotTo(HaveOccurred()) }) @@ -302,5 +307,31 @@ var _ = Describe("ServiceBrokerRepo", func() { }), )) }) + + When("a name filter is applied", func() { + BeforeEach(func() { + message.Names = []string{"second-broker"} + }) + + It("only returns the matching brokers", func() { + Expect(brokers).To(ConsistOf( + MatchFields(IgnoreExtras, Fields{ + "ServiceBroker": MatchFields(IgnoreExtras, Fields{ + "Name": Equal("second-broker"), + }), + }), + )) + }) + }) + + When("a nonexistent name filter is applied", func() { + BeforeEach(func() { + message.Names = []string{"no-such-broker"} + }) + + It("returns an empty list", func() { + Expect(brokers).To(BeEmpty()) + }) + }) }) }) diff --git a/go.mod b/go.mod index 3a420256c..3d54a8524 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ toolchain go1.22.3 require ( code.cloudfoundry.org/bytefmt v0.0.0-20211005130812-5bb3c17173e5 code.cloudfoundry.org/go-loggregator/v8 v8.0.5 + github.com/BooleanCat/go-functional v1.1.0 github.com/Masterminds/semver v1.5.0 github.com/PaesslerAG/jsonpath v0.1.1 github.com/SermoDigital/jose v0.9.2-0.20161205224733-f6df55f235c2 diff --git a/go.sum b/go.sum index 44ca9aef8..bbcdec57f 100644 --- a/go.sum +++ b/go.sum @@ -36,6 +36,8 @@ github.com/Azure/go-autorest/logger v0.2.1 h1:IG7i4p/mDa2Ce4TRyAO8IHnVhAVF3RFU+Z github.com/Azure/go-autorest/logger v0.2.1/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZmbF5NWuPV8+WeEW8= github.com/Azure/go-autorest/tracing v0.6.0 h1:TYi4+3m5t6K48TGI9AUdb+IzbnSxvnvUMfuitfgcfuo= github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU= +github.com/BooleanCat/go-functional v1.1.0 h1:ZU8ejc2u/71I5LZbI5qiJI75Ttw1FueLNmoccPt8nDI= +github.com/BooleanCat/go-functional v1.1.0/go.mod h1:Zd1xLrGFohrDdjojLUCrzSex40yf/PVP2KB86ha9Qqg= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/BurntSushi/toml v1.3.2 h1:o7IhLm0Msx3BaB+n3Ag7L8EVlByGnpq14C4YWiu/gL8= github.com/BurntSushi/toml v1.3.2/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ=