From 71ba2ad0a1eda273cde4033908058dc88179dbab Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Fri, 21 Jul 2023 14:32:11 +0000 Subject: [PATCH 1/3] Filter by label selector in `GET /v3/apps` --- api/payloads/app.go | 27 +++++++---- api/payloads/app_test.go | 47 ++++++++++++------- api/repositories/app_repository.go | 10 ++-- api/repositories/app_repository_test.go | 45 ++++++++++++++++-- api/repositories/repositories_suite_test.go | 8 ++++ .../service_binding_repository_test.go | 8 ---- tests/e2e/apps_test.go | 23 ++++++++- tests/e2e/e2e_suite_test.go | 6 +++ 8 files changed, 132 insertions(+), 42 deletions(-) diff --git a/api/payloads/app.go b/api/payloads/app.go index 9b8f49f80..246ec54d5 100644 --- a/api/payloads/app.go +++ b/api/payloads/app.go @@ -10,6 +10,7 @@ import ( "code.cloudfoundry.org/korifi/api/payloads/validation" "code.cloudfoundry.org/korifi/api/repositories" jellidation "github.com/jellydator/validation" + "k8s.io/apimachinery/pkg/labels" ) // DefaultLifecycleConfig is overwritten by main.go @@ -81,10 +82,11 @@ func (d AppSetCurrentDroplet) Validate() error { } type AppList struct { - Names string - GUIDs string - SpaceGuids string - OrderBy string + Names string + GUIDs string + SpaceGuids string + OrderBy string + LabelSelector labels.Selector } func (a AppList) Validate() error { @@ -95,14 +97,15 @@ func (a AppList) Validate() error { func (a *AppList) ToMessage() repositories.ListAppsMessage { return repositories.ListAppsMessage{ - Names: parse.ArrayParam(a.Names), - Guids: parse.ArrayParam(a.GUIDs), - SpaceGuids: parse.ArrayParam(a.SpaceGuids), + Names: parse.ArrayParam(a.Names), + Guids: parse.ArrayParam(a.GUIDs), + SpaceGuids: parse.ArrayParam(a.SpaceGuids), + LabelSelector: a.LabelSelector, } } func (a *AppList) SupportedKeys() []string { - return []string{"names", "guids", "space_guids", "order_by", "per_page", "page"} + return []string{"names", "guids", "space_guids", "order_by", "per_page", "page", "label_selector"} } func (a *AppList) DecodeFromURLValues(values url.Values) error { @@ -110,6 +113,14 @@ func (a *AppList) DecodeFromURLValues(values url.Values) error { a.GUIDs = values.Get("guids") a.SpaceGuids = values.Get("space_guids") a.OrderBy = values.Get("order_by") + + labelSelectorRequirements, err := labels.ParseToRequirements(values.Get("label_selector")) + if err != nil { + return err + } + + a.LabelSelector = labels.NewSelector().Add(labelSelectorRequirements...) + return nil } diff --git a/api/payloads/app_test.go b/api/payloads/app_test.go index f91a20cc8..42209d8da 100644 --- a/api/payloads/app_test.go +++ b/api/payloads/app_test.go @@ -7,6 +7,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/gstruct" + "k8s.io/apimachinery/pkg/labels" ) var _ = Describe("AppList", func() { @@ -19,17 +20,24 @@ var _ = Describe("AppList", func() { Expect(*actualAppList).To(Equal(expectedAppList)) }, - Entry("names", "names=name", payloads.AppList{Names: "name"}), - Entry("guids", "guids=guid", payloads.AppList{GUIDs: "guid"}), - Entry("space_guids", "space_guids=space_guid", payloads.AppList{SpaceGuids: "space_guid"}), - Entry("order_by created_at", "order_by=created_at", payloads.AppList{OrderBy: "created_at"}), - Entry("order_by -created_at", "order_by=-created_at", payloads.AppList{OrderBy: "-created_at"}), - Entry("order_by updated_at", "order_by=updated_at", payloads.AppList{OrderBy: "updated_at"}), - Entry("order_by -updated_at", "order_by=-updated_at", payloads.AppList{OrderBy: "-updated_at"}), - Entry("order_by name", "order_by=name", payloads.AppList{OrderBy: "name"}), - Entry("order_by -name", "order_by=-name", payloads.AppList{OrderBy: "-name"}), - Entry("order_by state", "order_by=state", payloads.AppList{OrderBy: "state"}), - Entry("order_by -state", "order_by=-state", payloads.AppList{OrderBy: "-state"}), + Entry("names", "names=name", payloads.AppList{Names: "name", LabelSelector: labels.Everything()}), + Entry("guids", "guids=guid", payloads.AppList{GUIDs: "guid", LabelSelector: labels.Everything()}), + Entry("space_guids", "space_guids=space_guid", payloads.AppList{SpaceGuids: "space_guid", LabelSelector: labels.Everything()}), + Entry("order_by created_at", "order_by=created_at", payloads.AppList{OrderBy: "created_at", LabelSelector: labels.Everything()}), + Entry("order_by -created_at", "order_by=-created_at", payloads.AppList{OrderBy: "-created_at", LabelSelector: labels.Everything()}), + Entry("order_by updated_at", "order_by=updated_at", payloads.AppList{OrderBy: "updated_at", LabelSelector: labels.Everything()}), + Entry("order_by -updated_at", "order_by=-updated_at", payloads.AppList{OrderBy: "-updated_at", LabelSelector: labels.Everything()}), + Entry("order_by name", "order_by=name", payloads.AppList{OrderBy: "name", LabelSelector: labels.Everything()}), + Entry("order_by -name", "order_by=-name", payloads.AppList{OrderBy: "-name", LabelSelector: labels.Everything()}), + Entry("order_by state", "order_by=state", payloads.AppList{OrderBy: "state", LabelSelector: labels.Everything()}), + Entry("order_by -state", "order_by=-state", payloads.AppList{OrderBy: "-state", LabelSelector: labels.Everything()}), + Entry("label_selector=foo", "label_selector=foo", payloads.AppList{LabelSelector: parseLabelSelector("foo")}), + Entry("label_selector=!foo", "label_selector=!foo", payloads.AppList{LabelSelector: parseLabelSelector("!foo")}), + Entry("label_selector=foo=bar", "label_selector=foo=bar", payloads.AppList{LabelSelector: parseLabelSelector("foo=bar")}), + Entry("label_selector=foo==bar", "label_selector=foo==bar", payloads.AppList{LabelSelector: parseLabelSelector("foo==bar")}), + Entry("label_selector=foo!=bar", "label_selector=foo!=bar", payloads.AppList{LabelSelector: parseLabelSelector("foo!=bar")}), + Entry("label_selector=foo in (bar1,bar2)", "label_selector=foo in (bar1,bar2)", payloads.AppList{LabelSelector: parseLabelSelector("foo in (bar1, bar2)")}), + Entry("label_selector=foo notin (bar1,bar2)", "label_selector=foo notin (bar1,bar2)", payloads.AppList{LabelSelector: parseLabelSelector("foo notin (bar1, bar2)")}), ) DescribeTable("invalid query", @@ -43,16 +51,19 @@ var _ = Describe("AppList", func() { Describe("ToMessage", func() { It("translates to repository message", func() { + labelSelector := parseLabelSelector("foo=bar") appList := payloads.AppList{ - Names: "n1,n2", - GUIDs: "g1,g2", - SpaceGuids: "s1,s2", - OrderBy: "created_at", + Names: "n1,n2", + GUIDs: "g1,g2", + SpaceGuids: "s1,s2", + OrderBy: "created_at", + LabelSelector: labelSelector, } Expect(appList.ToMessage()).To(Equal(repositories.ListAppsMessage{ - Names: []string{"n1", "n2"}, - Guids: []string{"g1", "g2"}, - SpaceGuids: []string{"s1", "s2"}, + Names: []string{"n1", "n2"}, + Guids: []string{"g1", "g2"}, + SpaceGuids: []string{"s1", "s2"}, + LabelSelector: labelSelector, })) }) }) diff --git a/api/repositories/app_repository.go b/api/repositories/app_repository.go index 97037a4e4..46102a4e0 100644 --- a/api/repositories/app_repository.go +++ b/api/repositories/app_repository.go @@ -21,6 +21,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -168,9 +169,10 @@ type SetAppDesiredStateMessage struct { } type ListAppsMessage struct { - Names []string - Guids []string - SpaceGuids []string + Names []string + Guids []string + SpaceGuids []string + LabelSelector labels.Selector } type byName []AppRecord @@ -323,7 +325,7 @@ func (f *AppRepo) ListApps(ctx context.Context, authInfo authorization.Info, mes } appList := &korifiv1alpha1.CFAppList{} - err := userClient.List(ctx, appList, client.InNamespace(ns)) + err := userClient.List(ctx, appList, client.InNamespace(ns), &client.ListOptions{LabelSelector: message.LabelSelector}) if k8serrors.IsForbidden(err) { continue diff --git a/api/repositories/app_repository_test.go b/api/repositories/app_repository_test.go index 068b322df..b485ed385 100644 --- a/api/repositories/app_repository_test.go +++ b/api/repositories/app_repository_test.go @@ -9,11 +9,13 @@ import ( "time" apierrors "code.cloudfoundry.org/korifi/api/errors" + "code.cloudfoundry.org/korifi/api/repositories" . "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/api/repositories/conditions" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/controllers/controllers/shared" "code.cloudfoundry.org/korifi/controllers/controllers/workloads/env" + "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils" "code.cloudfoundry.org/korifi/tests/matchers" "code.cloudfoundry.org/korifi/tools" "code.cloudfoundry.org/korifi/tools/k8s" @@ -50,7 +52,7 @@ var _ = Describe("AppRepository", func() { cfOrg = createOrgWithCleanup(testCtx, prefixedGUID("org")) cfSpace = createSpaceWithCleanup(testCtx, cfOrg.Name, prefixedGUID("space1")) - cfApp = createApp(cfSpace.Name) + cfApp = createAppWithGUID(cfSpace.Name, testutils.PrefixedGUID("cfapp1-")) }) Describe("GetApp", func() { @@ -238,7 +240,7 @@ var _ = Describe("AppRepository", func() { createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, cfSpace.Name) createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space2.Name) - cfApp2 = createApp(space2.Name) + cfApp2 = createAppWithGUID(space2.Name, testutils.PrefixedGUID("cfapp2-")) createApp(space3.Name) }) @@ -286,7 +288,7 @@ var _ = Describe("AppRepository", func() { var cfApp12 *korifiv1alpha1.CFApp BeforeEach(func() { - cfApp12 = createApp(cfSpace.Name) + cfApp12 = createAppWithGUID(cfSpace.Name, testutils.PrefixedGUID("cfapp12-")) }) Describe("filtering by name", func() { @@ -398,6 +400,43 @@ var _ = Describe("AppRepository", func() { }) }) }) + + Describe("filtering by label selector", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, cfApp, func() { + cfApp.Labels = map[string]string{"foo": "FOO1"} + })).To(Succeed()) + Expect(k8s.PatchResource(ctx, k8sClient, cfApp2, func() { + cfApp2.Labels = map[string]string{"foo": "FOO2"} + })).To(Succeed()) + Expect(k8s.PatchResource(ctx, k8sClient, cfApp12, func() { + cfApp12.Labels = map[string]string{"not_foo": "NOT_FOO"} + })).To(Succeed()) + }) + + DescribeTable("valid label selectors", + func(selector string, appGUIDPrefixes ...string) { + serviceBindings, err := appRepo.ListApps(context.Background(), authInfo, repositories.ListAppsMessage{ + LabelSelector: labelSelector(selector), + }) + Expect(err).NotTo(HaveOccurred()) + + matchers := []any{} + for _, prefix := range appGUIDPrefixes { + matchers = append(matchers, MatchFields(IgnoreExtras, Fields{"GUID": HavePrefix(prefix)})) + } + + Expect(serviceBindings).To(ConsistOf(matchers...)) + }, + Entry("key", "foo", "cfapp1-", "cfapp2-"), + Entry("!key", "!foo", "cfapp12-"), + Entry("key=value", "foo=FOO1", "cfapp1-"), + Entry("key==value", "foo==FOO2", "cfapp2-"), + Entry("key!=value", "foo!=FOO1", "cfapp2-", "cfapp12-"), + Entry("key in (value1,value2)", "foo in (FOO1,FOO2)", "cfapp1-", "cfapp2-"), + Entry("key notin (value1,value2)", "foo notin (FOO2)", "cfapp1-", "cfapp12-"), + ) + }) }) }) diff --git a/api/repositories/repositories_suite_test.go b/api/repositories/repositories_suite_test.go index 5076ef4b6..196c2833f 100644 --- a/api/repositories/repositories_suite_test.go +++ b/api/repositories/repositories_suite_test.go @@ -21,6 +21,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/cache" @@ -266,3 +267,10 @@ func createRoleBinding(ctx context.Context, userName, roleName, namespace string Expect(k8sClient.Create(ctx, &roleBinding)).To(Succeed()) return roleBinding } + +func labelSelector(s string) labels.Selector { + requirements, err := labels.ParseToRequirements(s) + Expect(err).NotTo(HaveOccurred()) + + return labels.NewSelector().Add(requirements...) +} diff --git a/api/repositories/service_binding_repository_test.go b/api/repositories/service_binding_repository_test.go index d75e28e0b..641ae6b7d 100644 --- a/api/repositories/service_binding_repository_test.go +++ b/api/repositories/service_binding_repository_test.go @@ -7,7 +7,6 @@ import ( "time" "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/labels" "code.cloudfoundry.org/korifi/api/authorization" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -683,10 +682,3 @@ var _ = Describe("ServiceBindingRepo", func() { }) }) }) - -func labelSelector(s string) labels.Selector { - requirements, err := labels.ParseToRequirements(s) - Expect(err).NotTo(HaveOccurred()) - - return labels.NewSelector().Add(requirements...) -} diff --git a/tests/e2e/apps_test.go b/tests/e2e/apps_test.go index 77a8c75ad..0ca4d862a 100644 --- a/tests/e2e/apps_test.go +++ b/tests/e2e/apps_test.go @@ -7,6 +7,7 @@ import ( "code.cloudfoundry.org/korifi/tools" "github.com/go-resty/resty/v2" + "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -35,9 +36,12 @@ var _ = Describe("Apps", func() { app1GUID, app2GUID, app3GUID string app4GUID, app5GUID, app6GUID string result resourceList[resource] + query string ) BeforeEach(func() { + query = "" + space2GUID = createSpace(generateGUID("space2"), commonTestOrgGUID) space3GUID = createSpace(generateGUID("space3"), commonTestOrgGUID) @@ -59,7 +63,7 @@ var _ = Describe("Apps", func() { JustBeforeEach(func() { var err error - resp, err = certClient.R().SetResult(&result).Get("/v3/apps") + resp, err = certClient.R().SetResult(&result).Get("/v3/apps" + query) Expect(err).NotTo(HaveOccurred()) }) @@ -78,6 +82,23 @@ var _ = Describe("Apps", func() { MatchFields(IgnoreExtras, Fields{"GUID": Equal(app4GUID)}), )) }) + + When("filtering by label selector", func() { + BeforeEach(func() { + label := uuid.NewString() + addAppLabels(app1GUID, map[string]string{label: ""}) + + query = "?label_selector=" + label + }) + + It("lists apps with matching labels", func() { + Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) + + Expect(result.Resources).To(ConsistOf( + MatchFields(IgnoreExtras, Fields{"GUID": Equal(app1GUID)}), + )) + }) + }) }) Describe("Create an app as a user", func() { diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 96e3723e0..c9a256227 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -734,6 +734,12 @@ func addServiceInstanceLabels(serviceInstanceGUID string, labels map[string]stri addLabels("/v3/service_instances/"+serviceInstanceGUID, labels) } +func addAppLabels(appGUID string, labels map[string]string) { + GinkgoHelper() + + addLabels("/v3/apps/"+appGUID, labels) +} + func addLabels(resourcePath string, labels map[string]string) { GinkgoHelper() From 826127a4a71fc9fcb9a9084f083bae182c701be2 Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Mon, 24 Jul 2023 09:50:58 +0000 Subject: [PATCH 2/3] Replace `labels.ParseToRequirements` with `labels.Parse` Co-authored-by: Giuseppe Capizzi --- api/payloads/app.go | 5 ++--- api/payloads/app_test.go | 22 ++++++++++----------- api/payloads/payloads_suite_test.go | 4 ++-- api/payloads/service_binding.go | 5 ++--- api/payloads/service_binding_test.go | 6 +++--- api/payloads/service_instance.go | 4 ++-- api/payloads/service_instance_test.go | 20 +++++++++---------- api/repositories/app_repository_test.go | 3 +-- api/repositories/repositories_suite_test.go | 5 ++--- 9 files changed, 35 insertions(+), 39 deletions(-) diff --git a/api/payloads/app.go b/api/payloads/app.go index 246ec54d5..4b86053be 100644 --- a/api/payloads/app.go +++ b/api/payloads/app.go @@ -114,12 +114,11 @@ func (a *AppList) DecodeFromURLValues(values url.Values) error { a.SpaceGuids = values.Get("space_guids") a.OrderBy = values.Get("order_by") - labelSelectorRequirements, err := labels.ParseToRequirements(values.Get("label_selector")) + selector, err := labels.Parse(values.Get("label_selector")) if err != nil { return err } - - a.LabelSelector = labels.NewSelector().Add(labelSelectorRequirements...) + a.LabelSelector = selector return nil } diff --git a/api/payloads/app_test.go b/api/payloads/app_test.go index 42209d8da..57b13c19d 100644 --- a/api/payloads/app_test.go +++ b/api/payloads/app_test.go @@ -20,17 +20,17 @@ var _ = Describe("AppList", func() { Expect(*actualAppList).To(Equal(expectedAppList)) }, - Entry("names", "names=name", payloads.AppList{Names: "name", LabelSelector: labels.Everything()}), - Entry("guids", "guids=guid", payloads.AppList{GUIDs: "guid", LabelSelector: labels.Everything()}), - Entry("space_guids", "space_guids=space_guid", payloads.AppList{SpaceGuids: "space_guid", LabelSelector: labels.Everything()}), - Entry("order_by created_at", "order_by=created_at", payloads.AppList{OrderBy: "created_at", LabelSelector: labels.Everything()}), - Entry("order_by -created_at", "order_by=-created_at", payloads.AppList{OrderBy: "-created_at", LabelSelector: labels.Everything()}), - Entry("order_by updated_at", "order_by=updated_at", payloads.AppList{OrderBy: "updated_at", LabelSelector: labels.Everything()}), - Entry("order_by -updated_at", "order_by=-updated_at", payloads.AppList{OrderBy: "-updated_at", LabelSelector: labels.Everything()}), - Entry("order_by name", "order_by=name", payloads.AppList{OrderBy: "name", LabelSelector: labels.Everything()}), - Entry("order_by -name", "order_by=-name", payloads.AppList{OrderBy: "-name", LabelSelector: labels.Everything()}), - Entry("order_by state", "order_by=state", payloads.AppList{OrderBy: "state", LabelSelector: labels.Everything()}), - Entry("order_by -state", "order_by=-state", payloads.AppList{OrderBy: "-state", LabelSelector: labels.Everything()}), + Entry("names", "names=name", payloads.AppList{Names: "name", LabelSelector: labels.NewSelector()}), + Entry("guids", "guids=guid", payloads.AppList{GUIDs: "guid", LabelSelector: labels.NewSelector()}), + Entry("space_guids", "space_guids=space_guid", payloads.AppList{SpaceGuids: "space_guid", LabelSelector: labels.NewSelector()}), + Entry("order_by created_at", "order_by=created_at", payloads.AppList{OrderBy: "created_at", LabelSelector: labels.NewSelector()}), + Entry("order_by -created_at", "order_by=-created_at", payloads.AppList{OrderBy: "-created_at", LabelSelector: labels.NewSelector()}), + Entry("order_by updated_at", "order_by=updated_at", payloads.AppList{OrderBy: "updated_at", LabelSelector: labels.NewSelector()}), + Entry("order_by -updated_at", "order_by=-updated_at", payloads.AppList{OrderBy: "-updated_at", LabelSelector: labels.NewSelector()}), + Entry("order_by name", "order_by=name", payloads.AppList{OrderBy: "name", LabelSelector: labels.NewSelector()}), + Entry("order_by -name", "order_by=-name", payloads.AppList{OrderBy: "-name", LabelSelector: labels.NewSelector()}), + Entry("order_by state", "order_by=state", payloads.AppList{OrderBy: "state", LabelSelector: labels.NewSelector()}), + Entry("order_by -state", "order_by=-state", payloads.AppList{OrderBy: "-state", LabelSelector: labels.NewSelector()}), Entry("label_selector=foo", "label_selector=foo", payloads.AppList{LabelSelector: parseLabelSelector("foo")}), Entry("label_selector=!foo", "label_selector=!foo", payloads.AppList{LabelSelector: parseLabelSelector("!foo")}), Entry("label_selector=foo=bar", "label_selector=foo=bar", payloads.AppList{LabelSelector: parseLabelSelector("foo=bar")}), diff --git a/api/payloads/payloads_suite_test.go b/api/payloads/payloads_suite_test.go index 134ced420..2330e3218 100644 --- a/api/payloads/payloads_suite_test.go +++ b/api/payloads/payloads_suite_test.go @@ -74,7 +74,7 @@ func decodeQuery[T any, PT keyedPayload[T]](query string) (PT, error) { } func parseLabelSelector(s string) labels.Selector { - reqs, err := labels.ParseToRequirements(s) + selector, err := labels.Parse(s) Expect(err).NotTo(HaveOccurred()) - return labels.NewSelector().Add(reqs...) + return selector } diff --git a/api/payloads/service_binding.go b/api/payloads/service_binding.go index eb1057229..d0c686b99 100644 --- a/api/payloads/service_binding.go +++ b/api/payloads/service_binding.go @@ -68,13 +68,12 @@ func (l *ServiceBindingList) DecodeFromURLValues(values url.Values) error { l.ServiceInstanceGUIDs = values.Get("service_instance_guids") l.Include = values.Get("include") - labelSelectorRequirements, err := labels.ParseToRequirements(values.Get("label_selector")) + selector, err := labels.Parse(values.Get("label_selector")) if err != nil { return err } - l.LabelSelector = labels.NewSelector().Add(labelSelectorRequirements...) - + l.LabelSelector = selector return nil } diff --git a/api/payloads/service_binding_test.go b/api/payloads/service_binding_test.go index 293d0f76e..6ce31a62b 100644 --- a/api/payloads/service_binding_test.go +++ b/api/payloads/service_binding_test.go @@ -19,9 +19,9 @@ var _ = Describe("ServiceBindingList", func() { Expect(decodeErr).NotTo(HaveOccurred()) Expect(*actualServiceBindingList).To(Equal(expectedServiceBindingList)) }, - Entry("app_guids", "app_guids=app_guid", payloads.ServiceBindingList{AppGUIDs: "app_guid", LabelSelector: labels.Everything()}), - Entry("service_instance_guids", "service_instance_guids=si_guid", payloads.ServiceBindingList{ServiceInstanceGUIDs: "si_guid", LabelSelector: labels.Everything()}), - Entry("include", "include=include", payloads.ServiceBindingList{Include: "include", LabelSelector: labels.Everything()}), + Entry("app_guids", "app_guids=app_guid", payloads.ServiceBindingList{AppGUIDs: "app_guid", LabelSelector: labels.NewSelector()}), + Entry("service_instance_guids", "service_instance_guids=si_guid", payloads.ServiceBindingList{ServiceInstanceGUIDs: "si_guid", LabelSelector: labels.NewSelector()}), + Entry("include", "include=include", payloads.ServiceBindingList{Include: "include", LabelSelector: labels.NewSelector()}), Entry("label_selector=foo", "label_selector=foo", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo")}), Entry("label_selector=!foo", "label_selector=!foo", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("!foo")}), Entry("label_selector=foo=bar", "label_selector=foo=bar", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo=bar")}), diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index b5d120722..feca432d8 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -166,11 +166,11 @@ func (l *ServiceInstanceList) DecodeFromURLValues(values url.Values) error { l.GUIDs = values.Get("guids") l.OrderBy = values.Get("order_by") - labelSelectorRequirements, err := labels.ParseToRequirements(values.Get("label_selector")) + selector, err := labels.Parse(values.Get("label_selector")) if err != nil { return err } - l.LabelSelector = labels.NewSelector().Add(labelSelectorRequirements...) + l.LabelSelector = selector return nil } diff --git a/api/payloads/service_instance_test.go b/api/payloads/service_instance_test.go index 22f9536c1..aa9fad91d 100644 --- a/api/payloads/service_instance_test.go +++ b/api/payloads/service_instance_test.go @@ -21,16 +21,16 @@ var _ = Describe("ServiceInstanceList", func() { Expect(decodeErr).NotTo(HaveOccurred()) Expect(*actualServiceInstanceList).To(Equal(expectedServiceInstanceList)) }, - Entry("names", "names=name", payloads.ServiceInstanceList{Names: "name", LabelSelector: labels.Everything()}), - Entry("space_guids", "space_guids=space_guid", payloads.ServiceInstanceList{SpaceGUIDs: "space_guid", LabelSelector: labels.Everything()}), - Entry("guids", "guids=guid", payloads.ServiceInstanceList{GUIDs: "guid", LabelSelector: labels.Everything()}), - Entry("created_at", "order_by=created_at", payloads.ServiceInstanceList{OrderBy: "created_at", LabelSelector: labels.Everything()}), - Entry("-created_at", "order_by=-created_at", payloads.ServiceInstanceList{OrderBy: "-created_at", LabelSelector: labels.Everything()}), - Entry("updated_at", "order_by=updated_at", payloads.ServiceInstanceList{OrderBy: "updated_at", LabelSelector: labels.Everything()}), - Entry("-updated_at", "order_by=-updated_at", payloads.ServiceInstanceList{OrderBy: "-updated_at", LabelSelector: labels.Everything()}), - Entry("name", "order_by=name", payloads.ServiceInstanceList{OrderBy: "name", LabelSelector: labels.Everything()}), - Entry("-name", "order_by=-name", payloads.ServiceInstanceList{OrderBy: "-name", LabelSelector: labels.Everything()}), - Entry("fields[xxx]", "fields[abc.d]=e", payloads.ServiceInstanceList{LabelSelector: labels.Everything()}), + Entry("names", "names=name", payloads.ServiceInstanceList{Names: "name", LabelSelector: labels.NewSelector()}), + Entry("space_guids", "space_guids=space_guid", payloads.ServiceInstanceList{SpaceGUIDs: "space_guid", LabelSelector: labels.NewSelector()}), + Entry("guids", "guids=guid", payloads.ServiceInstanceList{GUIDs: "guid", LabelSelector: labels.NewSelector()}), + Entry("created_at", "order_by=created_at", payloads.ServiceInstanceList{OrderBy: "created_at", LabelSelector: labels.NewSelector()}), + Entry("-created_at", "order_by=-created_at", payloads.ServiceInstanceList{OrderBy: "-created_at", LabelSelector: labels.NewSelector()}), + Entry("updated_at", "order_by=updated_at", payloads.ServiceInstanceList{OrderBy: "updated_at", LabelSelector: labels.NewSelector()}), + Entry("-updated_at", "order_by=-updated_at", payloads.ServiceInstanceList{OrderBy: "-updated_at", LabelSelector: labels.NewSelector()}), + Entry("name", "order_by=name", payloads.ServiceInstanceList{OrderBy: "name", LabelSelector: labels.NewSelector()}), + Entry("-name", "order_by=-name", payloads.ServiceInstanceList{OrderBy: "-name", LabelSelector: labels.NewSelector()}), + Entry("fields[xxx]", "fields[abc.d]=e", payloads.ServiceInstanceList{LabelSelector: labels.NewSelector()}), Entry("label_selector=foo", "label_selector=foo", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo")}), Entry("label_selector=!foo", "label_selector=!foo", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("!foo")}), Entry("label_selector=foo=bar", "label_selector=foo=bar", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo=bar")}), diff --git a/api/repositories/app_repository_test.go b/api/repositories/app_repository_test.go index b485ed385..6f49ef370 100644 --- a/api/repositories/app_repository_test.go +++ b/api/repositories/app_repository_test.go @@ -9,7 +9,6 @@ import ( "time" apierrors "code.cloudfoundry.org/korifi/api/errors" - "code.cloudfoundry.org/korifi/api/repositories" . "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/api/repositories/conditions" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" @@ -416,7 +415,7 @@ var _ = Describe("AppRepository", func() { DescribeTable("valid label selectors", func(selector string, appGUIDPrefixes ...string) { - serviceBindings, err := appRepo.ListApps(context.Background(), authInfo, repositories.ListAppsMessage{ + serviceBindings, err := appRepo.ListApps(context.Background(), authInfo, ListAppsMessage{ LabelSelector: labelSelector(selector), }) Expect(err).NotTo(HaveOccurred()) diff --git a/api/repositories/repositories_suite_test.go b/api/repositories/repositories_suite_test.go index 196c2833f..59dbce124 100644 --- a/api/repositories/repositories_suite_test.go +++ b/api/repositories/repositories_suite_test.go @@ -269,8 +269,7 @@ func createRoleBinding(ctx context.Context, userName, roleName, namespace string } func labelSelector(s string) labels.Selector { - requirements, err := labels.ParseToRequirements(s) + selector, err := labels.Parse(s) Expect(err).NotTo(HaveOccurred()) - - return labels.NewSelector().Add(requirements...) + return selector } From 282363950d09f7e73e57e1b11c23242f1a570e05 Mon Sep 17 00:00:00 2001 From: Giuseppe Capizzi Date: Mon, 24 Jul 2023 10:37:53 +0000 Subject: [PATCH 3/3] Move parsing of list label selector into repositories This simplifies the payloads code and tests Co-authored-by: Danail Branekov --- api/payloads/app.go | 11 +---- api/payloads/app_test.go | 36 +++++++--------- api/payloads/payloads_suite_test.go | 7 ---- api/payloads/service_binding.go | 11 +---- api/payloads/service_binding_test.go | 34 ++++----------- api/payloads/service_instance.go | 11 +---- api/payloads/service_instance_test.go | 41 +++++++------------ api/repositories/app_repository.go | 9 +++- api/repositories/app_repository_test.go | 27 ++++++++++-- api/repositories/repositories_suite_test.go | 7 ---- .../service_binding_repository.go | 9 +++- .../service_binding_repository_test.go | 12 +++++- .../service_instance_repository.go | 9 +++- .../service_instance_repository_test.go | 24 +++++++++-- 14 files changed, 118 insertions(+), 130 deletions(-) diff --git a/api/payloads/app.go b/api/payloads/app.go index 4b86053be..1fa3dd9b1 100644 --- a/api/payloads/app.go +++ b/api/payloads/app.go @@ -10,7 +10,6 @@ import ( "code.cloudfoundry.org/korifi/api/payloads/validation" "code.cloudfoundry.org/korifi/api/repositories" jellidation "github.com/jellydator/validation" - "k8s.io/apimachinery/pkg/labels" ) // DefaultLifecycleConfig is overwritten by main.go @@ -86,7 +85,7 @@ type AppList struct { GUIDs string SpaceGuids string OrderBy string - LabelSelector labels.Selector + LabelSelector string } func (a AppList) Validate() error { @@ -113,13 +112,7 @@ func (a *AppList) DecodeFromURLValues(values url.Values) error { a.GUIDs = values.Get("guids") a.SpaceGuids = values.Get("space_guids") a.OrderBy = values.Get("order_by") - - selector, err := labels.Parse(values.Get("label_selector")) - if err != nil { - return err - } - a.LabelSelector = selector - + a.LabelSelector = values.Get("label_selector") return nil } diff --git a/api/payloads/app_test.go b/api/payloads/app_test.go index 57b13c19d..317994871 100644 --- a/api/payloads/app_test.go +++ b/api/payloads/app_test.go @@ -7,7 +7,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/gstruct" - "k8s.io/apimachinery/pkg/labels" ) var _ = Describe("AppList", func() { @@ -20,24 +19,18 @@ var _ = Describe("AppList", func() { Expect(*actualAppList).To(Equal(expectedAppList)) }, - Entry("names", "names=name", payloads.AppList{Names: "name", LabelSelector: labels.NewSelector()}), - Entry("guids", "guids=guid", payloads.AppList{GUIDs: "guid", LabelSelector: labels.NewSelector()}), - Entry("space_guids", "space_guids=space_guid", payloads.AppList{SpaceGuids: "space_guid", LabelSelector: labels.NewSelector()}), - Entry("order_by created_at", "order_by=created_at", payloads.AppList{OrderBy: "created_at", LabelSelector: labels.NewSelector()}), - Entry("order_by -created_at", "order_by=-created_at", payloads.AppList{OrderBy: "-created_at", LabelSelector: labels.NewSelector()}), - Entry("order_by updated_at", "order_by=updated_at", payloads.AppList{OrderBy: "updated_at", LabelSelector: labels.NewSelector()}), - Entry("order_by -updated_at", "order_by=-updated_at", payloads.AppList{OrderBy: "-updated_at", LabelSelector: labels.NewSelector()}), - Entry("order_by name", "order_by=name", payloads.AppList{OrderBy: "name", LabelSelector: labels.NewSelector()}), - Entry("order_by -name", "order_by=-name", payloads.AppList{OrderBy: "-name", LabelSelector: labels.NewSelector()}), - Entry("order_by state", "order_by=state", payloads.AppList{OrderBy: "state", LabelSelector: labels.NewSelector()}), - Entry("order_by -state", "order_by=-state", payloads.AppList{OrderBy: "-state", LabelSelector: labels.NewSelector()}), - Entry("label_selector=foo", "label_selector=foo", payloads.AppList{LabelSelector: parseLabelSelector("foo")}), - Entry("label_selector=!foo", "label_selector=!foo", payloads.AppList{LabelSelector: parseLabelSelector("!foo")}), - Entry("label_selector=foo=bar", "label_selector=foo=bar", payloads.AppList{LabelSelector: parseLabelSelector("foo=bar")}), - Entry("label_selector=foo==bar", "label_selector=foo==bar", payloads.AppList{LabelSelector: parseLabelSelector("foo==bar")}), - Entry("label_selector=foo!=bar", "label_selector=foo!=bar", payloads.AppList{LabelSelector: parseLabelSelector("foo!=bar")}), - Entry("label_selector=foo in (bar1,bar2)", "label_selector=foo in (bar1,bar2)", payloads.AppList{LabelSelector: parseLabelSelector("foo in (bar1, bar2)")}), - Entry("label_selector=foo notin (bar1,bar2)", "label_selector=foo notin (bar1,bar2)", payloads.AppList{LabelSelector: parseLabelSelector("foo notin (bar1, bar2)")}), + Entry("names", "names=name", payloads.AppList{Names: "name"}), + Entry("guids", "guids=guid", payloads.AppList{GUIDs: "guid"}), + Entry("space_guids", "space_guids=space_guid", payloads.AppList{SpaceGuids: "space_guid"}), + Entry("order_by created_at", "order_by=created_at", payloads.AppList{OrderBy: "created_at"}), + Entry("order_by -created_at", "order_by=-created_at", payloads.AppList{OrderBy: "-created_at"}), + Entry("order_by updated_at", "order_by=updated_at", payloads.AppList{OrderBy: "updated_at"}), + Entry("order_by -updated_at", "order_by=-updated_at", payloads.AppList{OrderBy: "-updated_at"}), + Entry("order_by name", "order_by=name", payloads.AppList{OrderBy: "name"}), + Entry("order_by -name", "order_by=-name", payloads.AppList{OrderBy: "-name"}), + Entry("order_by state", "order_by=state", payloads.AppList{OrderBy: "state"}), + Entry("order_by -state", "order_by=-state", payloads.AppList{OrderBy: "-state"}), + Entry("label_selector=foo", "label_selector=foo", payloads.AppList{LabelSelector: "foo"}), ) DescribeTable("invalid query", @@ -51,19 +44,18 @@ var _ = Describe("AppList", func() { Describe("ToMessage", func() { It("translates to repository message", func() { - labelSelector := parseLabelSelector("foo=bar") appList := payloads.AppList{ Names: "n1,n2", GUIDs: "g1,g2", SpaceGuids: "s1,s2", OrderBy: "created_at", - LabelSelector: labelSelector, + LabelSelector: "foo=bar", } Expect(appList.ToMessage()).To(Equal(repositories.ListAppsMessage{ Names: []string{"n1", "n2"}, Guids: []string{"g1", "g2"}, SpaceGuids: []string{"s1", "s2"}, - LabelSelector: labelSelector, + LabelSelector: "foo=bar", })) }) }) diff --git a/api/payloads/payloads_suite_test.go b/api/payloads/payloads_suite_test.go index 2330e3218..070ae8383 100644 --- a/api/payloads/payloads_suite_test.go +++ b/api/payloads/payloads_suite_test.go @@ -9,7 +9,6 @@ import ( apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/payloads/validation" "gopkg.in/yaml.v2" - "k8s.io/apimachinery/pkg/labels" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -72,9 +71,3 @@ func decodeQuery[T any, PT keyedPayload[T]](query string) (PT, error) { return actual, decodeErr } - -func parseLabelSelector(s string) labels.Selector { - selector, err := labels.Parse(s) - Expect(err).NotTo(HaveOccurred()) - return selector -} diff --git a/api/payloads/service_binding.go b/api/payloads/service_binding.go index d0c686b99..371c8168b 100644 --- a/api/payloads/service_binding.go +++ b/api/payloads/service_binding.go @@ -7,7 +7,6 @@ import ( "code.cloudfoundry.org/korifi/api/payloads/validation" "code.cloudfoundry.org/korifi/api/repositories" jellidation "github.com/jellydator/validation" - "k8s.io/apimachinery/pkg/labels" ) type ServiceBindingCreate struct { @@ -48,7 +47,7 @@ type ServiceBindingList struct { AppGUIDs string ServiceInstanceGUIDs string Include string - LabelSelector labels.Selector + LabelSelector string } func (l *ServiceBindingList) ToMessage() repositories.ListServiceBindingsMessage { @@ -67,13 +66,7 @@ func (l *ServiceBindingList) DecodeFromURLValues(values url.Values) error { l.AppGUIDs = values.Get("app_guids") l.ServiceInstanceGUIDs = values.Get("service_instance_guids") l.Include = values.Get("include") - - selector, err := labels.Parse(values.Get("label_selector")) - if err != nil { - return err - } - - l.LabelSelector = selector + l.LabelSelector = values.Get("label_selector") return nil } diff --git a/api/payloads/service_binding_test.go b/api/payloads/service_binding_test.go index 6ce31a62b..b542101ce 100644 --- a/api/payloads/service_binding_test.go +++ b/api/payloads/service_binding_test.go @@ -8,7 +8,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/gstruct" - "k8s.io/apimachinery/pkg/labels" ) var _ = Describe("ServiceBindingList", func() { @@ -19,41 +18,24 @@ var _ = Describe("ServiceBindingList", func() { Expect(decodeErr).NotTo(HaveOccurred()) Expect(*actualServiceBindingList).To(Equal(expectedServiceBindingList)) }, - Entry("app_guids", "app_guids=app_guid", payloads.ServiceBindingList{AppGUIDs: "app_guid", LabelSelector: labels.NewSelector()}), - Entry("service_instance_guids", "service_instance_guids=si_guid", payloads.ServiceBindingList{ServiceInstanceGUIDs: "si_guid", LabelSelector: labels.NewSelector()}), - Entry("include", "include=include", payloads.ServiceBindingList{Include: "include", LabelSelector: labels.NewSelector()}), - Entry("label_selector=foo", "label_selector=foo", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo")}), - Entry("label_selector=!foo", "label_selector=!foo", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("!foo")}), - Entry("label_selector=foo=bar", "label_selector=foo=bar", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo=bar")}), - Entry("label_selector=foo==bar", "label_selector=foo==bar", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo==bar")}), - Entry("label_selector=foo!=bar", "label_selector=foo!=bar", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo!=bar")}), - Entry("label_selector=foo in (bar1,bar2)", "label_selector=foo in (bar1,bar2)", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo in (bar1, bar2)")}), - Entry("label_selector=foo notin (bar1,bar2)", "label_selector=foo notin (bar1,bar2)", payloads.ServiceBindingList{LabelSelector: parseLabelSelector("foo notin (bar1, bar2)")}), - ) - - DescribeTable("invalid query", - func(query string, expectedErrMsg string) { - _, decodeErr := decodeQuery[payloads.ServiceBindingList](query) - Expect(decodeErr).To(MatchError(ContainSubstring(expectedErrMsg))) - }, - Entry("invalid label_selector", "label_selector=~~~", "Invalid value"), + Entry("app_guids", "app_guids=app_guid", payloads.ServiceBindingList{AppGUIDs: "app_guid"}), + Entry("service_instance_guids", "service_instance_guids=si_guid", payloads.ServiceBindingList{ServiceInstanceGUIDs: "si_guid"}), + Entry("include", "include=include", payloads.ServiceBindingList{Include: "include"}), + Entry("label_selector=foo", "label_selector=foo", payloads.ServiceBindingList{LabelSelector: "foo"}), ) Describe("ToMessage", func() { var ( - payload payloads.ServiceBindingList - message repositories.ListServiceBindingsMessage - labelSelector labels.Selector + payload payloads.ServiceBindingList + message repositories.ListServiceBindingsMessage ) BeforeEach(func() { - labelSelector = parseLabelSelector("foo=bar") - payload = payloads.ServiceBindingList{ AppGUIDs: "app1,app2", ServiceInstanceGUIDs: "s1,s2", Include: "include", - LabelSelector: labelSelector, + LabelSelector: "foo=bar", } }) @@ -65,7 +47,7 @@ var _ = Describe("ServiceBindingList", func() { Expect(message).To(Equal(repositories.ListServiceBindingsMessage{ AppGUIDs: []string{"app1", "app2"}, ServiceInstanceGUIDs: []string{"s1", "s2"}, - LabelSelector: labelSelector, + LabelSelector: "foo=bar", })) }) }) diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index feca432d8..6cac9fba3 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -11,7 +11,6 @@ import ( "code.cloudfoundry.org/korifi/api/payloads/validation" "code.cloudfoundry.org/korifi/api/repositories" jellidation "github.com/jellydator/validation" - "k8s.io/apimachinery/pkg/labels" ) type ServiceInstanceCreate struct { @@ -134,7 +133,7 @@ type ServiceInstanceList struct { GUIDs string SpaceGUIDs string OrderBy string - LabelSelector labels.Selector + LabelSelector string } func (l ServiceInstanceList) Validate() error { @@ -165,12 +164,6 @@ func (l *ServiceInstanceList) DecodeFromURLValues(values url.Values) error { l.SpaceGUIDs = values.Get("space_guids") l.GUIDs = values.Get("guids") l.OrderBy = values.Get("order_by") - - selector, err := labels.Parse(values.Get("label_selector")) - if err != nil { - return err - } - - l.LabelSelector = selector + l.LabelSelector = values.Get("label_selector") return nil } diff --git a/api/payloads/service_instance_test.go b/api/payloads/service_instance_test.go index aa9fad91d..dc474625e 100644 --- a/api/payloads/service_instance_test.go +++ b/api/payloads/service_instance_test.go @@ -10,7 +10,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" - "k8s.io/apimachinery/pkg/labels" ) var _ = Describe("ServiceInstanceList", func() { @@ -21,23 +20,17 @@ var _ = Describe("ServiceInstanceList", func() { Expect(decodeErr).NotTo(HaveOccurred()) Expect(*actualServiceInstanceList).To(Equal(expectedServiceInstanceList)) }, - Entry("names", "names=name", payloads.ServiceInstanceList{Names: "name", LabelSelector: labels.NewSelector()}), - Entry("space_guids", "space_guids=space_guid", payloads.ServiceInstanceList{SpaceGUIDs: "space_guid", LabelSelector: labels.NewSelector()}), - Entry("guids", "guids=guid", payloads.ServiceInstanceList{GUIDs: "guid", LabelSelector: labels.NewSelector()}), - Entry("created_at", "order_by=created_at", payloads.ServiceInstanceList{OrderBy: "created_at", LabelSelector: labels.NewSelector()}), - Entry("-created_at", "order_by=-created_at", payloads.ServiceInstanceList{OrderBy: "-created_at", LabelSelector: labels.NewSelector()}), - Entry("updated_at", "order_by=updated_at", payloads.ServiceInstanceList{OrderBy: "updated_at", LabelSelector: labels.NewSelector()}), - Entry("-updated_at", "order_by=-updated_at", payloads.ServiceInstanceList{OrderBy: "-updated_at", LabelSelector: labels.NewSelector()}), - Entry("name", "order_by=name", payloads.ServiceInstanceList{OrderBy: "name", LabelSelector: labels.NewSelector()}), - Entry("-name", "order_by=-name", payloads.ServiceInstanceList{OrderBy: "-name", LabelSelector: labels.NewSelector()}), - Entry("fields[xxx]", "fields[abc.d]=e", payloads.ServiceInstanceList{LabelSelector: labels.NewSelector()}), - Entry("label_selector=foo", "label_selector=foo", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo")}), - Entry("label_selector=!foo", "label_selector=!foo", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("!foo")}), - Entry("label_selector=foo=bar", "label_selector=foo=bar", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo=bar")}), - Entry("label_selector=foo==bar", "label_selector=foo==bar", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo==bar")}), - Entry("label_selector=foo!=bar", "label_selector=foo!=bar", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo!=bar")}), - Entry("label_selector=foo in (bar1,bar2)", "label_selector=foo in (bar1,bar2)", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo in (bar1, bar2)")}), - Entry("label_selector=foo notin (bar1,bar2)", "label_selector=foo notin (bar1,bar2)", payloads.ServiceInstanceList{LabelSelector: parseLabelSelector("foo notin (bar1, bar2)")}), + Entry("names", "names=name", payloads.ServiceInstanceList{Names: "name"}), + Entry("space_guids", "space_guids=space_guid", payloads.ServiceInstanceList{SpaceGUIDs: "space_guid"}), + Entry("guids", "guids=guid", payloads.ServiceInstanceList{GUIDs: "guid"}), + Entry("created_at", "order_by=created_at", payloads.ServiceInstanceList{OrderBy: "created_at"}), + Entry("-created_at", "order_by=-created_at", payloads.ServiceInstanceList{OrderBy: "-created_at"}), + Entry("updated_at", "order_by=updated_at", payloads.ServiceInstanceList{OrderBy: "updated_at"}), + Entry("-updated_at", "order_by=-updated_at", payloads.ServiceInstanceList{OrderBy: "-updated_at"}), + Entry("name", "order_by=name", payloads.ServiceInstanceList{OrderBy: "name"}), + Entry("-name", "order_by=-name", payloads.ServiceInstanceList{OrderBy: "-name"}), + Entry("fields[xxx]", "fields[abc.d]=e", payloads.ServiceInstanceList{}), + Entry("label_selector=foo", "label_selector=foo", payloads.ServiceInstanceList{LabelSelector: "foo"}), ) DescribeTable("invalid query", @@ -46,25 +39,21 @@ var _ = Describe("ServiceInstanceList", func() { Expect(decodeErr).To(MatchError(ContainSubstring(expectedErrMsg))) }, Entry("invalid order_by", "order_by=foo", "value must be one of"), - Entry("invalid label_selector", "label_selector=~~~", "Invalid value"), ) Describe("ToMessage", func() { var ( - payload payloads.ServiceInstanceList - message repositories.ListServiceInstanceMessage - labelSelector labels.Selector + payload payloads.ServiceInstanceList + message repositories.ListServiceInstanceMessage ) BeforeEach(func() { - labelSelector = parseLabelSelector("foo=bar") - payload = payloads.ServiceInstanceList{ Names: "n1,n2", GUIDs: "g1,g2", SpaceGUIDs: "sg1,sg2", OrderBy: "order", - LabelSelector: labelSelector, + LabelSelector: "foo=bar", } }) @@ -77,7 +66,7 @@ var _ = Describe("ServiceInstanceList", func() { Names: []string{"n1", "n2"}, SpaceGUIDs: []string{"sg1", "sg2"}, GUIDs: []string{"g1", "g2"}, - LabelSelector: labelSelector, + LabelSelector: "foo=bar", })) }) }) diff --git a/api/repositories/app_repository.go b/api/repositories/app_repository.go index 46102a4e0..626422ebf 100644 --- a/api/repositories/app_repository.go +++ b/api/repositories/app_repository.go @@ -172,7 +172,7 @@ type ListAppsMessage struct { Names []string Guids []string SpaceGuids []string - LabelSelector labels.Selector + LabelSelector string } type byName []AppRecord @@ -317,6 +317,11 @@ func (f *AppRepo) ListApps(ctx context.Context, authInfo authorization.Info, mes SetPredicate(message.Guids, func(s korifiv1alpha1.CFApp) string { return s.Name }), } + labelSelector, err := labels.Parse(message.LabelSelector) + if err != nil { + return []AppRecord{}, apierrors.NewUnprocessableEntityError(err, "invalid label selector") + } + var filteredApps []korifiv1alpha1.CFApp spaceGUIDSet := NewSet(message.SpaceGuids...) for ns := range nsList { @@ -325,7 +330,7 @@ func (f *AppRepo) ListApps(ctx context.Context, authInfo authorization.Info, mes } appList := &korifiv1alpha1.CFAppList{} - err := userClient.List(ctx, appList, client.InNamespace(ns), &client.ListOptions{LabelSelector: message.LabelSelector}) + err := userClient.List(ctx, appList, client.InNamespace(ns), &client.ListOptions{LabelSelector: labelSelector}) if k8serrors.IsForbidden(err) { continue diff --git a/api/repositories/app_repository_test.go b/api/repositories/app_repository_test.go index 6f49ef370..8ae794d7e 100644 --- a/api/repositories/app_repository_test.go +++ b/api/repositories/app_repository_test.go @@ -229,6 +229,7 @@ var _ = Describe("AppRepository", func() { message ListAppsMessage appList []AppRecord cfApp2 *korifiv1alpha1.CFApp + listErr error ) BeforeEach(func() { @@ -244,12 +245,11 @@ var _ = Describe("AppRepository", func() { }) JustBeforeEach(func() { - var err error - appList, err = appRepo.ListApps(testCtx, authInfo, message) - Expect(err).NotTo(HaveOccurred()) + appList, listErr = appRepo.ListApps(testCtx, authInfo, message) }) It("returns all the AppRecord CRs where client has permission", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp2.Name)}), @@ -277,6 +277,7 @@ var _ = Describe("AppRepository", func() { }) It("does not list them", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).NotTo(ContainElement( MatchFields(IgnoreExtras, Fields{"GUID": Equal(nonCFApp.Name)}), )) @@ -307,6 +308,7 @@ var _ = Describe("AppRepository", func() { }) It("returns the matching apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp2.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp12.Name)}), @@ -322,6 +324,7 @@ var _ = Describe("AppRepository", func() { }) It("returns an empty list of apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(BeEmpty()) }) }) @@ -332,6 +335,7 @@ var _ = Describe("AppRepository", func() { }) It("returns the matching apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp2.Name)}), @@ -347,6 +351,7 @@ var _ = Describe("AppRepository", func() { }) It("returns an empty list of apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(BeEmpty()) }) }) @@ -357,6 +362,7 @@ var _ = Describe("AppRepository", func() { }) It("returns the matching apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp12.Name)}), @@ -373,6 +379,7 @@ var _ = Describe("AppRepository", func() { When("an App matches by Name but not by Space", func() { It("returns an empty list of apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(BeEmpty()) }) }) @@ -383,6 +390,7 @@ var _ = Describe("AppRepository", func() { }) It("returns an empty list of apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(BeEmpty()) }) }) @@ -394,6 +402,7 @@ var _ = Describe("AppRepository", func() { }) It("returns the matching apps", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(appList).To(HaveLen(1)) Expect(appList[0].GUID).To(Equal(cfApp12.Name)) }) @@ -416,7 +425,7 @@ var _ = Describe("AppRepository", func() { DescribeTable("valid label selectors", func(selector string, appGUIDPrefixes ...string) { serviceBindings, err := appRepo.ListApps(context.Background(), authInfo, ListAppsMessage{ - LabelSelector: labelSelector(selector), + LabelSelector: selector, }) Expect(err).NotTo(HaveOccurred()) @@ -435,6 +444,16 @@ var _ = Describe("AppRepository", func() { Entry("key in (value1,value2)", "foo in (FOO1,FOO2)", "cfapp1-", "cfapp2-"), Entry("key notin (value1,value2)", "foo notin (FOO2)", "cfapp1-", "cfapp12-"), ) + + When("the label selector is invalid", func() { + BeforeEach(func() { + message = ListAppsMessage{LabelSelector: "~"} + }) + + It("returns an error", func() { + Expect(listErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.UnprocessableEntityError{})) + }) + }) }) }) }) diff --git a/api/repositories/repositories_suite_test.go b/api/repositories/repositories_suite_test.go index 59dbce124..5076ef4b6 100644 --- a/api/repositories/repositories_suite_test.go +++ b/api/repositories/repositories_suite_test.go @@ -21,7 +21,6 @@ import ( rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/cache" @@ -267,9 +266,3 @@ func createRoleBinding(ctx context.Context, userName, roleName, namespace string Expect(k8sClient.Create(ctx, &roleBinding)).To(Succeed()) return roleBinding } - -func labelSelector(s string) labels.Selector { - selector, err := labels.Parse(s) - Expect(err).NotTo(HaveOccurred()) - return selector -} diff --git a/api/repositories/service_binding_repository.go b/api/repositories/service_binding_repository.go index c0aec8c08..e364edca2 100644 --- a/api/repositories/service_binding_repository.go +++ b/api/repositories/service_binding_repository.go @@ -85,7 +85,7 @@ type DeleteServiceBindingMessage struct { type ListServiceBindingsMessage struct { AppGUIDs []string ServiceInstanceGUIDs []string - LabelSelector labels.Selector + LabelSelector string } func (m CreateServiceBindingMessage) toCFServiceBinding() *korifiv1alpha1.CFServiceBinding { @@ -269,10 +269,15 @@ func (r *ServiceBindingRepo) ListServiceBindings(ctx context.Context, authInfo a SetPredicate(message.AppGUIDs, func(s korifiv1alpha1.CFServiceBinding) string { return s.Spec.AppRef.Name }), } + labelSelector, err := labels.Parse(message.LabelSelector) + if err != nil { + return []ServiceBindingRecord{}, apierrors.NewUnprocessableEntityError(err, "invalid label selector") + } + var filteredServiceBindings []korifiv1alpha1.CFServiceBinding for ns := range nsList { serviceBindingList := new(korifiv1alpha1.CFServiceBindingList) - err = userClient.List(ctx, serviceBindingList, client.InNamespace(ns), &client.ListOptions{LabelSelector: message.LabelSelector}) + err = userClient.List(ctx, serviceBindingList, client.InNamespace(ns), &client.ListOptions{LabelSelector: labelSelector}) if k8serrors.IsForbidden(err) { continue } diff --git a/api/repositories/service_binding_repository_test.go b/api/repositories/service_binding_repository_test.go index 641ae6b7d..7a359272d 100644 --- a/api/repositories/service_binding_repository_test.go +++ b/api/repositories/service_binding_repository_test.go @@ -473,7 +473,7 @@ var _ = Describe("ServiceBindingRepo", func() { DescribeTable("valid label selectors", func(selector string, serviceBindingGUIDPrefixes ...string) { serviceBindings, err := repo.ListServiceBindings(context.Background(), authInfo, repositories.ListServiceBindingsMessage{ - LabelSelector: labelSelector(selector), + LabelSelector: selector, }) Expect(err).NotTo(HaveOccurred()) @@ -492,6 +492,16 @@ var _ = Describe("ServiceBindingRepo", func() { Entry("key in (value1,value2)", "foo in (FOO1,FOO2)", "binding-1", "binding-2"), Entry("key notin (value1,value2)", "foo notin (FOO2)", "binding-1", "binding-3"), ) + + When("the label selector is invalid", func() { + BeforeEach(func() { + requestMessage = repositories.ListServiceBindingsMessage{LabelSelector: "~"} + }) + + It("returns an error", func() { + Expect(listErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.UnprocessableEntityError{})) + }) + }) }) When("filtered by multiple params", func() { diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index cdf6f6d5c..1f6e4b19f 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -80,7 +80,7 @@ type ListServiceInstanceMessage struct { Names []string SpaceGUIDs []string GUIDs []string - LabelSelector labels.Selector + LabelSelector string } type DeleteServiceInstanceMessage struct { @@ -204,6 +204,11 @@ func (r *ServiceInstanceRepo) ListServiceInstances(ctx context.Context, authInfo SetPredicate(message.GUIDs, func(s korifiv1alpha1.CFServiceInstance) string { return s.Name }), } + labelSelector, err := labels.Parse(message.LabelSelector) + if err != nil { + return []ServiceInstanceRecord{}, apierrors.NewUnprocessableEntityError(err, "invalid label selector") + } + spaceGUIDSet := NewSet(message.SpaceGUIDs...) var filteredServiceInstances []korifiv1alpha1.CFServiceInstance for ns := range nsList { @@ -212,7 +217,7 @@ func (r *ServiceInstanceRepo) ListServiceInstances(ctx context.Context, authInfo } serviceInstanceList := new(korifiv1alpha1.CFServiceInstanceList) - err = userClient.List(ctx, serviceInstanceList, client.InNamespace(ns), &client.ListOptions{LabelSelector: message.LabelSelector}) + err = userClient.List(ctx, serviceInstanceList, client.InNamespace(ns), &client.ListOptions{LabelSelector: labelSelector}) if k8serrors.IsForbidden(err) { continue } diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 2e291a6fe..ef6e97590 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -9,6 +9,7 @@ import ( apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/repositories" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/tests/matchers" "code.cloudfoundry.org/korifi/tools" "code.cloudfoundry.org/korifi/tools/k8s" "sigs.k8s.io/controller-runtime/pkg/client" @@ -359,6 +360,7 @@ var _ = Describe("ServiceInstanceRepository", func() { cfServiceInstance1, cfServiceInstance2, cfServiceInstance3 *korifiv1alpha1.CFServiceInstance nonCFNamespace string filters repositories.ListServiceInstanceMessage + listErr error serviceInstanceList []repositories.ServiceInstanceRecord ) @@ -382,13 +384,12 @@ var _ = Describe("ServiceInstanceRepository", func() { }) JustBeforeEach(func() { - var err error - serviceInstanceList, err = serviceInstanceRepo.ListServiceInstances(testCtx, authInfo, filters) - Expect(err).NotTo(HaveOccurred()) + serviceInstanceList, listErr = serviceInstanceRepo.ListServiceInstances(testCtx, authInfo, filters) }) When("no service instances exist in spaces where the user has permission", func() { It("returns an empty list of ServiceInstanceRecord", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(serviceInstanceList).To(BeEmpty()) }) }) @@ -400,6 +401,7 @@ var _ = Describe("ServiceInstanceRepository", func() { }) It("returns ServiceInstance records from only the spaces where the user has permission", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(serviceInstanceList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance1.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance2.Name)}), @@ -425,6 +427,7 @@ var _ = Describe("ServiceInstanceRepository", func() { }) It("returns only records for the ServiceInstances with matching spec.name fields", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(serviceInstanceList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance1.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance3.Name)}), @@ -441,7 +444,9 @@ var _ = Describe("ServiceInstanceRepository", func() { }, } }) + It("returns only records for the ServiceInstances within the matching spaces", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(serviceInstanceList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance2.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance3.Name)}), @@ -456,6 +461,7 @@ var _ = Describe("ServiceInstanceRepository", func() { } }) It("returns only records for the ServiceInstances within the matching spaces", func() { + Expect(listErr).NotTo(HaveOccurred()) Expect(serviceInstanceList).To(ConsistOf( MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance1.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance3.Name)}), @@ -479,7 +485,7 @@ var _ = Describe("ServiceInstanceRepository", func() { DescribeTable("valid label selectors", func(selector string, serviceBindingGUIDPrefixes ...string) { serviceInstances, err := serviceInstanceRepo.ListServiceInstances(context.Background(), authInfo, repositories.ListServiceInstanceMessage{ - LabelSelector: labelSelector(selector), + LabelSelector: selector, }) Expect(err).NotTo(HaveOccurred()) @@ -498,6 +504,16 @@ var _ = Describe("ServiceInstanceRepository", func() { Entry("key in (value1,value2)", "foo in (FOO1,FOO2)", "service-instance-1", "service-instance-2"), Entry("key notin (value1,value2)", "foo notin (FOO2)", "service-instance-1", "service-instance-3"), ) + + When("the label selector is invalid", func() { + BeforeEach(func() { + filters = repositories.ListServiceInstanceMessage{LabelSelector: "~"} + }) + + It("returns an error", func() { + Expect(listErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.UnprocessableEntityError{})) + }) + }) }) }) })