servicebinding / spec

Specification for binding services to k8s workloads
https://servicebinding.io
Apache License 2.0
92 stars 35 forks source link

Service label selector #179

Closed arthurdm closed 3 years ago

arthurdm commented 3 years ago

Fixes #169

This PR adds parity between spec.application and spec.service by allowing both to be selected via labels.

Given the nature that labels can match more than 1 resource I also edited the language around $SERVICE_BINDING_ROOT/<binding-name> and how to achieve uniqueness. I believe this was needed even before this PR, because you could currently have two different ServiceBinding CRs with the exact same spec.name field (eg. "database").

baijum commented 3 years ago

If there are different versions/targets of the same service, what will be the recommended strategy to pick the right version/target from an application?

Similar to the provider field, can the spec recommend another field to determine the right version/target? The field name could be environment or cluster or kube-cluster. (I expect values like dev, qa, stage, prod etc.)

arthurdm commented 3 years ago

hi @baijum - you still need to define the kind and apiVersion of the resource, the "oneOf" applies to {name, selector}. Is there a different scenario you're thinking about?

baijum commented 3 years ago

hi @baijum - you still need to define the kind and apiVersion of the resource, the "oneOf" applies to {name, selector}. Is there a different scenario you're thinking about?

I was thinking about the usage patterns that can be adopted by the language-specific application/framework.

For example, consider this Java library:

https://github.com/spring-cloud/spring-cloud-bindings/blob/12b88ac5fe4122459e560c5a98411f58c4694349/src/main/java/org/springframework/cloud/bindings/Bindings.java#L152

or this Python library:

https://pypi.org/project/pyservicebinding/

There are methods to filter bindings based on type and provider values. Since this PR is providing a mechanism to project different versions/targets of the same service, how will the application differentiate between the projected bindings? Or can we assume at a given time, only one bindings directory will be present in the $SERVICE_BINDING_ROOT?

arthurdm commented 3 years ago

how will the application differentiate between the projected bindings?

The key is the problem you describe is not introduced with this PR, it's already there. You can create 2 ServiceBinding CRs that point to different instances of the same service, so both type and provider would be the same.

baijum commented 3 years ago

how will the application differentiate between the projected bindings?

The key is the problem you describe is not introduced with this PR, it's already there. You can create 2 ServiceBinding CRs that point to different instances of the same service, so both type and provider would be the same.

Ok, let's handle this situation in the user guide. I have started working on that here: #175

arthurdm commented 3 years ago

hey everyone - I've updated the PR with the agreement from yesterday's community call to add the restriction that only a single label match (for either app or service) is taken into consideration.

scothis commented 3 years ago

I'm still rather down on adding a label selector to the service reference. As an attempt to create more symmetry between the application workload and service references, I created a draft proposal in #182 that will remove the label selector for application workloads.

One of the driving factors in adding the selector support in the first place was binding into CronJob resources where the target resource itself was not PodSpecable, but has children that are although they have a generated name. The ClusterApplicationResourceMapping support is a better way to manage this case since it can bind directly to CronJob resources.

arthurdm commented 3 years ago

One of the driving factors in adding the selector support in the first place was binding into CronJob resources where the target resource itself was not PodSpecable

I thought the main reason for the workload labels was for resources that had generated names (independent if they were PodSpecable or not). In my mind that's the biggest value-add for supporting labels.

scothis commented 3 years ago

I thought the main reason for the workload labels was for resources that had generated names

If the resource is mutable, you can create the target resource, get the actual name and then create the ServiceBinding resource with that name. It was immutable resource with generated names that were tricky (like a CronJob decomposing to a Job)

arthurdm commented 3 years ago

Maybe we can go through this a bit more in the hangout, as I am confused. :)

If I am using GitOps and I have my ServiceBinding CR in a Git repo I need to either know the name of the target resource or need a label that will be used.

scothis commented 3 years ago

If I am using GitOps and I have my ServiceBinding CR in a Git repo I need to either know the name of the target resource or need a label that will be used.

It's not that I don't think application label selectors have a unique use. I argued hard to get the capability added to the spec in the first place. ClusterApplciationMappings can pick up some of the original need that drove the proposal, but there are still use cases that would be lost.

However, I would rather remove the ability to define an application label selector than to nerf the existing capability by limiting it to a single "first" resource. The proposed language is not only going to surprise users (hopefully not in production), it's also significantly more complicated to implement. On the call community call where this proposal was discussed, there was a desire for symmetry between the service and application selectors. If symmetry is a goal, #182 is a better approach.

My desired resolution is to close both this PR and #182 unmerged. I still believe the behavior for a service label selector can be prototyped in a high level resource either decomposes to a ServiceBinding or acts as a mapping resource that can be referenced by a service binding. Once the semantics are stable and the need demonstrated, then we can expand the scope of the spec to add the behavior.

arthurdm commented 3 years ago

As discussed in today's hangout I will close this PR and we'll work towards merging #182 and then working on a more robust solution for both workloads and service labels.