Replace FieldSelector with LabelSelector in SC

[adamwalach]

Description
Controller-manager is using the FieldSelector. This feature is not supported for CRD. You can query only by name and namespace. Here is the k8s tests documenting that contract.

TBD
AC for this issue is not yet defined. We plan to be on the k8s office hours meeting - hopefully we will get some suggestions which solution is prefered.

Highlights:

Using a field selector to look up a single object in a list is extremely inefficient today. We'd likely need a way to build indexes before supporting that query pattern across all objects (especially across namespaces)

source: Issue about generic fieldSelector

Based on that comment, maybe the FieldSelector is not a good solution at all ;)

Solution:

• Replace with proper indexer for particular informer. See POC implementation.
• Only POC (1 day) - Use the LabelSelector and decorate the resource with additional labels, e.g.:

service-catalog.k8s.io/spec.clusterServiceBrokerName: helm-broker

It's redundant because you have this information under spec and also in labels but e.g. Prow is using this approach.

Concerns:

• We need to choose one approach. Consult someone from k8s. The K8s Office Hours

[mszostok]

We ask that question on k8s office hours, see the recording here.

We didn't get the clear statement which solution is more accurate.

Puja Abbassi said that probably we should go with labels for this thing
He also mentioned the Open Policy Agent. With OPA u can write the Rego policies to ensure the spec fields are copied to labels. Under the hood, it's just a webhook which is using your policies.

Joel Speed said that we should go with indexers as we had in POC cause it looks reasonable, it’s easier, not he is not a fan of duplication in spec and labels cause it's ugly. Other people agree with him.

My proposition is to do the POC for LabelSecletor check how much code we will need to write and how complicated it is.

But in my opinion for Service Catalog we should go with indexers, why?

• are simpler
• not required additional webhooks
• not duplicating information which can be easily out of sync
• sufficient for current usage (currently no special queries are performed)

Additionally, it's a Service Catalog implementation details and if in the future we will need some more complex logic for queries and indexers will not be maintainable then we can simply switch to LabelSelector but for now IMO using the LabelSelector is an overhead.

[adamwalach]

I tried to replace FieldSelectors with indexers and I found few issues.
We have 3 separate areas in SC code:

• svcat
• controller
• webhook server

Each one needs to keep its own definition of indexers and informers.
This can cause performance issues in case of svcat cause with every command we need to populate caches from scratch.

In case of webhook server it makes the architecture of the project more complicated. On startup we need to initialize indexers, and then pass some of them to request handlers - they cannot be created inside the handler cause cache would be initialized on every request.

I'll try with LabelSelectors

Tip: to find all fieldselectors in code search for 'List(listO'

[adamwalach]

We need to have following fields as labels:

ClusterServiceClass

• Spec.ExternalID
• Spec.ExternalName
• Spec.ClusterServiceBrokerName

ClusterServicePlan

• Spec.ExternalID
• Spec.ExternalName
• Spec.ClusterServiceClassRef.Name
• Spec.ClusterServiceBrokerName

ServiceClass

• Spec.ExternalID
• Spec.ExternalName
• Spec.ServiceBrokerName

ServicePlan

• Spec.ExternalID
• Spec.ExternalName
• Spec.ServiceBrokerName
• Spec.ServiceClassRef.Name

ServiceInstance

• Spec.ServicePlanRef.Name
• Spec.ClusterServiceClassRef.Name
• Spec.ServiceClassRef.Name

[mszostok]

Remember about DoD:

• execute make verify and it MUST end without error
• execute make test-unit and it MUST end without error
• execute make format
• check if your changes need to be applied also for svcat. If yes then modify it and then test it also manually. HINT: Create a binary:

  make svcat
  

  generated binary will be placed in bin/svcat/svcat
• finally, always generate a new image and update tag in chart
• finally, test service catalog by installing chart and walkthrough happy path flow both for namespace-scope and cluster-wide brokers. Happy path means creating and deleting all objects.

[adamwalach]

Follow up:
#3322

[adamwalach]

The estimate was off by ~10 points.
Things that took longer than expected:

• unit tests
• fix for a failing service broker: https://github.com/kyma-incubator/service-catalog/blob/crd-prod-impl/pkg/controller/controller_clusterservicebroker.go#L221